Re: [PATCH v6 2/2] drm: lcdif: Add support for i.MX8MP LCDIF variant

[Marek Vasut <marex@denx.de>]

On 6/30/22 10:30, Liu Ying wrote:
> Hi Marek,

Hi,

>>   drivers/gpu/drm/Makefile           |   2 +-
>>   drivers/gpu/drm/mxsfb/Kconfig      |  16 +
>>   drivers/gpu/drm/mxsfb/Makefile     |   2 +
>>   drivers/gpu/drm/mxsfb/lcdif_drv.c  | 341 ++++++++++++++++++++
>>   drivers/gpu/drm/mxsfb/lcdif_drv.h  |  44 +++
>>   drivers/gpu/drm/mxsfb/lcdif_kms.c  | 483 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/mxsfb/lcdif_regs.h | 257 +++++++++++++++
> 
> The mxsfb-drm driver is in the mxsfb directory as a successor of the
> legacy mxsfb fbdev driver. The fbdev driver is for i.MX23/28 lcdif.

That mxsfb-drm is also used on iMX8M{,M,N}

> So, in order to avoid confusion, maybe don't put the new lcdifv3 driver
> here. I would choose to create a new sub-directory in
> drivers/gpu/drm/imx and put it there. The full path can be
> drivers/gpu/drm/imx/lcdifv3, which matches the IP name(as called by
> design team).

It also matches the NXP downstream vendor kernel paths.

> If people don't like it because i.MX23 lcdif version
> register reads major version3, drivers/gpu/drm/imx/imx8mp-lcdif

It cannot, because this IP is also used in iMXRT.

> can be
> an alternative, though longer directory name. I tend to use lcdifv3
> since separate directory(imx vs mxsfb) hints totally different display
> controllers.

We've had this discussion about naming/placement before.

Placing the lcdif driver into mxsfb would allow code deduplication 
between the two drivers, that's why it is in mxsfb directory.

>>   7 files changed, 1144 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.c
>>   create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.h
>>   create mode 100644 drivers/gpu/drm/mxsfb/lcdif_kms.c
>>   create mode 100644 drivers/gpu/drm/mxsfb/lcdif_regs.h
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 13ef240b3d2b2..75b5ac7c2663c 100644
>> --- a/drivers/gpu/drm/Makefile
>> +++ b/drivers/gpu/drm/Makefile
>> @@ -130,7 +130,7 @@ obj-y			+= bridge/
>>   obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu/
>>   obj-$(CONFIG_DRM_ETNAVIV) += etnaviv/
>>   obj-y			+= hisilicon/
>> -obj-$(CONFIG_DRM_MXSFB)	+= mxsfb/
>> +obj-y			+= mxsfb/
>>   obj-y			+= tiny/
>>   obj-$(CONFIG_DRM_PL111) += pl111/
>>   obj-$(CONFIG_DRM_TVE200) += tve200/
>> diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
>> index 987170e16ebd6..873551b4552f5 100644
>> --- a/drivers/gpu/drm/mxsfb/Kconfig
>> +++ b/drivers/gpu/drm/mxsfb/Kconfig
>> @@ -19,3 +19,19 @@ config DRM_MXSFB
>>   	  i.MX28, i.MX6SX, i.MX7 and i.MX8M).
>>   
>>   	  If M is selected the module will be called mxsfb.
>> +
>> +config DRM_IMX_LCDIF
> 
> Perhaps, choose a less generic name here in case yet another new IP
> with similar name in future... Use DRM_IMX_LCDIFV3?
> 
>> +	tristate "i.MX LCDIFv3 LCD controller"
>> +	depends on DRM && OF
>> +	depends on COMMON_CLK
>> +	select DRM_MXS
>> +	select DRM_KMS_HELPER
>> +	select DRM_GEM_CMA_HELPER
>> +	select DRM_PANEL
>> +	select DRM_PANEL_BRIDGE
>> +	help
>> +	  Choose this option if you have an LCDIFv3 LCD controller.
>> +	  Those devices are found in various i.MX SoC (i.MX8MP,
>> +	  i.MXRT).
>> +
>> +	  If M is selected the module will be called imx-lcdif.
> 
> Same here. Use imx-lcdifv3?

The mxsfb LCDIF also supports multiple versions of the LCDIF IP (v3, v4, 
v6 at least). So calling a driver LCDIF v3 is about as confusing, since 
you cannot tell whether it is the iMX23 LCDIF v3 or the iMXRT/iMX8MP 
LCDIFv3 .

> Similar comment applies to the entire driver - less generic name.

[...]

>> +struct lcdif_drm_private {
>> +	void __iomem			*base;	/* registers */
>> +	struct clk			*clk;
>> +	struct clk			*clk_axi;
>> +	struct clk			*clk_disp_axi;
> 
> i.MX8mp RM mentions that clocks are pix_clk, apb_clk and b_clk.
> Why not use them?

That's only because the DT bindings are aligned between old and new LCDIF.

[...]

>> +	switch (format) {
>> +	case DRM_FORMAT_RGB565:
>> +		writel(CTRLDESCL0_5_BPP_16_RGB565,
>> +		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
>> +		break;
>> +	case DRM_FORMAT_RGB888:
>> +		writel(CTRLDESCL0_5_BPP_24_RGB888,
>> +		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
>> +		break;
>> +	case DRM_FORMAT_XRGB1555:
> 
> DRM_FORMAT_ARGB1555?

Why would there be formats with Alpha channel here ?
Because of the (unsupported) overlay planes ?

> 
>> +		writel(CTRLDESCL0_5_BPP_32_ARGB8888,
>> +		       lcdif->base + LCDC_V8_CTRLDESCL0_5);
>> +		break;
>> +	default:
>> +		dev_err(drm->dev, "Unknown pixel format 0x%x\n", format);
>> +		break;
>> +	}
> 
> lcdif_set_formats() is called in lcdif_crtc_mode_set_nofb(), so no fb,
> which means the framebuffer pixel format should be set in plane
> callback.

Do you have iMXRT and are/or you able to test overlay planes with this 
IP (the overlay planes which are currently not supported) ? Or is there 
any other iMX which has overlay plane available ?

[...]