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

[Grazvydas Ignotas]

On Thu, Dec 15, 2016 at 1:47 PM, Nath, Arindam <Arindam.Nath at amd.com> wrote:
>>-----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.

Wouldn't it be cleaner to return INT_MAX or something like that?
Right now the interface is inconsistent, it's failing the same way as
if userspace passed something invalid, while (IMHO) it should just say
"unlimited".

Gražvydas