Re: [PATCH v3 7/9] drm/amdgpu: map usermode queue into MES

[Felix Kuehling <felix.kuehling@xxxxxxx>]

Am 2023-04-06 um 03:45 schrieb Shashank Sharma:
> On 05/04/2023 23:18, Luben Tuikov wrote: So, then why isn't Felix in 
> the Cc tags? Doorbell changes touch that area too.
> > He's actually the only one you left out, other than the MLs emails.
> > Either add everyone to the Cc tags in the commit message, or only add
> > your Sob line and leave everyone to a --cc= on the command line. Both 
> > are
> > common practice and acceptable.
>
> This code touches KFD code, and that's why Felix needs to be involved 
> in the code-review process. But
>
> once code review is done, I don't want to spam his email every time 
> this patch is pushed into some branch,
>
> so he is not in cover-letter CC. This is how I prefer to drive the 
> code review of this patch, and I don't see a problem here.

It's a big patch series, and I don't have time to review the whole thing 
in detail. A CC tag on the patches that need my attention would help.

Thanks,
  Felix


> > > Also, I would like to express that in my opinion we are spending way 
> > > too
> > > much time in discussing the 'preferences' around cover letter,
> > I agree. But those aren't "preferences," they are common practices,
>
> This is not a common practice, its your interpretation of it.
>
> I also picked a few examples:
>
>
> https://patchwork.freedesktop.org/patch/531143/?series=116163&rev=1
>
> This patch has multiple communities in Cc, none in cover-letter (also 
> R-B'ed by you).
>
>
> https://patchwork.freedesktop.org/patch/505571/
>
> https://patchwork.freedesktop.org/patch/442410/
>
> These are some of patches which has multiple people missing in the 
> cover-letter CC than email-CC.
>
>
> https://patchwork.freedesktop.org/patch/530652/?series=116055&rev=1
>
> This patch has multiple people in email-cc but none in cover-letter CC.
>
>
> There could be tons of such examples for both, and the maintainers do 
> not have any problem with that,
>
> So I don't consider this as common practice in DRM community, its just 
> a preference, and hence it consumed
>
> a lot more time and efforts in this discussion than what it should have.
>
> - Shashank
>
> > like for instance not separating the Cc tags and the Sob tags with
> > an empty line, or shifting struct member names to the same column
> > for readability, and so on. Use "git log -- drivers/gpu/drm".
> >
> > Regards,
> > Luben
> >
> > > words, comments and variable names. No cover letter is perfect, no
> > > commit message is good enough to explain what is happening in code,
> > >
> > > no variable name is flawless and no comment explains what is going 
> > > on in
> > > code which is clear to everyone. These are very subjective to the
> > >
> > > author and the reader. The only deciding factor is if there is a
> > > community rule/guideline on that.
> > >
> > >
> > > I appreciate your time and suggestions but I also certainly do not want
> > > to spend this much time to discuss how should we add people in Cc.
> > >
> > > Let's keep preferences separate from code review process.
> > >
> > > - Shashank
> > >
> > > > A good tool to use is "scripts/get_maintainer.pl" which works
> > > > on a file, directory and even patch files.
> > > >
> > > > I usually include everyone get_maintainer.pl prints, and on 
> > > > subsequent patch
> > > > revisions, also people who have previously commented on the 
> > > > patchset, as they
> > > > might be interested to follow up. It's a good thing to do.
> > > >
> > > > Here are a couple of resources, in addition to DRM commits in the 
> > > > tree,
> > > > https://www.kernel.org/doc/html/v4.12/process/5.Posting.html#patch-formatting-and-changelogs 
> > > >
> > > > https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html#the-canonical-patch-format 
> > > >
> > > > (The second link includes links to more resources on good patch 
> > > > writing.)
> > > >
> > > > Hope this helps.
> > > >
> > > > Regards,
> > > > Luben
> > > >
> > > >
> > > > > - Shashank
> > > > >
> > > > > > Regards,
> > > > > > Luben
> > > > > >
> > > > > > > .../drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c | 70 
> > > > > > > +++++++++++++++++++
> > > > > > >     1 file changed, 70 insertions(+)
> > > > > > >
> > > > > > > diff --git 
> > > > > > > a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c 
> > > > > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> > > > > > > index 39e90ea32fcb..1627641a4a4e 100644
> > > > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> > > > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue_gfx_v11.c
> > > > > > > @@ -23,12 +23,73 @@
> > > > > > >     #include "amdgpu.h"
> > > > > > >     #include "amdgpu_userqueue.h"
> > > > > > >     #include "v11_structs.h"
> > > > > > > +#include "amdgpu_mes.h"
> > > > > > >         #define AMDGPU_USERQ_PROC_CTX_SZ PAGE_SIZE
> > > > > > >     #define AMDGPU_USERQ_GANG_CTX_SZ PAGE_SIZE
> > > > > > >     #define AMDGPU_USERQ_FW_CTX_SZ PAGE_SIZE
> > > > > > >     #define AMDGPU_USERQ_GDS_CTX_SZ PAGE_SIZE
> > > > > > >     +static int
> > > > > > > +amdgpu_userq_gfx_v11_map(struct amdgpu_userq_mgr *uq_mgr,
> > > > > > > +                         struct amdgpu_usermode_queue *queue)
> > > > > > > +{
> > > > > > > +    struct amdgpu_device *adev = uq_mgr->adev;
> > > > > > > +    struct mes_add_queue_input queue_input;
> > > > > > > +    int r;
> > > > > > > +
> > > > > > > +    memset(&queue_input, 0x0, sizeof(struct mes_add_queue_input));
> > > > > > > +
> > > > > > > +    queue_input.process_va_start = 0;
> > > > > > > +    queue_input.process_va_end = (adev->vm_manager.max_pfn - 1) 
> > > > > > > << AMDGPU_GPU_PAGE_SHIFT;
> > > > > > > +    queue_input.process_quantum = 100000; /* 10ms */
> > > > > > > +    queue_input.gang_quantum = 10000; /* 1ms */
> > > > > > > +    queue_input.paging = false;
> > > > > > > +
> > > > > > > +    queue_input.gang_context_addr = queue->gang_ctx_gpu_addr;
> > > > > > > +    queue_input.process_context_addr = queue->proc_ctx_gpu_addr;
> > > > > > > +    queue_input.inprocess_gang_priority = 
> > > > > > > AMDGPU_MES_PRIORITY_LEVEL_NORMAL;
> > > > > > > +    queue_input.gang_global_priority_level = 
> > > > > > > AMDGPU_MES_PRIORITY_LEVEL_NORMAL;
> > > > > > > +
> > > > > > > +    queue_input.process_id = queue->vm->pasid;
> > > > > > > +    queue_input.queue_type = queue->queue_type;
> > > > > > > +    queue_input.mqd_addr = queue->mqd.gpu_addr;
> > > > > > > +    queue_input.wptr_addr = queue->userq_prop.wptr_gpu_addr;
> > > > > > > +    queue_input.queue_size = queue->userq_prop.queue_size >> 2;
> > > > > > > +    queue_input.doorbell_offset = 
> > > > > > > queue->userq_prop.doorbell_index;
> > > > > > > +    queue_input.page_table_base_addr = 
> > > > > > > amdgpu_gmc_pd_addr(queue->vm->root.bo);
> > > > > > > +
> > > > > > > +    amdgpu_mes_lock(&adev->mes);
> > > > > > > +    r = adev->mes.funcs->add_hw_queue(&adev->mes, &queue_input);
> > > > > > > +    amdgpu_mes_unlock(&adev->mes);
> > > > > > > +    if (r) {
> > > > > > > +        DRM_ERROR("Failed to map queue in HW, err (%d)\n", r);
> > > > > > > +        return r;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    DRM_DEBUG_DRIVER("Queue %d mapped successfully\n", 
> > > > > > > queue->queue_id);
> > > > > > > +    return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void
> > > > > > > +amdgpu_userq_gfx_v11_unmap(struct amdgpu_userq_mgr *uq_mgr,
> > > > > > > +                           struct amdgpu_usermode_queue *queue)
> > > > > > > +{
> > > > > > > +    struct amdgpu_device *adev = uq_mgr->adev;
> > > > > > > +    struct mes_remove_queue_input queue_input;
> > > > > > > +    int r;
> > > > > > > +
> > > > > > > +    memset(&queue_input, 0x0, sizeof(struct 
> > > > > > > mes_remove_queue_input));
> > > > > > > +    queue_input.doorbell_offset = 
> > > > > > > queue->userq_prop.doorbell_index;
> > > > > > > +    queue_input.gang_context_addr = queue->gang_ctx_gpu_addr;
> > > > > > > +
> > > > > > > +    amdgpu_mes_lock(&adev->mes);
> > > > > > > +    r = adev->mes.funcs->remove_hw_queue(&adev->mes, 
> > > > > > > &queue_input);
> > > > > > > +    amdgpu_mes_unlock(&adev->mes);
> > > > > > > +    if (r)
> > > > > > > +        DRM_ERROR("Failed to unmap queue in HW, err (%d)\n", r);
> > > > > > > +}
> > > > > > > +
> > > > > > >     static int amdgpu_userq_gfx_v11_create_ctx_space(struct 
> > > > > > > amdgpu_userq_mgr *uq_mgr,
> > > > > > > struct amdgpu_usermode_queue *queue)
> > > > > > >     {
> > > > > > > @@ -129,6 +190,14 @@ amdgpu_userq_gfx_v11_mqd_create(struct 
> > > > > > > amdgpu_userq_mgr *uq_mgr, struct amdgpu_u
> > > > > > >             amdgpu_userq_set_ctx_space(uq_mgr, queue);
> > > > > > >         amdgpu_bo_unreserve(mqd->obj);
> > > > > > > +
> > > > > > > +    /* Map the queue in HW using MES ring */
> > > > > > > +    r = amdgpu_userq_gfx_v11_map(uq_mgr, queue);
> > > > > > > +    if (r) {
> > > > > > > +        DRM_ERROR("Failed to map userqueue (%d)\n", r);
> > > > > > > +        goto free_ctx;
> > > > > > > +    }
> > > > > > > +
> > > > > > >         DRM_DEBUG_DRIVER("MQD for queue %d created\n", 
> > > > > > > queue->queue_id);
> > > > > > >         return 0;
> > > > > > >     @@ -147,6 +216,7 @@ amdgpu_userq_gfx_v11_mqd_destroy(struct 
> > > > > > > amdgpu_userq_mgr *uq_mgr, struct amdgpu_
> > > > > > >     {
> > > > > > >         struct amdgpu_userq_ctx_space *mqd = &queue->mqd;
> > > > > > >     +    amdgpu_userq_gfx_v11_unmap(uq_mgr, queue);
> > > > > > >         amdgpu_userq_gfx_v11_destroy_ctx_space(uq_mgr, queue);
> > > > > > >         amdgpu_bo_free_kernel(&mqd->obj,
> > > > > > >                    &mqd->gpu_addr,