Re: [PATCH] drm: lcdif: Set and enable FIFO Panic threshold

[Liu Ying <victor.liu@nxp.com>]

Hi,

On Wed, 2022-10-26 at 23:20 +0200, Marek Vasut wrote:
> In case the LCDIFv3 is used to drive a 4k panel via i.MX8MP HDMI bridge,
> the LCDIFv3 becomes susceptible to FIFO underflows, which lead to nasty

s/lead/leads/

> flicker of the image on the panel, or image being shifted by half frame
> horizontally every second frame. The flicker can be easily triggered by
> running 3D application on top of weston compositor, like neverball or
> chromium. Surprisingly glmark2-es2-wayland or glmark2-es2-drm does not
> trigger this effect so easily.
> 
> Configure the FIFO Panic threshold register and enable the FIFO Panic
> mode, which internally boosts the NoC interconnect priority for LCDIFv3
> transactions in case of possible underflow. This mitigates the flicker
> effect on 4k panels as well.
> 
> Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant")
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Liu Ying <victor.liu@nxp.com>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Marco Felsch <m.felsch@pengutronix.de>
> Cc: Martyn Welch <martyn.welch@collabora.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/mxsfb/lcdif_kms.c  | 15 +++++++++++++++
>  drivers/gpu/drm/mxsfb/lcdif_regs.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index a5302006c02cd..aee7babb5fa5c 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -341,6 +341,18 @@ static void lcdif_enable_controller(struct lcdif_drm_private *lcdif)
>  	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
>  	reg |= CTRLDESCL0_5_EN;
>  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> +
> +	/* Set FIFO Panic watermarks, low 1/3, high 2/3 . */
> +	writel(FIELD_PREP(PANIC0_THRES_LOW_MASK, 1 * PANIC0_THRES_RANGE / 3) |
> +	       FIELD_PREP(PANIC0_THRES_HIGH_MASK, 2 * PANIC0_THRES_RANGE / 3),

Better to define PANIC0_THRES_{LOW,HIGH}(n) macros in lcdif_regs.h?

Downstream kernel uses the below threshold values:
a) i.MX8mp EVK board with LPDDR4
1/3 and 2/3 for LCDIF{1,2} + DSI/LVDS - default values in driver
1/2 and 3/4 for LCDIF3 + HDMI - set in device tree

b) i.MX8mp EVK board with DDR4
1/3 and 2/3 for LCDIF{1,2} + DSI/LVDS - default values in driver
2/3 and 3/3 for LCDIF3 + HDMI - set in devic tree

Jian told me that LCDIF3 needs different sets of threshold values for
different types of DDR to avoid 4k HDMI display issues and the
threshold values impact overall DDR/bus utilization(?), so downstream
kernel chooses to get optional threshold value properties from LCDIF DT
node.

Instead of always using 1/3 and 2/3, maybe there are three options:
1) Same to downstream kernel, take 1/3 and 2/3 as default values and
get optional threshold values from DT properties - no additional
properties are acceptable in the existing DT binding doc?
2) Check pixel clock rate, and if it is greater than a certain value,
use 2/3 and 3/3.  Otherwise, use 1/3 and 2/3.
3) Always use 2/3 and 3/3.

> +	       lcdif->base + LCDC_V8_PANIC0_THRES);
> +
> +	/*
> +	 * Enable FIFO Panic, this does not generate interrupt, but
> +	 * boosts NoC priority based on FIFO Panic watermarks.
> +	 */
> +	writel(INT_ENABLE_D1_PLANE_PANIC_EN,
> +	       lcdif->base + LCDC_V8_INT_ENABLE_D1);

This should be enabled _before_ LCDIF controller starts to fetch
pixels, otherwise, there is chance that the FIFO still underflows. 

>  }
>  
>  static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
> @@ -348,6 +360,9 @@ static void lcdif_disable_controller(struct lcdif_drm_private *lcdif)
>  	u32 reg;
>  	int ret;
>  
> +	/* Disable FIFO Panic NoC priority booster. */
> +	writel(0, lcdif->base + LCDC_V8_INT_ENABLE_D1);

Similar to enablement, this should be disabled _after_ LCDIF controller
stops fetching pixels.

> +
>  	reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
>  	reg &= ~CTRLDESCL0_5_EN;
>  	writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> index fb74eb5ccbf1d..3d2f81d6f995e 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h
> +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h
> @@ -255,6 +255,7 @@
>  
>  #define PANIC0_THRES_LOW_MASK		GENMASK(24, 16)
>  #define PANIC0_THRES_HIGH_MASK		GENMASK(8, 0)
> +#define PANIC0_THRES_RANGE		512

Should be 511? If high threshold is 3/3 and PANIC0_THRES_RANGE = 512,
PANIC0_THRES_HIGH will overflow and zero is set.

Regards,
Liu Ying

[...]