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

["Huang, Honglei1" <honghuan@amd.com>]

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