| Message ID | 20221026212002.542375-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: lcdif: Set and enable FIFO Panic threshold | expand |
Hi, On Wed, 2022-10-26 at 23:20 +0200, Marek Vasut wrote: > In case the LCDIFv3 is used to drive a 4k panel via i.MX8MP HDMI bridge, > the LCDIFv3 becomes susceptible to FIFO underflows, which lead to nasty s/lead/leads/ > flicker of the image on the panel, or image being shifted by half frame > horizontally every second frame. The flicker can be easily triggered by > running 3D application on top of weston compositor, like neverball or > chromium. Surprisingly glmark2-es2-wayland or glmark2-es2-drm does not > trigger this effect so easily. > > Configure the FIFO Panic threshold register and enable the FIFO Panic > mode, which internally boosts the NoC interconnect priority for LCDIFv3 > transactions in case of possible underflow. This mitigates the flicker > effect on 4k panels as well. > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Kieran Bingham <kieran.bingham@ideasonboard.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Liu Ying <victor.liu@nxp.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Marco Felsch <m.felsch@pengutronix.de> > Cc: Martyn Welch <martyn.welch@collabora.com> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > --- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 15 +++++++++++++++ > drivers/gpu/drm/mxsfb/lcdif_regs.h | 1 + > 2 files changed, 16 insertions(+) > > 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. > + 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. > } > > 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. > + > 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. Regards, Liu Ying [...]
Hi Marek,
I love your patch! Yet something to improve:
[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-tip/drm-tip linus/master v6.1-rc2 next-20221027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/drm-lcdif-Set-and-enable-FIFO-Panic-threshold/20221027-052135
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link: https://lore.kernel.org/r/20221026212002.542375-1-marex%40denx.de
patch subject: [PATCH] drm: lcdif: Set and enable FIFO Panic threshold
config: arm-randconfig-r004-20221026
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/a6b02f6e5bec37c467d42a457708d565bb10d939
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Marek-Vasut/drm-lcdif-Set-and-enable-FIFO-Panic-threshold/20221027-052135
git checkout a6b02f6e5bec37c467d42a457708d565bb10d939
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/drm/mxsfb/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/linux/swab.h:5,
from include/uapi/linux/byteorder/big_endian.h:14,
from include/linux/byteorder/big_endian.h:5,
from arch/arm/include/uapi/asm/byteorder.h:20,
from include/asm-generic/bitops/le.h:6,
from arch/arm/include/asm/bitops.h:269,
from include/linux/bitops.h:68,
from include/linux/kernel.h:22,
from include/linux/clk.h:13,
from drivers/gpu/drm/mxsfb/lcdif_kms.c:8:
drivers/gpu/drm/mxsfb/lcdif_kms.c: In function 'lcdif_enable_controller':
>> drivers/gpu/drm/mxsfb/lcdif_kms.c:334:16: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
334 | writel(FIELD_PREP(PANIC0_THRES_LOW_MASK, 1 * PANIC0_THRES_RANGE / 3) |
| ^~~~~~~~~~
include/uapi/linux/swab.h:115:54: note: in definition of macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
include/linux/byteorder/generic.h:88:21: note: in expansion of macro '__cpu_to_le32'
88 | #define cpu_to_le32 __cpu_to_le32
| ^~~~~~~~~~~~~
arch/arm/include/asm/io.h:290:47: note: in expansion of macro 'writel_relaxed'
290 | #define writel(v,c) ({ __iowmb(); writel_relaxed(v,c); })
| ^~~~~~~~~~~~~~
drivers/gpu/drm/mxsfb/lcdif_kms.c:334:9: note: in expansion of macro 'writel'
334 | writel(FIELD_PREP(PANIC0_THRES_LOW_MASK, 1 * PANIC0_THRES_RANGE / 3) |
| ^~~~~~
cc1: some warnings being treated as errors
vim +/FIELD_PREP +334 drivers/gpu/drm/mxsfb/lcdif_kms.c
320
321 static void lcdif_enable_controller(struct lcdif_drm_private *lcdif)
322 {
323 u32 reg;
324
325 reg = readl(lcdif->base + LCDC_V8_DISP_PARA);
326 reg |= DISP_PARA_DISP_ON;
327 writel(reg, lcdif->base + LCDC_V8_DISP_PARA);
328
329 reg = readl(lcdif->base + LCDC_V8_CTRLDESCL0_5);
330 reg |= CTRLDESCL0_5_EN;
331 writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
332
333 /* Set FIFO Panic watermarks, low 1/3, high 2/3 . */
> 334 writel(FIELD_PREP(PANIC0_THRES_LOW_MASK, 1 * PANIC0_THRES_RANGE / 3) |
335 FIELD_PREP(PANIC0_THRES_HIGH_MASK, 2 * PANIC0_THRES_RANGE / 3),
336 lcdif->base + LCDC_V8_PANIC0_THRES);
337
338 /*
339 * Enable FIFO Panic, this does not generate interrupt, but
340 * boosts NoC priority based on FIFO Panic watermarks.
341 */
342 writel(INT_ENABLE_D1_PLANE_PANIC_EN,
343 lcdif->base + LCDC_V8_INT_ENABLE_D1);
344 }
345
Hi Marek, On 22-10-26, Marek Vasut wrote: > In case the LCDIFv3 is used to drive a 4k panel via i.MX8MP HDMI bridge, > the LCDIFv3 becomes susceptible to FIFO underflows, which lead to nasty > flicker of the image on the panel, or image being shifted by half frame > horizontally every second frame. The flicker can be easily triggered by > running 3D application on top of weston compositor, like neverball or > chromium. Surprisingly glmark2-es2-wayland or glmark2-es2-drm does not > trigger this effect so easily. > > Configure the FIFO Panic threshold register and enable the FIFO Panic > mode, which internally boosts the NoC interconnect priority for LCDIFv3 > transactions in case of possible underflow. This mitigates the flicker > effect on 4k panels as well. > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Kieran Bingham <kieran.bingham@ideasonboard.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Liu Ying <victor.liu@nxp.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Marco Felsch <m.felsch@pengutronix.de> > Cc: Martyn Welch <martyn.welch@collabora.com> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > --- > drivers/gpu/drm/mxsfb/lcdif_kms.c | 15 +++++++++++++++ > drivers/gpu/drm/mxsfb/lcdif_regs.h | 1 + > 2 files changed, 16 insertions(+) > > 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), > + 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); Out of curiosity since we have a patch doing the exact same thing but didn't saw any improvements. Is there a reason why you enabled it here? We did this during lcdif_rpm_resume(). But as I said with a 1080P display we still saw the flickering, it disappeared first after rising the burst-size. Regards, Marco > } > > 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); > + > 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 > > #define LCDIF_MIN_XRES 120 > #define LCDIF_MIN_YRES 120 > -- > 2.35.1 > >
On 10/27/22 10:13, Marco Felsch 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), >> + 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); > > Out of curiosity since we have a patch doing the exact same thing but > didn't saw any improvements. Is there a reason why you enabled it here? It seems like the right thing to do here, when enabling the controller. > We did this during lcdif_rpm_resume(). But as I said with a 1080P > display we still saw the flickering, it disappeared first after rising > the burst-size. That's what the NXP downstream driver does too, isn't it ? That seems like the wrong place to me.
On 22-10-27, Marek Vasut wrote: > On 10/27/22 10:13, Marco Felsch 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), > > > + 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); > > > > Out of curiosity since we have a patch doing the exact same thing but > > didn't saw any improvements. Is there a reason why you enabled it here? > > It seems like the right thing to do here, when enabling the controller. > > > We did this during lcdif_rpm_resume(). But as I said with a 1080P > > display we still saw the flickering, it disappeared first after rising > > the burst-size. > > That's what the NXP downstream driver does too, isn't it ? That seems like > the wrong place to me. Yes, I think so. It's not about the place (if it wrong/correct) it is more about the PANIC mode itself. I'm curios about: 1) Do you still see the flickering with this patch and without the "burst-size increase" patch? 2) Do you still saw flickering after the "burst-size increase" patch applied and without this patch? I had no 4K display therefore I'm asking, but with 1080P we didn't saw any improvements without increasing the burst-size. My assumption was: if the panic mode does work, than I don't have to increase the burst-size. Regards, Marco
On 10/27/22 10:32, Marco Felsch 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), >>>> + 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); >>> >>> Out of curiosity since we have a patch doing the exact same thing but >>> didn't saw any improvements. Is there a reason why you enabled it here? >> >> It seems like the right thing to do here, when enabling the controller. >> >>> We did this during lcdif_rpm_resume(). But as I said with a 1080P >>> display we still saw the flickering, it disappeared first after rising >>> the burst-size. >> >> That's what the NXP downstream driver does too, isn't it ? That seems like >> the wrong place to me. > > Yes, I think so. It's not about the place (if it wrong/correct) it is > more about the PANIC mode itself. I'm curios about: > 1) Do you still see the flickering with this patch and without the > "burst-size increase" patch? No > 2) Do you still saw flickering after the "burst-size increase" patch > applied and without this patch? I did not try > I had no 4K display therefore I'm asking, but with 1080P we didn't saw > any improvements without increasing the burst-size. My assumption was: > if the panic mode does work, than I don't have to increase the > burst-size. I believe the burst size optimizes the DRAM access pattern -- longer bursts mean the DRAM controller can do longer sustained transfers from the DRAM, which means fewer gaps between transfers for the same amount of data, which means you can use the newly available spare bandwidth for some other transfer.
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 ? Seems like 1/3 and 2/3 provides enough FIFO margin for every scenario. >> + 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 ? >> } >> >> 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. >> + >> 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.
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
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. Regards, Marco
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 ?
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
Hi Marek, Liu, 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. 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? > > 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. I think that 1/3 and 2/3 should be fine for now too. Regards, Marco [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));
On 11/1/22 15:04, Marco Felsch wrote: > Hi Marek, Liu, Hi, [...] >>>> 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. I think this part is wrong. Consider that the FIFO fill drops below 2/3 so PANIC signal asserts. After a bit of time, the FIFO fill reaches full 3/3 (maybe during blanking period, where the data can be read in quickly without being scanned out again), and the PANIC signal de-asserts. So the LCDIF won't be in constant PANIC asserted, but it will be there for quite a bit longer. >>> 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. 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? Upstream 2.6 or 2.7 , so this tuning does not apply. >>> 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. > > I think that 1/3 and 2/3 should be fine for now too. All right, lemme re-test this, send V2, and then we can go from there. btw. can you resend that [PATCH] drm: lcdif: change burst size to 256B with Fixes tag , so it can trickle into stable releases ?
On 22-11-01, Marek Vasut wrote: > On 11/1/22 15:04, Marco Felsch wrote: > > Hi Marek, Liu, > > Hi, > > [...] > > > > > > 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. > > I think this part is wrong. > > Consider that the FIFO fill drops below 2/3 so PANIC signal asserts. ? I thought the PANIC is triggered if the FIFO drops below the 1/3 threshold and is active till the 2/3 threshold. > After a bit of time, the FIFO fill reaches full 3/3 (maybe during > blanking period, where the data can be read in quickly without being > scanned out again), and the PANIC signal de-asserts. > > So the LCDIF won't be in constant PANIC asserted, but it will be there for > quite a bit longer. > > > > > 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. 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? > > Upstream 2.6 or 2.7 , so this tuning does not apply. So your panic priority is what? > > > > 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. > > > > I think that 1/3 and 2/3 should be fine for now too. > > All right, lemme re-test this, send V2, and then we can go from there. Okay :) > btw. can you resend that [PATCH] drm: lcdif: change burst size to 256B with > Fixes tag , so it can trickle into stable releases ? Shure I will resend it with the tag added. Regards, Marco
On 11/1/22 17:06, Marco Felsch wrote: Hi, >>> 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. >> >> I think this part is wrong. >> >> Consider that the FIFO fill drops below 2/3 so PANIC signal asserts. > > ? I thought the PANIC is triggered if the FIFO drops below the 1/3 > threshold and is active till the 2/3 threshold. Yes, although I think the ASSERT/DEASSERT are one-way switches. >> After a bit of time, the FIFO fill reaches full 3/3 (maybe during >> blanking period, where the data can be read in quickly without being >> scanned out again), and the PANIC signal de-asserts. >> >> So the LCDIF won't be in constant PANIC asserted, but it will be there for >> quite a bit longer. >> >>>>> 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. 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? >> >> Upstream 2.6 or 2.7 , so this tuning does not apply. > > So your panic priority is what? If you tell me which register (physical address) to read, I will do that on this board right now. >>>>> 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. >>> >>> I think that 1/3 and 2/3 should be fine for now too. >> >> All right, lemme re-test this, send V2, and then we can go from there. > > Okay :) > >> btw. can you resend that [PATCH] drm: lcdif: change burst size to 256B with >> Fixes tag , so it can trickle into stable releases ? > > Shure I will resend it with the tag added. Thanks, I'll pick it via drm-misc then.
On 22-11-01, Marek Vasut wrote: > On 11/1/22 17:06, Marco Felsch wrote: > > Hi, > > > > > 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. > > > > > > I think this part is wrong. > > > > > > Consider that the FIFO fill drops below 2/3 so PANIC signal asserts. > > > > ? I thought the PANIC is triggered if the FIFO drops below the 1/3 > > threshold and is active till the 2/3 threshold. > > Yes, although I think the ASSERT/DEASSERT are one-way switches. > > > > After a bit of time, the FIFO fill reaches full 3/3 (maybe during > > > blanking period, where the data can be read in quickly without being > > > scanned out again), and the PANIC signal de-asserts. > > > > > > So the LCDIF won't be in constant PANIC asserted, but it will be there for > > > quite a bit longer. > > > > > > > > > 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. 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? > > > > > > Upstream 2.6 or 2.7 , so this tuning does not apply. > > > > So your panic priority is what? > > If you tell me which register (physical address) to read, I will do that on > this board right now. According our patch it is: #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) and we set it within imx8mp_hdmi_blk_ctrl_power_on() by: regmap_set_bits(bc->regmap, HDMI_TX_CONTROL0, HDMI_LCDIF_NOC_HURRY_PRIO(7)); imx8mp_hdmi_blk_ctrl_power_off(): regmap_clear_bits(bc->regmap, HDMI_TX_CONTROL0, HDMI_LCDIF_NOC_HURRY_PRIO(7)); > > > > > > > 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. > > > > > > > > I think that 1/3 and 2/3 should be fine for now too. > > > > > > All right, lemme re-test this, send V2, and then we can go from there. > > > > Okay :) > > > > > btw. can you resend that [PATCH] drm: lcdif: change burst size to 256B with > > > Fixes tag , so it can trickle into stable releases ? > > > > Shure I will resend it with the tag added. > > Thanks, I'll pick it via drm-misc then. Send it as v2. Regards, Marco
On 11/1/22 17:51, Marco Felsch wrote: > On 22-11-01, Marek Vasut wrote: >> On 11/1/22 17:06, Marco Felsch wrote: >> >> Hi, >> >>>>> 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. >>>> >>>> I think this part is wrong. >>>> >>>> Consider that the FIFO fill drops below 2/3 so PANIC signal asserts. >>> >>> ? I thought the PANIC is triggered if the FIFO drops below the 1/3 >>> threshold and is active till the 2/3 threshold. >> >> Yes, although I think the ASSERT/DEASSERT are one-way switches. >> >>>> After a bit of time, the FIFO fill reaches full 3/3 (maybe during >>>> blanking period, where the data can be read in quickly without being >>>> scanned out again), and the PANIC signal de-asserts. >>>> >>>> So the LCDIF won't be in constant PANIC asserted, but it will be there for >>>> quite a bit longer. >>>> >>>>>>> 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. 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? >>>> >>>> Upstream 2.6 or 2.7 , so this tuning does not apply. >>> >>> So your panic priority is what? >> >> If you tell me which register (physical address) to read, I will do that on >> this board right now. > > According our patch it is: > > #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) > > and we set it within imx8mp_hdmi_blk_ctrl_power_on() by: > > regmap_set_bits(bc->regmap, HDMI_TX_CONTROL0, > HDMI_LCDIF_NOC_HURRY_PRIO(7)); > > imx8mp_hdmi_blk_ctrl_power_off(): > > regmap_clear_bits(bc->regmap, HDMI_TX_CONTROL0, > HDMI_LCDIF_NOC_HURRY_PRIO(7)); Hmmm, bc->regmap does not exist in either upstream or downstream ATF versions per git grep, maybe you can give me the exact physical address of the register you would like me to read?
On 22-11-01, Marek Vasut wrote: > On 11/1/22 17:51, Marco Felsch wrote: > > On 22-11-01, Marek Vasut wrote: > > > On 11/1/22 17:06, Marco Felsch wrote: > > > > > > Hi, > > > > > > > > > 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. > > > > > > > > > > I think this part is wrong. > > > > > > > > > > Consider that the FIFO fill drops below 2/3 so PANIC signal asserts. > > > > > > > > ? I thought the PANIC is triggered if the FIFO drops below the 1/3 > > > > threshold and is active till the 2/3 threshold. > > > > > > Yes, although I think the ASSERT/DEASSERT are one-way switches. > > > > > > > > After a bit of time, the FIFO fill reaches full 3/3 (maybe during > > > > > blanking period, where the data can be read in quickly without being > > > > > scanned out again), and the PANIC signal de-asserts. > > > > > > > > > > So the LCDIF won't be in constant PANIC asserted, but it will be there for > > > > > quite a bit longer. > > > > > > > > > > > > > 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. 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? > > > > > > > > > > Upstream 2.6 or 2.7 , so this tuning does not apply. > > > > > > > > So your panic priority is what? > > > > > > If you tell me which register (physical address) to read, I will do that on > > > this board right now. > > > > According our patch it is: > > > > #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) > > > > and we set it within imx8mp_hdmi_blk_ctrl_power_on() by: > > > > regmap_set_bits(bc->regmap, HDMI_TX_CONTROL0, > > HDMI_LCDIF_NOC_HURRY_PRIO(7)); > > > > imx8mp_hdmi_blk_ctrl_power_off(): > > > > regmap_clear_bits(bc->regmap, HDMI_TX_CONTROL0, > > HDMI_LCDIF_NOC_HURRY_PRIO(7)); > > Hmmm, bc->regmap does not exist in either upstream or downstream ATF > versions per git grep, maybe you can give me the exact physical address of > the register you would like me to read? Arg.. sorry if I confused you. This is the code we are using within Linux. We don't wanna use the TF-A to configure these bits. Regards, Marco
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));
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), + 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); } 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); + 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 #define LCDIF_MIN_XRES 120 #define LCDIF_MIN_YRES 120
In case the LCDIFv3 is used to drive a 4k panel via i.MX8MP HDMI bridge, the LCDIFv3 becomes susceptible to FIFO underflows, which lead to nasty flicker of the image on the panel, or image being shifted by half frame horizontally every second frame. The flicker can be easily triggered by running 3D application on top of weston compositor, like neverball or chromium. Surprisingly glmark2-es2-wayland or glmark2-es2-drm does not trigger this effect so easily. Configure the FIFO Panic threshold register and enable the FIFO Panic mode, which internally boosts the NoC interconnect priority for LCDIFv3 transactions in case of possible underflow. This mitigates the flicker effect on 4k panels as well. Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Kieran Bingham <kieran.bingham@ideasonboard.com> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Liu Ying <victor.liu@nxp.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Marco Felsch <m.felsch@pengutronix.de> Cc: Martyn Welch <martyn.welch@collabora.com> Cc: Peng Fan <peng.fan@nxp.com> Cc: Sam Ravnborg <sam@ravnborg.org> --- drivers/gpu/drm/mxsfb/lcdif_kms.c | 15 +++++++++++++++ drivers/gpu/drm/mxsfb/lcdif_regs.h | 1 + 2 files changed, 16 insertions(+)