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

Huang, Honglei1
2026-04-24T10:12:41Z

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,
};

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