[PATCH v3 7/9] drm/amdgpu: map usermode queue into MES
Shashank Sharma
shashank.sharma at amd.com
Fri Apr 7 06:41:57 UTC 2023
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,
More information about the amd-gfx
mailing list