On 8/25/2021 2:29 PM, Christian König wrote:
Am 25.08.21 um 14:20 schrieb Lazar, Lijo:
On 8/25/2021 4:52 PM, Nirmoy Das wrote:
To get a hardware queue priority for a context, we are currently
mapping AMDGPU_CTX_PRIORITY_* to DRM_SCHED_PRIORITY_* and then
to hardware queue priority, which is not the right way to do that
as DRM_SCHED_PRIORITY_* is software scheduler's priority and it is
independent from a hardware queue priority.

Use userspace provided context priority, AMDGPU_CTX_PRIORITY_* to
map a context to proper hardware queue priority.

Signed-off-by: Nirmoy Das <nirmoy....@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 127 ++++++++++++++++------
  drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |   8 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c |  44 ++------
  3 files changed, 105 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index e7a010b7ca1f..c88c5c6c54a2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -43,14 +43,61 @@ const unsigned int amdgpu_ctx_num_entities[AMDGPU_HW_IP_NUM] = {
      [AMDGPU_HW_IP_VCN_JPEG]    =    1,
  };
  +bool amdgpu_ctx_priority_is_valid(int32_t ctx_prio)
+{
+    switch (ctx_prio) {
+    case AMDGPU_CTX_PRIORITY_UNSET:
+    case AMDGPU_CTX_PRIORITY_VERY_LOW:
+    case AMDGPU_CTX_PRIORITY_LOW:
+    case AMDGPU_CTX_PRIORITY_NORMAL:
+    case AMDGPU_CTX_PRIORITY_HIGH:
+    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
+        return true;
+    default:
+        return false;
+    }
+}
+
+static enum drm_sched_priority
+amdgpu_ctx_to_drm_sched_prio(int32_t ctx_prio)
+{
+    switch (ctx_prio) {
+    case AMDGPU_CTX_PRIORITY_UNSET:
+        return DRM_SCHED_PRIORITY_UNSET;
+
+    case AMDGPU_CTX_PRIORITY_VERY_LOW:
+        return DRM_SCHED_PRIORITY_MIN;
+
+    case AMDGPU_CTX_PRIORITY_LOW:
+        return DRM_SCHED_PRIORITY_MIN;
+
+    case AMDGPU_CTX_PRIORITY_NORMAL:
+        return DRM_SCHED_PRIORITY_NORMAL;
+
+    case AMDGPU_CTX_PRIORITY_HIGH:
+        return DRM_SCHED_PRIORITY_HIGH;
+
+    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
+        return DRM_SCHED_PRIORITY_HIGH;
+
+    /* This should not happen as we sanitized userspace provided priority
+     * already, WARN if this happens.
+     */
+    default:
+        WARN(1, "Invalid context priority %d\n", ctx_prio);
+        return DRM_SCHED_PRIORITY_NORMAL;
+    }
+
+}
+
  static int amdgpu_ctx_priority_permit(struct drm_file *filp,
-                      enum drm_sched_priority priority)
+                      int32_t priority)
  {
-    if (priority < 0 || priority >= DRM_SCHED_PRIORITY_COUNT)
+    if (!amdgpu_ctx_priority_is_valid(priority))
          return -EINVAL;
        /* NORMAL and below are accessible by everyone */
-    if (priority <= DRM_SCHED_PRIORITY_NORMAL)
+    if (priority <= AMDGPU_CTX_PRIORITY_NORMAL)
          return 0;
        if (capable(CAP_SYS_NICE))
@@ -62,26 +109,35 @@ static int amdgpu_ctx_priority_permit(struct drm_file *filp,
      return -EACCES;
  }
  -static enum gfx_pipe_priority amdgpu_ctx_sched_prio_to_compute_prio(enum drm_sched_priority prio) +static enum gfx_pipe_priority amdgpu_ctx_prio_to_compute_prio(int32_t prio)
  {
      switch (prio) {
-    case DRM_SCHED_PRIORITY_HIGH:
-    case DRM_SCHED_PRIORITY_KERNEL:
+    case AMDGPU_CTX_PRIORITY_HIGH:
+    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
          return AMDGPU_GFX_PIPE_PRIO_HIGH;
      default:
          return AMDGPU_GFX_PIPE_PRIO_NORMAL;
      }
  }
  -static unsigned int amdgpu_ctx_prio_sched_to_hw(struct amdgpu_device *adev,
-                         enum drm_sched_priority prio,
-                         u32 hw_ip)
+static unsigned int amdgpu_ctx_get_hw_prio(struct amdgpu_ctx *ctx, u32 hw_ip)
  {
+    struct amdgpu_device *adev = ctx->adev;
+    int32_t ctx_prio;
      unsigned int hw_prio;
  -    hw_prio = (hw_ip == AMDGPU_HW_IP_COMPUTE) ?
-            amdgpu_ctx_sched_prio_to_compute_prio(prio) :
-            AMDGPU_RING_PRIO_DEFAULT;
+    ctx_prio = (ctx->override_priority == AMDGPU_CTX_PRIORITY_UNSET) ?
+            ctx->init_priority : ctx->override_priority;
+
+    switch (hw_ip) {
+    case AMDGPU_HW_IP_COMPUTE:
+        hw_prio = amdgpu_ctx_prio_to_compute_prio(ctx_prio);
+        break;
+    default:
+        hw_prio = AMDGPU_RING_PRIO_DEFAULT;
+        break;
+    }
+
      hw_ip = array_index_nospec(hw_ip, AMDGPU_HW_IP_NUM);
      if (adev->gpu_sched[hw_ip][hw_prio].num_scheds == 0)
          hw_prio = AMDGPU_RING_PRIO_DEFAULT;
@@ -89,15 +145,17 @@ static unsigned int amdgpu_ctx_prio_sched_to_hw(struct amdgpu_device *adev,
      return hw_prio;
  }
  +
  static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip,
-                   const u32 ring)
+                  const u32 ring)
  {
      struct amdgpu_device *adev = ctx->adev;
      struct amdgpu_ctx_entity *entity;
      struct drm_gpu_scheduler **scheds = NULL, *sched = NULL;
      unsigned num_scheds = 0;
+    int32_t ctx_prio;
      unsigned int hw_prio;
-    enum drm_sched_priority priority;
+    enum drm_sched_priority drm_prio;
      int r;
        entity = kzalloc(struct_size(entity, fences, amdgpu_sched_jobs), @@ -105,10 +163,11 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip,
      if (!entity)
          return  -ENOMEM;
  +    ctx_prio = (ctx->override_priority == AMDGPU_CTX_PRIORITY_UNSET) ?
+            ctx->init_priority : ctx->override_priority;
      entity->sequence = 1;
-    priority = (ctx->override_priority == DRM_SCHED_PRIORITY_UNSET) ?
-                ctx->init_priority : ctx->override_priority;
-    hw_prio = amdgpu_ctx_prio_sched_to_hw(adev, priority, hw_ip);
+    hw_prio = amdgpu_ctx_get_hw_prio(ctx, hw_ip);
+    drm_prio = amdgpu_ctx_to_drm_sched_prio(ctx_prio);
        hw_ip = array_index_nospec(hw_ip, AMDGPU_HW_IP_NUM);
      scheds = adev->gpu_sched[hw_ip][hw_prio].sched;
@@ -124,7 +183,7 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip,
          num_scheds = 1;
      }
  -    r = drm_sched_entity_init(&entity->entity, priority, scheds, num_scheds, +    r = drm_sched_entity_init(&entity->entity, drm_prio, scheds, num_scheds,
                    &ctx->guilty);
      if (r)
          goto error_free_entity;
@@ -139,7 +198,7 @@ static int amdgpu_ctx_init_entity(struct amdgpu_ctx *ctx, u32 hw_ip,
  }
    static int amdgpu_ctx_init(struct amdgpu_device *adev,
-               enum drm_sched_priority priority,
+               int32_t priority,
                 struct drm_file *filp,
                 struct amdgpu_ctx *ctx)
  {
@@ -161,7 +220,7 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev,
      ctx->reset_counter_query = ctx->reset_counter;
      ctx->vram_lost_counter = atomic_read(&adev->vram_lost_counter);
      ctx->init_priority = priority;
-    ctx->override_priority = DRM_SCHED_PRIORITY_UNSET;
+    ctx->override_priority = AMDGPU_CTX_PRIORITY_UNSET;
        return 0;
  }
@@ -234,7 +293,7 @@ int amdgpu_ctx_get_entity(struct amdgpu_ctx *ctx, u32 hw_ip, u32 instance,
  static int amdgpu_ctx_alloc(struct amdgpu_device *adev,
                  struct amdgpu_fpriv *fpriv,
                  struct drm_file *filp,
-                enum drm_sched_priority priority,
+                int32_t priority,
                  uint32_t *id)
  {
      struct amdgpu_ctx_mgr *mgr = &fpriv->ctx_mgr;
@@ -397,19 +456,19 @@ int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
  {
      int r;
      uint32_t id;
-    enum drm_sched_priority priority;
+    int32_t priority;
        union drm_amdgpu_ctx *args = data;
      struct amdgpu_device *adev = drm_to_adev(dev);
      struct amdgpu_fpriv *fpriv = filp->driver_priv;
        id = args->in.ctx_id;
-    r = amdgpu_to_sched_priority(args->in.priority, &priority);
+    priority = args->in.priority;
        /* For backwards compatibility reasons, we need to accept
       * ioctls with garbage in the priority field */
-    if (r == -EINVAL)
-        priority = DRM_SCHED_PRIORITY_NORMAL;
+    if (!amdgpu_ctx_priority_is_valid(priority))
+        priority = AMDGPU_CTX_PRIORITY_NORMAL;
        switch (args->in.op) {
      case AMDGPU_CTX_OP_ALLOC_CTX:
@@ -515,9 +574,9 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
  }
    static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
-                        struct amdgpu_ctx_entity *aentity,
-                        int hw_ip,
-                        enum drm_sched_priority priority)
+                       struct amdgpu_ctx_entity *aentity,
+                       int hw_ip,
+                       int32_t priority)
  {
      struct amdgpu_device *adev = ctx->adev;
      unsigned int hw_prio;
@@ -525,12 +584,12 @@ static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
      unsigned num_scheds;
        /* set sw priority */
-    drm_sched_entity_set_priority(&aentity->entity, priority);
+    drm_sched_entity_set_priority(&aentity->entity,
+ amdgpu_ctx_to_drm_sched_prio(priority));
        /* set hw priority */
      if (hw_ip == AMDGPU_HW_IP_COMPUTE) {
-        hw_prio = amdgpu_ctx_prio_sched_to_hw(adev, priority,
-                              AMDGPU_HW_IP_COMPUTE);
+        hw_prio = amdgpu_ctx_get_hw_prio(ctx, hw_ip);
          hw_prio = array_index_nospec(hw_prio, AMDGPU_RING_PRIO_MAX);

Not related to this patch, but this kind of logic may break some day. There is a HWIP specific priority and there is another RING_PRIO which is unmapped to HWIP priority Ex: when a new priority is added for VCN which is higher than any of the existing priorities.

Yes, that's something I've noticed as well.

Either we should leave the exact mapping to the engine or have a global definition of possible hw priorities (the later is preferable I think).


Will  this be sufficient :

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
index d43fe2ed8116..937320293029 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.h
@@ -43,9 +43,9 @@
 #define AMDGPU_MAX_COMPUTE_QUEUES KGD_MAX_QUEUES

 enum gfx_pipe_priority {
-       AMDGPU_GFX_PIPE_PRIO_NORMAL = 1,
-       AMDGPU_GFX_PIPE_PRIO_HIGH,
-       AMDGPU_GFX_PIPE_PRIO_MAX
+       AMDGPU_GFX_PIPE_PRIO_NORMAL = AMDGPU_RING_PRIO_1,
+       AMDGPU_GFX_PIPE_PRIO_HIGH = AMDGPU_RING_PRIO_2,
+       AMDGPU_GFX_PIPE_PRIO_MAX = AMDGPU_RING_PRIO_3
 };

 /* Argument for PPSMC_MSG_GpuChangeState */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
index e713d31619fe..c54539faf209 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
@@ -37,7 +37,14 @@
 #define AMDGPU_MAX_UVD_ENC_RINGS       2

 #define AMDGPU_RING_PRIO_DEFAULT       1
-#define AMDGPU_RING_PRIO_MAX           AMDGPU_GFX_PIPE_PRIO_MAX
+
+enum amdgpu_ring_priority_level {
+       AMDGPU_RING_PRIO_0,
+       AMDGPU_RING_PRIO_1,
+       AMDGPU_RING_PRIO_2,
+       AMDGPU_RING_PRIO_3,
+       AMDGPU_RING_PRIO_MAX

+};


Regards,

Nirmoy


Christian.


Thanks,
Lijo

          scheds = adev->gpu_sched[hw_ip][hw_prio].sched;
          num_scheds = adev->gpu_sched[hw_ip][hw_prio].num_scheds;
@@ -540,14 +599,14 @@ static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
  }
    void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
-                  enum drm_sched_priority priority)
+                  int32_t priority)
  {
-    enum drm_sched_priority ctx_prio;
+    int32_t ctx_prio;
      unsigned i, j;
        ctx->override_priority = priority;
  -    ctx_prio = (ctx->override_priority == DRM_SCHED_PRIORITY_UNSET) ?
+    ctx_prio = (ctx->override_priority == AMDGPU_CTX_PRIORITY_UNSET) ?
              ctx->init_priority : ctx->override_priority;
      for (i = 0; i < AMDGPU_HW_IP_NUM; ++i) {
          for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index 14db16bc3322..a44b8b8ed39c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -47,8 +47,8 @@ struct amdgpu_ctx {
      spinlock_t            ring_lock;
      struct amdgpu_ctx_entity *entities[AMDGPU_HW_IP_NUM][AMDGPU_MAX_ENTITY_NUM];
      bool                preamble_presented;
-    enum drm_sched_priority        init_priority;
-    enum drm_sched_priority        override_priority;
+    int32_t                init_priority;
+    int32_t                override_priority;
      struct mutex            lock;
      atomic_t            guilty;
      unsigned long            ras_counter_ce;
@@ -75,8 +75,8 @@ void amdgpu_ctx_add_fence(struct amdgpu_ctx *ctx,
  struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
                         struct drm_sched_entity *entity,
                         uint64_t seq);
-void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
-                  enum drm_sched_priority priority);
+bool amdgpu_ctx_priority_is_valid(int32_t ctx_prio);
+void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, int32_t ctx_prio);
    int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
               struct drm_file *filp);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
index b7d861ed5284..e9b45089a28a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
@@ -32,37 +32,9 @@
  #include "amdgpu_sched.h"
  #include "amdgpu_vm.h"
  -int amdgpu_to_sched_priority(int amdgpu_priority,
-                 enum drm_sched_priority *prio)
-{
-    switch (amdgpu_priority) {
-    case AMDGPU_CTX_PRIORITY_VERY_HIGH:
-        *prio = DRM_SCHED_PRIORITY_HIGH;
-        break;
-    case AMDGPU_CTX_PRIORITY_HIGH:
-        *prio = DRM_SCHED_PRIORITY_HIGH;
-        break;
-    case AMDGPU_CTX_PRIORITY_NORMAL:
-        *prio = DRM_SCHED_PRIORITY_NORMAL;
-        break;
-    case AMDGPU_CTX_PRIORITY_LOW:
-    case AMDGPU_CTX_PRIORITY_VERY_LOW:
-        *prio = DRM_SCHED_PRIORITY_MIN;
-        break;
-    case AMDGPU_CTX_PRIORITY_UNSET:
-        *prio = DRM_SCHED_PRIORITY_UNSET;
-        break;
-    default:
-        WARN(1, "Invalid context priority %d\n", amdgpu_priority);
-        return -EINVAL;
-    }
-
-    return 0;
-}
-
  static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev,
                            int fd,
-                          enum drm_sched_priority priority)
+                          int32_t priority)
  {
      struct fd f = fdget(fd);
      struct amdgpu_fpriv *fpriv;
@@ -89,7 +61,7 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev,   static int amdgpu_sched_context_priority_override(struct amdgpu_device *adev,
                            int fd,
                            unsigned ctx_id,
-                          enum drm_sched_priority priority)
+                          int32_t priority)
  {
      struct fd f = fdget(fd);
      struct amdgpu_fpriv *fpriv;
@@ -124,7 +96,6 @@ int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
  {
      union drm_amdgpu_sched *args = data;
      struct amdgpu_device *adev = drm_to_adev(dev);
-    enum drm_sched_priority priority;
      int r;
        /* First check the op, then the op's argument.
@@ -138,21 +109,22 @@ int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
          return -EINVAL;
      }
  -    r = amdgpu_to_sched_priority(args->in.priority, &priority);
-    if (r)
-        return r;
+    if (!amdgpu_ctx_priority_is_valid(args->in.priority)) {
+        WARN(1, "Invalid context priority %d\n", args->in.priority);
+        return -EINVAL;
+    }
        switch (args->in.op) {
      case AMDGPU_SCHED_OP_PROCESS_PRIORITY_OVERRIDE:
          r = amdgpu_sched_process_priority_override(adev,
                                 args->in.fd,
-                               priority);
+                               args->in.priority);
          break;
      case AMDGPU_SCHED_OP_CONTEXT_PRIORITY_OVERRIDE:
          r = amdgpu_sched_context_priority_override(adev,
                                 args->in.fd,
                                 args->in.ctx_id,
-                               priority);
+                               args->in.priority);
          break;
      default:
          /* Impossible.


Reply via email to