Re: [PATCH 1/3] drm/gem-shmem: Map pages in mmap fault handler

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi,

thanks for reviewing.

Am 27.01.26 um 15:45 schrieb Boris Brezillon:
> Hello Thomas,
>
> On Tue, 27 Jan 2026 14:16:36 +0100
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>> Gem-shmem operates on pages instead of I/O memory ranges, so use them
>> for mmap. This will allow for tracking page dirty/accessed flags. If
>> hugepage support is available, insert the page's folio if possible.
>> Otherwise fall back to mapping individual pages.
>>
>> As the PFN is no longer required for hugepage mappings, simplify the
>> related code and make it depend on CONFIG_TRANSPARENT_HUGEPAGE. Prepare
>> for tracking folio status.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_gem_shmem_helper.c | 58 ++++++++++++++++----------
>>   1 file changed, 35 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index 3871a6d92f77..b6ddabbfcc52 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -553,17 +553,18 @@ EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
>>   static bool drm_gem_shmem_try_map_pmd(struct vm_fault *vmf, unsigned long addr,
>>   				      struct page *page)
>>   {
>> -#ifdef CONFIG_ARCH_SUPPORTS_PMD_PFNMAP
>> -	unsigned long pfn = page_to_pfn(page);
>> -	unsigned long paddr = pfn << PAGE_SHIFT;
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +	phys_addr_t paddr = page_to_phys(page);
>>   	bool aligned = (addr & ~PMD_MASK) == (paddr & ~PMD_MASK);
>>   
>> -	if (aligned &&
>> -	    pmd_none(*vmf->pmd) &&
>> -	    folio_test_pmd_mappable(page_folio(page))) {
>> -		pfn &= PMD_MASK >> PAGE_SHIFT;
>> -		if (vmf_insert_pfn_pmd(vmf, pfn, false) == VM_FAULT_NOPAGE)
>> -			return true;
>> +	if (aligned && pmd_none(*vmf->pmd)) {
>> +		struct folio *folio = page_folio(page);
>> +
>> +		if (folio_test_pmd_mappable(folio)) {
>> +			/* Read-only mapping; split upon write fault */
>> +			if (vmf_insert_folio_pmd(vmf, folio, false) == VM_FAULT_NOPAGE)
>> +				return true;
>> +		}
>>   	}
>>   #endif
>>   
>> @@ -576,13 +577,10 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>>   	struct drm_gem_object *obj = vma->vm_private_data;
>>   	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>   	loff_t num_pages = obj->size >> PAGE_SHIFT;
>> -	vm_fault_t ret;
>>   	struct page **pages = shmem->pages;
>> -	pgoff_t page_offset;
>> -	unsigned long pfn;
>> -
>> -	/* Offset to faulty address in the VMA. */
>> -	page_offset = vmf->pgoff - vma->vm_pgoff;
>> +	pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff; /* page offset within VMA */
>> +	struct page *page = pages[page_offset];
>> +	vm_fault_t ret;
>>   
>>   	dma_resv_lock(shmem->base.resv, NULL);
>>   
>> @@ -590,21 +588,35 @@ static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
>>   	    drm_WARN_ON_ONCE(obj->dev, !shmem->pages) ||
>>   	    shmem->madv < 0) {
>>   		ret = VM_FAULT_SIGBUS;
>> -		goto out;
>> +		goto err_dma_resv_unlock;
>>   	}
>>   
>> -	if (drm_gem_shmem_try_map_pmd(vmf, vmf->address, pages[page_offset])) {
>> -		ret = VM_FAULT_NOPAGE;
>> -		goto out;
>> +	page = pages[page_offset];
>> +	if (!page) {
>> +		ret = VM_FAULT_SIGBUS;
>> +		goto err_dma_resv_unlock;
>>   	}
>>   
>> -	pfn = page_to_pfn(pages[page_offset]);
>> -	ret = vmf_insert_pfn(vma, vmf->address, pfn);
>> +	if (drm_gem_shmem_try_map_pmd(vmf, vmf->address, page)) {
>> +		ret = VM_FAULT_NOPAGE;
>> +	} else {
>> +		struct folio *folio = page_folio(page);
>> +
>> +		get_page(page);
>> +
>> +		folio_lock(folio);
>> +
>> +		vmf->page = page;
>> +		ret = VM_FAULT_LOCKED;
>> +	}
>>   
>> - out:
>>   	dma_resv_unlock(shmem->base.resv);
>>   
>>   	return ret;
>> +
>> +err_dma_resv_unlock:
>> +	dma_resv_unlock(shmem->base.resv);
>> +	return ret;
> Why do we need an error path that's exactly the same as the success
> path?

I just found the explicit roll-back code easier to work with. But the 
other style does it as well. Noted for the next iteration.

Best regards
Thomas

>
>>   }
>>   
>>   static void drm_gem_shmem_vm_open(struct vm_area_struct *vma)
>> @@ -691,7 +703,7 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
>>   	if (ret)
>>   		return ret;
>>   
>> -	vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
>> +	vm_flags_mod(vma, VM_DONTEXPAND | VM_DONTDUMP, VM_PFNMAP);
>>   	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>>   	if (shmem->map_wc)
>>   		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> The rest looks reasonable, if everyone is happy with the less
> restrictive aspect that !PFNMAP implies, as mentioned by Matthew.

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)