On Thu, Sep 28, 2023 at 9:40 AM Shashank Sharma <shashank.sha...@amd.com> wrote:
>
>
> On 28/09/2023 15:27, Alex Deucher wrote:
> > On Thu, Sep 28, 2023 at 9:22 AM Shashank Sharma <shashank.sha...@amd.com>
> > wrote:
> >>
> >> On 14/09/2023 10:24, Shashank Sharma wrote:
> >>> On 14/09/2023 09:45, Christian König wrote:
> >>>> Am 08.09.23 um 18:04 schrieb Shashank Sharma:
> >>>>> A Memory queue descriptor (MQD) of a userqueue defines it in
> >>>>> the hw's context. As MQD format can vary between different
> >>>>> graphics IPs, we need gfx GEN specific handlers to create MQDs.
> >>>>>
> >>>>> This patch:
> >>>>> - Introduces MQD handler functions for the usermode queues.
> >>>>> - Adds new functions to create and destroy userqueue MQD for
> >>>>> GFX-GEN-11 IP
> >>>>>
> >>>>> V1: Worked on review comments from Alex:
> >>>>> - Make MQD functions GEN and IP specific
> >>>>>
> >>>>> V2: Worked on review comments from Alex:
> >>>>> - Reuse the existing adev->mqd[ip] for MQD creation
> >>>>> - Formatting and arrangement of code
> >>>>>
> >>>>> V3:
> >>>>> - Integration with doorbell manager
> >>>>>
> >>>>> V4: Review comments addressed:
> >>>>> - Do not create a new file for userq, reuse gfx_v11_0.c (Alex)
> >>>>> - Align name of structure members (Luben)
> >>>>> - Don't break up the Cc tag list and the Sob tag list in commit
> >>>>> message (Luben)
> >>>>> V5:
> >>>>> - No need to reserve the bo for MQD (Christian).
> >>>>> - Some more changes to support IP specific MQD creation.
> >>>>>
> >>>>> V6:
> >>>>> - Add a comment reminding us to replace the
> >>>>> amdgpu_bo_create_kernel()
> >>>>> calls while creating MQD object to amdgpu_bo_create() once
> >>>>> eviction
> >>>>> fences are ready (Christian).
> >>>>>
> >>>>> Cc: Alex Deucher <alexander.deuc...@amd.com>
> >>>>> Cc: Christian Koenig <christian.koe...@amd.com>
> >>>>> Signed-off-by: Shashank Sharma <shashank.sha...@amd.com>
> >>>>> Signed-off-by: Arvind Yadav <arvind.ya...@amd.com>
> >>>>> ---
> >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c | 16 ++++
> >>>>> drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 77
> >>>>> +++++++++++++++++++
> >>>>> .../gpu/drm/amd/include/amdgpu_userqueue.h | 7 ++
> >>>>> 3 files changed, 100 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> >>>>> index 44769423ba30..03fc8e89eafb 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.c
> >>>>> @@ -140,12 +140,28 @@ int amdgpu_userq_ioctl(struct drm_device *dev,
> >>>>> void *data,
> >>>>> return r;
> >>>>> }
> >>>>> +extern const struct amdgpu_userq_funcs userq_gfx_v11_funcs;
> >>>>> +
> >>>>> +static void
> >>>>> +amdgpu_userqueue_setup_gfx(struct amdgpu_userq_mgr *uq_mgr)
> >>>>> +{
> >>>>> + int maj;
> >>>>> + struct amdgpu_device *adev = uq_mgr->adev;
> >>>>> + uint32_t version = adev->ip_versions[GC_HWIP][0];
> >>>>> +
> >>>>> + /* We support usermode queue only for GFX V11 as of now */
> >>>>> + maj = IP_VERSION_MAJ(version);
> >>>>> + if (maj == 11)
> >>>>> + uq_mgr->userq_funcs[AMDGPU_HW_IP_GFX] = &userq_gfx_v11_funcs;
> >>>>> +}
> >>>> That belongs into gfx_v11.c and not here.
> >>>
> >>> Agree,
> >> On a second thought, we can't move it to gfx_v11.c, as this is the place
> >> where we are setting up the gfx_userqueue functions in fpriv->uq_mgr()
> >> for the first time, and we do not have another option but to check the
> >> IP and setup the functions here. The only other option to do this will
> >> be to move uq_mgr->userq_funcs to adev->gfx.userq_funcs and setup them
> >> with the IP init (as Alex once suggested). Please let me know your
> >> thoughts on this.
> > That seems cleaner to me. They should be global anyway and could be
> > set as part of the individual IP init sequences. Then the presence of
> > a pointer could be used to determine whether or not a particular IP
> > type supports user queues.
> >
> > Alex
> >
> So if I understand this correctly, this is how we are looking to arrange
> the userqueue IP functions:
>
> - Presence of adev->gfx.funcs.userqueue_funcs() will decide if this IP
> supports userqueue or not.
>
> - sw_init function of the IP will setup these fptrs like:
>
> in gfx_v11_0_sw_init :
>
> if (GFX_MAJ == 11)
>
> adev->gfx.funcs.userqueue_funcs = gfx_v11_0_userqueue_funcs
I was thinking something more like:
adev->userq_funcs[AMD_IP_BLOCK_TYPE_GFX] = gfx_v11_0_userqueue_funcs;
That way there would be one place for all of the all of the fptrs and
you could use the IP type to query the right one.
And then in the IOCTLs, you could just check if the pointer is valid. E.g.,
if (!adev->userq_funcs[ip_block_type])
return -EINVAL;
etc.
You could store any metadata relevant to each userq in the per fd user
queue manager and then just pass the state to the global userq
functions for each IP.
Alex
>
>
> In amdgpu_userqueue_ioctl:
>
> |
>
> CASE: create:
>
> amdgpu_userqueue_create()
>
> if (adev->gfx.funcs.userqueue_funcs) {
>
> adev->gfx.funcs.userqueue_funcs.create_mqd();
>
> }
>
>
> CASE: destroy:
>
> amdgpu_userqueue_destroy()
>
> if (adev->gfx.funcs.userqueue_funcs) {
>
> adev->gfx.funcs.userqueue_funcs.destroy_mqd();
>
> }
>
> and so on ...
>
> Am I getting this right ?
>
> - Shashank
>
> >> - Shashank
> >>
> >>>>> +
> >>>>> int amdgpu_userq_mgr_init(struct amdgpu_userq_mgr *userq_mgr,
> >>>>> struct amdgpu_device *adev)
> >>>>> {
> >>>>> mutex_init(&userq_mgr->userq_mutex);
> >>>>> idr_init_base(&userq_mgr->userq_idr, 1);
> >>>>> userq_mgr->adev = adev;
> >>>>> + amdgpu_userqueue_setup_gfx(userq_mgr);
> >>>>> return 0;
> >>>>> }
> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> >>>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> >>>>> index 0451533ddde4..6760abda08df 100644
> >>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
> >>>>> @@ -30,6 +30,7 @@
> >>>>> #include "amdgpu_psp.h"
> >>>>> #include "amdgpu_smu.h"
> >>>>> #include "amdgpu_atomfirmware.h"
> >>>>> +#include "amdgpu_userqueue.h"
> >>>>> #include "imu_v11_0.h"
> >>>>> #include "soc21.h"
> >>>>> #include "nvd.h"
> >>>>> @@ -6422,3 +6423,79 @@ const struct amdgpu_ip_block_version
> >>>>> gfx_v11_0_ip_block =
> >>>>> .rev = 0,
> >>>>> .funcs = &gfx_v11_0_ip_funcs,
> >>>>> };
> >>>>> +
> >>>>> +static int gfx_v11_0_userq_mqd_create(struct amdgpu_userq_mgr *uq_mgr,
> >>>>> + struct drm_amdgpu_userq_in *args_in,
> >>>>> + struct amdgpu_usermode_queue *queue)
> >>>>> +{
> >>>>> + struct amdgpu_device *adev = uq_mgr->adev;
> >>>>> + struct amdgpu_mqd *mqd_gfx_generic =
> >>>>> &adev->mqds[AMDGPU_HW_IP_GFX];
> >>>>> + struct drm_amdgpu_userq_mqd_gfx_v11_0 mqd_user;
> >>>>> + struct amdgpu_mqd_prop userq_props;
> >>>>> + int r;
> >>>>> +
> >>>>> + /* Incoming MQD parameters from userspace to be saved here */
> >>>>> + memset(&mqd_user, 0, sizeof(mqd_user));
> >>>>> +
> >>>>> + /* Structure to initialize MQD for userqueue using generic MQD
> >>>>> init function */
> >>>>> + memset(&userq_props, 0, sizeof(userq_props));
> >>>>> +
> >>>>> + if (args_in->mqd_size != sizeof(struct
> >>>>> drm_amdgpu_userq_mqd_gfx_v11_0)) {
> >>>>> + DRM_ERROR("MQD size mismatch\n");
> >>>>> + return -EINVAL;
> >>>>> + }
> >>>>> +
> >>>>> + if (copy_from_user(&mqd_user, u64_to_user_ptr(args_in->mqd),
> >>>>> args_in->mqd_size)) {
> >>>>> + DRM_ERROR("Failed to get user MQD\n");
> >>>>> + return -EFAULT;
> >>>>> + }
> >>>>> +
> >>>>> + /*
> >>>>> + * Create BO for actual Userqueue MQD now
> >>>>> + * Todo: replace the calls to bo_create_kernel() with
> >>>>> bo_create() and use
> >>>>> + * implicit pinning for the MQD buffers.
> >>>> Well not implicit pinning, but rather fencing of the BO.
> >>>>
> >>> Noted.
> >>>
> >>> - Shashank
> >>>
> >>>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>> + */
> >>>>> + r = amdgpu_bo_create_kernel(adev, mqd_gfx_generic->mqd_size,
> >>>>> PAGE_SIZE,
> >>>>> + AMDGPU_GEM_DOMAIN_GTT,
> >>>>> + &queue->mqd.obj,
> >>>>> + &queue->mqd.gpu_addr,
> >>>>> + &queue->mqd.cpu_ptr);
> >>>>> + if (r) {
> >>>>> + DRM_ERROR("Failed to allocate BO for userqueue (%d)", r);
> >>>>> + return -ENOMEM;
> >>>>> + }
> >>>>> + memset(queue->mqd.cpu_ptr, 0, mqd_gfx_generic->mqd_size);
> >>>>> +
> >>>>> + /* Initialize the MQD BO with user given values */
> >>>>> + userq_props.wptr_gpu_addr = mqd_user.wptr_va;
> >>>>> + userq_props.rptr_gpu_addr = mqd_user.rptr_va;
> >>>>> + userq_props.queue_size = mqd_user.queue_size;
> >>>>> + userq_props.hqd_base_gpu_addr = mqd_user.queue_va;
> >>>>> + userq_props.mqd_gpu_addr = queue->mqd.gpu_addr;
> >>>>> + userq_props.use_doorbell = true;
> >>>>> +
> >>>>> + r = mqd_gfx_generic->init_mqd(adev, (void *)queue->mqd.cpu_ptr,
> >>>>> &userq_props);
> >>>>> + if (r) {
> >>>>> + DRM_ERROR("Failed to initialize MQD for userqueue\n");
> >>>>> + goto free_mqd;
> >>>>> + }
> >>>>> +
> >>>>> + return 0;
> >>>>> +
> >>>>> +free_mqd:
> >>>>> + amdgpu_bo_free_kernel(&queue->mqd.obj, &queue->mqd.gpu_addr,
> >>>>> &queue->mqd.cpu_ptr);
> >>>>> + return r;
> >>>>> +}
> >>>>> +
> >>>>> +static void
> >>>>> +gfx_v11_0_userq_mqd_destroy(struct amdgpu_userq_mgr *uq_mgr, struct
> >>>>> amdgpu_usermode_queue *queue)
> >>>>> +{
> >>>>> + struct amdgpu_userq_obj *mqd = &queue->mqd;
> >>>>> +
> >>>>> + amdgpu_bo_free_kernel(&mqd->obj, &mqd->gpu_addr, &mqd->cpu_ptr);
> >>>>> +}
> >>>>> +
> >>>>> +const struct amdgpu_userq_funcs userq_gfx_v11_funcs = {
> >>>>> + .mqd_create = gfx_v11_0_userq_mqd_create,
> >>>>> + .mqd_destroy = gfx_v11_0_userq_mqd_destroy,
> >>>>> +};
> >>>>> diff --git a/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
> >>>>> b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
> >>>>> index 55ed6512a565..240f92796f00 100644
> >>>>> --- a/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
> >>>>> +++ b/drivers/gpu/drm/amd/include/amdgpu_userqueue.h
> >>>>> @@ -29,6 +29,12 @@
> >>>>> struct amdgpu_mqd_prop;
> >>>>> +struct amdgpu_userq_obj {
> >>>>> + void *cpu_ptr;
> >>>>> + uint64_t gpu_addr;
> >>>>> + struct amdgpu_bo *obj;
> >>>>> +};
> >>>>> +
> >>>>> struct amdgpu_usermode_queue {
> >>>>> int queue_type;
> >>>>> uint64_t doorbell_handle;
> >>>>> @@ -37,6 +43,7 @@ struct amdgpu_usermode_queue {
> >>>>> struct amdgpu_mqd_prop *userq_prop;
> >>>>> struct amdgpu_userq_mgr *userq_mgr;
> >>>>> struct amdgpu_vm *vm;
> >>>>> + struct amdgpu_userq_obj mqd;
> >>>>> };
> >>>>> struct amdgpu_userq_funcs {
