| Message ID | 1458569477-13364-1-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote: > It turns out that preserving framebuffers after the rmfb call breaks > vmwgfx userspace. This was originally introduced because it was thought > nobody relied on the behavior, but unfortunately it seems there are > exceptions. > > drm_framebuffer_remove may fail with -EINTR now, so a straight revert > is impossible. There is no way to remove the framebuffer from the lists > and active planes without introducing a race because of the different > locking requirements. Instead call drm_framebuffer_remove from a > workqueue, which is unaffected by signals. > > Cc: stable@vger.kernel.org #v4.4+ > Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.") > Testcase: kms_flip.flip-vs-rmfb-interruptible > References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Cc: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index e08f962288d9..b7d0b959f088 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev, > return 0; > } > > +struct drm_mode_rmfb_work { > + struct work_struct work; > + struct drm_framebuffer *fb; > +}; > + > +static void drm_mode_rmfb_work_fn(struct work_struct *w) > +{ > + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work); > + > + drm_framebuffer_remove(arg->fb); drm_framebuffer_remove still has the problem of not working correctly with atomic since atomic commit will complain if we try to do more than 1 commit per ww_acquire_ctx. I think we still need an atomic version of this. Also probably a more nasty igt testcase which uses the same fb on more than one plane to be able to hit this case. > +} > + > /** > * drm_mode_rmfb - remove an FB from the configuration > * @dev: drm device for the ioctl > @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev, > struct drm_framebuffer *fbl = NULL; > uint32_t *id = data; > int found = 0; > + struct drm_mode_rmfb_work arg; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EINVAL; > @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev, > mutex_unlock(&dev->mode_config.fb_lock); > mutex_unlock(&file_priv->fbs_lock); > > - drm_framebuffer_unreference(fb); Needs a comment here to explain that we evade EINTR/signals, and that it's not a trick to hide a locking inversion from lockdep. Otherwise I think this patch here is the best fix of all the approaches discussed on irc, under the constraint that we need some obviously save&small for cc: stable. -Daniel > + INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn); > + arg.fb = fb; > + > + schedule_work(&arg.work); > + flush_work(&arg.work); > + destroy_work_on_stack(&arg.work); > > return 0; > > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 21-03-16 om 18:37 schreef Daniel Vetter: > On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote: >> It turns out that preserving framebuffers after the rmfb call breaks >> vmwgfx userspace. This was originally introduced because it was thought >> nobody relied on the behavior, but unfortunately it seems there are >> exceptions. >> >> drm_framebuffer_remove may fail with -EINTR now, so a straight revert >> is impossible. There is no way to remove the framebuffer from the lists >> and active planes without introducing a race because of the different >> locking requirements. Instead call drm_framebuffer_remove from a >> workqueue, which is unaffected by signals. >> >> Cc: stable@vger.kernel.org #v4.4+ >> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.") >> Testcase: kms_flip.flip-vs-rmfb-interruptible >> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html >> Cc: Thomas Hellstrom <thellstrom@vmware.com> >> Cc: David Herrmann <dh.herrmann@gmail.com> >> --- >> drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++- >> 1 file changed, 19 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index e08f962288d9..b7d0b959f088 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev, >> return 0; >> } >> >> +struct drm_mode_rmfb_work { >> + struct work_struct work; >> + struct drm_framebuffer *fb; >> +}; >> + >> +static void drm_mode_rmfb_work_fn(struct work_struct *w) >> +{ >> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work); >> + >> + drm_framebuffer_remove(arg->fb); > drm_framebuffer_remove still has the problem of not working correctly with > atomic since atomic commit will complain if we try to do more than 1 > commit per ww_acquire_ctx. I think we still need an atomic version of > this. Also probably a more nasty igt testcase which uses the same fb on > more than one plane to be able to hit this case. That's true, but a separate bug. :) >> +} >> + >> /** >> * drm_mode_rmfb - remove an FB from the configuration >> * @dev: drm device for the ioctl >> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev, >> struct drm_framebuffer *fbl = NULL; >> uint32_t *id = data; >> int found = 0; >> + struct drm_mode_rmfb_work arg; >> >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) >> return -EINVAL; >> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev, >> mutex_unlock(&dev->mode_config.fb_lock); >> mutex_unlock(&file_priv->fbs_lock); >> >> - drm_framebuffer_unreference(fb); > Needs a comment here to explain that we evade EINTR/signals, and that it's > not a trick to hide a locking inversion from lockdep. > > Otherwise I think this patch here is the best fix of all the approaches > discussed on irc, under the constraint that we need some obviously > save&small for cc: stable. > Indeed, will add a comment.
On Tue, Mar 22, 2016 at 10:32:32AM +0100, Maarten Lankhorst wrote: > Op 21-03-16 om 18:37 schreef Daniel Vetter: > > On Mon, Mar 21, 2016 at 03:11:17PM +0100, Maarten Lankhorst wrote: > >> It turns out that preserving framebuffers after the rmfb call breaks > >> vmwgfx userspace. This was originally introduced because it was thought > >> nobody relied on the behavior, but unfortunately it seems there are > >> exceptions. > >> > >> drm_framebuffer_remove may fail with -EINTR now, so a straight revert > >> is impossible. There is no way to remove the framebuffer from the lists > >> and active planes without introducing a race because of the different > >> locking requirements. Instead call drm_framebuffer_remove from a > >> workqueue, which is unaffected by signals. > >> > >> Cc: stable@vger.kernel.org #v4.4+ > >> Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.") > >> Testcase: kms_flip.flip-vs-rmfb-interruptible > >> References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html > >> Cc: Thomas Hellstrom <thellstrom@vmware.com> > >> Cc: David Herrmann <dh.herrmann@gmail.com> > >> --- > >> drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++- > >> 1 file changed, 19 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >> index e08f962288d9..b7d0b959f088 100644 > >> --- a/drivers/gpu/drm/drm_crtc.c > >> +++ b/drivers/gpu/drm/drm_crtc.c > >> @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev, > >> return 0; > >> } > >> > >> +struct drm_mode_rmfb_work { > >> + struct work_struct work; > >> + struct drm_framebuffer *fb; > >> +}; > >> + > >> +static void drm_mode_rmfb_work_fn(struct work_struct *w) > >> +{ > >> + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work); > >> + > >> + drm_framebuffer_remove(arg->fb); > > drm_framebuffer_remove still has the problem of not working correctly with > > atomic since atomic commit will complain if we try to do more than 1 > > commit per ww_acquire_ctx. I think we still need an atomic version of > > this. Also probably a more nasty igt testcase which uses the same fb on > > more than one plane to be able to hit this case. > That's true, but a separate bug. :) Atm we only use drm_framebuffer_remove in atomic drivers to nuke the fbdev one at unload. With your patch userspace can't get there easily, and hence it must be fixed. Maybe separate prep patch (also cc: stable) upfront? -Daniel > >> +} > >> + > >> /** > >> * drm_mode_rmfb - remove an FB from the configuration > >> * @dev: drm device for the ioctl > >> @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev, > >> struct drm_framebuffer *fbl = NULL; > >> uint32_t *id = data; > >> int found = 0; > >> + struct drm_mode_rmfb_work arg; > >> > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EINVAL; > >> @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev, > >> mutex_unlock(&dev->mode_config.fb_lock); > >> mutex_unlock(&file_priv->fbs_lock); > >> > >> - drm_framebuffer_unreference(fb); > > Needs a comment here to explain that we evade EINTR/signals, and that it's > > not a trick to hide a locking inversion from lockdep. > > > > Otherwise I think this patch here is the best fix of all the approaches > > discussed on irc, under the constraint that we need some obviously > > save&small for cc: stable. > > > Indeed, will add a comment.
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e08f962288d9..b7d0b959f088 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3434,6 +3434,18 @@ int drm_mode_addfb2(struct drm_device *dev, return 0; } +struct drm_mode_rmfb_work { + struct work_struct work; + struct drm_framebuffer *fb; +}; + +static void drm_mode_rmfb_work_fn(struct work_struct *w) +{ + struct drm_mode_rmfb_work *arg = container_of(w, typeof(*arg), work); + + drm_framebuffer_remove(arg->fb); +} + /** * drm_mode_rmfb - remove an FB from the configuration * @dev: drm device for the ioctl @@ -3454,6 +3466,7 @@ int drm_mode_rmfb(struct drm_device *dev, struct drm_framebuffer *fbl = NULL; uint32_t *id = data; int found = 0; + struct drm_mode_rmfb_work arg; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -3474,7 +3487,12 @@ int drm_mode_rmfb(struct drm_device *dev, mutex_unlock(&dev->mode_config.fb_lock); mutex_unlock(&file_priv->fbs_lock); - drm_framebuffer_unreference(fb); + INIT_WORK_ONSTACK(&arg.work, drm_mode_rmfb_work_fn); + arg.fb = fb; + + schedule_work(&arg.work); + flush_work(&arg.work); + destroy_work_on_stack(&arg.work); return 0;
It turns out that preserving framebuffers after the rmfb call breaks vmwgfx userspace. This was originally introduced because it was thought nobody relied on the behavior, but unfortunately it seems there are exceptions. drm_framebuffer_remove may fail with -EINTR now, so a straight revert is impossible. There is no way to remove the framebuffer from the lists and active planes without introducing a race because of the different locking requirements. Instead call drm_framebuffer_remove from a workqueue, which is unaffected by signals. Cc: stable@vger.kernel.org #v4.4+ Fixes: 13803132818c ("drm/core: Preserve the framebuffer after removing it.") Testcase: kms_flip.flip-vs-rmfb-interruptible References: https://lists.freedesktop.org/archives/dri-devel/2016-March/102876.html Cc: Thomas Hellstrom <thellstrom@vmware.com> Cc: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/drm_crtc.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)