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&data=02%7C01%7Cluben.tuikov%40amd.com%7Cfa26637c8f4243baea4708d84088507f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637330301495842311&sdata=4CH%2Fu%2BbtRqKWJF5ZvRqaEeacOdTXJrLOTOz0Fi9ZwTo%3D&reserved=0
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
