Re: [PATCH v3 7/9] drm/amdgpu: map usermode queue into MES

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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,



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux