| Message ID | 20190904202938.110207-1-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: damage_helper: Fix race checking plane->state->fb | expand |
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
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)) {