| Message ID | 20200428210609.6793-6-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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? > +}; > + > +/* The v4l2 sub-device video device node is registered in read-only mode. */ > +#define V4L2_SUBDEV_CAP_RO_SUBDEV BIT(0) > +/* The v4l2 sub-device video device node is registered in read/write mode. */ > +#define V4L2_SUBDEV_CAP_RW_SUBDEV BIT(1) > + > /* Backwards compatibility define --- to be removed */ > #define v4l2_subdev_edid v4l2_edid > > +#define VIDIOC_SUBDEV_QUERYCAP _IOR('V', 0, struct v4l2_subdev_capability) > #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format) > #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format) > #define VIDIOC_SUBDEV_G_FRAME_INTERVAL _IOWR('V', 21, struct v4l2_subdev_frame_interval)
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...
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.
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@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. > 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. 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. 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. Regards, Hans
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@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. > > > > 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. > > 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.
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@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. >>> >> >> 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. >
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; +}; + +/* The v4l2 sub-device video device node is registered in read-only mode. */ +#define V4L2_SUBDEV_CAP_RO_SUBDEV BIT(0) +/* The v4l2 sub-device video device node is registered in read/write mode. */ +#define V4L2_SUBDEV_CAP_RW_SUBDEV BIT(1) + /* Backwards compatibility define --- to be removed */ #define v4l2_subdev_edid v4l2_edid +#define VIDIOC_SUBDEV_QUERYCAP _IOR('V', 0, struct v4l2_subdev_capability) #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format) #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format) #define VIDIOC_SUBDEV_G_FRAME_INTERVAL _IOWR('V', 21, struct v4l2_subdev_frame_interval)