Re: [PATCH 3/5] drm/udl: Get rid of dev->struct_mutex usage

[Sean Paul <seanpaul@chromium.org>]

On Tue, Mar 27, 2018 at 10:23:54AM +0200, Daniel Vetter wrote:
> It's only used to protect our page list, and only when we know we have
> a full reference. This means none of these code paths can ever race
> with the final unref, and hence we do not need dev->struct_mutex
> serialization and can simply switch to our own locking.
> 
> For more context the only magic the locked gem_free_object provides is
> that it prevents concurrent final unref (and destruction) of gem
> objects while anyone is holding dev->struct_mutex. This was used by
> i915 (and other drivers) to implement eviction handling with less
> headaches.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> Cc: Dave Airlie <airlied@redhat.com>
> ---
>  drivers/gpu/drm/udl/udl_dmabuf.c | 5 +++--
>  drivers/gpu/drm/udl/udl_drv.c    | 2 +-
>  drivers/gpu/drm/udl/udl_drv.h    | 2 ++
>  drivers/gpu/drm/udl/udl_gem.c    | 5 +++--
>  drivers/gpu/drm/udl/udl_main.c   | 2 ++
>  5 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c
> index 2867ed155ff6..0a20695eb120 100644
> --- a/drivers/gpu/drm/udl/udl_dmabuf.c
> +++ b/drivers/gpu/drm/udl/udl_dmabuf.c
> @@ -76,6 +76,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach,
>  	struct udl_drm_dmabuf_attachment *udl_attach = attach->priv;
>  	struct udl_gem_object *obj = to_udl_bo(attach->dmabuf->priv);
>  	struct drm_device *dev = obj->base.dev;
> +	struct udl_device *udl = dev->dev_private;
>  	struct scatterlist *rd, *wr;
>  	struct sg_table *sgt = NULL;
>  	unsigned int i;
> @@ -112,7 +113,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach,
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> -	mutex_lock(&dev->struct_mutex);
> +	mutex_lock(&udl->gem_lock);
>  
>  	rd = obj->sg->sgl;
>  	wr = sgt->sgl;
> @@ -137,7 +138,7 @@ static struct sg_table *udl_map_dma_buf(struct dma_buf_attachment *attach,
>  	attach->priv = udl_attach;
>  
>  err_unlock:
> -	mutex_unlock(&dev->struct_mutex);
> +	mutex_unlock(&udl->gem_lock);
>  	return sgt;
>  }
>  
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 3c45a3064726..9ef515df724b 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -53,7 +53,7 @@ static struct drm_driver driver = {
>  	.unload = udl_driver_unload,
>  
>  	/* gem hooks */
> -	.gem_free_object = udl_gem_free_object,
> +	.gem_free_object_unlocked = udl_gem_free_object,
>  	.gem_vm_ops = &udl_gem_vm_ops,
>  
>  	.dumb_create = udl_dumb_create,
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 2a75ab80527a..55c0cc309198 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -54,6 +54,8 @@ struct udl_device {
>  	struct usb_device *udev;
>  	struct drm_crtc *crtc;
>  
> +	struct mutex gem_lock;
> +
>  	int sku_pixel_limit;
>  
>  	struct urb_list urbs;
> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> index dee6bd9a3dd1..9a15cce22cce 100644
> --- a/drivers/gpu/drm/udl/udl_gem.c
> +++ b/drivers/gpu/drm/udl/udl_gem.c
> @@ -214,9 +214,10 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>  {
>  	struct udl_gem_object *gobj;
>  	struct drm_gem_object *obj;
> +	struct udl_device *udl = dev->dev_private;
>  	int ret = 0;
>  
> -	mutex_lock(&dev->struct_mutex);
> +	mutex_lock(&udl->gem_lock);
>  	obj = drm_gem_object_lookup(file, handle);
>  	if (obj == NULL) {
>  		ret = -ENOENT;
> @@ -236,6 +237,6 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>  out:
>  	drm_gem_object_put(&gobj->base);
>  unlock:
> -	mutex_unlock(&dev->struct_mutex);
> +	mutex_unlock(&udl->gem_lock);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index f1ec4528a73e..d518de8f496b 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -324,6 +324,8 @@ int udl_driver_load(struct drm_device *dev, unsigned long flags)
>  	udl->ddev = dev;
>  	dev->dev_private = udl;
>  
> +	mutex_init(&udl->gem_lock);
> +
>  	if (!udl_parse_vendor_descriptor(dev, udl->udev)) {
>  		ret = -ENODEV;
>  		DRM_ERROR("firmware not recognized. Assume incompatible device\n");
> -- 
> 2.16.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel