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
