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@xxxxxxxxx> >>>>> >>>>> 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@xxxxxxx> >>>>> Cc: Christian Koenig <christian.koenig@xxxxxxx> >>>>> Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxx> >>>>> --- >>>> 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 > >> >>> 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@xxxxxxx> >> 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@xxxxxxx> >> Cc: Pekka Paalanen <ppaalanen@xxxxxxxxx> >> Cc: Sebastian Wick <sebastian.wick@xxxxxxxxxx> >> Cc: Vitaly.Prosyak@xxxxxxx >> Cc: Uma Shankar <uma.shankar@xxxxxxxxx> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Cc: Joshua Ashton <joshua@xxxxxxxxx> >> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> >> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx >> Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx >> Reviewed-By: Joshua Ashton <joshua@xxxxxxxxx> >> Link: https://patchwork.freedesktop.org/patch/msgid/20230113162428.33874-3-harry.wentland@xxxxxxx >> Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx> >> >> 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. > 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, 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,
