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