On Thu, Dec 15, 2016 at 1:47 PM, Nath, Arindam <Arindam.Nath@xxxxxxx> wrote: >>-----Original Message----- >>From: Emil Velikov [mailto:emil.l.velikov@xxxxxxxxx] >>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@xxxxxxx> >>wrote: >>>>-----Original Message----- >>>>From: Emil Velikov [mailto:emil.l.velikov@xxxxxxxxx] >>>>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@xxxxxxx> wrote: >>>>> From: Arindam Nath <arindam.nath@xxxxxxx> >>>>> >>>>> 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@xxxxxxx> >>>>> Reviewed-by: Christian König <christian.koenig@xxxxxxx> >>>>> --- >>>>> 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
