[PATCH] drm/panfrost: Split io-pgtable requests properly

[PATCH] drm/panfrost: Split io-pgtable requests properly

[Robin Murphy]

Although we don't use 1GB block mappings, we still need to split
map/unmap requests at 1GB boundaries to match what io-pgtable expects.
Fix that, and add some explanation to make sense of it all.

Fixes: 3740b081795a ("drm/panfrost: Update io-pgtable API")
Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
The previous diff turned out to be not quite right, so I've not
included Dmitry's Tested-by given for that.
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index e246d914e7f6..4e83a1891f3e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -250,13 +250,22 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
 
 static size_t get_pgsize(u64 addr, size_t size, size_t *count)
 {
+	/*
+	 * io-pgtable only operates on multiple pages within a single table
+	 * entry, so we need to split at boundaries of the table size, i.e.
+	 * the next block size up. The distance from address A to the next
+	 * boundary of block size B is logically B - A % B, but in unsigned
+	 * two's complement where B is a power of two we get the equivalence
+	 * B - A % B == (B - A) % B == (n * B - A) % B, and choose n = 0 :)
+	 */
 	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;
+	blk_offset = -addr % SZ_1G ?: SZ_1G;
+	*count = min(blk_offset, size) / SZ_2M;
 	return SZ_2M;
 }
 
-- 
2.36.1.dirty

Re: [PATCH] drm/panfrost: Split io-pgtable requests properly

[Dmitry Osipenko]

On 11/8/22 20:06, Robin Murphy wrote:
> Although we don't use 1GB block mappings, we still need to split
> map/unmap requests at 1GB boundaries to match what io-pgtable expects.
> Fix that, and add some explanation to make sense of it all.
> 
> Fixes: 3740b081795a ("drm/panfrost: Update io-pgtable API")
> Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> The previous diff turned out to be not quite right, so I've not
> included Dmitry's Tested-by given for that.
> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index e246d914e7f6..4e83a1891f3e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -250,13 +250,22 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
>  
>  static size_t get_pgsize(u64 addr, size_t size, size_t *count)
>  {
> +	/*
> +	 * io-pgtable only operates on multiple pages within a single table
> +	 * entry, so we need to split at boundaries of the table size, i.e.
> +	 * the next block size up. The distance from address A to the next
> +	 * boundary of block size B is logically B - A % B, but in unsigned
> +	 * two's complement where B is a power of two we get the equivalence
> +	 * B - A % B == (B - A) % B == (n * B - A) % B, and choose n = 0 :)
> +	 */
>  	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;
> +	blk_offset = -addr % SZ_1G ?: SZ_1G;
> +	*count = min(blk_offset, size) / SZ_2M;
>  	return SZ_2M;
>  }
>  

This variant also works fine, thanks!

Tested-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>

-- 
Best regards,
Dmitry

Re: [PATCH] drm/panfrost: Split io-pgtable requests properly

[Steven Price]

On 08/11/2022 17:06, Robin Murphy wrote:
> Although we don't use 1GB block mappings, we still need to split
> map/unmap requests at 1GB boundaries to match what io-pgtable expects.
> Fix that, and add some explanation to make sense of it all.
> 
> Fixes: 3740b081795a ("drm/panfrost: Update io-pgtable API")
> Reported-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Steven Price <steven.price@arm.com>

I'll push to drm-misc-fixes.

Thanks,

Steve

> ---
> The previous diff turned out to be not quite right, so I've not
> included Dmitry's Tested-by given for that.
> ---
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index e246d914e7f6..4e83a1891f3e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -250,13 +250,22 @@ void panfrost_mmu_reset(struct panfrost_device *pfdev)
>  
>  static size_t get_pgsize(u64 addr, size_t size, size_t *count)
>  {
> +	/*
> +	 * io-pgtable only operates on multiple pages within a single table
> +	 * entry, so we need to split at boundaries of the table size, i.e.
> +	 * the next block size up. The distance from address A to the next
> +	 * boundary of block size B is logically B - A % B, but in unsigned
> +	 * two's complement where B is a power of two we get the equivalence
> +	 * B - A % B == (B - A) % B == (n * B - A) % B, and choose n = 0 :)
> +	 */
>  	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;
> +	blk_offset = -addr % SZ_1G ?: SZ_1G;
> +	*count = min(blk_offset, size) / SZ_2M;
>  	return SZ_2M;
>  }
>