[PATCH v9 12/14] drm/amdgpu: enable SDMA usermode queues

[Sharma, Shashank]

On 02/05/2024 15:55, Alex Deucher wrote:
> On Thu, May 2, 2024 at 1:47 AM Sharma, Shashank <shashank.sharma at amd.com> wrote:
>>
>> On 01/05/2024 22:41, Alex Deucher wrote:
>>> On Fri, Apr 26, 2024 at 10:27 AM Shashank Sharma
>>> <shashank.sharma at amd.com> wrote:
>>>> This patch does necessary modifications to enable the SDMA
>>>> usermode queues using the existing userqueue infrastructure.
>>>>
>>>> V9: introduced this patch in the series
>>>>
>>>> Cc: Christian König <Christian.Koenig at amd.com>
>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma at amd.com>
>>>> Signed-off-by: Arvind Yadav <arvind.yadav at amd.com>
>>>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c    | 2 +-
>>>>    drivers/gpu/drm/amd/amdgpu/mes_v11_0_userqueue.c | 4 ++++
>>>>    drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c           | 3 +++
>>>>    3 files changed, 8 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>>> index 781283753804..e516487e8db9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
>>>> @@ -189,7 +189,7 @@ amdgpu_userqueue_create(struct drm_file *filp, union drm_amdgpu_userq *args)
>>>>           int qid, r = 0;
>>>>
>>>>           /* Usermode queues are only supported for GFX/SDMA engines as of now */
>>>> -       if (args->in.ip_type != AMDGPU_HW_IP_GFX) {
>>>> +       if (args->in.ip_type != AMDGPU_HW_IP_GFX && args->in.ip_type != AMDGPU_HW_IP_DMA) {
>>>>                   DRM_ERROR("Usermode queue doesn't support IP type %u\n", args->in.ip_type);
>>>>                   return -EINVAL;
>>>>           }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0_userqueue.c b/drivers/gpu/drm/amd/amdgpu/mes_v11_0_userqueue.c
>>>> index a6c3037d2d1f..a5e270eda37b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0_userqueue.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0_userqueue.c
>>>> @@ -182,6 +182,10 @@ static int mes_v11_0_userq_create_ctx_space(struct amdgpu_userq_mgr *uq_mgr,
>>>>                   return r;
>>>>           }
>>>>
>>>> +       /* We don't need to set other FW objects for SDMA queues */
>>>> +       if (queue->queue_type == AMDGPU_HW_IP_DMA)
>>>> +               return 0;
>>>> +
>>>>           /* Shadow and GDS objects come directly from userspace */
>>>>           mqd->shadow_base_lo = mqd_user->shadow_va & 0xFFFFFFFC;
>>>>           mqd->shadow_base_hi = upper_32_bits(mqd_user->shadow_va);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
>>>> index 361835a61f2e..90354a70c807 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v6_0.c
>>>> @@ -1225,6 +1225,8 @@ static int sdma_v6_0_early_init(void *handle)
>>>>           return 0;
>>>>    }
>>>>
>>>> +extern const struct amdgpu_userq_funcs userq_mes_v11_0_funcs;
>>> Can you include the header rather than adding an extern?
>> Noted,
>>>> +
>>>>    static int sdma_v6_0_sw_init(void *handle)
>>>>    {
>>>>           struct amdgpu_ring *ring;
>>>> @@ -1265,6 +1267,7 @@ static int sdma_v6_0_sw_init(void *handle)
>>>>                   return -EINVAL;
>>>>           }
>>>>
>>>> +       adev->userq_funcs[AMDGPU_HW_IP_DMA] = &userq_mes_v11_0_funcs;
>>>>           return r;
>>>>    }
>>> I think we need a new mqd descriptor in amdgpu_drm.h as well since the
>>> sdma metadata is different from gfx and compute.
>> Can you please elaborate on this ? AFAIK SDMA queue doesn't need any
>> specific metadata objects (like GFX).
> Right.  I want to make it clear in the IOCTL interface what buffers
> are required for which ring types.  E.g., UMD might allocate a shadow
> buffer for SDMA, but they don't need it so there is no need to
> allocate it.  If we have separate mqd structures for every ring type,
> it makes it clear which additional buffers are needed for which ring
> types.

Agree, it makes sense.

- Shashank

> Alex
>
>> - Shashank
>>
>>> Alex
>>>
>>>> --
>>>> 2.43.2
>>>>