| Message ID | 20220726094302.2859456-1-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: lcdif: change burst size to 256B | expand |
On 7/26/22 11:43, Marco Felsch wrote: > FIFO underruns are seen if a AXI bus master with a higher priority do a > lot of memory access. Increase the burst size to 256B to avoid such > underruns and to improve the memory access efficiency. Sigh, this again ... > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c > index 1bec1279c8b5..1f22ea5896d5 100644 > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > @@ -143,8 +143,20 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags) > CTRLDESCL0_1_WIDTH(m->crtc_hdisplay), > lcdif->base + LCDC_V8_CTRLDESCL0_1); > > - writel(CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]), > - lcdif->base + LCDC_V8_CTRLDESCL0_3); > + /* > + * Undocumented P_SIZE and T_SIZE bit fields but according the > + * downstream kernel they control the AXI burst size. As of now there > + * are two known values: > + * 1 - 128Byte > + * 2 - 256Byte > + * > + * Downstream has set the burst size to 256Byte to improve the memory > + * efficiency so set it here too. This also reduces the FIFO underrun > + * possibility. > + */ > + ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) | > + CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]); > + writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3); > } Sometimes I wonder whether this might be some successor of MXSFB LCDIF_CTRL2n OUTSTANDING_REQS and BURST_LEN_B fields. +CC Liu, who seems to have a lot of knowledge about this IP. Reviewed-by: Marek Vasut <marex@denx.de>
On Tue, 2022-07-26 at 16:19 +0200, Marek Vasut wrote: > On 7/26/22 11:43, Marco Felsch wrote: > > FIFO underruns are seen if a AXI bus master with a higher priority > > do a > > lot of memory access. Increase the burst size to 256B to avoid such > > underruns and to improve the memory access efficiency. > > Sigh, this again ... > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > index 1bec1279c8b5..1f22ea5896d5 100644 > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > @@ -143,8 +143,20 @@ static void lcdif_set_mode(struct > > lcdif_drm_private *lcdif, u32 bus_flags) > > CTRLDESCL0_1_WIDTH(m->crtc_hdisplay), > > lcdif->base + LCDC_V8_CTRLDESCL0_1); > > > > - writel(CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb- > > >pitches[0]), > > - lcdif->base + LCDC_V8_CTRLDESCL0_3); > > + /* > > + * Undocumented P_SIZE and T_SIZE bit fields but according the > > + * downstream kernel they control the AXI burst size. As of now > > there I'm not sure if it is AXI burst size or any other burst size, though it seems to be AXI burst size. Cc'ing Jian who mentioned 'burst size' and changed it from 128B to 256B in the downstream kernel. > > + * are two known values: > > + * 1 - 128Byte > > + * 2 - 256Byte > > + * > > + * Downstream has set the burst size to 256Byte to improve the > > memory > > + * efficiency so set it here too. This also reduces the FIFO > > underrun > > + * possibility. > > + */ > > + ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) | > > + CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb- > > >pitches[0]); > > + writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3); Nit: I would write the register directly, instead of caching the value in ctrl. > > } > > Sometimes I wonder whether this might be some successor of MXSFB > LCDIF_CTRL2n OUTSTANDING_REQS and BURST_LEN_B fields. No idea... Liu Ying > > +CC Liu, who seems to have a lot of knowledge about this IP. > > Reviewed-by: Marek Vasut <marex@denx.de>
Hi Marek, Liu, On 22-07-26, Liu Ying wrote: > On Tue, 2022-07-26 at 16:19 +0200, Marek Vasut wrote: > > On 7/26/22 11:43, Marco Felsch wrote: > > > FIFO underruns are seen if a AXI bus master with a higher priority > > > do a > > > lot of memory access. Increase the burst size to 256B to avoid such > > > underruns and to improve the memory access efficiency. > > > > Sigh, this again ... I know.. we also tried the PANIC mode but this somehow didn't worked as documented. So this was the only way to reduce the underruns without adapting the interconnect prio for the hdmi-lcdif. > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > index 1bec1279c8b5..1f22ea5896d5 100644 > > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > @@ -143,8 +143,20 @@ static void lcdif_set_mode(struct > > > lcdif_drm_private *lcdif, u32 bus_flags) > > > CTRLDESCL0_1_WIDTH(m->crtc_hdisplay), > > > lcdif->base + LCDC_V8_CTRLDESCL0_1); > > > > > > - writel(CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb- > > > >pitches[0]), > > > - lcdif->base + LCDC_V8_CTRLDESCL0_3); > > > + /* > > > + * Undocumented P_SIZE and T_SIZE bit fields but according the > > > + * downstream kernel they control the AXI burst size. As of now > > > there > > I'm not sure if it is AXI burst size or any other burst size, though it > seems to be AXI burst size. > > Cc'ing Jian who mentioned 'burst size' and changed it from 128B to 256B > in the downstream kernel. Thanks. > > > + * are two known values: > > > + * 1 - 128Byte > > > + * 2 - 256Byte > > > + * > > > + * Downstream has set the burst size to 256Byte to improve the > > > memory > > > + * efficiency so set it here too. This also reduces the FIFO > > > underrun > > > + * possibility. > > > + */ > > > + ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) | > > > + CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb- > > > >pitches[0]); > > > + writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3); > > Nit: I would write the register directly, instead of caching the value > in ctrl. IMHO it's more readable that way. Regards, Marco > > > } > > > > Sometimes I wonder whether this might be some successor of MXSFB > > LCDIF_CTRL2n OUTSTANDING_REQS and BURST_LEN_B fields. > > No idea... > > Liu Ying > > > > > +CC Liu, who seems to have a lot of knowledge about this IP. > > > > Reviewed-by: Marek Vasut <marex@denx.de> > >
On 7/27/22 05:56, Marco Felsch wrote: > Hi Marek, Liu, Hi, > On 22-07-26, Liu Ying wrote: >> On Tue, 2022-07-26 at 16:19 +0200, Marek Vasut wrote: >>> On 7/26/22 11:43, Marco Felsch wrote: >>>> FIFO underruns are seen if a AXI bus master with a higher priority >>>> do a >>>> lot of memory access. Increase the burst size to 256B to avoid such >>>> underruns and to improve the memory access efficiency. >>> >>> Sigh, this again ... > > I know.. we also tried the PANIC mode but this somehow didn't worked as > documented. So this was the only way to reduce the underruns without > adapting the interconnect prio for the hdmi-lcdif. Right, the PANIC watermark didn't work on mxsfb for me either when it came to FIFO underruns. [...] >>>> + * are two known values: >>>> + * 1 - 128Byte >>>> + * 2 - 256Byte >>>> + * >>>> + * Downstream has set the burst size to 256Byte to improve the >>>> memory >>>> + * efficiency so set it here too. This also reduces the FIFO >>>> underrun >>>> + * possibility. >>>> + */ >>>> + ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) | >>>> + CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb- >>>>> pitches[0]); >>>> + writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3); >> >> Nit: I would write the register directly, instead of caching the value >> in ctrl. > > IMHO it's more readable that way. I agree, and we can also add to the variable in case there are more undocumented bits.
On 22-07-28, Liu Ying wrote: > On Wed, 2022-07-27 at 05:56 +0200, Marco Felsch wrote: > > Hi Marek, Liu, > > > > On 22-07-26, Liu Ying wrote: > > > On Tue, 2022-07-26 at 16:19 +0200, Marek Vasut wrote: > > > > On 7/26/22 11:43, Marco Felsch wrote: > > > > > FIFO underruns are seen if a AXI bus master with a higher > > > > > priority > > > > > do a > > > > > lot of memory access. Increase the burst size to 256B to avoid > > > > > such > > > > > underruns and to improve the memory access efficiency. > > > > > > > > Sigh, this again ... > > > > I know.. we also tried the PANIC mode but this somehow didn't worked > > as > > documented. So this was the only way to reduce the underruns without > > adapting the interconnect prio for the hdmi-lcdif. > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > index 1bec1279c8b5..1f22ea5896d5 100644 > > > > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > @@ -143,8 +143,20 @@ static void lcdif_set_mode(struct > > > > > lcdif_drm_private *lcdif, u32 bus_flags) > > > > > CTRLDESCL0_1_WIDTH(m->crtc_hdisplay), > > > > > lcdif->base + LCDC_V8_CTRLDESCL0_1); > > > > > > > > > > - writel(CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state- > > > > > >fb- > > > > > > pitches[0]), > > > > > > > > > > - lcdif->base + LCDC_V8_CTRLDESCL0_3); > > > > > + /* > > > > > + * Undocumented P_SIZE and T_SIZE bit fields but > > > > > according the > > > > > + * downstream kernel they control the AXI burst size. > > > > > As of now > > > > > there > > > > > > I'm not sure if it is AXI burst size or any other burst size, > > > though it > > > seems to be AXI burst size. > > > > > > Cc'ing Jian who mentioned 'burst size' and changed it from 128B to > > > 256B > > > in the downstream kernel. > > > > Thanks. > > Jian told me that it's AXI burst size. Thanks for asking him. Do you know anything about the PANIC mode? We tested it by: - using the interconnect patchsets [1] - added a patch for configuring the hdmi-lcdif interconnect via DT [not send upstream yet] - setting the PANIC threshold to thresh-low:1/2 and tresh-high:3/4 and enabled the INT_ENABLE_D1_PLANE_PANIC_EN within LCDC_V8_INT_ENABLE_D1 (like the downstream kernel) [no send upstream yet] - configured the 'LCDIF_NOC_HURRY' to 0x7 (highest prio) like you do within your downstream TF-A. But this didn't worked for us and for Marek if I got him correctly. My question is: Is the PANIC mode working as documented or are there some missing bits? You can test if the PANIC mode is working on the downstream kernel by reducing the AXI burst size back to 64B, connect a display with at least 1080P and do some heavy memory access. If panic mode is working you shouldn't see any display artifacts. I tested this before increasing the AXI burst size by setting the HDMI-LCDIF prio staticlly to a high prio like 0x5 and it was working with a 64B AXI burst size. Regards, Marco > Reviewed-by: Liu Ying <victor.liu@nxp.com> > >
On 22-07-28, Marco Felsch wrote: > On 22-07-28, Liu Ying wrote: > > On Wed, 2022-07-27 at 05:56 +0200, Marco Felsch wrote: > > > Hi Marek, Liu, > > > > > > On 22-07-26, Liu Ying wrote: > > > > On Tue, 2022-07-26 at 16:19 +0200, Marek Vasut wrote: > > > > > On 7/26/22 11:43, Marco Felsch wrote: > > > > > > FIFO underruns are seen if a AXI bus master with a higher > > > > > > priority > > > > > > do a > > > > > > lot of memory access. Increase the burst size to 256B to avoid > > > > > > such > > > > > > underruns and to improve the memory access efficiency. > > > > > > > > > > Sigh, this again ... > > > > > > I know.. we also tried the PANIC mode but this somehow didn't worked > > > as > > > documented. So this was the only way to reduce the underruns without > > > adapting the interconnect prio for the hdmi-lcdif. > > > > > > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > > index 1bec1279c8b5..1f22ea5896d5 100644 > > > > > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c > > > > > > @@ -143,8 +143,20 @@ static void lcdif_set_mode(struct > > > > > > lcdif_drm_private *lcdif, u32 bus_flags) > > > > > > CTRLDESCL0_1_WIDTH(m->crtc_hdisplay), > > > > > > lcdif->base + LCDC_V8_CTRLDESCL0_1); > > > > > > > > > > > > - writel(CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state- > > > > > > >fb- > > > > > > > pitches[0]), > > > > > > > > > > > > - lcdif->base + LCDC_V8_CTRLDESCL0_3); > > > > > > + /* > > > > > > + * Undocumented P_SIZE and T_SIZE bit fields but > > > > > > according the > > > > > > + * downstream kernel they control the AXI burst size. > > > > > > As of now > > > > > > there > > > > > > > > I'm not sure if it is AXI burst size or any other burst size, > > > > though it > > > > seems to be AXI burst size. > > > > > > > > Cc'ing Jian who mentioned 'burst size' and changed it from 128B to > > > > 256B > > > > in the downstream kernel. > > > > > > Thanks. > > > > Jian told me that it's AXI burst size. > > Thanks for asking him. Do you know anything about the PANIC mode? We > tested it by: > - using the interconnect patchsets [1] > - added a patch for configuring the hdmi-lcdif interconnect via DT [not > send upstream yet] > - setting the PANIC threshold to thresh-low:1/2 and tresh-high:3/4 and > enabled the INT_ENABLE_D1_PLANE_PANIC_EN within > LCDC_V8_INT_ENABLE_D1 (like the downstream kernel) [no send upstream > yet] > - configured the 'LCDIF_NOC_HURRY' to 0x7 (highest prio) like you do > within your downstream TF-A. > > But this didn't worked for us and for Marek if I got him correctly. My > question is: Is the PANIC mode working as documented or are there some > missing bits? > > You can test if the PANIC mode is working on the downstream kernel by > reducing the AXI burst size back to 64B, connect a display with at least > 1080P and do some heavy memory access. If panic mode is working you > shouldn't see any display artifacts. I tested this before increasing the > AXI burst size by setting the HDMI-LCDIF prio staticlly to a high prio > like 0x5 and it was working with a 64B AXI burst size. [1] https://lore.kernel.org/linux-arm-kernel/20220703091132.1412063-1-peng.fan@oss.nxp.com/ https://lore.kernel.org/all/20220708085632.1918323-1-peng.fan@oss.nxp.com/
On 7/26/22 11:43, Marco Felsch wrote: > FIFO underruns are seen if a AXI bus master with a higher priority do a > lot of memory access. Increase the burst size to 256B to avoid such > underruns and to improve the memory access efficiency. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> Uh, this fell through the cracks, sorry. Can you please collect my RB, add: Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") and send V2 ? btw. the Panic mode seems to work on this IP with 4k displays and HDMI.
Hi Marek, On 22-10-26, Marek Vasut wrote: > On 7/26/22 11:43, Marco Felsch wrote: > > FIFO underruns are seen if a AXI bus master with a higher priority do a > > lot of memory access. Increase the burst size to 256B to avoid such > > underruns and to improve the memory access efficiency. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > Uh, this fell through the cracks, sorry. > > Can you please collect my RB, add: > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > > and send V2 ? > > btw. the Panic mode seems to work on this IP with 4k displays and HDMI. Sounds interesting, how did you verified that? Regards, Marco
On 10/27/22 10:04, Marco Felsch wrote: > Hi Marek, Hi, > On 22-10-26, Marek Vasut wrote: >> On 7/26/22 11:43, Marco Felsch wrote: >>> FIFO underruns are seen if a AXI bus master with a higher priority do a >>> lot of memory access. Increase the burst size to 256B to avoid such >>> underruns and to improve the memory access efficiency. >>> >>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> >> >> Uh, this fell through the cracks, sorry. >> >> Can you please collect my RB, add: >> >> Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") >> >> and send V2 ? >> >> btw. the Panic mode seems to work on this IP with 4k displays and HDMI. > > Sounds interesting, how did you verified that? I plugged in a 4k panel into MX8MP HDMI port and saw this flicker, with the panic regs programmed correctly, there is no more flicker, see the commit messages of patch I posted yesterday for details, you were on CC: [PATCH] drm: lcdif: Set and enable FIFO Panic threshold
On 22-10-27, Marek Vasut wrote: > On 10/27/22 10:04, Marco Felsch wrote: > > Hi Marek, > > Hi, > > > On 22-10-26, Marek Vasut wrote: > > > On 7/26/22 11:43, Marco Felsch wrote: > > > > FIFO underruns are seen if a AXI bus master with a higher priority do a > > > > lot of memory access. Increase the burst size to 256B to avoid such > > > > underruns and to improve the memory access efficiency. > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > > > Uh, this fell through the cracks, sorry. > > > > > > Can you please collect my RB, add: > > > > > > Fixes: 9db35bb349a0 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") > > > > > > and send V2 ? > > > > > > btw. the Panic mode seems to work on this IP with 4k displays and HDMI. > > > > Sounds interesting, how did you verified that? > > I plugged in a 4k panel into MX8MP HDMI port and saw this flicker, with the > panic regs programmed correctly, there is no more flicker, see the commit > messages of patch I posted yesterday for details, you were on CC: > > [PATCH] drm: lcdif: Set and enable FIFO Panic threshold Yes, this was later in my inbox.. sorry for the noise. Regards, Marco
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c b/drivers/gpu/drm/mxsfb/lcdif_kms.c index 1bec1279c8b5..1f22ea5896d5 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c @@ -143,8 +143,20 @@ static void lcdif_set_mode(struct lcdif_drm_private *lcdif, u32 bus_flags) CTRLDESCL0_1_WIDTH(m->crtc_hdisplay), lcdif->base + LCDC_V8_CTRLDESCL0_1); - writel(CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]), - lcdif->base + LCDC_V8_CTRLDESCL0_3); + /* + * Undocumented P_SIZE and T_SIZE bit fields but according the + * downstream kernel they control the AXI burst size. As of now there + * are two known values: + * 1 - 128Byte + * 2 - 256Byte + * + * Downstream has set the burst size to 256Byte to improve the memory + * efficiency so set it here too. This also reduces the FIFO underrun + * possibility. + */ + ctrl = CTRLDESCL0_3_P_SIZE(2) | CTRLDESCL0_3_T_SIZE(2) | + CTRLDESCL0_3_PITCH(lcdif->crtc.primary->state->fb->pitches[0]); + writel(ctrl, lcdif->base + LCDC_V8_CTRLDESCL0_3); } static void lcdif_enable_controller(struct lcdif_drm_private *lcdif) diff --git a/drivers/gpu/drm/mxsfb/lcdif_regs.h b/drivers/gpu/drm/mxsfb/lcdif_regs.h index c70220651e3a..8e8bef175bf2 100644 --- a/drivers/gpu/drm/mxsfb/lcdif_regs.h +++ b/drivers/gpu/drm/mxsfb/lcdif_regs.h @@ -190,6 +190,10 @@ #define CTRLDESCL0_1_WIDTH(n) ((n) & 0xffff) #define CTRLDESCL0_1_WIDTH_MASK GENMASK(15, 0) +#define CTRLDESCL0_3_P_SIZE(n) (((n) << 20) & CTRLDESCL0_3_P_SIZE_MASK) +#define CTRLDESCL0_3_P_SIZE_MASK GENMASK(22, 20) +#define CTRLDESCL0_3_T_SIZE(n) (((n) << 16) & CTRLDESCL0_3_T_SIZE_MASK) +#define CTRLDESCL0_3_T_SIZE_MASK GENMASK(17, 16) #define CTRLDESCL0_3_PITCH(n) ((n) & 0xffff) #define CTRLDESCL0_3_PITCH_MASK GENMASK(15, 0)
FIFO underruns are seen if a AXI bus master with a higher priority do a lot of memory access. Increase the burst size to 256B to avoid such underruns and to improve the memory access efficiency. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/gpu/drm/mxsfb/lcdif_kms.c | 16 ++++++++++++++-- drivers/gpu/drm/mxsfb/lcdif_regs.h | 4 ++++ 2 files changed, 18 insertions(+), 2 deletions(-)