+ dri-devel
On Fri, Aug 14, 2020 at 3:14 PM Luben Tuikov <luben.tuikov@xxxxxxx> 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@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
