Re: [PATCH v4 03/20] drm/xe/vm: Add attributes struct as member of vma

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 23-06-2025 09:48, Matthew Brost wrote:
> On Fri, Jun 13, 2025 at 06:25:41PM +0530, Himal Prasad Ghimiray wrote:
>> The attribute of xe_vma will determine the migration policy and the
>> encoding of the page table entries (PTEs) for that vma.
>> This attribute helps manage how memory pages are moved and how their
>> addresses are translated. It will be used by madvise to set the
>> behavior of the vma.
>>
>> v2 (Matthew Brost)
>> - Add docs
>>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_vm_types.h | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
>> index bed6088e1bb3..c7663058a7e3 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_types.h
>> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
>> @@ -77,6 +77,27 @@ struct xe_userptr {
>>   #endif
>>   };
>>   
>> +/**
>> + * struct xe_vma_mem_attr - memory attributes associated with vma
>> + */
>> +struct xe_vma_mem_attr {
>> +	/** @preferred_loc: perferred memory_location*/
>> +	struct {
>> +		/** @preferred_loc.migration_policy: Policy for migration of pages */
>> +		u32 migration_policy;
>> +
>> +		/**
>> +		 * @preferred_loc.devmem_fd: used for determining pagemap_fd requested by user
>> +		 * DRM_XE_PREFERRED_LOC_DEFAULT_SYSTEM and DRM_XE_PREFERRED_LOC_DEFAULT_DEVICE mean
>> +		 * system memory or closest device memory respectively.
> 
> Nit, I generally prefer 80 character line wraps. I know checkpatch
> allows 100 but in general I think Xe generally wraps at 80 unless it is
> an odd case with nested code and an 80 character wrap makes the code
> unreadable.
> 
> 80 generally allows 2 windows split vertically in vi on a typical laptop
> which makes navigating the code a bit easier.

Noted. Will address it in next version.

> 
>> +		 */
>> +		u32 devmem_fd;
>> +	} preferred_loc;
>> +
>> +	/** @atomic_access: The atomic access type for the vma */
> 
> I'd reference the uAPI here. e.g., DRM_XE_VMA_ATOMIC_*

Sure

> 
> With the minor nits fixed:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>

Thanks

> 
> Matt
> 
>> +	u32 atomic_access;
>> +};
>> +
>>   struct xe_vma {
>>   	/** @gpuva: Base GPUVA object */
>>   	struct drm_gpuva gpuva;
>> @@ -135,6 +156,13 @@ struct xe_vma {
>>   	 * Needs to be signalled before UNMAP can be processed.
>>   	 */
>>   	struct xe_user_fence *ufence;
>> +
>> +	/**
>> +	 * @attr: The attributes of vma which determines the migration policy
>> +	 * and encoding of the PTEs for this vma.
>> +	 */
>> +	struct xe_vma_mem_attr attr;
>> +
>>   };
>>   
>>   /**
>> -- 
>> 2.34.1
>>