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.
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
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).
Also, I would like to express that in my opinion we are spending way too
much time in discussing the 'preferences' around cover letter,
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,
