Message-ID: <677a5492-8f88-4ad6-8652-278d24b1b8a1@nxp.com>
Date: Mon, 5 Aug 2024 11:23:59 +0800
From: Liu Ying <victor.liu@....com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org,
 p.zabel@...gutronix.de, airlied@...il.com, daniel@...ll.ch,
 maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de,
 robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
 shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
 festevam@...il.com, tglx@...utronix.de, vkoul@...nel.org, kishon@...nel.org,
 aisheng.dong@....com, agx@...xcpu.org, francesco@...cini.it, frank.li@....com
Subject: Re: [PATCH v2 06/16] drm/imx: Add i.MX8qxp Display Controller display
 engine

On 07/31/2024, Dmitry Baryshkov wrote:
> On Tue, Jul 30, 2024 at 02:25:41PM GMT, Liu Ying wrote:
>> On 07/28/2024, Dmitry Baryshkov wrote:
>>> On Fri, Jul 12, 2024 at 05:32:33PM GMT, Liu Ying wrote:
>>>> i.MX8qxp Display Controller display engine consists of all processing
>>>> units that operate in a display clock domain.  Add minimal feature
>>>> support with FrameGen and TCon so that the engine can output display
>>>> timings.  The display engine driver as a master binds FrameGen and
>>>> TCon drivers as components.  While at it, the display engine driver
>>>> is a component to be bound with the upcoming DRM driver.
>>>
>>> Generic question: why do you need so many small subdrivers? Are they
>>
>> As we model processing units, interrupt controller, display engine
>> and pixel engine as devices, relevant drivers are created to bind
>> them.
>>
>> Maxime insisted on splitting the main display controller(the overall
>> IP) into separate devices.  Also, Rob asked me to document every
>> processing units and the other sub-devices in v2.  So, splitting the
>> controller is kinda accepted from both DT PoV and DRM PoV.
> 
> I went back to the previous series, where Maxime commented on the
> "multiple devices glued together". With the split architecture it
> becomes even more strange, because now you have a separate IRQ
> controller which enumerates interrupts for all subdevices and then glues
> them back.
> 
> If it was an actually split device, I'd have expected that each of
> subdevices has interrupts going to the external controller, without the
> glue controller. Or that the glue controller has a limited set of the
> externally-generated interrupts that it further splits into per-block
> IRQs.

People may have different opinions on whether the main display controller
should be split into sub-devices or not, or even how it should be split,
which looks quite subjective.  Given this situation, I tend to follow
the kind of agreement reached by Rob and Maxime that it should be split
entirely and each processing unit should have a dt-binding doc.

> 
> Could you please point out the patches that describe and implement the
> <&dc0_irqsteer> device?

drivers/irqchip/irq-imx-irqsteer.c
Documentation/devicetree/bindings/interrupt-controller/fsl,irqsteer.yaml

You may find an irqsteer DT node in patch 13/16 in v2.

