drm: xlnx: zynqmp_dpsub: Enable plane in atomic update

Commit Message

Klymenko, Anatoliy May 21, 2024, 3:22 a.m. UTC

Unconditionally enable the DPSUB layer in the corresponding atomic plane
update callback. Setting the new display mode may require disabling and
re-enabling the CRTC. This effectively resets DPSUB to the default state
with all layers disabled. The original implementation of the plane atomic
update enables the corresponding DPSUB layer only if the framebuffer
format has changed. This would leave the layer disabled after switching to
a different display mode with the same framebuffer format.

Signed-off-by: Anatoliy Klymenko <[e-mail]>
---
 drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)


---
base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
change-id: 20240520-dp-layer-enable-7b561af29ca8

Best regards,

Comments

Laurent Pinchart May 22, 2024, 3:31 p.m. UTC | #1

Hi Anatoliy,

Thank you for the patch.

On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko wrote:
> Unconditionally enable the DPSUB layer in the corresponding atomic plane
> update callback. Setting the new display mode may require disabling and
> re-enabling the CRTC. This effectively resets DPSUB to the default state
> with all layers disabled.

Ah, I went through the code and I see that. Oops.

> The original implementation of the plane atomic
> update enables the corresponding DPSUB layer only if the framebuffer
> format has changed. This would leave the layer disabled after switching to
> a different display mode with the same framebuffer format.

Do we need a Fixes: tag or has this issue been there since the beginning
?

> Signed-off-by: Anatoliy Klymenko <[e-mail]>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> index 43bf416b33d5..c4f038e34814 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> @@ -120,9 +120,8 @@ static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
>  		zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
>  						   plane->state->alpha >> 8);
>  
> -	/* Enable or re-enable the plane if the format has changed. */
> -	if (format_changed)
> -		zynqmp_disp_layer_enable(layer);
> +	/* Enable or re-enable the plane. */
> +	zynqmp_disp_layer_enable(layer);

This should be safe for now, as the function will just write the plane
registers with identical values. The waste of CPU cycles may not be a
big issue, even if it would be best to avoid it.

What bothers me more is that we may be working around a larger problem.
Resetting the CRTC when disabling it affects the hardware state of the
whole device, and thus the state of all software DRM objects. The
hardware and software states lose sync. Maybe this patch is all that is
needed for now, but other similar issues could pop up in the future.

Would it be possible, at atomic check time, to detect the objects whose
hardware state will need to be synced, and marked that in their state ?
Then in zynqmp_dpsub_plane_atomic_update() you could re-enable the layer
based on that. You may need to subclass the drm_plane_state if there's
no field in that structure that is suitable to store the information.
The format_changed local variable would move there.

>  }
>  
>  static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {
> 
> ---
> base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> change-id: 20240520-dp-layer-enable-7b561af29ca8

Klymenko, Anatoliy May 22, 2024, 5:52 p.m. UTC | #2

Hi Laurent,

Thanks a lot for the review.

> -----Original Message-----
> From: Laurent Pinchart <[e-mail]>
> Sent: Wednesday, May 22, 2024 8:32 AM
> To: Klymenko, Anatoliy <[e-mail]>
> Cc: Tomi Valkeinen <[e-mail]>; Maarten
> Lankhorst <[e-mail]>; Maxime Ripard
> <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>;
> Simek, Michal <[e-mail]>; dri-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic
> update
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> Hi Anatoliy,
> 
> Thank you for the patch.
> 
> On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko wrote:
> > Unconditionally enable the DPSUB layer in the corresponding atomic
> plane
> > update callback. Setting the new display mode may require disabling
> and
> > re-enabling the CRTC. This effectively resets DPSUB to the default state
> > with all layers disabled.
> 
> Ah, I went through the code and I see that. Oops.
> 
> > The original implementation of the plane atomic
> > update enables the corresponding DPSUB layer only if the framebuffer
> > format has changed. This would leave the layer disabled after switching
> to
> > a different display mode with the same framebuffer format.
> 
> Do we need a Fixes: tag or has this issue been there since the beginning
> ?

Yes, this was introduced in the initial driver commit.

> 
> > Signed-off-by: Anatoliy Klymenko <[e-mail]>
> > ---
> >  drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > index 43bf416b33d5..c4f038e34814 100644
> > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > @@ -120,9 +120,8 @@ static void
> zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> >               zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> >                                                  plane->state->alpha >> 8);
> >
> > -     /* Enable or re-enable the plane if the format has changed. */
> > -     if (format_changed)
> > -             zynqmp_disp_layer_enable(layer);
> > +     /* Enable or re-enable the plane. */
> > +     zynqmp_disp_layer_enable(layer);
> 
> This should be safe for now, as the function will just write the plane
> registers with identical values. The waste of CPU cycles may not be a
> big issue, even if it would be best to avoid it.
> 

