Re: [RFC 13/29] drm/gpuvm: Introduce MADVISE Operations

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

Hi Danilo,

On 17-03-2025 19:57, Danilo Krummrich wrote:
> (Cc: dri-devel@lists.freedesktop.org, Boris)
> 
> Hi Himal,
> 
> Please make sure to copy in dri-devel for such patches.

Thanks for taking time for this. Will make sure to do same in future.

> 
> On Fri, Mar 14, 2025 at 01:32:10PM +0530, Himal Prasad Ghimiray wrote:
>> Introduce MADVISE operations that do not unmap the GPU VMA. These
>> operations split VMAs if the start or end addresses fall within existing
>> VMAs. The operations can create up to 2 REMAPS and 2 MAPs.
> 
> Can you please add some more motivation details for this patch? What exactly is
> it used for?

We are working on defining the madvise ioctl within the Xe driver,
which sets or unsets attributes to control page placement and PTE
(Page Table Entry) encoding for GPUVMA's within a user-provided
range.

The goal of this patch is to introduce drm_gpuva_ops, which will
create a drm_gpuva_op to cover the user-defined input range.

Let's assume there are multiple gpuvma's within a to d:

         drm_gpuva1                        drm_gpuva2
         attr_1 = x                        attr_1 = x1
         attr_2 = y                        attr_2 = y1
         attr_3 = z                        attr_3 = z1
[a----------------------------b-1][b-------------------c-1]

         drm_gpuva3
         attr_1 = x2
         attr_2 = y2
         attr_3 = z2
[c-------------------d-1]

Case 1)
In this case, the start and end of the user-provided range
lie within drm_gpuva1 a and b-1. We need to update attr_1 to x'.

drm_gpuva1:pre      drm_gpuva:New map         drm_gpuva1:next
  attr_1 = x         attr_1 = x'               attr_1 = x
  attr_2 = y         attr_2 = y                attr_2 = y
  attr_3 = z         attr_3 = z                attr_3 = z
[a---------start-1][start------- end-1][end---------------b-1]

In this scenario, behavior for ops_create is same as
drm_gpuvm_sm_map_ops_create
  REMAP:UNMAP drm_gpuva1 a to b
  REMAP:PREV a to start -1
  REMAP:NEXT end to b-1
  MAP: start to end

Case 2)
User provided input range covers multiple drm_gpuva's.

Start is in between a and b (drm_gpuva1). End is in between c and
d-1 (drm_gpuva2). We need to update attr_2 to y'.

drm_gpuva1:pre         drm_gpuva:Map1          drm_gpuva2
attr_1 = x             attr_1 = x             attr_1 = x1
attr_2 = y             attr_2 = y'            attr_2 = y'
attr_3 = z             attr_3 = z             attr_3 = z1
[a----------start-1][start-------------b-1][b-------------c - 1]

drm_gpuva:Map2  drm_gpuva3:Next
attr_1 = x2     attr_1 = x2
attr_2 = y'     attr_2 = y2
attr_3 = z2     attr_3 = z2
[c------ end-1][end------------d-1]

In this scenario:
REMAP:UNMAP drm_gpuva1 a to b
REMAP:PREV a to start -1
MAP: start to b

REMAP:UNMAP drm_gpuva3 a to b
REMAP:NEXT end+1 to e
MAP: d+1 to end

Behavior is entirely different than drm_gpuvm_sm_map_ops_create.

Case 3) Either of start or end lies within gpuvma and will have one
REMAP and one MAP.

Case 4) Both start and end are lying on existing drm_gpuva's
start/end. No OPS.

The behavior is not same as drm_gpuvm_sm_map_ops_create. Here we
don't merge the drm_gpuva's and there are no unmaps.

Currently, we don't want split to happen on bo backed vma's,
therefore skip_gem_obj_va is added.
[1] and [2] are the patches within driver using the ops and defining the 
ioctl

[1] 
https://lore.kernel.org/intel-xe/20250314080226.2059819-1-himal.prasad.ghimiray@intel.com/T/#m77dd9ea3507ed15bfcd2a1c410f9df17f79c69e1

[2] 
https://lore.kernel.org/intel-xe/20250314080226.2059819-1-himal.prasad.ghimiray@intel.com/T/#m2c49bf11661dec9d52edb8bf1bf9a553a709682e


