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

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

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.
> 
> 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.
> 
> 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. So 
> unless there are further details for that use-case which justify
> making 
> this somehow configurable, then maybe we should just stick to 1/3
> and 
> 2/3 for now. And once there is a valid use-case which does justify 
> making this configurable, then we can add the DT properties and all.
> 
> What do you think ?

No strong opinion from me on using LOW=1/3 and HIGH=2/3 thresholds for
now if they satisfy all current users of the upstream kernel.  Tuning
them in a certain way will be indeed needed once an usecase comes along
and still suffers from the FIFO underflows with those thresholds.

Regards,
Liu Ying