From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com ([192.55.52.120]:54079 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753349AbcCVJcg (ORCPT ); Tue, 22 Mar 2016 05:32:36 -0400 Subject: Re: [PATCH] drm/core: Do not preserve framebuffer on rmfb. To: Daniel Vetter References: <1458569477-13364-1-git-send-email-maarten.lankhorst@linux.intel.com> Cc: dri-devel , intel-gfx , Thomas Hellstrom , stable From: Maarten Lankhorst Message-ID: <56F11130.5020506@linux.intel.com> Date: Tue, 22 Mar 2016 10:32:32 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: stable-owner@vger.kernel.org List-ID: 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 >> Cc: David Herrmann >> --- >> 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.