Re: [PATCH] drm: damage_helper: Fix race checking plane->state->fb
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 wrote:
> > > >
> > > > From: Sean Paul
> > > >
> > > > 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
> > > > Cc: Deepak Rawat
> > > > Cc: Daniel Vetter
> > > > Cc: Thomas Hellstrom
> > > > Cc: Maarten Lankhorst
> > > > Cc: Maxime Ripard
> > > > Cc: Sean Paul
> > > > Cc: David Airlie
> > > > Cc: Daniel Vetter
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Cc: # v5.0+
> > > > Reported-by: Daniel Vetter
> > > > Signed-off-by: Sean Paul
> > > > ---
> > > > 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(>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
> > >
> > > 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(>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
--
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: damage_helper: Fix race checking plane->state->fb
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 wrote:
> > >
> > > From: Sean Paul
> > >
> > > 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
> > > Cc: Deepak Rawat
> > > Cc: Daniel Vetter
> > > Cc: Thomas Hellstrom
> > > Cc: Maarten Lankhorst
> > > Cc: Maxime Ripard
> > > Cc: Sean Paul
> > > Cc: David Airlie
> > > Cc: Daniel Vetter
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: # v5.0+
> > > Reported-by: Daniel Vetter
> > > Signed-off-by: Sean Paul
> > > ---
> > > 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(>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
> >
> > 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(>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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: damage_helper: Fix race checking plane->state->fb
On Thu, Sep 05, 2019 at 12:41:27PM +0200, Daniel Vetter wrote:
> On Wed, Sep 4, 2019 at 10:29 PM Sean Paul wrote:
> >
> > From: Sean Paul
> >
> > 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
> > Cc: Deepak Rawat
> > Cc: Daniel Vetter
> > Cc: Thomas Hellstrom
> > Cc: Maarten Lankhorst
> > Cc: Maxime Ripard
> > Cc: Sean Paul
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: # v5.0+
> > Reported-by: Daniel Vetter
> > Signed-off-by: Sean Paul
> > ---
> > 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(>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
>
> 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(>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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: damage_helper: Fix race checking plane->state->fb
On Wed, Sep 4, 2019 at 10:29 PM Sean Paul wrote:
>
> From: Sean Paul
>
> 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
> Cc: Deepak Rawat
> Cc: Daniel Vetter
> Cc: Thomas Hellstrom
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Sean Paul
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: dri-devel@lists.freedesktop.org
> Cc: # v5.0+
> Reported-by: Daniel Vetter
> Signed-off-by: Sean Paul
> ---
> 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(>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
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(>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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