> 
>>
>> If the input range is within the existing range, it creates REMAP:UNMAP,
>> REMAP:PREV, REMAP:NEXT, and MAP operations for the input range.
>>    Example:
>>    Input Range: 0x00007f0a54000000 to 0x00007f0a54400000
>>    GPU VMA: 0x0000000000000000 to 0x0000800000000000
>>    Operations Result:
>>    - REMAP:UNMAP: addr=0x0000000000000000, range=0x0000800000000000
>>    - REMAP:PREV: addr=0x0000000000000000, range=0x00007f0a54000000
>>    - REMAP:NEXT: addr=0x00007f0a54400000, range=0x000000f5abc00000
>>    - MAP: addr=0x00007f0a54000000, range=0x0000000000400000
> 
> This would be much easier to read if you'd pick some more human readable
> numbers.

This was the output which I printed from within the driver. Should have 
ensured it to be more explanatory. Can you provide some input on how can 
I make it more readable ?

> 
>>
>> If the input range starts at the beginning of one GPU VMA and ends at
>> the end of another VMA, covering multiple VMAs, the operations do nothing.
>>    Example:
>>    Input Range: 0x00007fc898800000 to 0x00007fc899000000
>>    GPU VMAs:
>>    - 0x0000000000000000 to 0x00007fc898800000
>>    - 0x00007fc898800000 to 0x00007fc898a00000
>>    - 0x00007fc898a00000 to 0x00007fc898c00000
>>    - 0x00007fc898c00000 to 0x00007fc899000000
>>    - 0x00007fc899000000 to 0x00007fc899200000
>>    Operations Result: None
> 
> Same here.
> 
>>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>>   drivers/gpu/drm/drm_gpuvm.c | 175 +++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_gpuvm.h     |   6 ++
>>   2 files changed, 180 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
>> index f9eb56f24bef..904a26641b21 100644
>> --- a/drivers/gpu/drm/drm_gpuvm.c
>> +++ b/drivers/gpu/drm/drm_gpuvm.c
>> @@ -2230,7 +2230,7 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   				ret = op_remap_cb(ops, priv, NULL, &n, &u);
>>   				if (ret)
>>   					return ret;
>> -				break;
>> +				return 0;
>>   			}
>>   		}
>>   	}
>> @@ -2240,6 +2240,143 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>>   			 req_obj, req_offset);
>>   }
>>   
>> +static int
>> +__drm_gpuvm_skip_split_map(struct drm_gpuvm *gpuvm,
>> +			   const struct drm_gpuvm_ops *ops, void *priv,
>> +			   u64 req_addr, u64 req_range,
>> +			   bool skip_gem_obj_va, u64 req_offset)
> 
> This looks like a full copy of __drm_gpuvm_sm_map(). I think you should extend
> __drm_gpuvm_sm_map() instead and add an optional flags parameter, e.g.

Unlike __drm_gpuvm_sm_map() this wont have any unmaps and merging of 
drm_gpuva's, hence I thought keeping it as separate ops is better. If 
you are of the opinion modifying __drm_gpuvm_sm_map on the basis of flag 
is more cleaner and maintainable, will change to it.


> 
> 	enum drm_gpuvm_madvise_flags {
> 		DRM_GPUVM_SKIP_GEM_OBJ_VA = BIT(0),
> 	}
> 
> Not sure whether "SKIP_GEM_OBJ_VA" is a good name, but I haven't gone through
> this to such extend that I could propose something better.
> 
>> +struct drm_gpuva_ops *
>> +drm_gpuvm_madvise_ops_create(struct drm_gpuvm *gpuvm,
>> +			     u64 req_addr, u64 req_range,
>> +			     bool skip_gem_obj_va, u64 req_offset)
> 
> Same here, I don't think we need a new function for this, but just the
> corresponding flags argument to the existing one.

Same as above

> 
> Besides that, when adding new functionality, please extend the documentation of
> drm_gpuvm accordingly. It's fine you didn't do so for the RFC of course. :)

Thanks for reminding. I Will definitely add proper documentation with 
conclusion on further design, just wanted to have opinions on this with 
RFC.

- Himal