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

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

On Tue, 2022-11-01 at 15:04 +0100, Marco Felsch wrote:
> Hi Marek, Liu,

Hi,

> 
> On 22-10-28, Liu Ying wrote:
> > On Fri, 2022-10-28 at 02:03 +0200, Marek Vasut wrote:
> > > On 10/27/22 19:47, Marco Felsch wrote:
> > > > On 22-10-27, Liu Ying wrote:
> > > > > On Thu, 2022-10-27 at 12:03 +0200, Marek Vasut wrote:
> > > > > > On 10/27/22 07:45, Liu Ying wrote:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > > 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.
> > > > > > 
> > > > > > Why 2/3 and 3/3 instead of 1/3 and 2/3 ?
> > > > > 
> > > > > 2/3 and 3/3 trigger panic signal more easily than 1/3 and
> > > > > 2/3.
> > > > > 
> > > > > > 
> > > > > > Seems like 1/3 and 2/3 provides enough FIFO margin for
> > > > > > every
> > > > > > scenario.
> > > > > 
> > > > > I didn't tune the threshold values.  What I was told is that
> > > > > some
> > > > > usecases suffer from the FIFO underflows with 1/3 and
> > > > > 2/3.  And,
> > > > > it
> > > > > appears that FIFO doesn't underflow with 1/2 and 3/4 or 2/3
> > > > > and
> > > > > 3/3 in
> > > > > those usecases.  That's why downstream kernel chooses to use
> > > > > 1/2
> > > > > and
> > > > > 3/4 or 2/3 and 3/3.
> > > > 
> > > > Hi Liu Marek,
> > > > 
> > > > I thought that: If the PANIC is enabled and the pre-configured
> > > > panic-priority is high enough, nothing should interrupt the
> > > > LCDIF
> > > > in
> > > > panic mode since it has the highest prio? So why does it the
> > > > downstream
> > > > kernel configure it differently for different use-cases?
> > > > 
> > > > Also IMHO the threshold should be taken wisely to not enter
> > > > panic
> > > > mode
> > > > to early to not block others from the bus e.g. the GPU.
> > > 
> > > As far as I understand the PANIC0_THRES, both thresholds are
> > > really 
> > > watermarks in the FIFO, 0=EMPTY, 1/3=LOW, 2/3=HIGH, 3/3=FULL.
> > > Under 
> > > normal conditions, the FIFO is filled above 1/3. When the FIFO
> > > fill 
> > > drops below LOW=1/3, the "PANIC" signal is asserted so the FIFO
> > > can
> > > be 
> > > refilled faster. When the FIFO fill raises above HIGH=2/3, the
> > > "PANIC" 
> > > signal is deasserted so the FIFO refills at normal rate again.
> 
> This matches exactly my picture of the hardware.
> 
> > > It seems to me the LOW=1/3 and HIGH=2/3 thresholds are the kind
> > > of
> > > good 
> > > balance. The LOW=2/3 and HIGH=FULL=3/3 seems like it would keep
> > > the 
> > > "PANIC" signal asserted much longer, which could indeed block
> > > others 
> > > from the bus.
> 
> Also I understood the thresholds in such a way, that the FIFO
> watermark
> must be higher but there is no place left when it is set to 3/3. In
> such
> case this means that the PANIC will never left once it was entered.
> 
> > > It also seems to me that tuning these thresholds might be related
> > > to 
> > > some special use-case of the SoC, and it is most likely not just
> > > the 
> > > LCDIF thresholds which have been adjusted in such case, I would
> > > expect 
> > > the NOC and GPV NIC priorities to be adjusted at that point too.
> 
> As far as I understood, the PANIC signal triggers the NOC to use the
> PANIC signal priorities instead of the normal ones. I found a patch
> laying in our downstream [1] repo which configures the threshold. 

The patch sets/clears the bits of the LCDIF_NOC_HURRY field.

> This
> raises the question which PANIC prio do you use? Do you have a patch
> for
> this? If I remember correctly some TF-A's like the NXP downstream one
> configure this but the upstream TF-A don't. Which TF-A do you use?

