Re: [PATCH v4 01/10] drm/amdgpu: UAPI for user queue management

[Shashank Sharma <shashank.sharma@amd.com>]

Hey Alex,

Thanks for the review comments, please find mine inline.

- Shashank

On 19/05/2023 23:03, Alex Deucher wrote:
> On Mon, Apr 24, 2023 at 1:39 PM Shashank Sharma <shashank.sharma@amd.com> wrote:
>> From: Alex Deucher <alexander.deucher@amd.com>
>>
>> This patch intorduces new UAPI/IOCTL for usermode graphics
>> queue. The userspace app will fill this structure and request
>> the graphics driver to add a graphics work queue for it. The
>> output of this UAPI is a queue id.
>>
>> This UAPI maps the queue into GPU, so the graphics app can start
>> submitting work to the queue as soon as the call returns.
>>
>> V2: Addressed review comments from Alex and Christian
>>      - Make the doorbell offset's comment clearer
>>      - Change the output parameter name to queue_id
>>
>> V3: Integration with doorbell manager
>>
>> V4:
>>      - Updated the UAPI doc (Pierre-Eric)
>>      - Created a Union for engine specific MQDs (Alex)
>>      - Added Christian's R-B
>>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Christian Koenig <christian.koenig@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@amd.com>
>> ---
>>   include/uapi/drm/amdgpu_drm.h | 95 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 95 insertions(+)
>>
>> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
>> index 4038abe8505a..083a1df8e8ef 100644
>> --- a/include/uapi/drm/amdgpu_drm.h
>> +++ b/include/uapi/drm/amdgpu_drm.h
>> @@ -54,6 +54,7 @@ extern "C" {
>>   #define DRM_AMDGPU_VM                  0x13
>>   #define DRM_AMDGPU_FENCE_TO_HANDLE     0x14
>>   #define DRM_AMDGPU_SCHED               0x15
>> +#define DRM_AMDGPU_USERQ               0x16
>>
>>   #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)
>> @@ -71,6 +72,7 @@ extern "C" {
>>   #define DRM_IOCTL_AMDGPU_VM            DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_VM, union drm_amdgpu_vm)
>>   #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle)
>>   #define DRM_IOCTL_AMDGPU_SCHED         DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>> +#define DRM_IOCTL_AMDGPU_USERQ         DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_USERQ, union drm_amdgpu_userq)
>>
>>   /**
>>    * DOC: memory domains
>> @@ -302,6 +304,99 @@ union drm_amdgpu_ctx {
>>          union drm_amdgpu_ctx_out out;
>>   };
>>
>> +/* user queue IOCTL */
>> +#define AMDGPU_USERQ_OP_CREATE 1
>> +#define AMDGPU_USERQ_OP_FREE   2
>> +
>> +#define AMDGPU_USERQ_MQD_FLAGS_SECURE  (1 << 0)
>> +#define AMDGPU_USERQ_MQD_FLAGS_AQL     (1 << 1)
>> +
>> +/*
>> + * MQD (memory queue descriptor) is a set of parameters which allow
>> + * the GPU to uniquely define and identify a usermode queue.
>> + */
>> +struct drm_amdgpu_userq_mqd_gfx {
>> +       /**
>> +        * @flags: flags to indicate special function for queue like secure
>> +        * buffer (TMZ). Unused for now.
>> +        */
>> +       __u32   flags;
>> +       /**
>> +        * @doorbell_handle: the handle of doorbell GEM object
>> +        * associated to this client.
>> +        */
>> +       __u32   doorbell_handle;
>> +       /**
>> +        * @doorbell_offset: 32-bit offset of the doorbell in the doorbell bo.
>> +        * Kernel will generate absolute doorbell offset using doorbell_handle
>> +        * and doorbell_offset in the doorbell bo.
>> +        */
>> +       __u32   doorbell_offset;
>> +       /**
>> +        * @queue_va: Virtual address of the GPU memory which holds the queue
>> +        * object. The queue holds the workload packets.
>> +        */
>> +       __u64   queue_va;
>> +       /**
>> +        * @queue_size: Size of the queue in bytes, this needs to be 256-byte
>> +        * aligned.
>> +        */
>> +       __u64   queue_size;
>> +       /**
>> +        * @rptr_va : Virtual address of the GPU memory which holds the ring RPTR.
>> +        * This object must be at least 8 byte in size and aligned to 8-byte offset.
>> +        */
>> +       __u64   rptr_va;
>> +       /**
>> +        * @wptr_va : Virtual address of the GPU memory which holds the ring WPTR.
>> +        * This object must be at least 8 byte in size and aligned to 8-byte offset.
>> +        *
>> +        * Queue, RPTR and WPTR can come from the same object, as long as the size
>> +        * and alignment related requirements are met.
>> +        */
>> +       __u64   wptr_va;
>> +       /**
>> +        * @shadow_va: Virtual address of the GPU memory to hold the shadow buffer.
>> +        * This must be a from a separate GPU object, and must be at least 4-page
>> +        * sized.
>> +        */
>> +       __u64   shadow_va;
> You were planning to add the other pointers here as well for gds and csa right?
That was decided in this patch set only, from a different review 
comment, so we are yet to work on it :)
>
>> +};
>> +
>> + /*
>> +  * MQD can be different for different GPU IP/engine. This union will
>> +  * contain structures of MQDs corresponding to different engines.
>> +  */
>> +union drm_amdgpu_userq_mqd {
>> +       struct drm_amdgpu_userq_mqd_gfx gfx;
>> +};
>> +
>> +struct drm_amdgpu_userq_in {
>> +       /** AMDGPU_USERQ_OP_* */
>> +       __u32   op;
>> +       /** Flags */
>> +       __u32   flags;
>> +       /** Queue handle for USERQ_OP_FREE */
>> +       __u32   queue_id;
>> +       /** the target GPU engine to execute workload (AMDGPU_HW_IP_*) */
>> +       __u32   ip_type;
>> +
>> +       /** Queue descriptor for USERQ_OP_CREATE */
>> +       union drm_amdgpu_userq_mqd mqd;
> Just thinking out loud here, I wonder if we should make this a
> pointer/size element so that we could make this variable sized in case
> some future MQD ends up being bigger.  I guess we could always add a
> AMDGPU_USERQ_OP_CREATE2 if we ever need to expand it.

I think it is a good idea and sounds scalable as well. Then we might not 
even need a union, we can just define MQD size and a ptr.

- Shashank

> Alex
>
>> +};
>> +
>> +struct drm_amdgpu_userq_out {
>> +       /** Queue handle */
>> +       __u32   queue_id;
>> +       /** Flags */
>> +       __u32   flags;
>> +};
>> +
>> +union drm_amdgpu_userq {
>> +       struct drm_amdgpu_userq_in in;
>> +       struct drm_amdgpu_userq_out out;
>> +};
>> +
>>   /* vm ioctl */
>>   #define AMDGPU_VM_OP_RESERVE_VMID      1
>>   #define AMDGPU_VM_OP_UNRESERVE_VMID    2
>> --
>> 2.40.0
>>