[PATCH] drm/core: Do not preserve framebuffer on rmfb.

[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 at 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 at vmware.com>
> Cc: David Herrmann <dh.herrmann at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch