Nouveau - Feb 2022 - [PATCH v2 13/22] drm/nouveau/kms: Remove redundant zpos initialisation

The nouveau KMS driver will call drm_plane_create_zpos_property() with
an init value depending on the plane purpose.

Since the initial value wasn't carried over in the state, the driver had
to set it again in nv50_wndw_reset(). However, the helpers have been
adjusted to set it properly at reset, so this is not needed anymore.

Cc: nouveau at lists.freedesktop.org
Cc: Ben Skeggs <bskeggs at redhat.com>
Cc: Karol Herbst <kherbst at redhat.com>
Cc: Lyude Paul <lyude at redhat.com>
Signed-off-by: Maxime Ripard <maxime at cerno.tech>
---
 drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
index 133c8736426a..0c1a2ea0ed04 100644
--- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
+++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
@@ -635,8 +635,6 @@ nv50_wndw_reset(struct drm_plane *plane)
 		plane->funcs->atomic_destroy_state(plane, plane->state);
 
 	__drm_atomic_helper_plane_reset(plane, &asyw->state);
-	plane->state->zpos = nv50_wndw_zpos_default(plane);
-	plane->state->normalized_zpos = nv50_wndw_zpos_default(plane);
 }
 
 static void
-- 
2.35.1

On Mon, Feb 21, 2022 at 11:00 AM Maxime Ripard <maxime at cerno.tech>
wrote:
>
> The nouveau KMS driver will call drm_plane_create_zpos_property() with
> an init value depending on the plane purpose.
>
> Since the initial value wasn't carried over in the state, the driver
had
> to set it again in nv50_wndw_reset(). However, the helpers have been
> adjusted to set it properly at reset, so this is not needed anymore.
>
> Cc: nouveau at lists.freedesktop.org
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Karol Herbst <kherbst at redhat.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> ---
>  drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> index 133c8736426a..0c1a2ea0ed04 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> @@ -635,8 +635,6 @@ nv50_wndw_reset(struct drm_plane *plane)
>                 plane->funcs->atomic_destroy_state(plane,
plane->state);
>
>         __drm_atomic_helper_plane_reset(plane, &asyw->state);
> -       plane->state->zpos = nv50_wndw_zpos_default(plane);
> -       plane->state->normalized_zpos =
nv50_wndw_zpos_default(plane);
so reading the surrounding code a little it feels like those
assignments actually do something. If my understanding is correct
plane->state points to &asyw->state, but asyw was just kzalloced in
this function. __drm_atomic_helper_plane_reset doesn't set the zpos or
normalized_zpos fields as long as zpos_property is 0, so those fields
won't be set with that change anymore.

I just don't know if it's fine like that or if this function should
set zpos_property instead or something. Anyway, the commit description
makes it sound like that an unneeded assignment would be removed here,
which doesn't seem to be the case. But I don't really know much about
all the drm API interactions, so it might just be fine, mostly asking
to get a better idea on how all those pieces fit together.
>  }
>
>  static void

> --
> 2.35.1
>

Reviewed-by: Lyude Paul <lyude at redhat.com>

I assume you'll handle pushing this yourself? (JFYI - using drm-misc for
pushing this is fine by me)

On Mon, 2022-02-21 at 10:59 +0100, Maxime Ripard wrote:
> The nouveau KMS driver will call drm_plane_create_zpos_property() with
> an init value depending on the plane purpose.
> 
> Since the initial value wasn't carried over in the state, the driver
had
> to set it again in nv50_wndw_reset(). However, the helpers have been
> adjusted to set it properly at reset, so this is not needed anymore.
> 
> Cc: nouveau at lists.freedesktop.org
> Cc: Ben Skeggs <bskeggs at redhat.com>
> Cc: Karol Herbst <kherbst at redhat.com>
> Cc: Lyude Paul <lyude at redhat.com>
> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
> ---
> ?drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 --
> ?1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> index 133c8736426a..0c1a2ea0ed04 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> @@ -635,8 +635,6 @@ nv50_wndw_reset(struct drm_plane *plane)
> ????????????????plane->funcs->atomic_destroy_state(plane,
plane->state);
> ?
> ????????__drm_atomic_helper_plane_reset(plane, &asyw->state);
> -???????plane->state->zpos = nv50_wndw_zpos_default(plane);
> -???????plane->state->normalized_zpos =
nv50_wndw_zpos_default(plane);
> ?}
> ?
> ?static void
-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

On Mon, 21 Feb 2022 10:59:09 +0100, Maxime Ripard wrote:
> The nouveau KMS driver will call drm_plane_create_zpos_property() with
> an init value depending on the plane purpose.
> 
> Since the initial value wasn't carried over in the state, the driver
had
> to set it again in nv50_wndw_reset(). However, the helpers have been
> adjusted to set it properly at reset, so this is not needed anymore.
> 
> [...]
Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime