From: Sean Paul seanpaul@chromium.org
Since the dirtyfb ioctl doesn't give us any hints as to which plane is scanning out the fb it's marking as damaged, we need to loop through planes to find it.
Currently we just reach into plane state and check, but that can race with another commit changing the fb out from under us. This patch locks the plane before checking the fb and will release the lock if the plane is not displaying the dirty fb.
Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") Cc: Rob Clark robdclark@gmail.com Cc: Deepak Rawat drawat@vmware.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Thomas Hellstrom thellstrom@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.0+ Reported-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Sean Paul seanpaul@chromium.org --- 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 8230dac01a89..3a4126dc2520 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -212,8 +212,14 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, drm_for_each_plane(plane, fb->dev) { struct drm_plane_state *plane_state;
- if (plane->state->fb != fb) + ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); + if (ret) + goto out; + + if (plane->state->fb != fb) { + drm_modeset_unlock(&plane->mutex); continue; + }
plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) {
On Wed, Sep 4, 2019 at 10:29 PM Sean Paul sean@poorly.run wrote:
From: Sean Paul seanpaul@chromium.org
Since the dirtyfb ioctl doesn't give us any hints as to which plane is scanning out the fb it's marking as damaged, we need to loop through planes to find it.
Currently we just reach into plane state and check, but that can race with another commit changing the fb out from under us. This patch locks the plane before checking the fb and will release the lock if the plane is not displaying the dirty fb.
Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") Cc: Rob Clark robdclark@gmail.com Cc: Deepak Rawat drawat@vmware.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Thomas Hellstrom thellstrom@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.0+ Reported-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Sean Paul seanpaul@chromium.org
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 8230dac01a89..3a4126dc2520 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -212,8 +212,14 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, drm_for_each_plane(plane, fb->dev) { struct drm_plane_state *plane_state;
if (plane->state->fb != fb)
ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);if (ret)
I think for paranoid safety we should have a WARN_ON(ret == -EALREADY) here. It should be impossible, but if it's not for some oddball reason, we'll blow up.
With that: Reviewed-by: Daniel Vetter daniel@ffwll.ch
But please give this a spin with some workloads and the ww_mutex slowpath debugging enabled, just to makre sure. -Daniel
goto out;if (plane->state->fb != fb) {drm_modeset_unlock(&plane->mutex); continue;} plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) {-- Sean Paul, Software Engineer, Google / Chromium OS
On Thu, Sep 05, 2019 at 12:41:27PM +0200, Daniel Vetter wrote:
On Wed, Sep 4, 2019 at 10:29 PM Sean Paul sean@poorly.run wrote:
From: Sean Paul seanpaul@chromium.org
Since the dirtyfb ioctl doesn't give us any hints as to which plane is scanning out the fb it's marking as damaged, we need to loop through planes to find it.
Currently we just reach into plane state and check, but that can race with another commit changing the fb out from under us. This patch locks the plane before checking the fb and will release the lock if the plane is not displaying the dirty fb.
Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") Cc: Rob Clark robdclark@gmail.com Cc: Deepak Rawat drawat@vmware.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Thomas Hellstrom thellstrom@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.0+ Reported-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Sean Paul seanpaul@chromium.org
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 8230dac01a89..3a4126dc2520 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -212,8 +212,14 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, drm_for_each_plane(plane, fb->dev) { struct drm_plane_state *plane_state;
if (plane->state->fb != fb)
ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);if (ret)I think for paranoid safety we should have a WARN_ON(ret == -EALREADY) here. It should be impossible, but if it's not for some oddball reason, we'll blow up.
drm_modeset_lock eats EALREADY and returns 0 for that case, so I guess it depends _how_ paranoid you want to be here :-)
With that: Reviewed-by: Daniel Vetter daniel@ffwll.ch
But please give this a spin with some workloads and the ww_mutex slowpath debugging enabled, just to makre sure.
Ok, had a chance to run through some tests this morning with CONFIG_DEBUG_WW_MUTEX_SLOWPATH and things lgtm
Sean
-Daniel
goto out;if (plane->state->fb != fb) {drm_modeset_unlock(&plane->mutex); continue;} plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) {-- Sean Paul, Software Engineer, Google / Chromium OS
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Sep 19, 2019 at 11:04:01AM -0400, Sean Paul wrote:
On Thu, Sep 05, 2019 at 12:41:27PM +0200, Daniel Vetter wrote:
On Wed, Sep 4, 2019 at 10:29 PM Sean Paul sean@poorly.run wrote:
From: Sean Paul seanpaul@chromium.org
Since the dirtyfb ioctl doesn't give us any hints as to which plane is scanning out the fb it's marking as damaged, we need to loop through planes to find it.
Currently we just reach into plane state and check, but that can race with another commit changing the fb out from under us. This patch locks the plane before checking the fb and will release the lock if the plane is not displaying the dirty fb.
Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") Cc: Rob Clark robdclark@gmail.com Cc: Deepak Rawat drawat@vmware.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Thomas Hellstrom thellstrom@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.0+ Reported-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Sean Paul seanpaul@chromium.org
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 8230dac01a89..3a4126dc2520 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -212,8 +212,14 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, drm_for_each_plane(plane, fb->dev) { struct drm_plane_state *plane_state;
if (plane->state->fb != fb)
ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);if (ret)I think for paranoid safety we should have a WARN_ON(ret == -EALREADY) here. It should be impossible, but if it's not for some oddball reason, we'll blow up.
drm_modeset_lock eats EALREADY and returns 0 for that case, so I guess it depends _how_ paranoid you want to be here :-)
Ah silly me, r-b as-is then. -Daniel
With that: Reviewed-by: Daniel Vetter daniel@ffwll.ch
But please give this a spin with some workloads and the ww_mutex slowpath debugging enabled, just to makre sure.
Ok, had a chance to run through some tests this morning with CONFIG_DEBUG_WW_MUTEX_SLOWPATH and things lgtm
Sean
-Daniel
goto out;if (plane->state->fb != fb) {drm_modeset_unlock(&plane->mutex); continue;} plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) {-- Sean Paul, Software Engineer, Google / Chromium OS
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
-- Sean Paul, Software Engineer, Google / Chromium OS
On Tue, Oct 08, 2019 at 11:50:33AM +0200, Daniel Vetter wrote:
On Thu, Sep 19, 2019 at 11:04:01AM -0400, Sean Paul wrote:
On Thu, Sep 05, 2019 at 12:41:27PM +0200, Daniel Vetter wrote:
On Wed, Sep 4, 2019 at 10:29 PM Sean Paul sean@poorly.run wrote:
From: Sean Paul seanpaul@chromium.org
Since the dirtyfb ioctl doesn't give us any hints as to which plane is scanning out the fb it's marking as damaged, we need to loop through planes to find it.
Currently we just reach into plane state and check, but that can race with another commit changing the fb out from under us. This patch locks the plane before checking the fb and will release the lock if the plane is not displaying the dirty fb.
Fixes: b9fc5e01d1ce ("drm: Add helper to implement legacy dirtyfb") Cc: Rob Clark robdclark@gmail.com Cc: Deepak Rawat drawat@vmware.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Thomas Hellstrom thellstrom@vmware.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard maxime.ripard@bootlin.com Cc: Sean Paul sean@poorly.run Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: stable@vger.kernel.org # v5.0+ Reported-by: Daniel Vetter daniel@ffwll.ch Signed-off-by: Sean Paul seanpaul@chromium.org
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 8230dac01a89..3a4126dc2520 100644 --- a/drivers/gpu/drm/drm_damage_helper.c +++ b/drivers/gpu/drm/drm_damage_helper.c @@ -212,8 +212,14 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, drm_for_each_plane(plane, fb->dev) { struct drm_plane_state *plane_state;
if (plane->state->fb != fb)
ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);if (ret)I think for paranoid safety we should have a WARN_ON(ret == -EALREADY) here. It should be impossible, but if it's not for some oddball reason, we'll blow up.
drm_modeset_lock eats EALREADY and returns 0 for that case, so I guess it depends _how_ paranoid you want to be here :-)
Ah silly me, r-b as-is then.
Thanks, pushed to -misc-next
Sean
-Daniel
With that: Reviewed-by: Daniel Vetter daniel@ffwll.ch
But please give this a spin with some workloads and the ww_mutex slowpath debugging enabled, just to makre sure.
Ok, had a chance to run through some tests this morning with CONFIG_DEBUG_WW_MUTEX_SLOWPATH and things lgtm
Sean
-Daniel
goto out;if (plane->state->fb != fb) {drm_modeset_unlock(&plane->mutex); continue;} plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) {-- Sean Paul, Software Engineer, Google / Chromium OS
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
-- Sean Paul, Software Engineer, Google / Chromium OS
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Sean Paul