The CPU time wasted on doubling down layer enablement is neglectable
compared to DP link training time.

> What bothers me more is that we may be working around a larger
> problem.
> Resetting the CRTC when disabling it affects the hardware state of the
> whole device, and thus the state of all software DRM objects. The
> hardware and software states lose sync. Maybe this patch is all that is
> needed for now, but other similar issues could pop up in the future.
> 

I had similar thoughts about proper HW state tracking, but that would be
rather large rework.

> Would it be possible, at atomic check time, to detect the objects whose
> hardware state will need to be synced, and marked that in their state ?
> Then in zynqmp_dpsub_plane_atomic_update() you could re-enable the
> layer
> based on that. You may need to subclass the drm_plane_state if there's
> no field in that structure that is suitable to store the information.
> The format_changed local variable would move there.
> 

Thank you for the idea! I'll check it out.

> >  }
> >
> >  static const struct drm_plane_helper_funcs
> zynqmp_dpsub_plane_helper_funcs = {
> >
> > ---
> > base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> > change-id: 20240520-dp-layer-enable-7b561af29ca8
> 
> --
> Regards,
> 
> Laurent Pinchart
--
Thank you,
Anatoliy

Laurent Pinchart May 22, 2024, 6:11 p.m. UTC | #3

On Wed, May 22, 2024 at 05:52:56PM +0000, Klymenko, Anatoliy wrote:
> On Wednesday, May 22, 2024 8:32 AM, Laurent Pinchart wrote:
> > On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko wrote:
> > > Unconditionally enable the DPSUB layer in the corresponding atomic plane
> > > update callback. Setting the new display mode may require disabling and
> > > re-enabling the CRTC. This effectively resets DPSUB to the default state
> > > with all layers disabled.
> > 
> > Ah, I went through the code and I see that. Oops.
> > 
> > > The original implementation of the plane atomic
> > > update enables the corresponding DPSUB layer only if the framebuffer
> > > format has changed. This would leave the layer disabled after switching to
> > > a different display mode with the same framebuffer format.
> > 
> > Do we need a Fixes: tag or has this issue been there since the beginning
> > ?
> 
> Yes, this was introduced in the initial driver commit.
> 
> > > Signed-off-by: Anatoliy Klymenko <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > index 43bf416b33d5..c4f038e34814 100644
> > > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > @@ -120,9 +120,8 @@ static void
> > zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> > >               zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> > >                                                  plane->state->alpha >> 8);
> > >
> > > -     /* Enable or re-enable the plane if the format has changed. */
> > > -     if (format_changed)
> > > -             zynqmp_disp_layer_enable(layer);
> > > +     /* Enable or re-enable the plane. */
> > > +     zynqmp_disp_layer_enable(layer);
> > 
> > This should be safe for now, as the function will just write the plane
> > registers with identical values. The waste of CPU cycles may not be a
> > big issue, even if it would be best to avoid it.
> 
> The CPU time wasted on doubling down layer enablement is neglectable
> compared to DP link training time.

Good point.

> > What bothers me more is that we may be working around a larger
> > problem.
> > Resetting the CRTC when disabling it affects the hardware state of the
> > whole device, and thus the state of all software DRM objects. The
> > hardware and software states lose sync. Maybe this patch is all that is
> > needed for now, but other similar issues could pop up in the future.
> 
> I had similar thoughts about proper HW state tracking, but that would be
> rather large rework.
> 
> > Would it be possible, at atomic check time, to detect the objects whose
> > hardware state will need to be synced, and marked that in their state ?
> > Then in zynqmp_dpsub_plane_atomic_update() you could re-enable the
> > layer
> > based on that. You may need to subclass the drm_plane_state if there's
> > no field in that structure that is suitable to store the information.
> > The format_changed local variable would move there.
> 
> Thank you for the idea! I'll check it out.

If it ends up being overkill I think this patch is probably OK. A
comment to explain the reasoning in the code would be nice though.

