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

From: Maarten Lankhorst <maarten.lankhorst@canonical.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: Wed, 10 Jul 2013 14:03:21 +0200	[thread overview]
Message-ID: <51DD4D89.3090409@canonical.com> (raw)
In-Reply-To: <1373457273-5800-1-git-send-email-daniel.vetter@ffwll.ch>

Op 10-07-13 13:54, Daniel Vetter schreef:
> 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.
>
> 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.

This pleases me, but I think it's not thorough enough.

	if (prime_attach->dir == dir)
		return prime_attach->sgt;

^ That check must go too. I don't think recursive map_dma_buf is valid.

and unmap_dma_buf should set prime_attach->dir = DMA_NONE; again.

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