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

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
>

Hi,

On Mon, Feb 21, 2022 at 05:42:36PM +0100, Karol Herbst
wrote:
> 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.
If you're looking at the code without that patch series, you're right.

These patches change that however:
https://lore.kernel.org/dri-devel/20220221095918.18763-7-maxime at cerno.tech/
https://lore.kernel.org/dri-devel/20220221095918.18763-8-maxime at cerno.tech/

So, once they have been applied those assignments are made in
__drm_atomic_helper_plane_reset and are no longer relevant here.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220222/f2e22ebf/attachment-0001.sig>

On Tue, Feb 22, 2022 at 3:02 PM Maxime Ripard <maxime at cerno.tech>
wrote:
>
> Hi,
>
> On Mon, Feb 21, 2022 at 05:42:36PM +0100, Karol Herbst wrote:
> > 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.
>
> If you're looking at the code without that patch series, you're
right.
>
> These patches change that however:
> https://lore.kernel.org/dri-devel/20220221095918.18763-7-maxime at
cerno.tech/
> https://lore.kernel.org/dri-devel/20220221095918.18763-8-maxime at
cerno.tech/
>
> So, once they have been applied those assignments are made in
> __drm_atomic_helper_plane_reset and are no longer relevant here.
>
yeah, I saw those, but I see now where I got confused: the arguments
of __drm_atomic_helper_plane_reset and
__drm_atomic_helper_plane_state_reset are swapped, so I thought
&asyw->state being all 0 was the second arg to
__drm_atomic_helper_plane_state_reset. Yeah the code is alright then.

Reviewed-by: Karol Herbst <kherbst at redhat.com>
> Maxime