[PATCH] drm/panfrost: Calculate lock region size correctly

[PATCH] drm/panfrost: Calculate lock region size correctly

[Steven Price]

It turns out that when locking a region, the region must be a naturally
aligned power of 2. The upshot of this is that if the desired region
crosses a 'large boundary' the region size must be increased
significantly to ensure that the locked region completely covers the
desired region. Previous calculations (including in kbase for the
proprietary driver) failed to take this into account.

Since it's known that the lock region must be naturally aligned we can
compute the required size by looking at the highest bit position which
changes between the start/end of the lock region (subtracting 1 from the
end because the end address is exclusive). The start address is then
aligned based on the size (this is technically unnecessary as the
hardware will ignore these bits, but the spec advises to do this "to
avoid confusion").

Signed-off-by: Steven Price <steven.price@arm.com>
---
See previous discussion[1] for more details. This bug also existed in
the 'kbase' driver, so it's unlikely to actually hit very often.

This patch is based on drm-misc-next-fixes as it builds on top of
Alyssa's changes to lock_region.

[1] https://lore.kernel.org/dri-devel/6fe675c4-d22b-22da-ba3c-f6d33419b9ed@arm.com/

 drivers/gpu/drm/panfrost/panfrost_mmu.c | 33 +++++++++++++++++++------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index dfe5f1d29763..afec15bb3db5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -58,17 +58,36 @@ static int write_cmd(struct panfrost_device *pfdev, u32 as_nr, u32 cmd)
 }
 
 static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
