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

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

 



On 2020-08-14 3:28 p.m., Alex Deucher wrote:
> + 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.

Sure, I can revert this. Personally, I don't like single letter
variables as they are very inexpressive and hard to search for.
I thought "prio" to be easier to type than "priority", but I can
revert this.

Regards,
Luben

> 
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=02%7C01%7Cluben.tuikov%40amd.com%7Cfa26637c8f4243baea4708d84088507f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637330301495842311&amp;sdata=4CH%2Fu%2BbtRqKWJF5ZvRqaEeacOdTXJrLOTOz0Fi9ZwTo%3D&amp;reserved=0

_______________________________________________
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