Re: [PATCH 2/2] drm/scheduler: Remove priority macro INVALID

+ dri-devel

On Fri, Aug 14, 2020 at 3:14 PM Luben Tuikov luben.tuikov@amd.com wrote:

Remove DRM_SCHED_PRIORITY_INVALID. We no longer carry around an invalid priority and cut it off at the source.

Backwards compatibility behaviour of AMDGPU CTX IOCTL passing in garbage for context priority from user space and then mapping that to DRM_SCHED_PRIORITY_NORMAL is preserved.

Signed-off-by: Luben Tuikov luben.tuikov@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 21 ++++---- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 62 +++++++++++++++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h | 3 +- include/drm/gpu_scheduler.h | 1 - 4 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c index fd97ac34277b..10d3bfa416c0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c @@ -384,42 +384,41 @@ static int amdgpu_ctx_query2(struct amdgpu_device *adev, int amdgpu_ctx_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) {

•   int r;
  

•   int res;
    uint32_t id;
  

•   enum drm_sched_priority priority;
  

•   enum drm_sched_priority prio;
  

The variable renaming is not directly related to the functional change in the patch and should be split out as a separate patch is you think it should be applied. I personally prefer the original naming. The driver uses r or ret for return value checking pretty consistently. I also prefer to spell things out unless the names are really long. Makes it more obvious what they are for. Same comment on the functions below as well.

Alex

    union drm_amdgpu_ctx *args = data;
    struct amdgpu_device *adev = dev->dev_private;
    struct amdgpu_fpriv *fpriv = filp->driver_priv;

•   r = 0;
    id = args->in.ctx_id;
  

•   priority = amdgpu_to_sched_priority(args->in.priority);
  

•   res = amdgpu_to_sched_priority(args->in.priority, &prio);
  
    /* For backwards compatibility reasons, we need to accept
     * ioctls with garbage in the priority field */
  

•   if (priority == DRM_SCHED_PRIORITY_INVALID)
  

•           priority = DRM_SCHED_PRIORITY_NORMAL;
  

•   if (res == -EINVAL)
  

•           prio = DRM_SCHED_PRIORITY_NORMAL;
  
    switch (args->in.op) {
    case AMDGPU_CTX_OP_ALLOC_CTX:
  

•           r = amdgpu_ctx_alloc(adev, fpriv, filp, priority, &id);
  

•           res = amdgpu_ctx_alloc(adev, fpriv, filp, prio, &id);
            args->out.alloc.ctx_id = id;
            break;
    case AMDGPU_CTX_OP_FREE_CTX:
  

•           r = amdgpu_ctx_free(fpriv, id);
  

•           res = amdgpu_ctx_free(fpriv, id);
            break;
    case AMDGPU_CTX_OP_QUERY_STATE:
  

•           r = amdgpu_ctx_query(adev, fpriv, id, &args->out);
  

•           res = amdgpu_ctx_query(adev, fpriv, id, &args->out);
            break;
    case AMDGPU_CTX_OP_QUERY_STATE2:
  

•           r = amdgpu_ctx_query2(adev, fpriv, id, &args->out);
  

•           res = amdgpu_ctx_query2(adev, fpriv, id, &args->out);
            break;
    default:
            return -EINVAL;
    }
  
  

•   return r;
  

•   return res;
  

}

struct amdgpu_ctx *amdgpu_ctx_get(struct amdgpu_fpriv *fpriv, uint32_t id) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c index e05bc22a0a49..2398eb646043 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c @@ -32,24 +32,32 @@

#include "amdgpu_vm.h"

-enum drm_sched_priority amdgpu_to_sched_priority(int amdgpu_priority) +int amdgpu_to_sched_priority(int amdgpu_priority,

•                        enum drm_sched_priority *prio)
  

