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

[Felix Kuehling]

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,