(Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx, Boris)
Hi Himal,
Please make sure to copy in dri-devel for such patches.
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?
>
> 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.
>
> 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.
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.
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. :)
