Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Wednesday 10 July 2013 13:54:33 Daniel Vetter wrote:
> I've checked both implementations (radeon/nouveau) and they both grab
> the page array from ttm simply by dereferencing it and then wrapping
> it up with drm_prime_pages_to_sg in the callback and map it with
> dma_map_sg (in the helper).

Have you checked drm_gem_cma_prime_get_sg_table (in drm_gem_cma_helper.c) as 
well ?

> Only the grabbing of the underlying page array is anything we need to
> be concerned about, and either those pages are pinned independently,
> or we're screwed no matter what.
> 
> And indeed, nouveau/radeon pin the backing storage in their
> attach/detach functions.
> 
> The only thing we might claim it does is prevent concurrent mapping of
> dma_buf attachments. But a) that's not allowed and b) the current code
> is racy already since it checks whether the sg mapping exists _before_
> grabbing the lock.
> 
> So the dev->struct_mutex locking here does absolutely nothing useful,
> but only distracts. Remove it.
> 
> This should also help Maarten's work to eventually pin the backing
> storage more dynamically by preventing locking inversions around
> dev->struct_mutex.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_prime.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 85e450e..64a99b3 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -167,8 +167,6 @@ static struct sg_table *drm_gem_map_dma_buf(struct
> dma_buf_attachment *attach, if (WARN_ON(prime_attach->dir != DMA_NONE))
>  		return ERR_PTR(-EBUSY);
> 
> -	mutex_lock(&obj->dev->struct_mutex);
> -
>  	sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
> 
>  	if (!IS_ERR(sgt)) {
> @@ -182,7 +180,6 @@ static struct sg_table *drm_gem_map_dma_buf(struct
> dma_buf_attachment *attach, }
>  	}
> 
> -	mutex_unlock(&obj->dev->struct_mutex);
>  	return sgt;
>  }
-- 
Regards,

Laurent Pinchart