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

[Danilo Krummrich <dakr@kernel.org>]

(Cc: dri-devel@lists.freedesktop.org, 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@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.

	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. :)