Re: [PATCH v3 5/6] drm/gem-shmem: Track folio accessed/dirty status in mmap

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 27.05.26 um 12:18 schrieb Boris Brezillon:
> Hello Thomas,
>
> I'm inlining the diff you posted so I can comment on it directly before
> it's officially posted to the list.

Thanks, I'll incorporate those in the next test rev or the official patch.

Best regards
Thomas

>
> On Wed, 27 May 2026 08:56:33 +0200
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index 545933c7f712..07e117673124 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -554,21 +554,6 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev,
>>   }
>>   EXPORT_SYMBOL_GPL(drm_gem_shmem_dumb_create);
>>   
>> -static void drm_gem_shmem_record_mkwrite(struct vm_fault *vmf)
>> -{
>> -	struct vm_area_struct *vma = vmf->vma;
>> -	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;
>> -	pgoff_t page_offset = vmf->pgoff - vma->vm_pgoff; /* page offset within VMA */
>> -
>> -	if (drm_WARN_ON(obj->dev, !shmem->pages || page_offset >= num_pages))
>> -		return;
>> -
>> -	file_update_time(vma->vm_file);
>> -	folio_mark_dirty(page_folio(shmem->pages[page_offset]));
>> -}
>> -
>>   static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>>   				 unsigned long pfn)
>>   {
>> @@ -581,23 +566,16 @@ static vm_fault_t try_insert_pfn(struct vm_fault *vmf, unsigned int order,
>>   
>>   		if (aligned &&
>>   		    folio_test_pmd_mappable(page_folio(pfn_to_page(pfn)))) {
>> -			vm_fault_t ret;
>> -
>>   			pfn &= PMD_MASK >> PAGE_SHIFT;
>>   
>> -			/* Unlike PTEs which are automatically upgraded to
>> +			/*
>> +			 * Unlike PTEs which are automatically upgraded to
>>   			 * writeable entries, the PMD upgrades go through
>>   			 * .huge_fault(). Make sure we pass the "write" info
>>   			 * along in that case.
>> -			 * This also means we have to record the write fault
>> -			 * here, instead of in .pfn_mkwrite().
>>   			 */
>> -			ret = vmf_insert_pfn_pmd(vmf, pfn,
>> -						 vmf->flags & FAULT_FLAG_WRITE);
>> -			if (ret == VM_FAULT_NOPAGE && (vmf->flags & FAULT_FLAG_WRITE))
>> -				drm_gem_shmem_record_mkwrite(vmf);
>> -
>> -			return ret;
>> +			return vmf_insert_pfn_pmd(vmf, pfn,
>> +						  vmf->flags & FAULT_FLAG_WRITE);
> I believe we can go back to
>
> 			return vmf_insert_pfn_pmd(vmf, pfn, false);
>
> if the mappings are no longer adjusted to catch write accesses.
>
>>   		}
>>   #endif
>>   	}
>> @@ -635,8 +613,15 @@ static vm_fault_t drm_gem_shmem_any_fault(struct vm_fault *vmf, unsigned int ord
>>   	pfn = page_to_pfn(page);
>>   
>>   	ret = try_insert_pfn(vmf, order, pfn);
>> -	if (ret == VM_FAULT_NOPAGE)
>> +	if (ret == VM_FAULT_NOPAGE) {
>>   		folio_mark_accessed(folio);
>> +		/*
>> +		 * Always record write access to the buffer. The natural
>> +		 * place would be pfn_mkwrite, but this breaks KVM.
>> +		 */
>> +		file_update_time(vma->vm_file);
>> +		folio_mark_dirty(folio);
> We can be a bit smarter here:
>
> 		/*
> 		 * Always record write access to the buffer if the
> 		 * mapping is writeable. The natural place would be
> 		 * pfn_mkwrite, but this breaks KVM.
> 		 */
> 		if (vma->vm_flags & VM_WRITE) {
> 			file_update_time(vma->vm_file);
> 			folio_mark_dirty(folio);
> 		}
>
>> +	}
> The rest looks good to me.
>
> Regards,
>
> Boris

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