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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@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@cisco.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@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.

-- 
Regards,

Sakari Ailus