[libcamera-devel] [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl

[Hans Verkuil]

On 06/05/2020 20:34, Sakari Ailus wrote:
> Hi Hans, Jacopo,
> 
> On Wed, May 06, 2020 at 03:29:03PM +0200, Hans Verkuil wrote:
>> On 29/04/2020 10:18, Sakari Ailus wrote:
>>> Hi Jacopo,
>>>
>>> On Wed, Apr 29, 2020 at 10:09:49AM +0200, Jacopo Mondi wrote:
>>>> Hi Sakari,
>>>>
>>>> On Wed, Apr 29, 2020 at 12:28:58AM +0300, Sakari Ailus wrote:
>>>>> Hi Jacopo,
>>>>>
>>>>> On Tue, Apr 28, 2020 at 11:06:08PM +0200, Jacopo Mondi wrote:
>>>>>> From: Hans Verkuil <hans.verkuil at cisco.com>
>>>>>>
>>>>>> While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl
>>>>>> that apps can call to determine that it is indeed a V4L2 device, there
>>>>>> is currently no equivalent for v4l-subdev nodes. Adding this ioctl will
>>>>>> solve that, and it will allow utilities like v4l2-compliance to be used
>>>>>> with these devices as well.
>>>>>>
>>>>>> SUBDEV_QUERYCAP currently returns the version and subdev_caps of the
>>>>>> subdevice. Define as the initial set of subdev_caps the read-only or
>>>>>> read/write flags, to signal to userspace which set of IOCTLs are
>>>>>> available on the subdevice.
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil at cisco.com>
>>>>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>>>>> ---
>>>>>>  drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
>>>>>>  include/uapi/linux/v4l2-subdev.h      | 15 +++++++++++++++
>>>>>>  2 files changed, 27 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> index f3fe515b8ccb..b8c0071aa4d0 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>>  #include <linux/types.h>
>>>>>>  #include <linux/videodev2.h>
>>>>>>  #include <linux/export.h>
>>>>>> +#include <linux/version.h>
>>>>>>
>>>>>>  #include <media/v4l2-ctrls.h>
>>>>>>  #include <media/v4l2-device.h>
>>>>>> @@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>>>  	int rval;
>>>>>>
>>>>>>  	switch (cmd) {
>>>>>> +	case VIDIOC_SUBDEV_QUERYCAP: {
>>>>>> +		struct v4l2_subdev_capability *cap = arg;
>>>>>> +
>>>>>> +		memset(cap, 0, sizeof(*cap));
>>>>>> +		cap->version = LINUX_VERSION_CODE;
>>>>>> +		cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
>>>>>> +					      : V4L2_SUBDEV_CAP_RW_SUBDEV;
>>>>>> +
>>>>>> +		return 0;
>>>>>> +	}
>>>>>> +
>>>>>>  	case VIDIOC_QUERYCTRL:
>>>>>>  		/*
>>>>>>  		 * TODO: this really should be folded into v4l2_queryctrl (this
>>>>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
>>>>>> index 03970ce30741..89dc8f2ba6b3 100644
>>>>>> --- a/include/uapi/linux/v4l2-subdev.h
>>>>>> +++ b/include/uapi/linux/v4l2-subdev.h
>>>>>> @@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
>>>>>>  	__u32 reserved[8];
>>>>>>  };
>>>>>>
>>>>>> +/**
>>>>>> + * struct v4l2_subdev_capability - subdev capabilities
>>>>>> + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
>>>>>> + */
>>>>>> +struct v4l2_subdev_capability {
>>>>>> +	__u32 version;
>>>>>> +	__u32 subdev_caps;
>>>>>
>>>>> How do you intend to address additional fields being added to the struct in
>>>>> the future? Something else than what's been done in V4L2 traditionally?
>>>>>
>>>>
>>>> I'm not sure I get what you mean here, so I assume I don't know what
>>>> "has been done in V4L2 traditionally" and why what I have here goes
>>>> against it...
>>>
>>> I can't help noticing you have no reserved fields in your IOCTL argument
>>> struct. That has generally been the way V4L2 IOCTLs have been extended when
>>> there's been a need to.
>>>
>>> Media controller adopted a different approach to that recently, based on
>>> the argument size. We've discussed doing that on V4L2 but I don't think
>>> a decision has been made.
>>>
>>
>> While I agree that using the argument size to do 'versioning' of the API
>> is a better solution, the fact is that historically we always used a 'reserved'
>> field. And I think we need to do that here as well. It's consistent with
>> the existing subdev ioctls, so I would be in favor of adding a 'u32 reserved[6];'
>> field.
> 
> Agreed. Could be even 14, in practice it matters little performance-wise.

True. Let's make it 14.

Regards,

	Hans

> 
>>
>> If there are such strong feelings against it that it wouldn't be merged, then
>> we can always just leave it as-is. It's not worth blocking this patch just
>> because of that.
> 
> I'm not (strongly) pushing either here; just that we need to make a
> decision. I'm in favour of the reserved field for the same reason. I was
> just wondering whether it was intentional. :-)
> 
>>
>> BTW, one thing that should be changed is the name 'subdev_caps': just name it
>> 'capabilities'. It's a field in a struct v4l2_subdev_capability, so it is
>> already obvious that this is subdev specific.
>