All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: [PATCH 03/20] drm/prime: remove cargo-cult locking from map_sg helper
Date: Thu, 15 Aug 2013 00:02:32 +0200	[thread overview]
Message-ID: <1376517769-7171-4-git-send-email-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <1376517769-7171-1-git-send-email-daniel.vetter@ffwll.ch>

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>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: 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 a35f206..f115962 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;
 }
 
-- 
1.8.3.2

  parent reply	other threads:[~2013-08-14 22:03 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-14 22:02 [PATCH 00/20] prime patches Daniel Vetter
2013-08-14 22:02 ` [PATCH 01/20] drm: use common drm_gem_dmabuf_release in i915/exynos drivers Daniel Vetter
2013-08-14 22:02 ` [PATCH 02/20] drm/exynos: explicit store base gem object in dma_buf->priv Daniel Vetter
2013-08-14 22:02 ` Daniel Vetter [this message]
2013-08-14 22:02 ` [PATCH 04/20] drm/prime: add a bit of documentation about gem_obj->import_attach Daniel Vetter
2013-08-14 22:02 ` [PATCH 05/20] drm/gem: move drm_gem_object_handle_unreference_unlocked into drm_gem.c Daniel Vetter
2013-08-14 22:02 ` [PATCH 06/20] drm/gem: remove bogus NULL check from drm_gem_object_handle_unreference_unlocked Daniel Vetter
2013-08-14 22:02 ` [PATCH 07/20] drm/gem: WARN about unbalanced handle refcounts Daniel Vetter
2013-08-14 22:02 ` [PATCH 08/20] drm/gem: fix up flink name create race Daniel Vetter
2013-08-14 22:02 ` [PATCH 09/20] drm/prime: fix error path in drm_gem_prime_fd_to_handle Daniel Vetter
2013-08-14 22:02 ` [PATCH 10/20] drm/gem: make drm_gem_object_handle_unreference_unlocked static Daniel Vetter
2013-08-14 22:02 ` [PATCH 11/20] drm/gem: create drm_gem_dumb_destroy Daniel Vetter
     [not found]   ` <20130815072447.GC21854@nuc-i3427.alporthouse.com>
2013-08-15  8:03     ` Daniel Vetter
2013-08-14 22:02 ` [PATCH 12/20] drm/prime: use proper pointer in drm_gem_prime_handle_to_fd Daniel Vetter
2013-08-14 22:02 ` [PATCH 13/20] drm/prime: shrink critical section protected by prime lock Daniel Vetter
2013-08-14 22:02 ` [PATCH 14/20] drm/prime: clarify logic a bit in drm_gem_prime_fd_to_handle Daniel Vetter
2013-08-14 22:02 ` [PATCH 15/20] drm/gem: switch dev->object_name_lock to a mutex Daniel Vetter
2013-08-14 22:02 ` [PATCH 16/20] drm/gem: completely close gem_open vs. gem_close races Daniel Vetter
2013-08-14 22:02 ` [PATCH 17/20] drm/prime: proper locking+refcounting for obj->dma_buf link Daniel Vetter
2013-08-14 22:02 ` [PATCH 18/20] drm/prime: Simplify drm_gem_remove_prime_handles Daniel Vetter
2013-08-14 22:02 ` [PATCH 19/20] drm/prime: make drm_prime_lookup_buf_handle static Daniel Vetter
2013-08-14 22:02 ` [PATCH 20/20] drm/prime: Always add exported buffers to the handle cache Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1376517769-7171-4-git-send-email-daniel.vetter@ffwll.ch \
    --to=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=laurent.pinchart@ideasonboard.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.