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

Felix Kuehling
2026-04-27T21:05:10Z

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.

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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>