Re: [PATCH 3/7] drm/gpusvm: mark pages as dirty

[Matthew Auld <matthew.auld@intel.com>]

On 20/03/2025 19:33, Matthew Brost wrote:
> On Thu, Mar 20, 2025 at 08:29:42PM +0100, Thomas Hellström wrote:
>> On Thu, 2025-03-20 at 17:30 +0000, Matthew Auld wrote:
>>> If the memory is going to be accessed by the device, make sure we
>>> mark
>>> the pages accordingly such that the kernel knows this. This aligns
>>> with
>>> the xe-userptr code.
>>>
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>   drivers/gpu/drm/drm_gpusvm.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gpusvm.c
>>> b/drivers/gpu/drm/drm_gpusvm.c
>>> index 7f1cf5492bba..5b4ecd36dff1 100644
>>> --- a/drivers/gpu/drm/drm_gpusvm.c
>>> +++ b/drivers/gpu/drm/drm_gpusvm.c
>>> @@ -1471,6 +1471,7 @@ int drm_gpusvm_range_get_pages(struct
>>> drm_gpusvm *gpusvm,
>>>   			pages[i] = page;
>>>   		} else {
>>>   			dma_addr_t addr;
>>> +			unsigned int k;
>>>   
>>>   			if (is_zone_device_page(page) || zdd) {
>>>   				err = -EOPNOTSUPP;
>>> @@ -1489,6 +1490,14 @@ int drm_gpusvm_range_get_pages(struct
>>> drm_gpusvm *gpusvm,
>>>   			range->dma_addr[j] =
>>> drm_pagemap_device_addr_encode
>>>   				(addr, DRM_INTERCONNECT_SYSTEM,
>>> order,
>>>   				 dma_dir);
>>> +
>>> +			for (k = 0; k < 1u << order; k++) {
>>> +				if (!ctx->read_only)
>>> +					set_page_dirty_lock(page);
>>> +
>>> +				mark_page_accessed(page);
>>> +				page++;
>>> +			}
>>
>> Actually I think the userptr code did this unnecessarily. This is done
>> in the CPU page-fault handler, which means it's taken care of during
>> hmm_range_fault(). Now if the CPU PTE happens to be present and
>> writeable there will be no fault, but that was done when the page was
>> faulted in anyway.
>>
>> If there was a page cleaning event in between so the dirty flag was
>> dropped, then my understanding is that in addition to an invalidation
>> notifier, also the CPU PTE is zapped, so that it will be dirtied again
>> on the next write access, either by the CPU faulting the page or
>> hmm_range_fault() if there is a GPU page-fault.
>>
>> So I think we're good without this patch.
>>
> 
> I was going to suggest the same thing as Thomas - we are good without
> this patch for the reasons he states.

Ah, will drop this then. Thanks.

> 
> Matt
> 
>> /Thomas
>>
>>
>>
>>>   		}
>>>   		i += 1 << order;
>>>   		num_dma_mapped = i;
>>