-			u64 iova, u64 size)
+			u64 region_start, u64 size)
 {
 	u8 region_width;
-	u64 region = iova & PAGE_MASK;
+	u64 region;
+	u64 region_size;
+	u64 region_end = region_start + size;
 
-	/* The size is encoded as ceil(log2) minus(1), which may be calculated
-	 * with fls. The size must be clamped to hardware bounds.
+	if (!size)
+		return;
+
+	/*
+	 * The locked region is a naturally aligned power of 2 block encoded as
+	 * log2 minus(1).
+	 * Calculate the desired start/end and look for the highest bit which
+	 * differs. The smallest naturally aligned block must include this bit
+	 * change the desired region starts with this bit (and subsequent bits)
+	 * zeroed and ends with the bit (and subsequent bits) set to one.
+	 *
 	 */
-	size = max_t(u64, size, AS_LOCK_REGION_MIN_SIZE);
-	region_width = fls64(size - 1) - 1;
-	region |= region_width;
+	region_size = region_start ^ (region_end - 1);
+	region_width = max(fls64(region_size),
+			   const_ilog2(AS_LOCK_REGION_MIN_SIZE)) - 1;
+
+	/*
+	 * Mask off the low bits of region_start (which would be ignored by
+	 * the hardware anyway)
+	 */
+	region_start &= GENMASK_ULL(63, region_width);
+
+	region = region_width | region_start;
 
 	/* Lock the region that needs to be updated */
 	mmu_write(pfdev, AS_LOCKADDR_LO(as_nr), region & 0xFFFFFFFFUL);
-- 
2.30.2

Re: [PATCH] drm/panfrost: Calculate lock region size correctly

[Boris Brezillon]

On Thu,  2 Sep 2021 15:00:38 +0100
Steven Price <steven.price@arm.com> wrote:

> It turns out that when locking a region, the region must be a naturally
> aligned power of 2. The upshot of this is that if the desired region
> crosses a 'large boundary' the region size must be increased
> significantly to ensure that the locked region completely covers the
> desired region. Previous calculations (including in kbase for the
> proprietary driver) failed to take this into account.
> 
> Since it's known that the lock region must be naturally aligned we can
> compute the required size by looking at the highest bit position which
> changes between the start/end of the lock region (subtracting 1 from the
> end because the end address is exclusive). The start address is then
> aligned based on the size (this is technically unnecessary as the
> hardware will ignore these bits, but the spec advises to do this "to
> avoid confusion").
> 
> Signed-off-by: Steven Price <steven.price@arm.com>
> ---
> See previous discussion[1] for more details. This bug also existed in
> the 'kbase' driver, so it's unlikely to actually hit very often.
> 
> This patch is based on drm-misc-next-fixes as it builds on top of
> Alyssa's changes to lock_region.
> 
> [1] https://lore.kernel.org/dri-devel/6fe675c4-d22b-22da-ba3c-f6d33419b9ed@arm.com/
> 
>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 33 +++++++++++++++++++------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index dfe5f1d29763..afec15bb3db5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -58,17 +58,36 @@ static int write_cmd(struct panfrost_device *pfdev, u32 as_nr, u32 cmd)
>  }
>  
>  static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
> -			u64 iova, u64 size)
> +			u64 region_start, u64 size)
>  {
>  	u8 region_width;
> -	u64 region = iova & PAGE_MASK;
> +	u64 region;
> +	u64 region_size;
> +	u64 region_end = region_start + size;
>  
> -	/* The size is encoded as ceil(log2) minus(1), which may be calculated
> -	 * with fls. The size must be clamped to hardware bounds.
> +	if (!size)
> +		return;
> +
> +	/*
> +	 * The locked region is a naturally aligned power of 2 block encoded as
> +	 * log2 minus(1).
> +	 * Calculate the desired start/end and look for the highest bit which
> +	 * differs. The smallest naturally aligned block must include this bit
> +	 * change the desired region starts with this bit (and subsequent bits)
> +	 * zeroed and ends with the bit (and subsequent bits) set to one.
> +	 *

Nit: you can drop the empty comment line.

>  	 */
> -	size = max_t(u64, size, AS_LOCK_REGION_MIN_SIZE);
> -	region_width = fls64(size - 1) - 1;
> -	region |= region_width;
> +	region_size = region_start ^ (region_end - 1);

Hm, is region_size really encoding the size of the region to lock? I
mean, the logic seems correct but I wonder if it wouldn't be better to
drop the region_size variable and inline
'region_start ^ (region_end - 1)' in the region_width calculation to
avoid confusion.

Looks good otherwise.

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> +	region_width = max(fls64(region_size),
> +			   const_ilog2(AS_LOCK_REGION_MIN_SIZE)) - 1;
> +
> +	/*
> +	 * Mask off the low bits of region_start (which would be ignored by
> +	 * the hardware anyway)
> +	 */
> +	region_start &= GENMASK_ULL(63, region_width);
> +
> +	region = region_width | region_start;
>  
>  	/* Lock the region that needs to be updated */
>  	mmu_write(pfdev, AS_LOCKADDR_LO(as_nr), region & 0xFFFFFFFFUL);

Re: [PATCH] drm/panfrost: Calculate lock region size correctly

[Steven Price]

On 03/09/2021 09:51, Boris Brezillon wrote:
> On Thu,  2 Sep 2021 15:00:38 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
>> It turns out that when locking a region, the region must be a naturally
>> aligned power of 2. The upshot of this is that if the desired region
>> crosses a 'large boundary' the region size must be increased
>> significantly to ensure that the locked region completely covers the
>> desired region. Previous calculations (including in kbase for the
>> proprietary driver) failed to take this into account.
>>
>> Since it's known that the lock region must be naturally aligned we can
>> compute the required size by looking at the highest bit position which
>> changes between the start/end of the lock region (subtracting 1 from the
>> end because the end address is exclusive). The start address is then
>> aligned based on the size (this is technically unnecessary as the
>> hardware will ignore these bits, but the spec advises to do this "to
>> avoid confusion").
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> See previous discussion[1] for more details. This bug also existed in
>> the 'kbase' driver, so it's unlikely to actually hit very often.
>>
>> This patch is based on drm-misc-next-fixes as it builds on top of
>> Alyssa's changes to lock_region.
>>
>> [1] https://lore.kernel.org/dri-devel/6fe675c4-d22b-22da-ba3c-f6d33419b9ed@arm.com/
>>
>>  drivers/gpu/drm/panfrost/panfrost_mmu.c | 33 +++++++++++++++++++------
>>  1 file changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index dfe5f1d29763..afec15bb3db5 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -58,17 +58,36 @@ static int write_cmd(struct panfrost_device *pfdev, u32 as_nr, u32 cmd)
>>  }
>>  
>>  static void lock_region(struct panfrost_device *pfdev, u32 as_nr,
>> -			u64 iova, u64 size)
>> +			u64 region_start, u64 size)
>>  {
>>  	u8 region_width;
>> -	u64 region = iova & PAGE_MASK;
>> +	u64 region;
>> +	u64 region_size;
>> +	u64 region_end = region_start + size;
>>  
>> -	/* The size is encoded as ceil(log2) minus(1), which may be calculated
>> -	 * with fls. The size must be clamped to hardware bounds.
>> +	if (!size)
>> +		return;
>> +
>> +	/*
>> +	 * The locked region is a naturally aligned power of 2 block encoded as
>> +	 * log2 minus(1).
>> +	 * Calculate the desired start/end and look for the highest bit which
>> +	 * differs. The smallest naturally aligned block must include this bit
>> +	 * change the desired region starts with this bit (and subsequent bits)
>> +	 * zeroed and ends with the bit (and subsequent bits) set to one.
>> +	 *
> 
> Nit: you can drop the empty comment line.

Whoops - I reordered this comment and didn't spot the blank line getting
left.

>>  	 */
>> -	size = max_t(u64, size, AS_LOCK_REGION_MIN_SIZE);
>> -	region_width = fls64(size - 1) - 1;
>> -	region |= region_width;
>> +	region_size = region_start ^ (region_end - 1);
> 
> Hm, is region_size really encoding the size of the region to lock? I
> mean, the logic seems correct but I wonder if it wouldn't be better to
> drop the region_size variable and inline
> 'region_start ^ (region_end - 1)' in the region_width calculation to
> avoid confusion.

Yeah I wasn't happy about the variable name either, but I couldn't think
of a better one. Inlining it into the following line nicely avoids the
problem ;)

> Looks good otherwise.
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Thanks, I'll post a v2 in case anyone else has other comments.

Steve

>> +	region_width = max(fls64(region_size),
>> +			   const_ilog2(AS_LOCK_REGION_MIN_SIZE)) - 1;
>> +
>> +	/*
>> +	 * Mask off the low bits of region_start (which would be ignored by
>> +	 * the hardware anyway)
>> +	 */
>> +	region_start &= GENMASK_ULL(63, region_width);
>> +
>> +	region = region_width | region_start;
>>  
>>  	/* Lock the region that needs to be updated */
>>  	mmu_write(pfdev, AS_LOCKADDR_LO(as_nr), region & 0xFFFFFFFFUL);
>