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

[Marek Vasut]

On 4/4/22 14:23, Lucas Stach wrote:
> Hi Marek,

Hi,

> not a full review right now, just the first things that I noticed while
> playing around with this.
> 
> Am Dienstag, dem 22.03.2022 um 15:28 +0100 schrieb Marek Vasut:
>> Add support for i.MX8MP LCDIF variant. This is called LCDIFv3 and is
>> completely different from the LCDIFv3 found in i.MX23 in that it has
>> a completely scrambled register layout compared to all previous LCDIF
>> variants. The new LCDIFv3 also supports 36bit address space.
>>
>> Add a separate driver which is really a fork of MXSFB driver with the
>> i.MX8MP LCDIF variant handling filled in.

[...]

>>   drivers/gpu/drm/mxsfb/Kconfig      |  16 +
>>   drivers/gpu/drm/mxsfb/Makefile     |   2 +
>>   drivers/gpu/drm/mxsfb/lcdif_drv.c  | 367 +++++++++++++++++++++
>>   drivers/gpu/drm/mxsfb/lcdif_drv.h  |  48 +++
>>   drivers/gpu/drm/mxsfb/lcdif_kms.c  | 492 +++++++++++++++++++++++++++++
>>   drivers/gpu/drm/mxsfb/lcdif_regs.h | 243 ++++++++++++++
> 
> Not sure about this placement. I know you hope to share some code with
> the other mxsfb driver, but I would prefer to add this into
> drivers/gpu/drm/imx, same as the DCSS. Another driver for the imx21-
> lcdif will also be added there and we'll move the ipuv3 driver into its
> own subdirectory to make it clear that there are multiple separate
> drivers.

I do disagree with that. This new LCDIF and old LCDIF (mxsfb) are very 
similar except for bit shuffling, and yes, I would like to share common 
code between those two drivers as much as possible.

Placing this driver and mxsfb driver into the same directory makes it easy.

>>   6 files changed, 1168 insertions(+)
>>   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/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
>> index 987170e16ebd6..deb84f99d2fca 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_LCDIF
> 
> The config option name and also the DRM driver name are way too
> generic. Every 3rd SoC out there has a block called lcdif to drive a
> panel...

Do you have any other example of LCDIF ? I only see LCDIF from MX23 
(sgtl design) and this new LCDIF , which still seems like an evolution 
of that sgtl design.

> Maybe something like CONFIG_FSL_LCDIF_V3 and fsl-lcdif-v3 for the name?

Except that LCDIF v3 is the LCDIF present in i.MX23 , which makes this 
even more confusing. Any other ideas ?

[...]

>> +static struct drm_framebuffer *
>> +lcdif_fb_create(struct drm_device *dev, struct drm_file *file_priv,
>> +		const struct drm_mode_fb_cmd2 *mode_cmd)
>> +{
>> +	const struct drm_format_info *info;
>> +
>> +	info = drm_get_format_info(dev, mode_cmd);
>> +	if (!info)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	if (mode_cmd->width * info->cpp[0] != mode_cmd->pitches[0]) {
>> +		dev_dbg(dev->dev, "Invalid pitch: fb width must match pitch\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
> 
> That's not true. One of the major advantages of the LCDIFv3 vs. the
> other controllers supported by mxsfb is that it actually has a
> configurable pitch (CTRLDESCL0_3) separate from the display width.

Right, dropped.

[...]

>> +static int lcdif_rpm_suspend(struct device *dev)
>> +{
>> +	struct drm_device *drm = dev_get_drvdata(dev);
>> +	struct lcdif_drm_private *lcdif = drm->dev_private;
>> +
>> +	/* These clock supply the DISPLAY CLOCK Domain */
>> +	clk_disable_unprepare(lcdif->clk);
> 
> The pixel clock is really only needed when the display is active, so I
> think it would be better to keep this in the modeset path.

Let's continue this in the MXSFB patch discussion so its in one place.

[...]

>> +static void
>> +lcdif_update_buffer(struct lcdif_drm_private *lcdif, struct drm_plane *plane)
>> +{
>> +	dma_addr_t paddr;
>> +	u32 reg;
>> +
>> +	paddr = lcdif_get_fb_paddr(plane);
>> +	if (!paddr)
>> +		return;
>> +
>> +	writel(lower_32_bits(paddr),
>> +	       lcdif->base + LCDC_V8_CTRLDESCL_LOW0_4);
>> +	writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
>> +	       lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
>> +
>> +	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
>> +	reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
>> +	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> 
> The shadow load enable should typically go into the atomic_flush
> function, together with drm_crtc_arm_vblank_event to avoid races.

Can you elaborate on this further ? Why ? What races ?

>> +static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags)
>> +{
>> +	struct drm_display_mode *m = &lcdif->crtc.state->adjusted_mode;
>> +	u32 ctrl = 0;
>> +
>> +	if (m->flags & DRM_MODE_FLAG_PHSYNC)
>> +		ctrl |= CTRL_INV_HS;
>> +	if (m->flags & DRM_MODE_FLAG_PVSYNC)
>> +		ctrl |= CTRL_INV_VS;
>> +	/* Make sure Data Enable is high active by default */
>> +	if (!(bus_flags & DRM_BUS_FLAG_DE_LOW))
>> +		ctrl |= CTRL_INV_DE;
> 
> The above three controls seems to have the wrong polarity. Bit set
> means low active according to the register documentation and the PVI in
> the HDMI path, which has configurable input signal polarity, seems to
> agree with that.

I seem to recall seeing something about DE polarity being inverted in 
odd way in the NXP downstream driver, and differently for each LCDIF 
instance. Isn't that what you're seeing with HDMI ?

[...]

>> +static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
>> +{
>> +	u32 reg;
>> +
>> +	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
>> +	reg &= ~CTRLDESCL0_5_EN;
>> +	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> 
> The downstream driver claims that this bit only takes effect on the end
> of frame, so we should wait here to make sure that DMA is really
> stopped.

[...]

The rest should be fixed.