[PATCH v3 7/9] drm/amdgpu: map usermode queue into MES
Shashank Sharma
shashank.sharma at amd.com
Thu Apr 6 07:45:03 UTC 2023
On 05/04/2023 23:18, Luben Tuikov wrote:
> On 2023-04-05 06:06, Shashank Sharma wrote:
>> On 04/04/2023 22:58, Luben Tuikov wrote:
>>> On 2023-04-04 12:36, Shashank Sharma wrote:
>>>> On 04/04/2023 18:30, Luben Tuikov wrote:
>>>>> On 2023-03-29 12:04, Shashank Sharma wrote:
>>>>>> From: Shashank Sharma <contactshashanksharma at gmail.com>
>>>>>>
>>>>>> This patch adds new functions to map/unmap a usermode queue into
>>>>>> the FW, using the MES ring. As soon as this mapping is done, the
>>>>>> queue would be considered ready to accept the workload.
>>>>>>
>>>>>> V1: Addressed review comments from Alex on the RFC patch series
>>>>>> - Map/Unmap should be IP specific.
>>>>>> V2:
>>>>>> Addressed review comments from Christian:
>>>>>> - Fix the wptr_mc_addr calculation (moved into another patch)
>>>>>> Addressed review comments from Alex:
>>>>>> - Do not add fptrs for map/unmap
>>>>>>
>>>>>> V3: Integration with doorbell manager
>>>>>>
>>>>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>>>>> Cc: Christian Koenig <christian.koenig at amd.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma at amd.com>
>>>>>> ---
>>>>> Just add all your Cc right here, and let git-send-email figure it out.
>>>>> Between the Cc tags and the SMTP CC list, Felix is the only one missing.
>>>> No, that's not how it is.
>>>>
>>>> You keep people cc'ed in the cover letter so that they get informed
>>>> every time this patch is pushed/used on any opensource branch.
>>> The cover letter is optional, and you can add Cc tags
>>> into the cover letter and then git-send-email would extract those
>>> (any and all) tags from the cover letter too.
>>>
>>> When you pick and choose whom to add in the Cc tags, and whom to
>>> add to the SMTP CC list, it creates division.
>>
>> Exactly my point, there is no guideline on whom to add in Cc
>> cover-letter and whom to add manually, its all preference.
>>
>> Now different people can have different preference, and a review comment
>> on what is your preference of what to
>>
>> keep on cover letter does seem like a nitpick.
> I am describing consensus. Take a look at DRM commits to see what
> people do. It'd be nice if you followed that
No, not every DRM patch needs to be like that. I have added some
examples below.
>>>> People who are added manually in cc are required for this code review
>>>> session.
>>> No such rule exists. It is best to add all the Cc into the commit message,
>>> so that it is preserved in Git history.
>> I believe this is also not a rule, we are discussing preferences only.
>> It is my preference that I want to keep only Alex and Christian in Cc.
>>> For instance, I just randomly did "git log drivers/gpu/drm/*.[hc]" in
>>> amd-staging-drm-next, and this is the first commit which came up,
>>>
>>> commit 097ee58f3ddf7d
>>> Author: Harry Wentland <harry.wentland at amd.com>
>>> Date: Fri Jan 13 11:24:09 2023 -0500
>>>
>>> drm/connector: print max_requested_bpc in state debugfs
>>>
>>> This is useful to understand the bpc defaults and
>>> support of a driver.
>>>
>>> Signed-off-by: Harry Wentland <harry.wentland at amd.com>
>>> Cc: Pekka Paalanen <ppaalanen at gmail.com>
>>> Cc: Sebastian Wick <sebastian.wick at redhat.com>
>>> Cc: Vitaly.Prosyak at amd.com
>>> Cc: Uma Shankar <uma.shankar at intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Cc: Joshua Ashton <joshua at froggi.es>
>>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>>> Cc: dri-devel at lists.freedesktop.org
>>> Cc: amd-gfx at lists.freedesktop.org
>>> Reviewed-By: Joshua Ashton <joshua at froggi.es>
>>> Link: https://patchwork.freedesktop.org/patch/msgid/20230113162428.33874-3-harry.wentland@amd.com
>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
>>>
>>> As you can see the whole Cc list and the MLs are part of the Cc tags.
>>> And the rest of the commits are also good examples of how to do it.
>>> (Don't worry about the Link tag--it is automatically added by tools
>>> maintainers use, although some use Lore.)
>>> This preserves things in Git history, and it's a good thing if we need
>>> to data mine and brainstorm later on on patches, design, and so on.
>> No, this is not random, this is actually well planned. All of these
> I never said it is "random"--it is not, it is well planned because
> everyone submitting to DRM does this--it's common practice.
>> people here in Cc are either the maintainers or respective domain experts or
>>
>> contributors of color management feature and keeping them in CC is about
>> how this color management feature is being carried forward, so this is
>>
>> exactly aligned with my point. Do note that this is a DRM level change
>> (not AMDGPU level).
> 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.
>> 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