From: Robin Murphy <robin.murphy@arm.com> To: Dmitry Osipenko <dmitry.osipenko@collabora.com>, robh@kernel.org, tomeu.vizoso@collabora.com Cc: linux-arm-kernel@lists.infradead.org, alyssa.rosenzweig@collabora.com, dri-devel@lists.freedesktop.org, steven.price@arm.com Subject: Re: [PATCH] drm/panfrost: Update io-pgtable API Date: Mon, 7 Nov 2022 16:50:38 +0000 [thread overview] Message-ID: <26572537-a41f-6a95-1700-3643636f02a0@arm.com> (raw) In-Reply-To: <df8a6d56-823d-936a-1ce8-faed48c0e338@collabora.com> On 2022-11-04 20:48, Dmitry Osipenko wrote: > On 11/4/22 23:37, Robin Murphy wrote: >> On 2022-11-04 20:11, Dmitry Osipenko wrote: >>> On 8/23/22 01:01, Robin Murphy wrote: >>>> Convert to io-pgtable's bulk {map,unmap}_pages() APIs, to help the old >>>> single-page interfaces eventually go away. Unmapping heap BOs still >>>> wants to be done a page at a time, but everything else can get the full >>>> benefit of the more efficient interface. >>>> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>>> --- >>>> drivers/gpu/drm/panfrost/panfrost_mmu.c | 40 +++++++++++++++---------- >>>> 1 file changed, 25 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> index b285a8001b1d..e246d914e7f6 100644 >>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> @@ -248,11 +248,15 @@ void panfrost_mmu_reset(struct panfrost_device >>>> *pfdev) >>>> mmu_write(pfdev, MMU_INT_MASK, ~0); >>>> } >>>> -static size_t get_pgsize(u64 addr, size_t size) >>>> +static size_t get_pgsize(u64 addr, size_t size, size_t *count) >>>> { >>>> - if (addr & (SZ_2M - 1) || size < SZ_2M) >>>> - return SZ_4K; >>>> + size_t blk_offset = -addr % SZ_2M; >>>> + if (blk_offset || size < SZ_2M) { >>>> + *count = min_not_zero(blk_offset, size) / SZ_4K; >>>> + return SZ_4K; >>>> + } >>>> + *count = size / SZ_2M; >>>> return SZ_2M; >>>> } >>>> @@ -287,12 +291,16 @@ static int mmu_map_sg(struct panfrost_device >>>> *pfdev, struct panfrost_mmu *mmu, >>>> dev_dbg(pfdev->dev, "map: as=%d, iova=%llx, paddr=%lx, >>>> len=%zx", mmu->as, iova, paddr, len); >>>> while (len) { >>>> - size_t pgsize = get_pgsize(iova | paddr, len); >>>> + size_t pgcount, mapped = 0; >>>> + size_t pgsize = get_pgsize(iova | paddr, len, &pgcount); >>>> - ops->map(ops, iova, paddr, pgsize, prot, GFP_KERNEL); >>>> - iova += pgsize; >>>> - paddr += pgsize; >>>> - len -= pgsize; >>>> + ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot, >>>> + GFP_KERNEL, &mapped); >>>> + /* Don't get stuck if things have gone wrong */ >>>> + mapped = max(mapped, pgsize); >>>> + iova += mapped; >>>> + paddr += mapped; >>>> + len -= mapped; >>>> } >>>> } >>>> @@ -344,15 +352,17 @@ void panfrost_mmu_unmap(struct >>>> panfrost_gem_mapping *mapping) >>>> mapping->mmu->as, iova, len); >>>> while (unmapped_len < len) { >>>> - size_t unmapped_page; >>>> - size_t pgsize = get_pgsize(iova, len - unmapped_len); >>>> + size_t unmapped_page, pgcount; >>>> + size_t pgsize = get_pgsize(iova, len - unmapped_len, &pgcount); >>>> - if (ops->iova_to_phys(ops, iova)) { >>>> - unmapped_page = ops->unmap(ops, iova, pgsize, NULL); >>>> - WARN_ON(unmapped_page != pgsize); >>>> + if (bo->is_heap) >>>> + pgcount = 1; >>>> + if (!bo->is_heap || ops->iova_to_phys(ops, iova)) { >>>> + unmapped_page = ops->unmap_pages(ops, iova, pgsize, >>>> pgcount, NULL); >>>> + WARN_ON(unmapped_page != pgsize * pgcount); >>> >>> This patch causes this WARN_ON to trigger. It doesn't happen all the >>> time, I see that the whole unmapped area is mapped. Initially, I thought >>> that this happens because it tries to unmap a partially mapped range, >>> but I checked that ops->iova_to_phys() returns address for all 4k chunks. >>> >>> For example the pgsize * pgcount = 0x8000000, while returned >>> unmapped_page = 0x6000000. >>> >>> I don't see this problem with this patch reverted. This is using today's >>> linux-next. Any ideas? >> >> What's the base IOVA in such a case? I'm wondering if the truncated size >> lines up to any interesting boundary. Presumably you're not seeing any >> additional warnings from io-pgtable itself? > > No warnings from io-pgtable. It succeeds for 0x32000000 and fails for > 0x3a000000 using same size 0x8000000. It actually fails only for the > 0x3a000000 as far as I see from my logs. Perhaps it indeed has to do > something with the boundary. Bleh, indeed even though we don't use 1GB block mappings, we still need to split at 1GB boundaries to match what the IOMMU API will do, and thus what io-pgtable expects. I guess I hadn't really considered that we might ever have that much graphics memory in play at once... The fix probably looks like this: diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index e246d914e7f6..6abc7d3726dd 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -256,7 +256,9 @@ static size_t get_pgsize(u64 addr, size_t size, size_t *count) *count = min_not_zero(blk_offset, size) / SZ_4K; return SZ_4K; } - *count = size / SZ_2M; + + blk_offset = -addr % SZ_1G; + *count = min_not_zero(blk_offset, size) / SZ_2M; return SZ_2M; } Thanks, Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
WARNING: multiple messages have this Message-ID (diff)
From: Robin Murphy <robin.murphy@arm.com> To: Dmitry Osipenko <dmitry.osipenko@collabora.com>, robh@kernel.org, tomeu.vizoso@collabora.com Cc: dri-devel@lists.freedesktop.org, alyssa.rosenzweig@collabora.com, linux-arm-kernel@lists.infradead.org, steven.price@arm.com Subject: Re: [PATCH] drm/panfrost: Update io-pgtable API Date: Mon, 7 Nov 2022 16:50:38 +0000 [thread overview] Message-ID: <26572537-a41f-6a95-1700-3643636f02a0@arm.com> (raw) In-Reply-To: <df8a6d56-823d-936a-1ce8-faed48c0e338@collabora.com> On 2022-11-04 20:48, Dmitry Osipenko wrote: > On 11/4/22 23:37, Robin Murphy wrote: >> On 2022-11-04 20:11, Dmitry Osipenko wrote: >>> On 8/23/22 01:01, Robin Murphy wrote: >>>> Convert to io-pgtable's bulk {map,unmap}_pages() APIs, to help the old >>>> single-page interfaces eventually go away. Unmapping heap BOs still >>>> wants to be done a page at a time, but everything else can get the full >>>> benefit of the more efficient interface. >>>> >>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com> >>>> --- >>>> drivers/gpu/drm/panfrost/panfrost_mmu.c | 40 +++++++++++++++---------- >>>> 1 file changed, 25 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> index b285a8001b1d..e246d914e7f6 100644 >>>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c >>>> @@ -248,11 +248,15 @@ void panfrost_mmu_reset(struct panfrost_device >>>> *pfdev) >>>> mmu_write(pfdev, MMU_INT_MASK, ~0); >>>> } >>>> -static size_t get_pgsize(u64 addr, size_t size) >>>> +static size_t get_pgsize(u64 addr, size_t size, size_t *count) >>>> { >>>> - if (addr & (SZ_2M - 1) || size < SZ_2M) >>>> - return SZ_4K; >>>> + size_t blk_offset = -addr % SZ_2M; >>>> + if (blk_offset || size < SZ_2M) { >>>> + *count = min_not_zero(blk_offset, size) / SZ_4K; >>>> + return SZ_4K; >>>> + } >>>> + *count = size / SZ_2M; >>>> return SZ_2M; >>>> } >>>> @@ -287,12 +291,16 @@ static int mmu_map_sg(struct panfrost_device >>>> *pfdev, struct panfrost_mmu *mmu, >>>> dev_dbg(pfdev->dev, "map: as=%d, iova=%llx, paddr=%lx, >>>> len=%zx", mmu->as, iova, paddr, len); >>>> while (len) { >>>> - size_t pgsize = get_pgsize(iova | paddr, len); >>>> + size_t pgcount, mapped = 0; >>>> + size_t pgsize = get_pgsize(iova | paddr, len, &pgcount); >>>> - ops->map(ops, iova, paddr, pgsize, prot, GFP_KERNEL); >>>> - iova += pgsize; >>>> - paddr += pgsize; >>>> - len -= pgsize; >>>> + ops->map_pages(ops, iova, paddr, pgsize, pgcount, prot, >>>> + GFP_KERNEL, &mapped); >>>> + /* Don't get stuck if things have gone wrong */ >>>> + mapped = max(mapped, pgsize); >>>> + iova += mapped; >>>> + paddr += mapped; >>>> + len -= mapped; >>>> } >>>> } >>>> @@ -344,15 +352,17 @@ void panfrost_mmu_unmap(struct >>>> panfrost_gem_mapping *mapping) >>>> mapping->mmu->as, iova, len); >>>> while (unmapped_len < len) { >>>> - size_t unmapped_page; >>>> - size_t pgsize = get_pgsize(iova, len - unmapped_len); >>>> + size_t unmapped_page, pgcount; >>>> + size_t pgsize = get_pgsize(iova, len - unmapped_len, &pgcount); >>>> - if (ops->iova_to_phys(ops, iova)) { >>>> - unmapped_page = ops->unmap(ops, iova, pgsize, NULL); >>>> - WARN_ON(unmapped_page != pgsize); >>>> + if (bo->is_heap) >>>> + pgcount = 1; >>>> + if (!bo->is_heap || ops->iova_to_phys(ops, iova)) { >>>> + unmapped_page = ops->unmap_pages(ops, iova, pgsize, >>>> pgcount, NULL); >>>> + WARN_ON(unmapped_page != pgsize * pgcount); >>> >>> This patch causes this WARN_ON to trigger. It doesn't happen all the >>> time, I see that the whole unmapped area is mapped. Initially, I thought >>> that this happens because it tries to unmap a partially mapped range, >>> but I checked that ops->iova_to_phys() returns address for all 4k chunks. >>> >>> For example the pgsize * pgcount = 0x8000000, while returned >>> unmapped_page = 0x6000000. >>> >>> I don't see this problem with this patch reverted. This is using today's >>> linux-next. Any ideas? >> >> What's the base IOVA in such a case? I'm wondering if the truncated size >> lines up to any interesting boundary. Presumably you're not seeing any >> additional warnings from io-pgtable itself? > > No warnings from io-pgtable. It succeeds for 0x32000000 and fails for > 0x3a000000 using same size 0x8000000. It actually fails only for the > 0x3a000000 as far as I see from my logs. Perhaps it indeed has to do > something with the boundary. Bleh, indeed even though we don't use 1GB block mappings, we still need to split at 1GB boundaries to match what the IOMMU API will do, and thus what io-pgtable expects. I guess I hadn't really considered that we might ever have that much graphics memory in play at once... The fix probably looks like this: diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c index e246d914e7f6..6abc7d3726dd 100644 --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c @@ -256,7 +256,9 @@ static size_t get_pgsize(u64 addr, size_t size, size_t *count) *count = min_not_zero(blk_offset, size) / SZ_4K; return SZ_4K; } - *count = size / SZ_2M; + + blk_offset = -addr % SZ_1G; + *count = min_not_zero(blk_offset, size) / SZ_2M; return SZ_2M; } Thanks, Robin.
next prev parent reply other threads:[~2022-11-07 16:52 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-22 22:01 [PATCH] drm/panfrost: Update io-pgtable API Robin Murphy 2022-08-22 22:01 ` Robin Murphy 2022-08-23 2:51 ` Alyssa Rosenzweig 2022-08-23 2:51 ` Alyssa Rosenzweig 2022-08-23 10:42 ` Robin Murphy 2022-08-23 10:42 ` Robin Murphy 2022-08-31 12:49 ` Alyssa Rosenzweig 2022-08-31 12:49 ` Alyssa Rosenzweig 2022-09-01 10:17 ` Steven Price 2022-09-01 10:17 ` Steven Price 2022-11-04 20:11 ` Dmitry Osipenko 2022-11-04 20:11 ` Dmitry Osipenko 2022-11-04 20:37 ` Robin Murphy 2022-11-04 20:37 ` Robin Murphy 2022-11-04 20:48 ` Dmitry Osipenko 2022-11-04 20:48 ` Dmitry Osipenko 2022-11-07 16:50 ` Robin Murphy [this message] 2022-11-07 16:50 ` Robin Murphy 2022-11-08 14:11 ` Dmitry Osipenko 2022-11-08 14:11 ` Dmitry Osipenko
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=26572537-a41f-6a95-1700-3643636f02a0@arm.com \ --to=robin.murphy@arm.com \ --cc=alyssa.rosenzweig@collabora.com \ --cc=dmitry.osipenko@collabora.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=robh@kernel.org \ --cc=steven.price@arm.com \ --cc=tomeu.vizoso@collabora.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: 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.