On Tue, Feb 22, 2022 at 3:02 PM Maxime Ripard maxime@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@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@lists.freedesktop.org Cc: Ben Skeggs bskeggs@redhat.com Cc: Karol Herbst kherbst@redhat.com Cc: Lyude Paul lyude@redhat.com Signed-off-by: Maxime Ripard maxime@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@cerno.tech/ https://lore.kernel.org/dri-devel/20220221095918.18763-8-maxime@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@redhat.com
Maxime