AFAIK, upstream TF-A also has code to control the LCDIF_NOC_HURRY
field, but i.MX8m power domain driver in upstream kernel doesn't use
TF-A so the control logic in TF-A doesn't take effect.

I'm assuming the LCDIF_NOC_HURRY field has to be somehow configured so
that the thresholds set in LCDIF would avoid LCDIF from FIFO underrun.

Maybe, you may send your patch[1] out for review?

Regards,
Liu Ying

[...] 

> 
> [1]
> 8<-------------------------------------------------------------------
> --
> From b964a83b45c2e2610b8240fbcac776df075c88e2 Mon Sep 17 00:00:00
> 2001
> From: Marco Felsch <m.felsch@pengutronix.de>
> Date: Fri, 22 Jul 2022 11:08:19 +0200
> Subject: [PATCH] soc: imx: imx8mp-blk-ctrl: set the LCDIF hurry
> priority
> 
> Status: WIP, needs clarification with NXP first.
> 
> Set the LCDIF hurry priority to highest possible value so FIFO
> underruns
> can be avoided. The hurry priority will be set by the BLKCTL hw as
> soon
> as the LCDIF panic signal is active and set back to 'normal' priority
> after the panic signal is released.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/soc/imx/imx8mp-blk-ctrl.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/soc/imx/imx8mp-blk-ctrl.c
> b/drivers/soc/imx/imx8mp-blk-ctrl.c
> index 9852714eb2a4..14e744772a01 100644
> --- a/drivers/soc/imx/imx8mp-blk-ctrl.c
> +++ b/drivers/soc/imx/imx8mp-blk-ctrl.c
> @@ -201,6 +201,9 @@ static const struct imx8mp_blk_ctrl_data
> imx8mp_hsio_blk_ctl_dev_data = {
>  #define HDMI_RTX_CLK_CTL3	0x70
>  #define HDMI_RTX_CLK_CTL4	0x80
>  #define HDMI_TX_CONTROL0	0x200
> +#define HDMI_LCDIF_NOC_HURRY_PRIO_MASK	GENMASK(14, 12)
> +#define HDMI_LCDIF_NOC_HURRY_PRIO(p)	\
> +	(((p) << 12) & HDMI_LCDIF_NOC_HURRY_PRIO_MASK)
>  
>  static void imx8mp_hdmi_blk_ctrl_power_on(struct imx8mp_blk_ctrl
> *bc,
>  					  struct imx8mp_blk_ctrl_domain
> *domain)
> @@ -217,6 +220,8 @@ static void imx8mp_hdmi_blk_ctrl_power_on(struct
> imx8mp_blk_ctrl *bc,
>  		regmap_set_bits(bc->regmap, HDMI_RTX_CLK_CTL1,
> BIT(11));
>  		regmap_set_bits(bc->regmap, HDMI_RTX_RESET_CTL0,
>  				BIT(4) | BIT(5) | BIT(6));
> +		regmap_set_bits(bc->regmap, HDMI_TX_CONTROL0,
> +				HDMI_LCDIF_NOC_HURRY_PRIO(7));
>  		break;
>  	case IMX8MP_HDMIBLK_PD_PAI:
>  		regmap_set_bits(bc->regmap, HDMI_RTX_CLK_CTL1,
> BIT(17));
> @@ -273,6 +278,12 @@ static void
> imx8mp_hdmi_blk_ctrl_power_off(struct imx8mp_blk_ctrl *bc,
>  		regmap_clear_bits(bc->regmap, HDMI_RTX_CLK_CTL0,
>  				  BIT(16) | BIT(17) | BIT(18) |
>  				  BIT(19) | BIT(20));
> +		/*
> +		 * Set priority to highest level case of LCDIF panic to
> avoid
> +		 * FIFO underruns.
> +		 */
> +		regmap_clear_bits(bc->regmap, HDMI_TX_CONTROL0,
> +				  HDMI_LCDIF_NOC_HURRY_PRIO(7));
>  		break;
>  	case IMX8MP_HDMIBLK_PD_PAI:
>  		regmap_clear_bits(bc->regmap, HDMI_RTX_RESET_CTL0,
> BIT(18));