The Documentation/DMA-API-HOWTO.txt states that dma_map_sg returns the
numer of the created entries in the DMA address space. However the
subsequent calls to dma_sync_sg_for_{device,cpu} and dma_unmap_sg must be
called with the original number of the entries passed to dma_map_sg. The
sg_table->nents in turn holds the result of the dma_map_sg call as stated
in include/linux/scatterlist.h. A common mistake was to ignore a result
of the dma_map_sg function and don't use the sg_table->orig_nents at all.
To avoid such issues, lets use common dma-mapping wrappers operating
directly on the struct sg_table objects and adjust references to the
nents and orig_nents respectively.
Signed-off-by: Marek Szyprowski <[email protected]>
---
For more information, see '[PATCH v3 00/25] DRM: fix struct sg_table nents
vs. orig_nents misuse' thread: https://lkml.org/lkml/2020/5/5/187
---
drivers/gpu/drm/drm_cache.c | 2 +-
drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +++++++++-----
drivers/gpu/drm/drm_prime.c | 13 +++++++------
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
index 03e01b0..63bd497 100644
--- a/drivers/gpu/drm/drm_cache.c
+++ b/drivers/gpu/drm/drm_cache.c
@@ -127,7 +127,7 @@ static void drm_cache_flush_clflush(struct page *pages[],
struct sg_page_iter sg_iter;
mb(); /*CLFLUSH is ordered only by using memory barriers*/
- for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
+ for_each_sg_page(st->sgl, &sg_iter, st->orig_nents, 0)
drm_clflush_page(sg_page_iter_page(&sg_iter));
mb(); /*Make sure that all cache line entry is flushed*/
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index df31e57..cfcfc0d 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -117,8 +117,8 @@ void drm_gem_shmem_free_object(struct drm_gem_object *obj)
kvfree(shmem->pages);
} else {
if (shmem->sgt) {
- dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
- shmem->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
+ DMA_BIDIRECTIONAL);
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
}
@@ -395,8 +395,7 @@ void drm_gem_shmem_purge_locked(struct drm_gem_object *obj)
WARN_ON(!drm_gem_shmem_is_purgeable(shmem));
- dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
- shmem->sgt->nents, DMA_BIDIRECTIONAL);
+ dma_unmap_sgtable(obj->dev->dev, shmem->sgt, DMA_BIDIRECTIONAL);
sg_free_table(shmem->sgt);
kfree(shmem->sgt);
shmem->sgt = NULL;
@@ -623,12 +622,17 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *obj)
goto err_put_pages;
}
/* Map the pages for use by the h/w. */
- dma_map_sg(obj->dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
+ ret = dma_map_sgtable(obj->dev->dev, sgt, DMA_BIDIRECTIONAL);
+ if (ret)
+ goto err_free_sgt;
shmem->sgt = sgt;
return sgt;
+err_free_sgt:
+ sg_free_table(sgt);
+ kfree(sgt);
err_put_pages:
drm_gem_shmem_put_pages(shmem);
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 282774e..3e7cb02 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -617,6 +617,7 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
{
struct drm_gem_object *obj = attach->dmabuf->priv;
struct sg_table *sgt;
+ int ret;
if (WARN_ON(dir == DMA_NONE))
return ERR_PTR(-EINVAL);
@@ -626,11 +627,12 @@ struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
else
sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
- if (!dma_map_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC)) {
+ ret = dma_map_sgtable_attrs(attach->dev, sgt, dir,
+ DMA_ATTR_SKIP_CPU_SYNC);
+ if (ret) {
sg_free_table(sgt);
kfree(sgt);
- sgt = ERR_PTR(-ENOMEM);
+ sgt = ERR_PTR(ret);
}
return sgt;
@@ -652,8 +654,7 @@ void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
if (!sgt)
return;
- dma_unmap_sg_attrs(attach->dev, sgt->sgl, sgt->nents, dir,
- DMA_ATTR_SKIP_CPU_SYNC);
+ dma_unmap_sgtable_attrs(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC);
sg_free_table(sgt);
kfree(sgt);
}
@@ -975,7 +976,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages,
*/
page_index = 0;
dma_index = 0;
- for_each_sg(sgt->sgl, sg, sgt->nents, count) {
+ for_each_sg(sgt->sgl, sg, sgt->orig_nents, count) {
page_len = sg->length;
page = sg_page(sg);
dma_len = sg_dma_len(sg);
--
1.9.1
> - for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
> + for_each_sg_page(st->sgl, &sg_iter, st->orig_nents, 0)
Would it make sense to also add a for_each_sgtable_page helper that
hides the use of orig_nents? To be used like:
for_each_sgtable_page(st, &sg_iter, 0) {
> + for_each_sg(sgt->sgl, sg, sgt->orig_nents, count) {
Same here, e.g.
for_each_sgtable_entry(sgt, sg, count) {
?
Hi Christoph,
On 05.05.2020 12:15, Christoph Hellwig wrote:
>> - for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
>> + for_each_sg_page(st->sgl, &sg_iter, st->orig_nents, 0)
> Would it make sense to also add a for_each_sgtable_page helper that
> hides the use of orig_nents? To be used like:
>
> for_each_sgtable_page(st, &sg_iter, 0) {
We would need two helpers:
for_each_sgtable_cpu_page() and for_each_sgtable_dma_page().
I considered them, but then I found that there are already
for_each_sg_page(), for_each_sg_dma_page() and various special iterators
like sg_page_iter, sg_dma_page_iter and sg_mapping_iter. Too bad that
they are almost not used, at least in the DRM subsystem. I wonder if it
make sense to apply them or simply provide the two above mentioned
wrappers?
>> + for_each_sg(sgt->sgl, sg, sgt->orig_nents, count) {
> Same here, e.g.
>
> for_each_sgtable_entry(sgt, sg, count) {
>
> ?
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Tue, May 05, 2020 at 12:51:58PM +0200, Marek Szyprowski wrote:
> Hi Christoph,
>
> On 05.05.2020 12:15, Christoph Hellwig wrote:
> >> - for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
> >> + for_each_sg_page(st->sgl, &sg_iter, st->orig_nents, 0)
> > Would it make sense to also add a for_each_sgtable_page helper that
> > hides the use of orig_nents? To be used like:
> >
> > for_each_sgtable_page(st, &sg_iter, 0) {
>
> We would need two helpers:
>
> for_each_sgtable_cpu_page() and for_each_sgtable_dma_page().
>
> I considered them, but then I found that there are already
> for_each_sg_page(), for_each_sg_dma_page() and various special iterators
> like sg_page_iter, sg_dma_page_iter and sg_mapping_iter. Too bad that
> they are almost not used, at least in the DRM subsystem. I wonder if it
> make sense to apply them or simply provide the two above mentioned
> wrappers?
None of the helpers helps with passing the right parameters from the
sg_table. So in doube we'd need wrappers for all of the above, or
none..
Hi Christoph,
On 05.05.2020 13:09, Christoph Hellwig wrote:
> On Tue, May 05, 2020 at 12:51:58PM +0200, Marek Szyprowski wrote:
>> On 05.05.2020 12:15, Christoph Hellwig wrote:
>>>> - for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
>>>> + for_each_sg_page(st->sgl, &sg_iter, st->orig_nents, 0)
>>> Would it make sense to also add a for_each_sgtable_page helper that
>>> hides the use of orig_nents? To be used like:
>>>
>>> for_each_sgtable_page(st, &sg_iter, 0) {
>> We would need two helpers:
>>
>> for_each_sgtable_cpu_page() and for_each_sgtable_dma_page().
>>
>> I considered them, but then I found that there are already
>> for_each_sg_page(), for_each_sg_dma_page() and various special iterators
>> like sg_page_iter, sg_dma_page_iter and sg_mapping_iter. Too bad that
>> they are almost not used, at least in the DRM subsystem. I wonder if it
>> make sense to apply them or simply provide the two above mentioned
>> wrappers?
> None of the helpers helps with passing the right parameters from the
> sg_table. So in doube we'd need wrappers for all of the above, or
> none..
I've played a bit with the code and the existing scatterlist iterators -
for_each_sg_page() and for_each_sg_dma_page(). I've found them quite handy!
The biggest advantage of them is that they always iterate over
scatterlist in PAGE_SIZE units, what should make the code much easier to
understand. Here is example of their application to the function that
started this thread:
int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page
**pages,
dma_addr_t *addrs, int max_entries)
{
struct sg_dma_page_iter dma_iter;
struct sg_page_iter page_iter;
if (addrs)
for_each_sgtable_dma_page(sgt, &dma_iter, 0)
*addrs++ = sg_page_iter_dma_address(&dma_iter);
if (pages)
for_each_sgtable_page(sgt, &page_iter, 0)
*pages++ = sg_page_iter_page(&page_iter);
return 0;
}
After applying those iterators where possible (they can be used only for
reading the scatterlist), we would just need to add 2 trivial wrappers
to use them with sg_table objects:
#define for_each_sgtable_page(sgt, piter, pgoffset) \
for_each_sg_page(sgt->sgl, piter, sgt->orig_nents, pgoffset)
#define for_each_sgtable_dma_page(sgt, dma_iter, pgoffset) \
for_each_sg_dma_page(sgt->sgl, dma_iter, sgt->nents, pgoffset)
Then we would just need one more helper to construct scatterlist, as the
above two are read-only don't allow to modify scatterlist:
#define for_each_sgtable_sg(sgt, sg, i) \
for_each_sg(sgt->sgl, sg, sgt->orig_nents, i)
With the above 3 helpers we can probably get rid of all instances of
sg_table->{nents,orig_nents} from the DRM code. I will prepare patches soon.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
On Fri, May 08, 2020 at 09:12:13AM +0200, Marek Szyprowski wrote:
> Then we would just need one more helper to construct scatterlist, as the
> above two are read-only don't allow to modify scatterlist:
>
> #define for_each_sgtable_sg(sgt, sg, i)??????????????? \
> ?????? for_each_sg(sgt->sgl, sg, sgt->orig_nents, i)
>
> With the above 3 helpers we can probably get rid of all instances of
> sg_table->{nents,orig_nents} from the DRM code. I will prepare patches soon.
Sounds great, thanks!
Hi Christoph,
On 08.05.2020 09:16, Christoph Hellwig wrote:
> On Fri, May 08, 2020 at 09:12:13AM +0200, Marek Szyprowski wrote:
>> Then we would just need one more helper to construct scatterlist, as the
>> above two are read-only don't allow to modify scatterlist:
>>
>> #define for_each_sgtable_sg(sgt, sg, i) \
>> for_each_sg(sgt->sgl, sg, sgt->orig_nents, i)
>>
>> With the above 3 helpers we can probably get rid of all instances of
>> sg_table->{nents,orig_nents} from the DRM code. I will prepare patches soon.
> Sounds great, thanks!
It turned out that the 4th helper (for_each_sgtable_dma_sg) was needed
as some drivers makes use of the larger than the PAGE_SIZE unit for DMA
mapped pages.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland