Re: [RFC/POC PATCH 01/12] drm/amdgpu: add SVM UAPI definitions

["Christian König" <christian.koenig@amd.com>]

On 4/20/26 14:07, Honglei Huang wrote:
> From: Honglei Huang <honghuan@amd.com>
> 
> Add amdgpu drm SVM API definitions built on the
> DRM GPUSVM framework.
> 
> This includes:
> - DRM_AMDGPU_GEM_SVM ioctl
> - AMDGPU_SVM_FLAG_* flags
> - AMDGPU_SVM_OP_SET_ATTR / AMDGPU_SVM_OP_GET_ATTR operations
> - AMDGPU_SVM_ATTR_* attribute types
> - AMDGPU_SVM_LOCATION_SYSMEM / AMDGPU_SVM_LOCATION_UNDEFINED
> - struct drm_amdgpu_svm_attribute and struct drm_amdgpu_gem_svm
> 
> Signed-off-by: Honglei Huang <honghuan@amd.com>
> ---
>  include/uapi/drm/amdgpu_drm.h | 39 +++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 406a42be4..bed71ed9b 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -58,6 +58,7 @@ extern "C" {
>  #define DRM_AMDGPU_USERQ_SIGNAL		0x17
>  #define DRM_AMDGPU_USERQ_WAIT		0x18
>  #define DRM_AMDGPU_GEM_LIST_HANDLES	0x19
> +#define DRM_AMDGPU_GEM_SVM		0x1a
>  
>  #define DRM_IOCTL_AMDGPU_GEM_CREATE	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_CREATE, union drm_amdgpu_gem_create)
>  #define DRM_IOCTL_AMDGPU_GEM_MMAP	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_MMAP, union drm_amdgpu_gem_mmap)
> @@ -79,6 +80,7 @@ extern "C" {
>  #define DRM_IOCTL_AMDGPU_USERQ_SIGNAL	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_USERQ_SIGNAL, struct drm_amdgpu_userq_signal)
>  #define DRM_IOCTL_AMDGPU_USERQ_WAIT	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_USERQ_WAIT, struct drm_amdgpu_userq_wait)
>  #define DRM_IOCTL_AMDGPU_GEM_LIST_HANDLES DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_LIST_HANDLES, struct drm_amdgpu_gem_list_handles)
> +#define DRM_IOCTL_AMDGPU_GEM_SVM	DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_SVM, struct drm_amdgpu_gem_svm)
>  
>  /**
>   * DOC: memory domains
> @@ -1665,6 +1667,43 @@ struct drm_color_ctm_3x4 {
>  	__u64 matrix[12];
>  };
>  
> +#define AMDGPU_SVM_FLAG_HOST_ACCESS		0x00000001
> +#define AMDGPU_SVM_FLAG_COHERENT		0x00000002
> +#define AMDGPU_SVM_FLAG_HIVE_LOCAL		0x00000004
> +#define AMDGPU_SVM_FLAG_GPU_RO			0x00000008
> +#define AMDGPU_SVM_FLAG_GPU_EXEC		0x00000010
> +#define AMDGPU_SVM_FLAG_GPU_READ_MOSTLY		0x00000020
> +#define AMDGPU_SVM_FLAG_GPU_ALWAYS_MAPPED	0x00000040
> +#define AMDGPU_SVM_FLAG_EXT_COHERENT		0x00000080
> +
> +#define AMDGPU_SVM_OP_SET_ATTR		0
> +#define AMDGPU_SVM_OP_GET_ATTR		1
> +
> +#define AMDGPU_SVM_ATTR_PREFERRED_LOC		0
> +#define AMDGPU_SVM_ATTR_PREFETCH_LOC		1

Up till here the interface makes perfect sense, but then it becomes a bit fuzzy.

> +#define AMDGPU_SVM_ATTR_ACCESS			2
> +#define AMDGPU_SVM_ATTR_ACCESS_IN_PLACE		3
> +#define AMDGPU_SVM_ATTR_NO_ACCESS		4

Why are those separate attributes? What is the difference between those?

> +#define AMDGPU_SVM_ATTR_SET_FLAGS		5
> +#define AMDGPU_SVM_ATTR_CLR_FLAGS		6

Why is that separated into set and clear flags?

> +#define AMDGPU_SVM_ATTR_GRANULARITY		7
> +
> +#define AMDGPU_SVM_LOCATION_SYSMEM		0
> +#define AMDGPU_SVM_LOCATION_UNDEFINED		0xffffffff

No location for device local memory?

> +
> +struct drm_amdgpu_svm_attribute {
> +	__u32 type;
> +	__u32 value;
> +};
> +
> +struct drm_amdgpu_gem_svm {
> +	__u64 start_addr;
> +	__u64 size;
> +	__u32 operation;
> +	__u32 nattr;
> +	__u64 attrs_ptr;
> +};

Those struct make perfect sense but clearly need documentation. Preferable as kerneldoc.

And we usually use unions in this header to separate the input from the output parameters.

Regards,
Christian.

> +
>  #if defined(__cplusplus)
>  }
>  #endif