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

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

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.

> 
> > > +	       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.
> 
> That means _before_ DISP_PARA_DISP_ON or CTRLDESCL0_5_EN ?

I'm not sure which one triggers LCDIF controller start to fetch pixels,
but it doesn't hurt to enable FIFO panic before both of the two I
think.

> 
> > >   }
> > >   
> > >   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.
> 
> Same question as above applies, which bits controls that part,
> DISP_ON 
> or CTRLDESCL0_5_EN ? I suspect the later.

Again, I'm not sure, but it doesn't hurt to disable FIFO panic after
both of the two I think.

> 
> > > +
> > >   	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.
> 
> Let's rename this to PANIC0_THRES_MAX too.

Sounds good.

Regards,
Liu Ying