Re: [PATCH v10 01/10] drm/shmem-helper: Simplify page offset calculation in fault handler

["Loïc Molinari" <loic.molinari@collabora.com>]

Hi Boris,

On 01/12/2025 09:05, Boris Brezillon wrote:
> On Fri, 28 Nov 2025 19:52:43 +0100
> Loïc Molinari <loic.molinari@collabora.com> wrote:
> 
>> For a fault at address addr, the page offset is
>>    page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT
>>                = ((addr & PAGE_MASK) - vma->vm_start) >> PAGE_SHIFT
>> 	      = (addr - vma->vm_start) >> PAGE_SHIFT
>>
>> Since the faulty logical page offset based on VMA is
>>    vmf->pgoff = vma->vm_pgoff + ((addr - vma->vm_start) >> PAGE_SHIFT)
>>
>> We can slightly simplify the calculation using
>>    page_offset = vmf->pgoff - vma->vm_pgoff
>>
>> Signed-off-by: Loïc Molinari <loic.molinari@collabora.com>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> One nit below
> 
>> ---
>>   drivers/gpu/drm/drm_gem_shmem_helper.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index dc94a27710e5..be89be1c804c 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -577,8 +577,8 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>>   	struct page *page;
>>   	pgoff_t page_offset;
>>   
>> -	/* We don't use vmf->pgoff since that has the fake offset */
>> -	page_offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT;
>> +	/* Offset to faulty address in the VMA (without the fake offset). */
> 
> It's weird to say "without the fake offset" here, because IIUC, both
> vmf->pgoff and vma->vm_pgoff contain the fake offset. And that's fine,
> the problem really is when one of the subtraction operand is not
> relative to the fake offset.

Yes, both values contain the fake offset. vma->vm_pgoff is the actual 
fake offset (mmap offset in the GEM context). vmf->pgoff is the fake 
offset added to the offset we're looking for (offset from start of VMA 
to faulty address). So the difference just gets rid of it, hence the 
precision, but now that I read it again after a few weeks, it's a bit 
misleading so I'll just remove it.

Regards,
Loïc

>> +	page_offset = vmf->pgoff - vma->vm_pgoff;
>>   
>>   	dma_resv_lock(shmem->base.resv, NULL);
>>   
>