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

[Shashank Sharma <shashank.sharma@xxxxxxx>]

On 06/04/2023 17:16, Felix Kuehling wrote:
> 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.
Hey Felix,

noted. There are only 2 patches which touch the KFD code, I will keep 
you in CC only for those.

- Shashank


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