| Message ID | 20200325090741.21957-2-bigbeeshane@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | AMDGPU / RADEON / DRM Fix mapping of user pages | expand |
>-----Original Message----- >From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of >Shane Francis >Sent: Wednesday, March 25, 2020 5:08 AM >To: dri-devel@lists.freedesktop.org >Cc: airlied@linux.ie; linux-kernel@vger.kernel.org; bigbeeshane@gmail.com; >amd-gfx-request@lists.freedesktop.org; alexander.deucher@amd.com; >christian.koenig@amd.com >Subject: [PATCH v4 1/3] drm/prime: use dma length macro when mapping sg > >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> >--- > 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); This looks correct to me. Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> M > >-- >2.26.0 > >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
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
Am 27.03.20 um 08:54 schrieb Marek Szyprowski: > 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 :( Well it is at least better than before because it makes most drivers work correctly again. See we only fill the pages array because some drivers (like Exynos) are still buggy and require this. Accessing the pages of an DMA-buf imported sg_table is illegal and should be fixed in the drivers. > [SNIP] > > I will send a patch in a few minutes with the above fixed code. That is certainly a good idea for now, but could we also put on somebodies todo list an item to fix Exynos? Thanks in advance, Christian. > > Best regards
On 2020-03-27 08:54, Marek Szyprowski wrote: > 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. Here is the fix: https://patchwork.freedesktop.org/patch/359081/ Best regards
Hi Christian, On 2020-03-27 09:11, Christian König wrote: > Am 27.03.20 um 08:54 schrieb Marek Szyprowski: >> 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 :( > > Well it is at least better than before because it makes most drivers > work correctly again. Well, I'm not sure that a half-broken fix should be considered as a fix ;) Anyway, I just got the comment from Shane, that my patch is fixing the issues with amdgpu and radeon, while still working fine for exynos, so it is indeed a proper fix. > See we only fill the pages array because some drivers (like Exynos) > are still buggy and require this. Exynos driver use this pages array internally. > > Accessing the pages of an DMA-buf imported sg_table is illegal and > should be fixed in the drivers. True, but in meantime we should avoid breaking stuff which worked fine for ages. > >> [SNIP] >> >> I will send a patch in a few minutes with the above fixed code. > > That is certainly a good idea for now, but could we also put on > somebodies todo list an item to fix Exynos? Yes, I can take a look into removing the use of the internal pages array. It is used mainly for implementing vmap for fbdev emulation, but this can be handled in a different way. Best regards
Hi On 2020-03-27 10:10, Marek Szyprowski wrote: > Hi Christian, > > On 2020-03-27 09:11, Christian König wrote: >> Am 27.03.20 um 08:54 schrieb Marek Szyprowski: >>> 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 :( >> >> Well it is at least better than before because it makes most drivers >> work correctly again. > > Well, I'm not sure that a half-broken fix should be considered as a > fix ;) > > Anyway, I just got the comment from Shane, that my patch is fixing the > issues with amdgpu and radeon, while still working fine for exynos, so > it is indeed a proper fix. Today I've noticed that this patch went to final v5.6 without even a day of testing in linux-next, so v5.6 is broken on Exynos and probably a few other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays function. Best regards
On Mon, Mar 30, 2020 at 9:18 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Today I've noticed that this patch went to final v5.6 without even a day > of testing in linux-next, so v5.6 is broken on Exynos and probably a few > other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays > function. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > Not sure what the full merge pipeline is here, but my original patch was not sent to the stable mailing list. So I would assume that it went through the normal release gates / checks ? If there was a fault on the way I uploaded the patches I do apologise however I did follow the normal guidelines as far as I understood them. Personally I did validate this patch on systems with Intel and AMD GFX, unfortunately I do not have any ARM based dev kits that are able to run mainline (was never able to get an Nvidia TK1 to boot outside of L4T)
On Mon, Mar 30, 2020 at 9:18 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Today I've noticed that this patch went to final v5.6 without even a day > of testing in linux-next, so v5.6 is broken on Exynos and probably a few > other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays > function. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
On Mon, Mar 30, 2020 at 9:18 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Today I've noticed that this patch went to final v5.6 without even a day > of testing in linux-next, so v5.6 is broken on Exynos and probably a few > other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays > function. > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland > FYI These changes are now in the 5.5-stable queue.
On Mon, Mar 30, 2020 at 4:18 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi > > On 2020-03-27 10:10, Marek Szyprowski wrote: > > Hi Christian, > > > > On 2020-03-27 09:11, Christian König wrote: > >> Am 27.03.20 um 08:54 schrieb Marek Szyprowski: > >>> 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 :( > >> > >> Well it is at least better than before because it makes most drivers > >> work correctly again. > > > > Well, I'm not sure that a half-broken fix should be considered as a > > fix ;) > > > > Anyway, I just got the comment from Shane, that my patch is fixing the > > issues with amdgpu and radeon, while still working fine for exynos, so > > it is indeed a proper fix. > > Today I've noticed that this patch went to final v5.6 without even a day > of testing in linux-next, so v5.6 is broken on Exynos and probably a few > other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays > function. Please commit your patch and cc stable. Alex > > 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
Hi Alex, On 2020-03-30 15:23, Alex Deucher wrote: > On Mon, Mar 30, 2020 at 4:18 AM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> Hi >> >> On 2020-03-27 10:10, Marek Szyprowski wrote: >>> Hi Christian, >>> >>> On 2020-03-27 09:11, Christian König wrote: >>>> Am 27.03.20 um 08:54 schrieb Marek Szyprowski: >>>>> 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 :( >>>> Well it is at least better than before because it makes most drivers >>>> work correctly again. >>> Well, I'm not sure that a half-broken fix should be considered as a >>> fix ;) >>> >>> Anyway, I just got the comment from Shane, that my patch is fixing the >>> issues with amdgpu and radeon, while still working fine for exynos, so >>> it is indeed a proper fix. >> Today I've noticed that this patch went to final v5.6 without even a day >> of testing in linux-next, so v5.6 is broken on Exynos and probably a few >> other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays >> function. > Please commit your patch and cc stable. I've already did that: https://lkml.org/lkml/2020/3/27/555 Best regards
On Tue, Mar 31, 2020 at 1:25 AM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi Alex, > > On 2020-03-30 15:23, Alex Deucher wrote: > > On Mon, Mar 30, 2020 at 4:18 AM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> Hi > >> > >> On 2020-03-27 10:10, Marek Szyprowski wrote: > >>> Hi Christian, > >>> > >>> On 2020-03-27 09:11, Christian König wrote: > >>>> Am 27.03.20 um 08:54 schrieb Marek Szyprowski: > >>>>> 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 :( > >>>> Well it is at least better than before because it makes most drivers > >>>> work correctly again. > >>> Well, I'm not sure that a half-broken fix should be considered as a > >>> fix ;) > >>> > >>> Anyway, I just got the comment from Shane, that my patch is fixing the > >>> issues with amdgpu and radeon, while still working fine for exynos, so > >>> it is indeed a proper fix. > >> Today I've noticed that this patch went to final v5.6 without even a day > >> of testing in linux-next, so v5.6 is broken on Exynos and probably a few > >> other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays > >> function. > > Please commit your patch and cc stable. > > I've already did that: https://lkml.org/lkml/2020/3/27/555 Do you have drm-misc commit rights or do you need someone to commit this for you? Alex > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
Hi Alex, On 2020-03-31 16:10, Alex Deucher wrote: > On Tue, Mar 31, 2020 at 1:25 AM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> Hi Alex, >> >> On 2020-03-30 15:23, Alex Deucher wrote: >>> On Mon, Mar 30, 2020 at 4:18 AM Marek Szyprowski >>> <m.szyprowski@samsung.com> wrote: >>>> Hi >>>> >>>> On 2020-03-27 10:10, Marek Szyprowski wrote: >>>>> Hi Christian, >>>>> >>>>> On 2020-03-27 09:11, Christian König wrote: >>>>>> Am 27.03.20 um 08:54 schrieb Marek Szyprowski: >>>>>>> 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 :( >>>>>> Well it is at least better than before because it makes most drivers >>>>>> work correctly again. >>>>> Well, I'm not sure that a half-broken fix should be considered as a >>>>> fix ;) >>>>> >>>>> Anyway, I just got the comment from Shane, that my patch is fixing the >>>>> issues with amdgpu and radeon, while still working fine for exynos, so >>>>> it is indeed a proper fix. >>>> Today I've noticed that this patch went to final v5.6 without even a day >>>> of testing in linux-next, so v5.6 is broken on Exynos and probably a few >>>> other ARM archs, which rely on the drm_prime_sg_to_page_addr_arrays >>>> function. >>> Please commit your patch and cc stable. >> I've already did that: https%3A%2F%2Flkml.org%2Flkml%2F2020%2F3%2F27%2F555 > Do you have drm-misc commit rights or do you need someone to commit > this for you? I have no access to drm-misc. Best regards
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);
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> --- drivers/gpu/drm/drm_prime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)