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

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

 



+ 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux