From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper Date: Wed, 10 Jul 2013 15:46:04 +0200 Message-ID: <2834403.v6KyAMRBUk@avalon> References: <1373457273-5800-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [95.142.166.194]) by gabe.freedesktop.org (Postfix) with ESMTP id 9370DE5CD9 for ; Wed, 10 Jul 2013 06:45:29 -0700 (PDT) In-Reply-To: <1373457273-5800-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: dri-devel@lists.freedesktop.org Cc: Daniel Vetter List-Id: dri-devel@lists.freedesktop.org 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 > Signed-off-by: Daniel Vetter > --- > 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