[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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
> 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
> Cc: Maarten Lankhorst
> Signed-off-by: Daniel Vetter
Acked-by: Laurent Pinchart
> ---
> 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(>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(>dev->struct_mutex);
> return sgt;
> }
--
Regards,
Laurent Pinchart
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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 createsmaps
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.pinch...@ideasonboard.com
Cc: Maarten Lankhorst maarten.lankho...@canonical.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Acked-by: Laurent Pinchart laurent.pinch...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
Op 10-07-13 16:48, 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.
>
> 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
> 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
> 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(>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(>dev->struct_mutex);
> return sgt;
> }
>
acked
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
On Wed, Jul 10, 2013 at 5:18 PM, Konrad Rzeszutek Wilk wrote: >> So after a bit of irc chatting with Maarten this seems to be more >> involved. The above check is to cache the dma mapping, but the >> implementation is bogus in tons of ways: >> - If direction changes we don't bother with unmaping and freeing the >> mapping, but simply leak it. >> - This will break if the dma mapping needs explicit syncing since the >> helpers don't call sync_to_cpu/sync_to_device anywhere. > > Right, and I believe I signed up for that. Well, the breakage runs deeper since atm ttm doesn't have any concept of syncing from/to the device dma. Neither has i915. So this little issue here is just the tip of the iceberg ... >> So I think I'll decline to poke around more in this hornet nest and >> leave it at the locking removal. > > .. and I get the hornet nest :-). Is there a IRC log of what you guys talked > about so I don't omit certain pieces of code. The above is pretty much the summary, actually more lines than what we've discussed on irc ;-) It's on #dri-devel though: http://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel_html=true_names==2013-07-10 -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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
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
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(>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(>dev->struct_mutex);
return sgt;
}
--
1.8.3.2
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
On Wed, Jul 10, 2013 at 3:46 PM, Laurent Pinchart wrote: > 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 ? Indeed I've missed to check this one since I've based this branch originally on an older drm-next version without the cma dma-buf stuff. drm_gem_cma_prime_get_sg_table only calls kzalloc and the creates the sg table with dma_get_sgtable. It doesn't touch any gem object state otherwise. So looks safe. I'll amed the commit message a bit. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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(>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(>dev->struct_mutex);
> return sgt;
> }
--
Regards,
Laurent Pinchart
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
On Wed, Jul 10, 2013 at 2:03 PM, Maarten Lankhorst wrote: > 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. So after a bit of irc chatting with Maarten this seems to be more involved. The above check is to cache the dma mapping, but the implementation is bogus in tons of ways: - If direction changes we don't bother with unmaping and freeing the mapping, but simply leak it. - This will break if the dma mapping needs explicit syncing since the helpers don't call sync_to_cpu/sync_to_device anywhere. So I think I'll decline to poke around more in this hornet nest and leave it at the locking removal. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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
> 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(>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(>dev->struct_mutex);
> return sgt;
> }
>
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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.
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(>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(>dev->struct_mutex);
return sgt;
}
--
1.8.3.2
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
> So after a bit of irc chatting with Maarten this seems to be more > involved. The above check is to cache the dma mapping, but the > implementation is bogus in tons of ways: > - If direction changes we don't bother with unmaping and freeing the > mapping, but simply leak it. > - This will break if the dma mapping needs explicit syncing since the > helpers don't call sync_to_cpu/sync_to_device anywhere. Right, and I believe I signed up for that. > > So I think I'll decline to poke around more in this hornet nest and > leave it at the locking removal. .. and I get the hornet nest :-). Is there a IRC log of what you guys talked about so I don't omit certain pieces of code.
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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.
Cc: Maarten Lankhorst maarten.lankho...@canonical.com
Signed-off-by: Daniel Vetter daniel.vet...@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;
}
--
1.8.3.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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.lankho...@canonical.com
Signed-off-by: Daniel Vetter daniel.vet...@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;
}
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
On Wed, Jul 10, 2013 at 2:03 PM, Maarten Lankhorst maarten.lankho...@canonical.com wrote: 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. So after a bit of irc chatting with Maarten this seems to be more involved. The above check is to cache the dma mapping, but the implementation is bogus in tons of ways: - If direction changes we don't bother with unmaping and freeing the mapping, but simply leak it. - This will break if the dma mapping needs explicit syncing since the helpers don't call sync_to_cpu/sync_to_device anywhere. So I think I'll decline to poke around more in this hornet nest and leave it at the locking removal. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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.lankho...@canonical.com
Signed-off-by: Daniel Vetter daniel.vet...@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
On Wed, Jul 10, 2013 at 3:46 PM, Laurent Pinchart laurent.pinch...@ideasonboard.com wrote: 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 ? Indeed I've missed to check this one since I've based this branch originally on an older drm-next version without the cma dma-buf stuff. drm_gem_cma_prime_get_sg_table only calls kzalloc and the createsmaps the sg table with dma_get_sgtable. It doesn't touch any gem object state otherwise. So looks safe. I'll amed the commit message a bit. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/prime: remove cargo-cult locking from map_sg helper
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 createsmaps
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.pinch...@ideasonboard.com
Cc: Maarten Lankhorst maarten.lankho...@canonical.com
Signed-off-by: Daniel Vetter daniel.vet...@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;
}
--
1.8.3.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
So after a bit of irc chatting with Maarten this seems to be more involved. The above check is to cache the dma mapping, but the implementation is bogus in tons of ways: - If direction changes we don't bother with unmaping and freeing the mapping, but simply leak it. - This will break if the dma mapping needs explicit syncing since the helpers don't call sync_to_cpu/sync_to_device anywhere. Right, and I believe I signed up for that. So I think I'll decline to poke around more in this hornet nest and leave it at the locking removal. .. and I get the hornet nest :-). Is there a IRC log of what you guys talked about so I don't omit certain pieces of code. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
On Wed, Jul 10, 2013 at 5:18 PM, Konrad Rzeszutek Wilk konrad.w...@oracle.com wrote: So after a bit of irc chatting with Maarten this seems to be more involved. The above check is to cache the dma mapping, but the implementation is bogus in tons of ways: - If direction changes we don't bother with unmaping and freeing the mapping, but simply leak it. - This will break if the dma mapping needs explicit syncing since the helpers don't call sync_to_cpu/sync_to_device anywhere. Right, and I believe I signed up for that. Well, the breakage runs deeper since atm ttm doesn't have any concept of syncing from/to the device dma. Neither has i915. So this little issue here is just the tip of the iceberg ... So I think I'll decline to poke around more in this hornet nest and leave it at the locking removal. .. and I get the hornet nest :-). Is there a IRC log of what you guys talked about so I don't omit certain pieces of code. The above is pretty much the summary, actually more lines than what we've discussed on irc ;-) It's on #dri-devel though: http://people.freedesktop.org/~cbrill/dri-log/?channel=dri-develshow_html=truehighlight_names=date=2013-07-10 -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/prime: remove cargo-cult locking from map_sg helper
Op 10-07-13 16:48, 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.
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 createsmaps
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.pinch...@ideasonboard.com
Cc: Maarten Lankhorst maarten.lankho...@canonical.com
Signed-off-by: Daniel Vetter daniel.vet...@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;
}
acked
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