{ switch (amdgpu_priority) { case AMDGPU_CTX_PRIORITY_VERY_HIGH:

•           return DRM_SCHED_PRIORITY_HW;
  

•           *prio = DRM_SCHED_PRIORITY_HW;
  

•           break;
    case AMDGPU_CTX_PRIORITY_HIGH:
  

•           return DRM_SCHED_PRIORITY_SW;
  

•           *prio = DRM_SCHED_PRIORITY_SW;
  

•           break;
    case AMDGPU_CTX_PRIORITY_NORMAL:
  

•           return DRM_SCHED_PRIORITY_NORMAL;
  

•           *prio = DRM_SCHED_PRIORITY_NORMAL;
  

•           break;
    case AMDGPU_CTX_PRIORITY_LOW:
    case AMDGPU_CTX_PRIORITY_VERY_LOW:
  

•           return DRM_SCHED_PRIORITY_MIN;
  

•           *prio = DRM_SCHED_PRIORITY_MIN;
  

•           break;
    case AMDGPU_CTX_PRIORITY_UNSET:
  

•           return DRM_SCHED_PRIORITY_UNSET;
  

•           *prio = DRM_SCHED_PRIORITY_UNSET;
  

•           break;
    default:
            WARN(1, "Invalid context priority %d\n", amdgpu_priority);
  

•           return DRM_SCHED_PRIORITY_INVALID;
  

•           return -EINVAL;
    }
  

•   return 0;
  

}

static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, @@ -116,30 +124,42 @@ int amdgpu_sched_ioctl(struct drm_device *dev, void *data, { union drm_amdgpu_sched *args = data; struct amdgpu_device *adev = dev->dev_private;

•   enum drm_sched_priority priority;
  

•   int r;
  

•   enum drm_sched_priority prio;
  

•   int res;
  
  

•   priority = amdgpu_to_sched_priority(args->in.priority);
  

•   if (priority == DRM_SCHED_PRIORITY_INVALID)
  

•   /* First check the op, then the op's argument.
  

•    */
  

•   switch (args->in.op) {
  

•   case AMDGPU_SCHED_OP_PROCESS_PRIORITY_OVERRIDE:
  

•   case AMDGPU_SCHED_OP_CONTEXT_PRIORITY_OVERRIDE:
  

•           break;
  

•   default:
  

•           DRM_ERROR("Invalid sched op specified: %d\n", args->in.op);
            return -EINVAL;
  

•   }
  

•   res = amdgpu_to_sched_priority(args->in.priority, &prio);
  

•   if (res)
  

•           return res;
  
    switch (args->in.op) {
    case AMDGPU_SCHED_OP_PROCESS_PRIORITY_OVERRIDE:
  

•           r = amdgpu_sched_process_priority_override(adev,
  

•                                                      args->in.fd,
  

•                                                      priority);
  

•           res = amdgpu_sched_process_priority_override(adev,
  

•                                                        args->in.fd,
  

•                                                        prio);
            break;
    case AMDGPU_SCHED_OP_CONTEXT_PRIORITY_OVERRIDE:
  

•           r = amdgpu_sched_context_priority_override(adev,
  

•                                                      args->in.fd,
  

•                                                      args->in.ctx_id,
  

•                                                      priority);
  

•           res = amdgpu_sched_context_priority_override(adev,
  

•                                                        args->in.fd,
  

•                                                        args->in.ctx_id,
  

•                                                        prio);
            break;
    default:
  

•           DRM_ERROR("Invalid sched op specified: %d\n", args->in.op);
  

•           r = -EINVAL;
  

•           /* Impossible.
  

•            */
  

•           res = -EINVAL;
            break;
    }
  
  

•   return r;
  

•   return res;
  

} diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h index 12299fd95691..67e5b2472f6a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.h @@ -30,7 +30,8 @@ enum drm_sched_priority; struct drm_device; struct drm_file;

-enum drm_sched_priority amdgpu_to_sched_priority(int amdgpu_priority); +int amdgpu_to_sched_priority(int amdgpu_priority,

•                        enum drm_sched_priority *prio);
  

int amdgpu_sched_ioctl(struct drm_device *dev, void *data, struct drm_file *filp);

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 8ae9b8f73bf9..d6ee3b2e8407 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -44,7 +44,6 @@ enum drm_sched_priority { DRM_SCHED_PRIORITY_HIGH,

    DRM_SCHED_PRIORITY_COUNT,

•   DRM_SCHED_PRIORITY_INVALID = -1,
    DRM_SCHED_PRIORITY_UNSET = -2
  

};

-- 2.28.0.215.g878e727637

---

amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx