[PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)

[Nath, Arindam]

>-----Original Message-----
>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>Sent: Thursday, December 15, 2016 5:01 PM
>To: Nath, Arindam
>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>Koenig, Christian
>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>handles (v3)
>
>On 15 December 2016 at 07:30, Nath, Arindam <Arindam.Nath at amd.com>
>wrote:
>>>-----Original Message-----
>>>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>>>Sent: Wednesday, December 14, 2016 9:26 PM
>>>To: Nath, Arindam
>>>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>>>Koenig, Christian
>>>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>>>handles (v3)
>>>
>>>On 12 December 2016 at 18:49,  <arindam.nath at amd.com> wrote:
>>>> From: Arindam Nath <arindam.nath at amd.com>
>>>>
>>>> Change History
>>>> --------------
>>>>
>>>> v3: changes suggested by Christian
>>>> - Add a check for UVD IP block using AMDGPU_HW_IP_UVD
>>>>   query type.
>>>> - Add a check for asic_type to be less than
>>>>   CHIP_POLARIS10 since starting Polaris, we support
>>>>   unlimited UVD instances.
>>>> - Add kerneldoc style comment for
>>>>   amdgpu_uvd_used_handles().
>>>>
>>>> v2: as suggested by Christian
>>>> - Add a new query AMDGPU_INFO_NUM_HANDLES
>>>> - Create a helper function to return the number
>>>>   of currently used UVD handles.
>>>> - Modify the logic to count the number of used
>>>>   UVD handles since handles can be freed in
>>>>   non-linear fashion.
>>>>
>>>> v1:
>>>> - User might want to query the maximum number of UVD
>>>>   instances supported by firmware. In addition to that,
>>>>   if there are multiple applications using UVD handles
>>>>   at the same time, he might also want to query the
>>>>   currently used number of handles.
>>>>
>>>>   For this we add two variables max_handles and
>>>>   used_handles inside drm_amdgpu_info_hw_ip. So now
>>>>   an application (or libdrm) can use AMDGPU_INFO IOCTL
>>>>   with AMDGPU_INFO_HW_IP_INFO query type to get these
>>>>   values.
>>>>
>>>> Signed-off-by: Arindam Nath <arindam.nath at amd.com>
>>>> Reviewed-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21
>>>+++++++++++++++++++++
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 25
>>>+++++++++++++++++++++++++
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.h |  1 +
>>>>  include/uapi/drm/amdgpu_drm.h           |  9 +++++++++
>>>>  4 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> index 174eb59..3273d8c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> @@ -570,6 +570,27 @@ static int amdgpu_info_ioctl(struct drm_device
>>>*dev, void *data, struct drm_file
>>>>                         return -EINVAL;
>>>>                 }
>>>>         }
>>>> +       case AMDGPU_INFO_NUM_HANDLES: {
>>>> +               struct drm_amdgpu_info_num_handles handle;
>>>> +
>>>> +               switch (info->query_hw_ip.type) {
>>>> +               case AMDGPU_HW_IP_UVD:
>>>> +                       /* Starting Polaris, we support unlimited UVD handles */
>>>> +                       if (adev->asic_type < CHIP_POLARIS10) {
>>>> +                               handle.uvd_max_handles = adev->uvd.max_handles;
>>>> +                               handle.uvd_used_handles =
>>>amdgpu_uvd_used_handles(adev);
>>>> +
>>>> +                               return copy_to_user(out, &handle,
>>>> +                                       min((size_t)size, sizeof(handle))) ? -EFAULT : 0;
>>>> +                       } else {
>>>> +                               return -EINVAL;
>>>Using EINVAL doesn't seem right here. As per man 3 ioctl
>>>
>>>      EINVAL The request or arg argument is not valid for this device.
>>>
>>>A bit further down you can see the one you want.
>>>
>>>      ENODEV The fildes argument refers to a valid STREAMS device, but
>>>the corresponding device driver does not support the ioctl() function.
>>
>> Emil, ENODEV would mean the driver does not support ioctl() itself. But in
>our case ioctl() is supported.
>>
>> Since we extract the query type from arg passed to ioctl(), and it is this
>query AMDGPU_INFO_NUM_HANDLES which is not supported by driver (for
>> Polaris), doesn’t returning EINVAL make more sense here?
>>
>Unless I'm reading the code incorrectly - CHIP_POLARIS10 and older do
>not have support the query. Thus ENODEV is the one you want to use.
>Furthermore EINVAL is (mostly) used to indicate incorrect input
>(failed input validation) which userspace uses to check if kernel is
>too old/does not support X.

Actually, the code says only asics older than CHIP_POLARIS10 support the query, beyond it doesn’t.
>
>If in doubt I recommend checking how other drivers are handling
>things. From memory at least i915 uses the above.

I will check how other drivers handle this.

Thanks,
Arindam
>
>Thanks
>Emil