[PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)
Christian König
deathsimple at vodafone.de
Thu Dec 15 14:12:24 UTC 2016
Am 15.12.2016 um 13:33 schrieb Nath, Arindam:
>> -----Original Message-----
>> From: Grazvydas Ignotas [mailto:notasas at gmail.com]
>> Sent: Thursday, December 15, 2016 5:52 PM
>> To: Nath, Arindam
>> Cc: Emil Velikov; David Airlie; Deucher, Alexander; ML dri-devel; amd-gfx
>> mailing list; Koenig, Christian
>> Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>> handles (v3)
>>
>> 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".
> We need to return an error no. consistent with what ioctl() expects to return. This will enable userspace to take corrective action (or no action at all) depending on the error code.
Well, we could return INT_MAX for the maximum numbers of handles to note
that there is not limit, but I also prefer returning an error code to
note that this won't work as expected.
Regarding which error code to return I think that Emil has the right
idea here.
Returning -EINVAL usually means that userspace provided an invalid
value, but in this case it doesn't matter which value the UMD provide
all of them would be invalid because starting with Polaris the
hardware/firmware simply doesn't work this way any more.
So using -ENODEV or maybe -ENODATA indeed sound like the right think to
do here.
Regards,
Christian.
>
> Thanks,
> Arindam
>> Gražvydas
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list