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

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

On 4/21/26 05:37, Kuehling, Felix wrote:
> 
> On 2026-04-20 08:15, Christian König wrote:
>>
>> 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?
> 
> This looks like it's based on the KFD SVM API. We created this so we could set or clear specific flags for address ranges without know what other flags were set or not set on different parts of those address ranges already.
> 
> E.g. you may have set an RO flag for pages 1-5, and set a COHERENT flag for pages 3-7. Now you want to clear EXEC for pages 0-8. If you specify an exact flags parameter, you wipe out all those other settings that have different values for different pages in the range. Instead this API lets you say "clear the EXEC flag on pages 0-8 without touching any of the other flags".
> 
> Alternatively we could have made all those flags completely separate boolean attributes. Making them flags that can be set/cleared in this way is more economical for how they are stored and manipulated.

Yeah that's exactly what I thought as solution as well.

I mean the kernel can store them internally as flags, but we should clearly have a clean and coherent uAPI for them.

Regards,
Christian.

> 
> Regards,
>   Felix
> 
> 
>>
>>> +#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