> 
>>
>>> used to represent the flexibility of the pipeline? Can you instantiate
>>
>> No. They are just used to bind the devices created from DT.
>>
>>> these units directly from the de(?) driver and reference created
>>> structures without the need to create subdevices?
>>
>> Given the separated devices created from DT, I can't.
> 
> I'd allow Maxime to override me here, but I think that subblocks should
> not be described in DT, unless that is required to describe
> possible versatility in the display pipeline.
> 
>>>>
>>>> Signed-off-by: Liu Ying <victor.liu@....com>
>>>> ---
>>>> v2:
>>>> * Use OF alias id to get instance id.
>>>> * Add dev member to struct dc_tc.
>>>>
>>>>  drivers/gpu/drm/imx/Kconfig     |   1 +
>>>>  drivers/gpu/drm/imx/Makefile    |   1 +
>>>>  drivers/gpu/drm/imx/dc/Kconfig  |   5 +
>>>>  drivers/gpu/drm/imx/dc/Makefile |   5 +
>>>>  drivers/gpu/drm/imx/dc/dc-de.c  | 151 +++++++++++++
>>>>  drivers/gpu/drm/imx/dc/dc-de.h  |  62 ++++++
>>>>  drivers/gpu/drm/imx/dc/dc-drv.c |  32 +++
>>>>  drivers/gpu/drm/imx/dc/dc-drv.h |  24 +++
>>>>  drivers/gpu/drm/imx/dc/dc-fg.c  | 366 ++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/imx/dc/dc-tc.c  | 137 ++++++++++++
>>>>  10 files changed, 784 insertions(+)
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/Kconfig
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/Makefile
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-de.h
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-drv.h
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-fg.c
>>>>  create mode 100644 drivers/gpu/drm/imx/dc/dc-tc.c
>>>>
>>>> diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
>>>> index 03535a15dd8f..3e8c6edbc17c 100644
>>>> --- a/drivers/gpu/drm/imx/Kconfig
>>>> +++ b/drivers/gpu/drm/imx/Kconfig
>>>> @@ -1,5 +1,6 @@
>>>>  # SPDX-License-Identifier: GPL-2.0-only
>>>>  
>>>> +source "drivers/gpu/drm/imx/dc/Kconfig"
>>>>  source "drivers/gpu/drm/imx/dcss/Kconfig"
>>>>  source "drivers/gpu/drm/imx/ipuv3/Kconfig"
>>>>  source "drivers/gpu/drm/imx/lcdc/Kconfig"
>>>> diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
>>>> index 86f38e7c7422..c7b317640d71 100644
>>>> --- a/drivers/gpu/drm/imx/Makefile
>>>> +++ b/drivers/gpu/drm/imx/Makefile
>>>> @@ -1,5 +1,6 @@
>>>>  # SPDX-License-Identifier: GPL-2.0
>>>>  
>>>> +obj-$(CONFIG_DRM_IMX8_DC) += dc/
>>>>  obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
>>>>  obj-$(CONFIG_DRM_IMX) += ipuv3/
>>>>  obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
>>>> diff --git a/drivers/gpu/drm/imx/dc/Kconfig b/drivers/gpu/drm/imx/dc/Kconfig
>>>> new file mode 100644
>>>> index 000000000000..32d7471c49d0
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/imx/dc/Kconfig
>>>> @@ -0,0 +1,5 @@
>>>> +config DRM_IMX8_DC
>>>> +	tristate "Freescale i.MX8 Display Controller Graphics"
>>>> +	depends on DRM && COMMON_CLK && OF && (ARCH_MXC || COMPILE_TEST)
>>>> +	help
>>>> +	  enable Freescale i.MX8 Display Controller(DC) graphics support
>>>> diff --git a/drivers/gpu/drm/imx/dc/Makefile b/drivers/gpu/drm/imx/dc/Makefile
>>>> new file mode 100644
>>>> index 000000000000..56de82d53d4d
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/imx/dc/Makefile
>>>> @@ -0,0 +1,5 @@
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +imx8-dc-drm-objs := dc-de.o dc-drv.o dc-fg.o dc-tc.o
>>>> +
>>>> +obj-$(CONFIG_DRM_IMX8_DC) += imx8-dc-drm.o
>>>> diff --git a/drivers/gpu/drm/imx/dc/dc-de.c b/drivers/gpu/drm/imx/dc/dc-de.c
>>>> new file mode 100644
>>>> index 000000000000..2c8268b76b08
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/imx/dc/dc-de.c
>>>> @@ -0,0 +1,151 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Copyright 2024 NXP
>>>> + */
>>>> +
>>>> +#include <linux/component.h>
>>>> +#include <linux/container_of.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_platform.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/pm.h>
>>>> +#include <linux/pm_runtime.h>
>>>> +
>>>> +#include <drm/drm_managed.h>
>>>> +
>>>> +#include "dc-de.h"
>>>> +#include "dc-drv.h"
>>>> +
>>>> +#define POLARITYCTRL		0xc
>>>> +#define  POLEN_HIGH		BIT(2)
>>>> +
>>>> +struct dc_de_priv {
>>>> +	struct dc_de engine;
>>>> +	void __iomem *reg_top;
>>>> +};
>>>> +
>>>> +static inline struct dc_de_priv *to_de_priv(struct dc_de *de)
>>>> +{
>>>> +	return container_of(de, struct dc_de_priv, engine);
>>>> +}
>>>> +
>>>> +static inline void
>>>> +dc_dec_write(struct dc_de *de, unsigned int offset, u32 value)
>>>> +{
>>>> +	struct dc_de_priv *priv = to_de_priv(de);
>>>> +
>>>> +	writel(value, priv->reg_top + offset);
>>>
>>> Is there a point in this wrapper? Can you call writel directly? This
>>
>> At least, it helps finding read/write ops upon interested devices through
>> 'git grep'.
> 
> git grep writel also works.

I said "interested devices".  For example, write ops upon LayerBlend can
be easily found by 'git grep dc_lb_write'.  'git grep writel' is not enough
to tell interested device.

> 
>>
>> Also, since we have dc_*_write_mask() helpers, it doesn't look too bad to
>> have dc_*_read/write() helpers.
> 
> Please use regmap_update_bits instead of dc_*_write_mask.

Then, you are suggesting to use both regmap_update_bits and writel.
This doesn't look very consistent.  Why not use regmap_write and
regmap_update_bits?

Anyway, this is a bit bike-shedding.  If you don't have too strong opinion
on this, I'd hope to keep the read/write ops as-is.

> 
>>
>>> question generally applies to the driver. I see a lot of small functions
>>> which can be inlined without losing the clarity.
>>
>> Can you please point out typical ones?
> 
> dc_fg_enable_shden(), dc_fg_syncmode(), dc_dec_init()

I'll inline them and some others.

> 
> To provide an example, this is the code from dc_crtc_atomic_enable().
> 
> 	dc_fg_displaymode(dc_crtc->fg, FG_DM_SEC_ON_TOP);
> 	dc_fg_panic_displaymode(dc_crtc->fg, FG_DM_CONSTCOL);
> 	dc_fg_cfg_videomode(dc_crtc->fg, adj);
> 
> the FG parameters are fixed here. I'd expect a single call from dc_dcrtc
> to dc_fg, which internally does all the settings. This removes a need to
> export low-level details to other modules.

The display modes set by dc_fg_displaymode() and dc_fg_panic_displaymode()
are kinda key settings for KMS.  IMHO, setting them in ->atomic_enable()
makes maintenance and code reading a bit easier though with trivial and
neglectable performance penalty since they are done for each mode setting.

Anyway, since you asked, I'll move the two to dc_fg_init() and move
some others where apply.

> 
>>
>>>
>>>> +}
>>>> +
>>>> +static void dc_dec_init(struct dc_de *de)
>>>> +{
>>>> +	dc_dec_write(de, POLARITYCTRL, POLEN_HIGH);
>>>> +}
>>>> +
>>>> +static int dc_de_bind(struct device *dev, struct device *master, void *data)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct dc_drm_device *dc_drm = data;
>>>> +	struct dc_de_priv *priv;
>>>> +	int ret;
>>>> +
>>>> +	priv = drmm_kzalloc(&dc_drm->base, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->reg_top = devm_platform_ioremap_resource_byname(pdev, "top");
>>>> +	if (IS_ERR(priv->reg_top))
>>>> +		return PTR_ERR(priv->reg_top);
>>>> +
>>>> +	priv->engine.irq_shdld = platform_get_irq_byname(pdev, "shdload");
>>>> +	if (priv->engine.irq_shdld < 0)
>>>> +		return priv->engine.irq_shdld;
>>>> +
>>>> +	priv->engine.irq_framecomplete =
>>>> +				platform_get_irq_byname(pdev, "framecomplete");
>>>> +	if (priv->engine.irq_framecomplete < 0)
>>>> +		return priv->engine.irq_framecomplete;
>>>> +
>>>> +	priv->engine.irq_seqcomplete =
>>>> +				platform_get_irq_byname(pdev, "seqcomplete");
>>>> +	if (priv->engine.irq_seqcomplete < 0)
>>>> +		return priv->engine.irq_seqcomplete;
>>>> +
>>>> +	priv->engine.id = of_alias_get_id(dev->of_node, "dc0-display-engine");
>>>
>>> Is this alias documented somewhere? Is it Acked by DT maintainers?
>>
>> I see aliases nodes in about 10 .yaml files as examples.
>> If needed, I can add them to examples.
>>
>> Rob said "Ideally, no" to use alias in v1. However, IMHO, it is the only
>> appropriate way to get instance id. In v1 review cycles, we've seen kinda
>> 4 ways:
>>
>> 1) fsl,dc-*-id DT property
>>    Rejected by Krzystof.
>>
>> 2) OF alias
>>
>> 3) OF graph ports (Rob)
>>    This doesn't directly get instance id but just tell the connections.
>>    Since there are too many input/output options between some processing
>>    units, I hope we don't end up using this approach, as I mentioned in v1.
>>    It seems be difficult for display driver to handle those ports.   
>>
>>    VC4 Hardware Video Scaler(HVS) is not using OF graph ports to tell the
>>    connections to display controllers, either. See brcm,bcm2835-hvs.yaml.
>>  
>> 4) fsl,imx8qxp-dc-*{id} DT compatible string
>>    It doesn't seem necessary to add the id information to compatible string.
> 
> For the similar issue we ended up hardcoding IO address / masks into the
> driver. This is far from being optimal (and I'd like to get away from

I thought about using IO address to tell instance id in driver before v1,
and chose not to do that since it's not straightforward and kinda abusing IO
address.

> it). If we were designing drm/msm from scratch now, we'd probably have used OF
> graph port IDs.

Don't know drm/msm and it's backing HW(s), so not sure how OF graph ports
can bring benefits for it.  For i.MX8 DC, it will be too complex for DT and
display driver to use OF graph ports. 

> 
>>
>>>
>>>> +	if (priv->engine.id < 0) {
>>>> +		dev_err(dev, "failed to get alias id: %d\n", priv->engine.id);
>>>> +		return priv->engine.id;
>>>> +	}
>>>> +
>>>> +	priv->engine.dev = dev;
>>>> +
>>>> +	dev_set_drvdata(dev, priv);
>>>> +
>>>> +	ret = devm_pm_runtime_enable(dev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	dc_drm->de[priv->engine.id] = &priv->engine;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>
>>
>> -- 
>> Regards,
>> Liu Ying
>>
> 

-- 
Regards,
Liu Ying

Powered by blists - more mailing lists