RE: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush
On Thursday, September 21, 2023 4:57 PM Lucas Stach
wrote:
> Am Donnerstag, dem 21.09.2023 um 08:18 + schrieb Ying Liu:
> > On Thursday, September 21, 2023 3:55 PM Lucas Stach
> wrote:
> > > Am Donnerstag, dem 21.09.2023 um 07:13 + schrieb Ying Liu:
> > > > Hi,
> > > >
> > > > On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> > > wrote:
> > > > > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > > > > state before the scanout is started.
> > > > >
> > > > > Signed-off-by: Lucas Stach
> > > > > ---
> > > > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +-
> > > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > index f5bfe8b52920..4acf6914a8d1 100644
> > > > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct
> drm_crtc
> > > > > *crtc,
> > > > > static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> > > > > struct drm_atomic_state *state)
> > > > > {
> > > > > + struct drm_crtc_state *crtc_state =
> > > > > drm_atomic_get_new_crtc_state(state,
> > > > > +
> crtc);
> > > > > struct lcdif_drm_private *lcdif =
> > > > > to_lcdif_drm_private(crtc->dev);
> > > > > struct drm_pending_vblank_event *event;
> > > > > u32 reg;
> > > > > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct
> drm_crtc
> > > > > *crtc,
> > > > > reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > > > > writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > > > >
> > > > > + if (drm_atomic_crtc_needs_modeset(crtc_state))
> > > > > + lcdif_enable_controller(lcdif);
> > > >
> > > > Moving lcdif_enable_controller() function call from atomic_enable to
> > > > atomic_flush would change CRTC vs bridge enablement order, which
> > > > effectively makes bridge enablement happen prior to CRTC enablement,
> > > > see drm_atomic_helper_commit_tail_rpm() detail implementation. The
> > > > reversed order potentially causes malfunctions of the bridge.
> > > >
> > > This has nothing to do with the bridge after the LCDIF controller. The
> >
> > IMHO, the reserved CRTC vs bridge enablement order is relevant for
> > the overall display pipeline.
> >
> Ah, I see what you mean now. Moving the controller enable into flush is
> violating the documented flow that bridges might assume the display
> link to be running in their enable callback. I don't think any of the
> bridges connected to the LCDIF really care about this, but it's bad
> nevertheless.
Not sure if the first bridges(all in SoCs for now) connected to LCDIF care
about the order. But, it's really about the overall display pipeline including
wild out-of-SoC bridges in a chain.
>
> This seems to be a generic issue with the RPM commit tail. Enabling the
> display link in the CRTC atomic_enable without all the plane state
> being set up in the HW sounds wrong and I think we also don't want to
Right, seems to be a generic issue.
Regards,
Liu Ying
> hoist (duplicate) all the plane setup into the CRTC enable.
>
> I'll look into this some more to see if we can do something better that
> doesn't violate the bridge chain constraints.
>
> Regards,
> Lucas
>
> > > RPM commit_tail implementation enables the CRTC before all the plane
> > > state is set up. To avoid having to program the plane state in the CRTC
> > > enable this series defers the actual controller enable to the last step
> > > of the atomic commit. This way the plane state is programmed the usual
> > > way via the atomic update_plane callback and we don't need to duplicate
> > > this setup.
> >
> > I understand the patch avoids some duplications via deferral controller
> > enablement time point, but the reversed CRTC vs bridge enablement order
> > is the concern here.
> >
> > >
> > > > BTW, even if it's ok to move the function call, it would be better to
> > > > call
> > > > lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> > > > set so that the original sequence is kept. Also, LCDIF chapter in SoC
> > > > RMs
> > > > indicates that the shadow load enablement is the last step in
> > > > "Software flow diagram".
> > >
> > > This flow chart shows how the double buffered update should work, it
> > > doesn't show the initial controller enable sequence. Without the shadow
> >
> > You are right. The downstream driver also enables shadow load before
> > the entire controller.
> >
> > Regards,
> > Liu Ying
> >
> > > load enable bit being set before the controller enable I could observe
> > > sporadic issues on the first frame where the DMA engine would read the
> > > wrong memory address.
> > >
> > > Regards,
> > > Lucas
> > >
> > > >
> > > > Regards,
> > > > Liu Ying
> > > >
> > > > > +
> > > > > event = crtc->state->event;
> > > > > crtc->state->event
Re: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush
Am Donnerstag, dem 21.09.2023 um 08:18 + schrieb Ying Liu:
> On Thursday, September 21, 2023 3:55 PM Lucas Stach
> wrote:
> > Am Donnerstag, dem 21.09.2023 um 07:13 + schrieb Ying Liu:
> > > Hi,
> > >
> > > On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> > wrote:
> > > > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > > > state before the scanout is started.
> > > >
> > > > Signed-off-by: Lucas Stach
> > > > ---
> > > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > index f5bfe8b52920..4acf6914a8d1 100644
> > > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> > > > *crtc,
> > > > static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> > > > struct drm_atomic_state *state)
> > > > {
> > > > + struct drm_crtc_state *crtc_state =
> > > > drm_atomic_get_new_crtc_state(state,
> > > > +
> > > > crtc);
> > > > struct lcdif_drm_private *lcdif =
> > > > to_lcdif_drm_private(crtc->dev);
> > > > struct drm_pending_vblank_event *event;
> > > > u32 reg;
> > > > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> > > > *crtc,
> > > > reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > > > writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > > >
> > > > + if (drm_atomic_crtc_needs_modeset(crtc_state))
> > > > + lcdif_enable_controller(lcdif);
> > >
> > > Moving lcdif_enable_controller() function call from atomic_enable to
> > > atomic_flush would change CRTC vs bridge enablement order, which
> > > effectively makes bridge enablement happen prior to CRTC enablement,
> > > see drm_atomic_helper_commit_tail_rpm() detail implementation. The
> > > reversed order potentially causes malfunctions of the bridge.
> > >
> > This has nothing to do with the bridge after the LCDIF controller. The
>
> IMHO, the reserved CRTC vs bridge enablement order is relevant for
> the overall display pipeline.
>
Ah, I see what you mean now. Moving the controller enable into flush is
violating the documented flow that bridges might assume the display
link to be running in their enable callback. I don't think any of the
bridges connected to the LCDIF really care about this, but it's bad
nevertheless.
This seems to be a generic issue with the RPM commit tail. Enabling the
display link in the CRTC atomic_enable without all the plane state
being set up in the HW sounds wrong and I think we also don't want to
hoist (duplicate) all the plane setup into the CRTC enable.
I'll look into this some more to see if we can do something better that
doesn't violate the bridge chain constraints.
Regards,
Lucas
> > RPM commit_tail implementation enables the CRTC before all the plane
> > state is set up. To avoid having to program the plane state in the CRTC
> > enable this series defers the actual controller enable to the last step
> > of the atomic commit. This way the plane state is programmed the usual
> > way via the atomic update_plane callback and we don't need to duplicate
> > this setup.
>
> I understand the patch avoids some duplications via deferral controller
> enablement time point, but the reversed CRTC vs bridge enablement order
> is the concern here.
>
> >
> > > BTW, even if it's ok to move the function call, it would be better to call
> > > lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> > > set so that the original sequence is kept. Also, LCDIF chapter in SoC RMs
> > > indicates that the shadow load enablement is the last step in
> > > "Software flow diagram".
> >
> > This flow chart shows how the double buffered update should work, it
> > doesn't show the initial controller enable sequence. Without the shadow
>
> You are right. The downstream driver also enables shadow load before
> the entire controller.
>
> Regards,
> Liu Ying
>
> > load enable bit being set before the controller enable I could observe
> > sporadic issues on the first frame where the DMA engine would read the
> > wrong memory address.
> >
> > Regards,
> > Lucas
> >
> > >
> > > Regards,
> > > Liu Ying
> > >
> > > > +
> > > > event = crtc->state->event;
> > > > crtc->state->event = NULL;
> > > >
> > > > @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct
> > drm_crtc
> > > > *crtc,
> > > >
> > > > writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> > > >lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> > > > }
> > > > - lcdif_enable_controller(lcdif);
> > > >
> > > > drm_crtc_vblank_on(crtc);
> > > > }
> > > > --
> > > > 2.39.2
> > >
>
RE: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush
On Thursday, September 21, 2023 3:55 PM Lucas Stach
wrote:
> Am Donnerstag, dem 21.09.2023 um 07:13 + schrieb Ying Liu:
> > Hi,
> >
> > On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> wrote:
> > > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > > state before the scanout is started.
> > >
> > > Signed-off-by: Lucas Stach
> > > ---
> > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > index f5bfe8b52920..4acf6914a8d1 100644
> > > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> > > *crtc,
> > > static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> > > struct drm_atomic_state *state)
> > > {
> > > + struct drm_crtc_state *crtc_state =
> > > drm_atomic_get_new_crtc_state(state,
> > > + crtc);
> > > struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
> > > struct drm_pending_vblank_event *event;
> > > u32 reg;
> > > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> > > *crtc,
> > > reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > > writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> > >
> > > + if (drm_atomic_crtc_needs_modeset(crtc_state))
> > > + lcdif_enable_controller(lcdif);
> >
> > Moving lcdif_enable_controller() function call from atomic_enable to
> > atomic_flush would change CRTC vs bridge enablement order, which
> > effectively makes bridge enablement happen prior to CRTC enablement,
> > see drm_atomic_helper_commit_tail_rpm() detail implementation. The
> > reversed order potentially causes malfunctions of the bridge.
> >
> This has nothing to do with the bridge after the LCDIF controller. The
IMHO, the reserved CRTC vs bridge enablement order is relevant for
the overall display pipeline.
> RPM commit_tail implementation enables the CRTC before all the plane
> state is set up. To avoid having to program the plane state in the CRTC
> enable this series defers the actual controller enable to the last step
> of the atomic commit. This way the plane state is programmed the usual
> way via the atomic update_plane callback and we don't need to duplicate
> this setup.
I understand the patch avoids some duplications via deferral controller
enablement time point, but the reversed CRTC vs bridge enablement order
is the concern here.
>
> > BTW, even if it's ok to move the function call, it would be better to call
> > lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> > set so that the original sequence is kept. Also, LCDIF chapter in SoC RMs
> > indicates that the shadow load enablement is the last step in
> > "Software flow diagram".
>
> This flow chart shows how the double buffered update should work, it
> doesn't show the initial controller enable sequence. Without the shadow
You are right. The downstream driver also enables shadow load before
the entire controller.
Regards,
Liu Ying
> load enable bit being set before the controller enable I could observe
> sporadic issues on the first frame where the DMA engine would read the
> wrong memory address.
>
> Regards,
> Lucas
>
> >
> > Regards,
> > Liu Ying
> >
> > > +
> > > event = crtc->state->event;
> > > crtc->state->event = NULL;
> > >
> > > @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct
> drm_crtc
> > > *crtc,
> > >
> > > writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> > > lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> > > }
> > > - lcdif_enable_controller(lcdif);
> > >
> > > drm_crtc_vblank_on(crtc);
> > > }
> > > --
> > > 2.39.2
> >
Re: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush
Am Donnerstag, dem 21.09.2023 um 07:13 + schrieb Ying Liu:
> Hi,
>
> On Wednesday, September 20, 2023 6:31 PM Lucas Stach
> wrote:
> > Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> > state before the scanout is started.
> >
> > Signed-off-by: Lucas Stach
> > ---
> > drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > index f5bfe8b52920..4acf6914a8d1 100644
> > --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> > @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> > *crtc,
> > static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> > struct drm_atomic_state *state)
> > {
> > + struct drm_crtc_state *crtc_state =
> > drm_atomic_get_new_crtc_state(state,
> > + crtc);
> > struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
> > struct drm_pending_vblank_event *event;
> > u32 reg;
> > @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> > reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> > writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
> >
> > + if (drm_atomic_crtc_needs_modeset(crtc_state))
> > + lcdif_enable_controller(lcdif);
>
> Moving lcdif_enable_controller() function call from atomic_enable to
> atomic_flush would change CRTC vs bridge enablement order, which
> effectively makes bridge enablement happen prior to CRTC enablement,
> see drm_atomic_helper_commit_tail_rpm() detail implementation. The
> reversed order potentially causes malfunctions of the bridge.
>
This has nothing to do with the bridge after the LCDIF controller. The
RPM commit_tail implementation enables the CRTC before all the plane
state is set up. To avoid having to program the plane state in the CRTC
enable this series defers the actual controller enable to the last step
of the atomic commit. This way the plane state is programmed the usual
way via the atomic update_plane callback and we don't need to duplicate
this setup.
> BTW, even if it's ok to move the function call, it would be better to call
> lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
> set so that the original sequence is kept. Also, LCDIF chapter in SoC RMs
> indicates that the shadow load enablement is the last step in
> "Software flow diagram".
This flow chart shows how the double buffered update should work, it
doesn't show the initial controller enable sequence. Without the shadow
load enable bit being set before the controller enable I could observe
sporadic issues on the first frame where the DMA engine would read the
wrong memory address.
Regards,
Lucas
>
> Regards,
> Liu Ying
>
> > +
> > event = crtc->state->event;
> > crtc->state->event = NULL;
> >
> > @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> > *crtc,
> >
> > writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> >lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> > }
> > - lcdif_enable_controller(lcdif);
> >
> > drm_crtc_vblank_on(crtc);
> > }
> > --
> > 2.39.2
>
RE: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush
Hi,
On Wednesday, September 20, 2023 6:31 PM Lucas Stach
wrote:
> Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
> state before the scanout is started.
>
> Signed-off-by: Lucas Stach
> ---
> drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> index f5bfe8b52920..4acf6914a8d1 100644
> --- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
> +++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
> @@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc
> *crtc,
> static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
> struct drm_atomic_state *state)
> {
> + struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state,
> + crtc);
> struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
> struct drm_pending_vblank_event *event;
> u32 reg;
> @@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc
> *crtc,
> reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
> writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
>
> + if (drm_atomic_crtc_needs_modeset(crtc_state))
> + lcdif_enable_controller(lcdif);
Moving lcdif_enable_controller() function call from atomic_enable to
atomic_flush would change CRTC vs bridge enablement order, which
effectively makes bridge enablement happen prior to CRTC enablement,
see drm_atomic_helper_commit_tail_rpm() detail implementation. The
reversed order potentially causes malfunctions of the bridge.
BTW, even if it's ok to move the function call, it would be better to call
lcdif_enable_controller() before CTRLDESCL0_5_SHADOW_LOAD_EN is
set so that the original sequence is kept. Also, LCDIF chapter in SoC RMs
indicates that the shadow load enablement is the last step in
"Software flow diagram".
Regards,
Liu Ying
> +
> event = crtc->state->event;
> crtc->state->event = NULL;
>
> @@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc
> *crtc,
>
> writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
> lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
> }
> - lcdif_enable_controller(lcdif);
>
> drm_crtc_vblank_on(crtc);
> }
> --
> 2.39.2
Re: [PATCH 2/5] drm: lcdif: move controller enable into atomic_flush
On 9/20/23 12:31, Lucas Stach wrote:
Allow drm_atomic_helper_commit_tail_rpm to setup all the plane
state before the scanout is started.
Signed-off-by: Lucas Stach
---
drivers/gpu/drm/mxsfb/lcdif_kms.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mxsfb/lcdif_kms.c
b/drivers/gpu/drm/mxsfb/lcdif_kms.c
index f5bfe8b52920..4acf6914a8d1 100644
--- a/drivers/gpu/drm/mxsfb/lcdif_kms.c
+++ b/drivers/gpu/drm/mxsfb/lcdif_kms.c
@@ -505,6 +505,8 @@ static int lcdif_crtc_atomic_check(struct drm_crtc *crtc,
static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state,
+ crtc);
struct lcdif_drm_private *lcdif = to_lcdif_drm_private(crtc->dev);
struct drm_pending_vblank_event *event;
u32 reg;
@@ -513,6 +515,9 @@ static void lcdif_crtc_atomic_flush(struct drm_crtc *crtc,
reg |= CTRLDESCL0_5_SHADOW_LOAD_EN;
writel(reg, lcdif->base + LCDC_V8_CTRLDESCL0_5);
+ if (drm_atomic_crtc_needs_modeset(crtc_state))
+ lcdif_enable_controller(lcdif);
+
event = crtc->state->event;
crtc->state->event = NULL;
@@ -552,7 +557,6 @@ static void lcdif_crtc_atomic_enable(struct drm_crtc *crtc,
writel(CTRLDESCL_HIGH0_4_ADDR_HIGH(upper_32_bits(paddr)),
lcdif->base + LCDC_V8_CTRLDESCL_HIGH0_4);
}
- lcdif_enable_controller(lcdif);
drm_crtc_vblank_on(crtc);
}
Reviewed-by: Marek Vasut
