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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
Date: Thu, 11 Jul 2013 10:00:31 +0200	[thread overview]
Message-ID: <2004690.ggUZnDQevD@avalon> (raw)
In-Reply-To: <1373467725-19652-1-git-send-email-daniel.vetter@ffwll.ch>

Hi Daniel,

Thanks for the patch.

On Wednesday 10 July 2013 16:48:45 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).
> 
> 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.
> 
> Since I've created this patch cma prime support for dma_buf was added.
> drm_gem_cma_prime_get_sg_table only calls kzalloc and the creates&maps
> the sg table with dma_get_sgtable. It doesn't touch any gem object
> state otherwise. So the cma helpers also look safe.
> 
> 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.
> 
> v2: Add analysis for recently added cma helper prime code.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

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