> > >  }
> > >
> > >  static const struct drm_plane_helper_funcs
> > zynqmp_dpsub_plane_helper_funcs = {
> > >
> > > ---
> > > base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> > > change-id: 20240520-dp-layer-enable-7b561af29ca8

Klymenko, Anatoliy May 22, 2024, 6:14 p.m. UTC | #4

> -----Original Message-----
> From: Laurent Pinchart <[e-mail]>
> Sent: Wednesday, May 22, 2024 11:11 AM
> To: Klymenko, Anatoliy <[e-mail]>
> Cc: Tomi Valkeinen <[e-mail]>; Maarten
> Lankhorst <[e-mail]>; Maxime Ripard
> <[e-mail]>; Thomas Zimmermann <[e-mail]>;
> David Airlie <[e-mail]>; Daniel Vetter <[e-mail]>;
> Simek, Michal <[e-mail]>; dri-
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: Re: [PATCH] drm: xlnx: zynqmp_dpsub: Enable plane in atomic
> update
> 
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
> 
> 
> On Wed, May 22, 2024 at 05:52:56PM +0000, Klymenko, Anatoliy wrote:
> > On Wednesday, May 22, 2024 8:32 AM, Laurent Pinchart wrote:
> > > On Mon, May 20, 2024 at 08:22:31PM -0700, Anatoliy Klymenko
> wrote:
> > > > Unconditionally enable the DPSUB layer in the corresponding
> atomic plane
> > > > update callback. Setting the new display mode may require
> disabling and
> > > > re-enabling the CRTC. This effectively resets DPSUB to the default
> state
> > > > with all layers disabled.
> > >
> > > Ah, I went through the code and I see that. Oops.
> > >
> > > > The original implementation of the plane atomic
> > > > update enables the corresponding DPSUB layer only if the
> framebuffer
> > > > format has changed. This would leave the layer disabled after
> switching to
> > > > a different display mode with the same framebuffer format.
> > >
> > > Do we need a Fixes: tag or has this issue been there since the
> beginning
> > > ?
> >
> > Yes, this was introduced in the initial driver commit.
> >
> > > > Signed-off-by: Anatoliy Klymenko <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/xlnx/zynqmp_kms.c | 5 ++---
> > > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > > index 43bf416b33d5..c4f038e34814 100644
> > > > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
> > > > @@ -120,9 +120,8 @@ static void
> > > zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
> > > >               zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
> > > >                                                  plane->state->alpha >> 8);
> > > >
> > > > -     /* Enable or re-enable the plane if the format has changed. */
> > > > -     if (format_changed)
> > > > -             zynqmp_disp_layer_enable(layer);
> > > > +     /* Enable or re-enable the plane. */
> > > > +     zynqmp_disp_layer_enable(layer);
> > >
> > > This should be safe for now, as the function will just write the plane
> > > registers with identical values. The waste of CPU cycles may not be a
> > > big issue, even if it would be best to avoid it.
> >
> > The CPU time wasted on doubling down layer enablement is
> neglectable
> > compared to DP link training time.
> 
> Good point.
> 
> > > What bothers me more is that we may be working around a larger
> > > problem.
> > > Resetting the CRTC when disabling it affects the hardware state of the
> > > whole device, and thus the state of all software DRM objects. The
> > > hardware and software states lose sync. Maybe this patch is all that is
> > > needed for now, but other similar issues could pop up in the future.
> >
> > I had similar thoughts about proper HW state tracking, but that would
> be
> > rather large rework.
> >
> > > Would it be possible, at atomic check time, to detect the objects
> whose
> > > hardware state will need to be synced, and marked that in their state
> ?
> > > Then in zynqmp_dpsub_plane_atomic_update() you could re-enable
> the
> > > layer
> > > based on that. You may need to subclass the drm_plane_state if
> there's
> > > no field in that structure that is suitable to store the information.
> > > The format_changed local variable would move there.
> >
> > Thank you for the idea! I'll check it out.
> 
> If it ends up being overkill I think this patch is probably OK. A
> comment to explain the reasoning in the code would be nice though.
> 

Sure, I'll add this.

> > > >  }
> > > >
> > > >  static const struct drm_plane_helper_funcs
> > > zynqmp_dpsub_plane_helper_funcs = {
> > > >
> > > > ---
> > > > base-commit: 673087d8b023faf34b84e8faf63bbeea3da87bab
> > > > change-id: 20240520-dp-layer-enable-7b561af29ca8
> 
> --
> Regards,
> 
> Laurent Pinchart
--
Thank you,
Anatoliy

Patch

diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c b/drivers/gpu/drm/xlnx/zynqmp_kms.c
index 43bf416b33d5..c4f038e34814 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_kms.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c
@@ -120,9 +120,8 @@  static void zynqmp_dpsub_plane_atomic_update(struct drm_plane *plane,
 		zynqmp_disp_blend_set_global_alpha(dpsub->disp, true,
 						   plane->state->alpha >> 8);
 
-	/* Enable or re-enable the plane if the format has changed. */
-	if (format_changed)
-		zynqmp_disp_layer_enable(layer);
+	/* Enable or re-enable the plane. */
+	zynqmp_disp_layer_enable(layer);
 }
 
 static const struct drm_plane_helper_funcs zynqmp_dpsub_plane_helper_funcs = {