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