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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > > Cc: Ben Skeggs <bskeggs@xxxxxxxxxx> > > Cc: Karol Herbst <kherbst@xxxxxxxxxx> > > Cc: Lyude Paul <lyude@xxxxxxxxxx> > > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx/ https://lore.kernel.org/dri-devel/20220221095918.18763-8-maxime@xxxxxxxxxx/ So, once they have been applied those assignments are made in __drm_atomic_helper_plane_reset and are no longer relevant here. Maxime
Attachment:
signature.asc
Description: PGP signature
