Hi Danilo,
On 17-03-2025 19:57, Danilo Krummrich wrote:
(Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx, 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@xxxxxxxxx/T/#m77dd9ea3507ed15bfcd2a1c410f9df17f79c69e1
[2]
https://lore.kernel.org/intel-xe/20250314080226.2059819-1-himal.prasad.ghimiray@xxxxxxxxx/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@xxxxxxxxxx>
Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@xxxxxxxxx>
---
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
