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@xxxxxxx> 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@xxxxxxx>
>> ---
>> 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@xxxxxxx/
>>
>>  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@xxxxxxxxxxxxx>

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);
>