Message ID | 1459423563-27558-1-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Op 31-03-16 om 13:26 schreef Maarten Lankhorst: > 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. > Ping?
On Thu, Mar 31, 2016 at 01:26:03PM +0200, 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. > > Changes since v1: > - Add comment. > Changes since v2: > - Add fastpath for refcount = 1. (danvet) > > 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> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> But definitely want a t-b from Thomas before applying, since he reported this regression. -Daniel > --- > drivers/gpu/drm/drm_crtc.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 55ffde5a3a4a..743bece1f579 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 > @@ -3474,7 +3486,22 @@ 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); > + /* > + * drm_framebuffer_remove may fail with -EINTR on pending signals, > + * so run this in a separate stack as there's no way to correctly > + * handle this after the fb is already removed from the lookup table. > + */ > + if (atomic_read(&fb->refcount.refcount) > 1) { > + struct drm_mode_rmfb_work arg; > + > + 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); > + } else > + drm_framebuffer_unreference(fb); > > return 0; > > -- > 2.1.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi, Sorry for the late response, been very busy with other stuff lately. I've tested this version against drm-fixes and it indeed fixes the problem, as far as I can tell. Tested-by: Thomas Hellstrom <thellstrom@vmware.com> On 03/31/2016 01:26 PM, 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. > > Changes since v1: > - Add comment. > Changes since v2: > - Add fastpath for refcount = 1. (danvet) > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_dri-2Ddevel_2016-2DMarch_102876.html&d=BQIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=_2qOX1NGnSnJOTgqvu1Ud574i5T3fLDlX91oUS3WXXI&s=9D34PFYdb1PT2vzX_M_7lNVoSebfM9-KsAqe5AXAQbQ&e= > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Cc: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/gpu/drm/drm_crtc.c | 29 ++++++++++++++++++++++++++++- > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 55ffde5a3a4a..743bece1f579 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 > @@ -3474,7 +3486,22 @@ 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); > + /* > + * drm_framebuffer_remove may fail with -EINTR on pending signals, > + * so run this in a separate stack as there's no way to correctly > + * handle this after the fb is already removed from the lookup table. > + */ > + if (atomic_read(&fb->refcount.refcount) > 1) { > + struct drm_mode_rmfb_work arg; > + > + 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); > + } else > + drm_framebuffer_unreference(fb); > > return 0; >
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 55ffde5a3a4a..743bece1f579 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 @@ -3474,7 +3486,22 @@ 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); + /* + * drm_framebuffer_remove may fail with -EINTR on pending signals, + * so run this in a separate stack as there's no way to correctly + * handle this after the fb is already removed from the lookup table. + */ + if (atomic_read(&fb->refcount.refcount) > 1) { + struct drm_mode_rmfb_work arg; + + 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); + } else + drm_framebuffer_unreference(fb); return 0;