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

[Igor Torrente <igor.torrente@collabora.com>]

Hi Tomas,

I tested your patch and Boris's modifications on top of your patch, and
I'm happy to report that both seem to work just fine.

BR,
Igor Torrente

On 5/27/26 07:32, Thomas Zimmermann wrote:
> 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
>