From: Marek Szyprowski <m.szyprowski@samsung.com> To: Shane Francis <bigbeeshane@gmail.com>, dri-devel@lists.freedesktop.org Cc: airlied@linux.ie, linux-kernel@vger.kernel.org, amd-gfx-request@lists.freedesktop.org, alexander.deucher@amd.com, christian.koenig@amd.com Subject: Re: [v4,1/3] drm/prime: use dma length macro when mapping sg Date: Fri, 27 Mar 2020 08:54:53 +0100 [thread overview] Message-ID: <4aef60ff-d9e4-d3d0-1a28-8c2dc3b94271@samsung.com> (raw) In-Reply-To: <20200325090741.21957-2-bigbeeshane@gmail.com> Hi All, On 2020-03-25 10:07, Shane Francis wrote: > As dma_map_sg can reorganize scatter-gather lists in a > way that can cause some later segments to be empty we should > always use the sg_dma_len macro to fetch the actual length. > > This could now be 0 and not need to be mapped to a page or > address array > > Signed-off-by: Shane Francis <bigbeeshane@gmail.com> > Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> This patch landed in linux-next 20200326 and it causes a kernel panic on various Exynos SoC based boards. > --- > drivers/gpu/drm/drm_prime.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 86d9b0e45c8c..1de2cde2277c 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -967,7 +967,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, > > index = 0; > for_each_sg(sgt->sgl, sg, sgt->nents, count) { > - len = sg->length; > + len = sg_dma_len(sg); > page = sg_page(sg); > addr = sg_dma_address(sg); > Sorry, but this code is wrong :( The scatterlist elements (sg) describes memory chunks in physical memory and in the DMA (IO virtual) space. However in general, you cannot assume 1:1 mapping between them. If you access sg_page(sg) (basically sg->page), you must match it with sg->length. When you access sg_dma_address(sg) (again, in most cases it is sg->dma_address), then you must match it with sg_dma_len(sg). The sg->dma_address might not be the dma address of the sg->page. In some cases (when IOMMU is available, it performs aggregation of the scatterlist chunks and a few other, minor requirements), the whole scatterlist might be mapped into contiguous DMA address space and filled only to the first sg element. The proper way to iterate over a scatterlists to get both the pages and the DMA addresses assigned to them is: int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries) { unsigned count; struct scatterlist *sg; struct page *page; u32 page_len, page_index; dma_addr_t addr; u32 dma_len, dma_index; page_index = 0; dma_index = 0; for_each_sg(sgt->sgl, sg, sgt->nents, count) { page_len = sg->length; page = sg_page(sg); dma_len = sg_dma_len(sg); addr = sg_dma_address(sg); while (pages && page_len > 0) { if (WARN_ON(page_index >= max_entries)) return -1; pages[page_index] = page; page++; page_len -= PAGE_SIZE; page_index++; } while (addrs && dma_len > 0) { if (WARN_ON(dma_index >= max_entries)) return -1; addrs[dma_index] = addr; addr += PAGE_SIZE; dma_len -= PAGE_SIZE; dma_index++; } } return 0; } I will send a patch in a few minutes with the above fixed code. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
WARNING: multiple messages have this Message-ID (diff)
From: Marek Szyprowski <m.szyprowski@samsung.com> To: Shane Francis <bigbeeshane@gmail.com>, dri-devel@lists.freedesktop.org Cc: airlied@linux.ie, alexander.deucher@amd.com, linux-kernel@vger.kernel.org, amd-gfx-request@lists.freedesktop.org, christian.koenig@amd.com Subject: Re: [v4,1/3] drm/prime: use dma length macro when mapping sg Date: Fri, 27 Mar 2020 08:54:53 +0100 [thread overview] Message-ID: <4aef60ff-d9e4-d3d0-1a28-8c2dc3b94271@samsung.com> (raw) In-Reply-To: <20200325090741.21957-2-bigbeeshane@gmail.com> Hi All, On 2020-03-25 10:07, Shane Francis wrote: > As dma_map_sg can reorganize scatter-gather lists in a > way that can cause some later segments to be empty we should > always use the sg_dma_len macro to fetch the actual length. > > This could now be 0 and not need to be mapped to a page or > address array > > Signed-off-by: Shane Francis <bigbeeshane@gmail.com> > Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> This patch landed in linux-next 20200326 and it causes a kernel panic on various Exynos SoC based boards. > --- > drivers/gpu/drm/drm_prime.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index 86d9b0e45c8c..1de2cde2277c 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -967,7 +967,7 @@ int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, > > index = 0; > for_each_sg(sgt->sgl, sg, sgt->nents, count) { > - len = sg->length; > + len = sg_dma_len(sg); > page = sg_page(sg); > addr = sg_dma_address(sg); > Sorry, but this code is wrong :( The scatterlist elements (sg) describes memory chunks in physical memory and in the DMA (IO virtual) space. However in general, you cannot assume 1:1 mapping between them. If you access sg_page(sg) (basically sg->page), you must match it with sg->length. When you access sg_dma_address(sg) (again, in most cases it is sg->dma_address), then you must match it with sg_dma_len(sg). The sg->dma_address might not be the dma address of the sg->page. In some cases (when IOMMU is available, it performs aggregation of the scatterlist chunks and a few other, minor requirements), the whole scatterlist might be mapped into contiguous DMA address space and filled only to the first sg element. The proper way to iterate over a scatterlists to get both the pages and the DMA addresses assigned to them is: int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page **pages, dma_addr_t *addrs, int max_entries) { unsigned count; struct scatterlist *sg; struct page *page; u32 page_len, page_index; dma_addr_t addr; u32 dma_len, dma_index; page_index = 0; dma_index = 0; for_each_sg(sgt->sgl, sg, sgt->nents, count) { page_len = sg->length; page = sg_page(sg); dma_len = sg_dma_len(sg); addr = sg_dma_address(sg); while (pages && page_len > 0) { if (WARN_ON(page_index >= max_entries)) return -1; pages[page_index] = page; page++; page_len -= PAGE_SIZE; page_index++; } while (addrs && dma_len > 0) { if (WARN_ON(dma_index >= max_entries)) return -1; addrs[dma_index] = addr; addr += PAGE_SIZE; dma_len -= PAGE_SIZE; dma_index++; } } return 0; } I will send a patch in a few minutes with the above fixed code. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-03-27 7:55 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-25 9:07 [PATCH v4 0/3] AMDGPU / RADEON / DRM Fix mapping of user pages Shane Francis 2020-03-25 9:07 ` Shane Francis 2020-03-25 9:07 ` [PATCH v4 1/3] drm/prime: use dma length macro when mapping sg Shane Francis 2020-03-25 9:07 ` Shane Francis 2020-03-25 13:56 ` Ruhl, Michael J 2020-03-25 13:56 ` Ruhl, Michael J [not found] ` <CGME20200327075458eucas1p2f1011560c5d2d2a754d2394f56367ebb@eucas1p2.samsung.com> 2020-03-27 7:54 ` Marek Szyprowski [this message] 2020-03-27 7:54 ` [v4,1/3] " Marek Szyprowski 2020-03-27 8:11 ` Christian König 2020-03-27 8:11 ` Christian König 2020-03-27 9:10 ` Marek Szyprowski 2020-03-27 9:10 ` Marek Szyprowski 2020-03-30 8:18 ` Marek Szyprowski 2020-03-30 8:18 ` Marek Szyprowski 2020-03-30 9:39 ` Shane Francis 2020-03-30 9:39 ` Shane Francis 2020-03-30 12:32 ` Shane Francis 2020-03-30 12:32 ` Shane Francis 2020-03-30 12:34 ` Shane Francis 2020-03-30 12:34 ` Shane Francis 2020-03-30 13:23 ` Alex Deucher 2020-03-30 13:23 ` Alex Deucher 2020-03-31 5:25 ` Marek Szyprowski 2020-03-31 5:25 ` Marek Szyprowski 2020-03-31 14:10 ` Alex Deucher 2020-03-31 14:10 ` Alex Deucher 2020-03-31 14:29 ` Marek Szyprowski 2020-03-31 14:29 ` Marek Szyprowski [not found] ` <CGME20200327082702eucas1p1a66dd059480c9d02b8f0f3ab225db2c3@eucas1p1.samsung.com> 2020-03-27 8:26 ` Marek Szyprowski 2020-03-27 8:26 ` Marek Szyprowski 2020-03-25 9:07 ` [PATCH v4 2/3] drm/amdgpu: fix scatter-gather mapping with user pages Shane Francis 2020-03-25 9:07 ` Shane Francis 2020-03-25 14:00 ` Ruhl, Michael J 2020-03-25 14:00 ` Ruhl, Michael J 2020-03-25 9:07 ` [PATCH v4 3/3] drm/radeon: " Shane Francis 2020-03-25 9:07 ` Shane Francis 2020-03-25 14:19 ` Ruhl, Michael J 2020-03-25 14:19 ` Ruhl, Michael J 2020-03-25 15:54 ` Shane Francis 2020-03-25 15:54 ` Shane Francis 2020-03-25 16:08 ` Alex Deucher 2020-03-25 16:08 ` Alex Deucher 2020-03-25 13:11 ` [PATCH v4 0/3] AMDGPU / RADEON / DRM Fix mapping of " Ruhl, Michael J 2020-03-25 13:11 ` Ruhl, Michael J
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=4aef60ff-d9e4-d3d0-1a28-8c2dc3b94271@samsung.com \ --to=m.szyprowski@samsung.com \ --cc=airlied@linux.ie \ --cc=alexander.deucher@amd.com \ --cc=amd-gfx-request@lists.freedesktop.org \ --cc=bigbeeshane@gmail.com \ --cc=christian.koenig@amd.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-kernel@vger.kernel.org \ /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: linkBe 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.