[PATCH] drm/amdgpu: Add out parameters to drm_amdgpu_sched
Arunpravin Paneer Selvam
arunpravin.paneerselvam at amd.com
Mon May 2 09:51:26 UTC 2022
On 5/2/2022 11:26 AM, Christian König wrote:
> Am 01.05.22 um 21:45 schrieb Arunpravin Paneer Selvam:
>>
>> On 5/1/2022 12:59 AM, Christian König wrote:
>>> Am 30.04.22 um 17:14 schrieb Arunpravin Paneer Selvam:
>>>> - Add pipe and queue as out parameters to support high priority
>>>> queue test enabled in libdrm basic test suite.
>>>>
>>>> - Fetch amdgpu_ring pointer and pass sched info to userspace
>>>>
>>>> - Improve amdgpu_sched_ioctl() function
>>>>
>>>> The related merge request for enabling high priority test case are
>>>> libdrm -
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fdrm%2F-%2Fmerge_requests%2F235&data=05%7C01%7CArunpravin.PaneerSelvam%40amd.com%7Ce7a876029be4439b742808da2adfb82a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637869437601696844%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jXIBtdA4seXl%2BYKsY2SDu%2B9tPoH6tfvUUXIRl4IosJc%3D&reserved=0
>>>> mesa -
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2F-%2Fmerge_requests%2F16262&data=05%7C01%7CArunpravin.PaneerSelvam%40amd.com%7Ce7a876029be4439b742808da2adfb82a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637869437601696844%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=%2F%2FwMJ86SXxP7K9STA%2B1x5rox1F5CPEy3JIhFMCjqwiY%3D&reserved=0
>>>>
>>>> Signed-off-by: Arunpravin Paneer Selvam
>>>> <Arunpravin.PaneerSelvam at amd.com>
>>>
>>> Well that is certainly a NAK since as far as I can see this breaks
>>> the UAPI in a not backward compatible way.
>>>
>>> Then we absolutely should not pass scheduler info to userspace. That
>>> should be completely transparent to userspace.
>>>
>>> So can you summarize once more why that should be necessary?
>> I added a new test case named high priority queue test in libdrm
>> basic test suite to validate the kernel feature patch named
>> high priority gfx pipe. In the test case, we are creating a context
>> and overriding the priority as HIGH, submitting a simple NOP
>> command to test the job scheduled on high priority pipe / queue. This
>> we have manually verified using the sysfs entry
>> amdgpu_fence_info where fence signaled on gfx high priority pipe
>> (gfx_0.1.0). To ensure this in a unit test case, we require
>> pipe and queue info.
>
> Completely drop that requirement, it's not even remotely valid
> justification for an UAPI change.
>
> The IOCTLs are for supporting userspace APIs like OpenGL, Vulkan,
> VA-API etc.. and *not* unit tests.
>
> If you need additional information for unit tests we need to add that
> to debugfs instead and as you wrote we already have that in the form
> of the amdgpu_fence_info file.
sure, we will drop this requirement.
Hi Alex,
I verified the sysfs entry amdgpu_fence_info, a submitted high priority
job fence signaled on gfx_0.1.0, shall we push the kernel patch named
Enable high priority gfx queue
into staging branch.
Thanks,
Arun.
>
> Regards,
> Christian.
>
>>
>> Thanks,
>> Arun
>>>
>>> Regards,
>>> Christian.
>>>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 114
>>>> ++++++++--------------
>>>> include/uapi/drm/amdgpu_drm.h | 14 ++-
>>>> 2 files changed, 53 insertions(+), 75 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
>>>> index e9b45089a28a..fc2864b612d9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
>>>> @@ -32,106 +32,72 @@
>>>> #include "amdgpu_sched.h"
>>>> #include "amdgpu_vm.h"
>>>> -static int amdgpu_sched_process_priority_override(struct
>>>> amdgpu_device *adev,
>>>> - int fd,
>>>> - int32_t priority)
>>>> +int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
>>>> + struct drm_file *filp)
>>>> {
>>>> - struct fd f = fdget(fd);
>>>> + union drm_amdgpu_sched *args = data;
>>>> + struct amdgpu_ctx *ctx, *ctx_ptr;
>>>> + struct drm_sched_entity *entity;
>>>> struct amdgpu_fpriv *fpriv;
>>>> - struct amdgpu_ctx *ctx;
>>>> - uint32_t id;
>>>> - int r;
>>>> -
>>>> - if (!f.file)
>>>> - return -EINVAL;
>>>> -
>>>> - r = amdgpu_file_to_fpriv(f.file, &fpriv);
>>>> - if (r) {
>>>> - fdput(f);
>>>> - return r;
>>>> - }
>>>> -
>>>> - idr_for_each_entry(&fpriv->ctx_mgr.ctx_handles, ctx, id)
>>>> - amdgpu_ctx_priority_override(ctx, priority);
>>>> -
>>>> - fdput(f);
>>>> - return 0;
>>>> -}
>>>> -
>>>> -static int amdgpu_sched_context_priority_override(struct
>>>> amdgpu_device *adev,
>>>> - int fd,
>>>> - unsigned ctx_id,
>>>> - int32_t priority)
>>>> -{
>>>> + struct amdgpu_ring *ring;
>>>> + u32 fd = args->in.fd;
>>>> struct fd f = fdget(fd);
>>>> - struct amdgpu_fpriv *fpriv;
>>>> - struct amdgpu_ctx *ctx;
>>>> + u32 id;
>>>> int r;
>>>> if (!f.file)
>>>> return -EINVAL;
>>>> r = amdgpu_file_to_fpriv(f.file, &fpriv);
>>>> - if (r) {
>>>> - fdput(f);
>>>> - return r;
>>>> - }
>>>> + if (r)
>>>> + goto out_fd;
>>>> - ctx = amdgpu_ctx_get(fpriv, ctx_id);
>>>> + ctx = amdgpu_ctx_get(fpriv, args->in.ctx_id);
>>>> if (!ctx) {
>>>> - fdput(f);
>>>> - return -EINVAL;
>>>> - }
>>>> -
>>>> - amdgpu_ctx_priority_override(ctx, priority);
>>>> - amdgpu_ctx_put(ctx);
>>>> - fdput(f);
>>>> -
>>>> - return 0;
>>>> -}
>>>> -
>>>> -int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
>>>> - struct drm_file *filp)
>>>> -{
>>>> - union drm_amdgpu_sched *args = data;
>>>> - struct amdgpu_device *adev = drm_to_adev(dev);
>>>> - int r;
>>>> -
>>>> - /* 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;
>>>> + r = -EINVAL;
>>>> + goto out_fd;
>>>> }
>>>> if (!amdgpu_ctx_priority_is_valid(args->in.priority)) {
>>>> WARN(1, "Invalid context priority %d\n", args->in.priority);
>>>> - return -EINVAL;
>>>> + r = -EINVAL;
>>>> + goto out_ctx;
>>>> }
>>>> switch (args->in.op) {
>>>> case AMDGPU_SCHED_OP_PROCESS_PRIORITY_OVERRIDE:
>>>> - r = amdgpu_sched_process_priority_override(adev,
>>>> - args->in.fd,
>>>> - args->in.priority);
>>>> + /* Retrieve all ctx handles and override priority */
>>>> + idr_for_each_entry(&fpriv->ctx_mgr.ctx_handles, ctx_ptr, id)
>>>> + amdgpu_ctx_priority_override(ctx_ptr, args->in.priority);
>>>> break;
>>>> case AMDGPU_SCHED_OP_CONTEXT_PRIORITY_OVERRIDE:
>>>> - r = amdgpu_sched_context_priority_override(adev,
>>>> - args->in.fd,
>>>> - args->in.ctx_id,
>>>> - args->in.priority);
>>>> + /* Override priority for a given context */
>>>> + amdgpu_ctx_priority_override(ctx, args->in.priority);
>>>> break;
>>>> default:
>>>> - /* Impossible.
>>>> - */
>>>> + DRM_ERROR("Invalid sched op specified: %d\n", args->in.op);
>>>> r = -EINVAL;
>>>> - break;
>>>> + goto out_ctx;
>>>> }
>>>> + r = amdgpu_ctx_get_entity(ctx, args->in.ip_type, 0,
>>>> args->in.ring,
>>>> + &entity);
>>>> + if (r)
>>>> + goto out_ctx;
>>>> +
>>>> + /* Fetch amdgpu_ring pointer */
>>>> + ring = to_amdgpu_ring(entity->rq->sched);
>>>> +
>>>> + /* Pass sched info to userspace */
>>>> + memset(args, 0, sizeof(*args));
>>>> + args->out.info.pipe = ring->pipe;
>>>> + args->out.info.queue = ring->queue;
>>>> +
>>>> +out_ctx:
>>>> + amdgpu_ctx_put(ctx);
>>>> +out_fd:
>>>> + fdput(f);
>>>> +
>>>> return r;
>>>> }
>>>> diff --git a/include/uapi/drm/amdgpu_drm.h
>>>> b/include/uapi/drm/amdgpu_drm.h
>>>> index 1d65c1fbc4ec..e93f1b6c4561 100644
>>>> --- a/include/uapi/drm/amdgpu_drm.h
>>>> +++ b/include/uapi/drm/amdgpu_drm.h
>>>> @@ -70,7 +70,7 @@ extern "C" {
>>>> #define DRM_IOCTL_AMDGPU_WAIT_FENCES DRM_IOWR(DRM_COMMAND_BASE +
>>>> DRM_AMDGPU_WAIT_FENCES, union drm_amdgpu_wait_fences)
>>>> #define DRM_IOCTL_AMDGPU_VM DRM_IOWR(DRM_COMMAND_BASE +
>>>> DRM_AMDGPU_VM, union drm_amdgpu_vm)
>>>> #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE
>>>> DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union
>>>> drm_amdgpu_fence_to_handle)
>>>> -#define DRM_IOCTL_AMDGPU_SCHED DRM_IOW(DRM_COMMAND_BASE +
>>>> DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>>> +#define DRM_IOCTL_AMDGPU_SCHED DRM_IOWR(DRM_COMMAND_BASE +
>>>> DRM_AMDGPU_SCHED, union drm_amdgpu_sched)
>>>> /**
>>>> * DOC: memory domains
>>>> @@ -308,6 +308,11 @@ union drm_amdgpu_vm {
>>>> #define AMDGPU_SCHED_OP_PROCESS_PRIORITY_OVERRIDE 1
>>>> #define AMDGPU_SCHED_OP_CONTEXT_PRIORITY_OVERRIDE 2
>>>> +struct drm_amdgpu_sched_info {
>>>> + __u32 pipe;
>>>> + __u32 queue;
>>>> +};
>>>> +
>>>> struct drm_amdgpu_sched_in {
>>>> /* AMDGPU_SCHED_OP_* */
>>>> __u32 op;
>>>> @@ -315,10 +320,17 @@ struct drm_amdgpu_sched_in {
>>>> /** AMDGPU_CTX_PRIORITY_* */
>>>> __s32 priority;
>>>> __u32 ctx_id;
>>>> + __u32 ip_type;
>>>> + __u32 ring;
>>>> +};
>>>> +
>>>> +struct drm_amdgpu_sched_out {
>>>> + struct drm_amdgpu_sched_info info;
>>>> };
>>>> union drm_amdgpu_sched {
>>>> struct drm_amdgpu_sched_in in;
>>>> + struct drm_amdgpu_sched_out out;
>>>> };
>>>> /*
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220502/6c61c1fc/attachment-0001.htm>
More information about the amd-gfx
mailing list