| Message ID | 20210817232604.160029-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/damage_helper: Fix handling of cursor dirty buffers | expand |
On Tue, Aug 17, 2021 at 04:26:04PM -0700, José Roberto de Souza wrote: > Cursors don't have a framebuffer so the fb comparisson was always > failing and atomic state was being committed without any plane state. > > So here checking if objects match when checking cursors. This looks extremely backwards ... what exactly is this fixing? If this isn't based on a real world compositor usage but some igt, then I'd say the igt here is very wrong. -Daniel > Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Rob Clark <robdclark@gmail.com> > Cc: Deepak Rawat <drawat@vmware.com> > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/drm_damage_helper.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c > index 8eeff0c7bdd47..595187d97c131 100644 > --- a/drivers/gpu/drm/drm_damage_helper.c > +++ b/drivers/gpu/drm/drm_damage_helper.c > @@ -157,12 +157,18 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, > retry: > drm_for_each_plane(plane, fb->dev) { > struct drm_plane_state *plane_state; > + bool match; > > ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); > if (ret) > goto out; > > - if (plane->state->fb != fb) { > + match = plane->state->fb == fb; > + /* Check if objs match to handle dirty buffers of cursors */ > + if (plane->type == DRM_PLANE_TYPE_CURSOR && plane->state->fb) > + match |= fb->obj[0] == plane->state->fb->obj[0]; > + > + if (!match) { > drm_modeset_unlock(&plane->mutex); > continue; > } > -- > 2.32.0 >
On Wed, 2021-08-18 at 11:54 +0200, Daniel Vetter wrote: > On Tue, Aug 17, 2021 at 04:26:04PM -0700, José Roberto de Souza wrote: > > Cursors don't have a framebuffer so the fb comparisson was always > > failing and atomic state was being committed without any plane state. > > > > So here checking if objects match when checking cursors. > > This looks extremely backwards ... what exactly is this fixing? If this > isn't based on a real world compositor usage but some igt, then I'd say > the igt here is very wrong. Yes it is IGT. Writing to cursor buffer current in the screen and calling drmModeDirtyFB() causes a empty atomic commit by drm_atomic_helper_dirtyfb(). > -Daniel > > > Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Rob Clark <robdclark@gmail.com> > > Cc: Deepak Rawat <drawat@vmware.com> > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/drm_damage_helper.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c > > index 8eeff0c7bdd47..595187d97c131 100644 > > --- a/drivers/gpu/drm/drm_damage_helper.c > > +++ b/drivers/gpu/drm/drm_damage_helper.c > > @@ -157,12 +157,18 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, > > retry: > > drm_for_each_plane(plane, fb->dev) { > > struct drm_plane_state *plane_state; > > + bool match; > > > > ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); > > if (ret) > > goto out; > > > > - if (plane->state->fb != fb) { > > + match = plane->state->fb == fb; > > + /* Check if objs match to handle dirty buffers of cursors */ > > + if (plane->type == DRM_PLANE_TYPE_CURSOR && plane->state->fb) > > + match |= fb->obj[0] == plane->state->fb->obj[0]; > > + > > + if (!match) { > > drm_modeset_unlock(&plane->mutex); > > continue; > > } > > -- > > 2.32.0 > > >
On Wed, Aug 18, 2021 at 04:44:44PM +0000, Souza, Jose wrote: > On Wed, 2021-08-18 at 11:54 +0200, Daniel Vetter wrote: > > On Tue, Aug 17, 2021 at 04:26:04PM -0700, José Roberto de Souza wrote: > > > Cursors don't have a framebuffer so the fb comparisson was always > > > failing and atomic state was being committed without any plane state. > > > > > > So here checking if objects match when checking cursors. > > > > This looks extremely backwards ... what exactly is this fixing? If this > > isn't based on a real world compositor usage but some igt, then I'd say > > the igt here is very wrong. > > Yes it is IGT. > Writing to cursor buffer current in the screen and calling > drmModeDirtyFB() causes a empty atomic commit by > drm_atomic_helper_dirtyfb(). Ok if the cursor write is done through legacy cursor uapi then trying to make that work with dirtyfb doesn't make sense. But you've found a bug at least, namely the empty commit. I think that should be filtered out, and our dirtyfb testcases (I hope we have some vendor-agnostic kms_dirtyfb already) should be extended with a testcase where we call dirtyfb on an fb which is not currently used anywhere. Wrt the cursor: The legacy cursor ioctls get remapped onto the cursor plane, which means they come in as full commits. So there's really not dirtyfb required afterwards. The funny thing about the cursor ioctls is that they never supported frontbuffer rendering. You _always_ had to call them to upload data. So the test is invalid from a functional pov too. -Daniel > > > > -Daniel > > > > > Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Rob Clark <robdclark@gmail.com> > > > Cc: Deepak Rawat <drawat@vmware.com> > > > Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/drm_damage_helper.c | 8 +++++++- > > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c > > > index 8eeff0c7bdd47..595187d97c131 100644 > > > --- a/drivers/gpu/drm/drm_damage_helper.c > > > +++ b/drivers/gpu/drm/drm_damage_helper.c > > > @@ -157,12 +157,18 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, > > > retry: > > > drm_for_each_plane(plane, fb->dev) { > > > struct drm_plane_state *plane_state; > > > + bool match; > > > > > > ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); > > > if (ret) > > > goto out; > > > > > > - if (plane->state->fb != fb) { > > > + match = plane->state->fb == fb; > > > + /* Check if objs match to handle dirty buffers of cursors */ > > > + if (plane->type == DRM_PLANE_TYPE_CURSOR && plane->state->fb) > > > + match |= fb->obj[0] == plane->state->fb->obj[0]; > > > + > > > + if (!match) { > > > drm_modeset_unlock(&plane->mutex); > > > continue; > > > } > > > -- > > > 2.32.0 > > > > > >
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c index 8eeff0c7bdd47..595187d97c131 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -157,12 +157,18 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, retry: drm_for_each_plane(plane, fb->dev) { struct drm_plane_state *plane_state; + bool match; ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); if (ret) goto out; - if (plane->state->fb != fb) { + match = plane->state->fb == fb; + /* Check if objs match to handle dirty buffers of cursors */ + if (plane->type == DRM_PLANE_TYPE_CURSOR && plane->state->fb) + match |= fb->obj[0] == plane->state->fb->obj[0]; + + if (!match) { drm_modeset_unlock(&plane->mutex); continue; }
Cursors don't have a framebuffer so the fb comparisson was always failing and atomic state was being committed without any plane state. So here checking if objects match when checking cursors. Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Rob Clark <robdclark@gmail.com> Cc: Deepak Rawat <drawat@vmware.com> Cc: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/drm_damage_helper.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)