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

Huang, Honglei1
2026-04-28T02:24:54Z

On 4/28/2026 5:05 AM, Felix Kuehling wrote:
>
> On 2026-04-24 06:12, Huang, Honglei1 wrote:
>>
>>
>> On 4/23/2026 7:06 PM, Huang, Honglei1 wrote:
>>>
>>>
>>> On 4/23/2026 6:39 PM, Christian König wrote:
>>>> On 4/23/26 08:21, Huang, Honglei1 wrote:
>>>>>
>>>>>
>>>>> On 4/20/2026 11:37 PM, Christian König wrote:
>>>>>> On 4/20/26 15:30, Huang, Honglei1 wrote:
>>>>>>> On 4/20/2026 8:15 PM, 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?
>>>>>>>
>>>>>>> Really thanks for the comments, I have some content mistaken in
>>>>>>> V2, so I updated the V3 to fix that. For the header they are
>>>>>>> same. for other content please review the V3, sorry about that.
>>>>>>> And will fix the concern you raised in next version.
>>>>>>>
>>>>>>> So the meaning of AMDGPU_SVM_ATTR_ACCESS and
>>>>>>> AMDGPU_SVM_ATTR_NO_ACCESS are clear, GPU can access it or not,
>>>>>>> and the SVM can set the preferred location, it can be in VRAM or
>>>>>>> system, for AMDGPU_SVM_ATTR_ACCESS it can be migrated between RAM
>>>>>>> and VRAM. For AMDGPU_SVM_ATTR_ACCESS_IN_PLACE,
>>>>>>> it can not migrate, GPU only can access it in the initial place.
>>>>>>
>>>>>> Yeah but that doesn't then the interface doesn't seem to make
>>>>>> sense since such states are mutual exclusive.
>>>>>>
>>>>>> It would make sense when you have some attribute which is named
>>>>>> (for example) AMDGPU_SVM_ATTR_ACCESS which can have the values
>>>>>> INACCESSIBLE, IN_PLACE, MIGRATE.
>>>>>
>>>>> Got it so can I change the UAPI to the following format?
>>>>>
>>>>> enum amdgpu_ioctl_svm_attr_type {
>>>>>      AMDGPU_IOCTL_SVM_ATTR_PREFERRED_LOC,
>>>>>      AMDGPU_IOCTL_SVM_ATTR_PREFETCH_LOC,
>>>>>      AMDGPU_IOCTL_SVM_ATTR_ACCESS,
>>>>>      AMDGPU_IOCTL_SVM_ATTR_SET_FLAGS,
>>>>>      AMDGPU_IOCTL_SVM_ATTR_CLR_FLAGS,
>>>>>      AMDGPU_IOCTL_SVM_ATTR_GRANULARITY
>>>>> };
>>>>>
>>>>> enum amdgpu_ioctl_svm_location {
>>>>
>>>> The enum name could probably be improved, but apart from that looks
>>>> reasonable to me.
>>>
>>> Will improve the name.
>>>>
>>>>>      AMDGPU_SVM_ACCESS_INACCESSIBLE = 1,
>>>>>      AMDGPU_SVM_ACCESS_IN_PLACE = 2,
>>>>>      AMDGPU_SVM_ACCESS_MIGRATE = 3,
>>>>> };
>>>>>
>>>>>>
>>>>>>>>> +#define AMDGPU_SVM_ATTR_SET_FLAGS        5
>>>>>>>>> +#define AMDGPU_SVM_ATTR_CLR_FLAGS        6
>>>>>>>>
>>>>>>>> Why is that separated into set and clear flags?
>>>>>>>
>>>>>>> This method inherits from KFD and is also designed to be
>>>>>>> compatible with upper layer applications such as ROCR.
>>>>>>
>>>>>> That is *not* sufficient as justification. We need to document why
>>>>>> that is necessary and *not* just say ROCR works that way.
>>>>>>
>>>>>> As far as I can see just a SET_FLAGS should be sufficient.
>>>>>
>>>>> Accoding to the reply form Felix, CLR_FLAGS provides a convenient
>>>>> method for deleting large-scale flags, do we need to redesign this
>>>>> part?
>>>>
>>>> I think we should expose those flags as individual attributes then.
>>>
>>> Got it will do.
>>>
>>>>
>>>>>>
>>>>>>>>> +#define AMDGPU_SVM_ATTR_GRANULARITY        7
>>>>>>>>> +
>>>>>>>>> +#define AMDGPU_SVM_LOCATION_SYSMEM        0
>>>>>>>>> +#define AMDGPU_SVM_LOCATION_UNDEFINED 0xffffffff
>>>>>>>>
>>>>>>>> No location for device local memory?
>>>>>>>
>>>>>>> Vaule > 0 means for device memory, in xe_svm, it seems like it
>>>>>>> uses fd for device local memory.
>>>>>>
>>>>>> Absolute clear NAK for that approach. This interface is per FD!
>>>>>>
>>>>>> We need some value AMDGPU_SVM_LOCATION_DEVICE which means that the
>>>>>> memory should be migrated to the current device.
>>>>>>
>>>>>> We also need to make sure that setting attributes for different
>>>>>> devices doesn't affect each other.
>>>>>
>>>>>
>>>>> I Totally agreed with your thoughts, but according to the reply
>>>>> from matt, it seems like we need to consider the P2P/multi GPU
>>>>> situation.
>>>>
>>>> When the drm_svm or pagemap component has already code to deal with
>>>> that then it is probably ok to have the same interface.
>>>>
>>>> When when XE only hacked that together on their own then that is a
>>>> bit questionable because getting the lifetime right is usually tricky.
>>>>
>>>>>
>>>>> So do I need to add a AMDGPU_SVM_LOCATION_DEVICE  flag or do I need
>>>>> to modify the UAPI to align with xe_svm?
>>>>
>>>> I think we need to full clarify how XE works here. E.g. that you can
>>>> specify both 0 as well as give the fd to get the memory migrated to
>>>> the local device sounds odd.
>>>
>>> Got it, for this part maybe require more discussion and time to fully
>>> understand, so this part will remain unchanged in the next version.
>>>
>>
>> Hi Christian, Felix,
>>
>> Based on the v3 review, I've reworked the SVM UAPI. Please let me know
>> if anything still looks off before I post v4.
>>
>> Changes with v3 UAPI:
>>   - OP / ATTR_TYPE / ACCESS / LOCATION converted to documented enums.
>>   - Three ACCESS_* attribute types collapsed into a single
>>     AMDGPU_SVM_ATTR_ACCESS carrying enum amdgpu_ioctl_svm_access.
>>   - ACCESS/MIGRATE renamed to ALLOW_MIGRATE to means permitted, not
>> required
>>   - SET_FLAGS / CLR_FLAGS removed; each former flag is now its own
>>     attribute carrying enum amdgpu_ioctl_svm_flag_value: CLR / SET
>>
>>
>>   enum amdgpu_ioctl_svm_op {
>>           AMDGPU_SVM_OP_SET_ATTR = 0,
>>           AMDGPU_SVM_OP_GET_ATTR = 1,
>>   };
>>
>>   enum amdgpu_ioctl_svm_attr_type {
>>           AMDGPU_SVM_ATTR_PREFERRED_LOC           = 0,
>>           AMDGPU_SVM_ATTR_PREFETCH_LOC            = 1,
>>           AMDGPU_SVM_ATTR_ACCESS                  = 2,
>>           AMDGPU_SVM_ATTR_GRANULARITY             = 3,
>>           AMDGPU_SVM_ATTR_HOST_ACCESS             = 4,
>>           AMDGPU_SVM_ATTR_COHERENT                = 5,
>>           AMDGPU_SVM_ATTR_EXT_COHERENT            = 6,
>>           AMDGPU_SVM_ATTR_HIVE_LOCAL              = 7,
>>           AMDGPU_SVM_ATTR_GPU_RO                  = 8,
>>           AMDGPU_SVM_ATTR_GPU_EXEC                = 9,
>>           AMDGPU_SVM_ATTR_GPU_READ_MOSTLY         = 10,
>>           AMDGPU_SVM_ATTR_GPU_ALWAYS_MAPPED       = 11,
>>   };
>>
>>   enum amdgpu_ioctl_svm_access {
>>           AMDGPU_SVM_ACCESS_INACCESSIBLE          = 0,
>>           AMDGPU_SVM_ACCESS_IN_PLACE              = 1,
>>           AMDGPU_SVM_ACCESS_ALLOW_MIGRATE         = 2,
>>   };
>>
>>   enum amdgpu_ioctl_svm_location {
>>           AMDGPU_SVM_LOCATION_SYSMEM              = 0,
>>           AMDGPU_SVM_LOCATION_UNDEFINED           = 0xffffffff,
>>   };
>>
>>   enum amdgpu_ioctl_svm_flag_value {
>>           AMDGPU_SVM_FLAG_CLR                     = 0,
>>           AMDGPU_SVM_FLAG_SET                     = 1,
>>   };
>
> Looks reasonable to me. But I'm not sure you really need enum
> amdgpu_ioctl_svm_flag_value. I'd just use 0 and non-zero to mean false
> and true.
>

Got it, will remove amdgpu_ioctl_svm_flag_value, and use 0 and non-zero
to mean false or true for the flags.

Regards,
Honglei

> Regards,
>   Felix
>
>
>>
>>
>> Regards,
>> Honglei
>>
>>> Regards,
>>> Honglei
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Regards,
>>>>> Honglei
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +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.
>>>>>>>
>>>>>>> Got it will add documentation for it and will use unions in next
>>>>>>> version. Really thanks for the comments.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Honglei
>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> +
>>>>>>>>>     #if defined(__cplusplus)
>>>>>>>>>     }
>>>>>>>>>     #endif
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>