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

[Thomas Hellstrom <thellstrom@vmware.com>]

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;
>  

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx