| Message ID | 1430480030-29136-2-git-send-email-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hans, Thank you for the patch. On Friday 01 May 2015 13:33:48 Hans Verkuil 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. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 19 +++++++++++++++++++ > include/uapi/linux/v4l2-subdev.h | 12 ++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c > b/drivers/media/v4l2-core/v4l2-subdev.c index 6359606..2ab1f7d 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -25,6 +25,7 @@ > #include <linux/types.h> > #include <linux/videodev2.h> > #include <linux/export.h> > +#include <linux/version.h> Nitpicking, I'd insert version.h between types.h and videodev2.h to keep entries alphabetically sorted (I know that export.h is misplaced, but that's the only one.). > #include <media/v4l2-ctrls.h> > #include <media/v4l2-device.h> > @@ -187,6 +188,24 @@ static long subdev_do_ioctl(struct file *file, unsigned > int cmd, void *arg) #endif > > switch (cmd) { > + case VIDIOC_SUBDEV_QUERYCAP: { > + struct v4l2_subdev_capability *cap = arg; > + > + cap->version = LINUX_VERSION_CODE; > + cap->device_caps = 0; > + cap->pads = 0; > + cap->entity_id = 0; > +#if defined(CONFIG_MEDIA_CONTROLLER) > + if (sd->entity.parent) { > + cap->device_caps = V4L2_SUBDEV_CAP_ENTITY; > + cap->pads = sd->entity.num_pads; > + cap->entity_id = sd->entity.id; > + } > +#endif > + memset(cap->reserved, 0, sizeof(cap->reserved)); > + break; > + } > + > case VIDIOC_QUERYCTRL: > return v4l2_queryctrl(vfh->ctrl_handler, arg); > > diff --git a/include/uapi/linux/v4l2-subdev.h > b/include/uapi/linux/v4l2-subdev.h index dbce2b5..e48b9fd 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -154,9 +154,21 @@ struct v4l2_subdev_selection { > __u32 reserved[8]; > }; > > +struct v4l2_subdev_capability { > + __u32 version; > + __u32 device_caps; > + __u32 pads; If we restrict pad number reporting to subdevs that are also entities, should we report the number of pads at all here ? Applications could find it out through the MC API using the entity ID. I don't have a too strong opinion on this for now, but we should consider whether reporting the same information through two different means wouldn't cause issues. > + __u32 entity_id; > + __u32 reserved[48]; > +}; > + > +/* This v4l2_subdev is also a media entity and the entity_id field is valid > */ +#define V4L2_SUBDEV_CAP_ENTITY (1 << 0) > + > /* 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)
On 05/04/2015 12:20 AM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Friday 01 May 2015 13:33:48 Hans Verkuil 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. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 19 +++++++++++++++++++ >> include/uapi/linux/v4l2-subdev.h | 12 ++++++++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c >> b/drivers/media/v4l2-core/v4l2-subdev.c index 6359606..2ab1f7d 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -25,6 +25,7 @@ >> #include <linux/types.h> >> #include <linux/videodev2.h> >> #include <linux/export.h> >> +#include <linux/version.h> > > Nitpicking, I'd insert version.h between types.h and videodev2.h to keep > entries alphabetically sorted (I know that export.h is misplaced, but that's > the only one.). > >> #include <media/v4l2-ctrls.h> >> #include <media/v4l2-device.h> >> @@ -187,6 +188,24 @@ static long subdev_do_ioctl(struct file *file, unsigned >> int cmd, void *arg) #endif >> >> switch (cmd) { >> + case VIDIOC_SUBDEV_QUERYCAP: { >> + struct v4l2_subdev_capability *cap = arg; >> + >> + cap->version = LINUX_VERSION_CODE; >> + cap->device_caps = 0; >> + cap->pads = 0; >> + cap->entity_id = 0; >> +#if defined(CONFIG_MEDIA_CONTROLLER) >> + if (sd->entity.parent) { >> + cap->device_caps = V4L2_SUBDEV_CAP_ENTITY; >> + cap->pads = sd->entity.num_pads; >> + cap->entity_id = sd->entity.id; >> + } >> +#endif >> + memset(cap->reserved, 0, sizeof(cap->reserved)); >> + break; >> + } >> + >> case VIDIOC_QUERYCTRL: >> return v4l2_queryctrl(vfh->ctrl_handler, arg); >> >> diff --git a/include/uapi/linux/v4l2-subdev.h >> b/include/uapi/linux/v4l2-subdev.h index dbce2b5..e48b9fd 100644 >> --- a/include/uapi/linux/v4l2-subdev.h >> +++ b/include/uapi/linux/v4l2-subdev.h >> @@ -154,9 +154,21 @@ struct v4l2_subdev_selection { >> __u32 reserved[8]; >> }; >> >> +struct v4l2_subdev_capability { >> + __u32 version; >> + __u32 device_caps; >> + __u32 pads; > > If we restrict pad number reporting to subdevs that are also entities, should > we report the number of pads at all here ? Applications could find it out > through the MC API using the entity ID. I don't have a too strong opinion on > this for now, but we should consider whether reporting the same information > through two different means wouldn't cause issues. My problem is that that would require e.g. v4l2-compliance to hunt for the media device when all it needs is the number of pads. I don't mind dropping this, but then we need a good way to find the associated media device (and sysfs is not a good way). The goal is to be able to write v4l2-compliance tests, so I need this info one way or another. Regards, Hans > >> + __u32 entity_id; >> + __u32 reserved[48]; >> +}; >> + >> +/* This v4l2_subdev is also a media entity and the entity_id field is valid >> */ +#define V4L2_SUBDEV_CAP_ENTITY (1 << 0) >> + >> /* 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thanks for the patch! On Fri, May 01, 2015 at 01:33:48PM +0200, Hans Verkuil 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. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 19 +++++++++++++++++++ > include/uapi/linux/v4l2-subdev.h | 12 ++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index 6359606..2ab1f7d 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -25,6 +25,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> > @@ -187,6 +188,24 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) > #endif > > switch (cmd) { > + case VIDIOC_SUBDEV_QUERYCAP: { > + struct v4l2_subdev_capability *cap = arg; > + > + cap->version = LINUX_VERSION_CODE; > + cap->device_caps = 0; > + cap->pads = 0; > + cap->entity_id = 0; > +#if defined(CONFIG_MEDIA_CONTROLLER) > + if (sd->entity.parent) { > + cap->device_caps = V4L2_SUBDEV_CAP_ENTITY; > + cap->pads = sd->entity.num_pads; > + cap->entity_id = sd->entity.id; > + } > +#endif > + memset(cap->reserved, 0, sizeof(cap->reserved)); > + break; > + } > + > case VIDIOC_QUERYCTRL: > return v4l2_queryctrl(vfh->ctrl_handler, arg); > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index dbce2b5..e48b9fd 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -154,9 +154,21 @@ struct v4l2_subdev_selection { > __u32 reserved[8]; > }; > > +struct v4l2_subdev_capability { > + __u32 version; > + __u32 device_caps; This is called capabilities in struct v4l2_capability. I'd follow the same pattern. > + __u32 pads; > + __u32 entity_id; What's the use case for the entity_id field btw.? Supposing that the user wouldn't be using the MC interface to obtain it, is the entity_id relevant in this context? Or is your intent first open the sub-device, and then find out more information on the entity? > + __u32 reserved[48]; Why 48? As memory is typically allocated in powers of two (or n^2 + (n-1)^2), how about aligning it accordingly? I don't think we lose anything by making this e.g. 60. Although 28 would probably suffice as well (or 29 with the pads field removed as discussed). Even that much sounds like a lot. > +}; > + > +/* This v4l2_subdev is also a media entity and the entity_id field is valid */ > +#define V4L2_SUBDEV_CAP_ENTITY (1 << 0) > + > /* 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 07/02/15 15:01, Sakari Ailus wrote: > Hi Hans, > > Thanks for the patch! > > On Fri, May 01, 2015 at 01:33:48PM +0200, Hans Verkuil 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. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> drivers/media/v4l2-core/v4l2-subdev.c | 19 +++++++++++++++++++ >> include/uapi/linux/v4l2-subdev.h | 12 ++++++++++++ >> 2 files changed, 31 insertions(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c >> index 6359606..2ab1f7d 100644 >> --- a/drivers/media/v4l2-core/v4l2-subdev.c >> +++ b/drivers/media/v4l2-core/v4l2-subdev.c >> @@ -25,6 +25,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> >> @@ -187,6 +188,24 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) >> #endif >> >> switch (cmd) { >> + case VIDIOC_SUBDEV_QUERYCAP: { >> + struct v4l2_subdev_capability *cap = arg; >> + >> + cap->version = LINUX_VERSION_CODE; >> + cap->device_caps = 0; >> + cap->pads = 0; >> + cap->entity_id = 0; >> +#if defined(CONFIG_MEDIA_CONTROLLER) >> + if (sd->entity.parent) { >> + cap->device_caps = V4L2_SUBDEV_CAP_ENTITY; >> + cap->pads = sd->entity.num_pads; >> + cap->entity_id = sd->entity.id; >> + } >> +#endif >> + memset(cap->reserved, 0, sizeof(cap->reserved)); >> + break; >> + } >> + >> case VIDIOC_QUERYCTRL: >> return v4l2_queryctrl(vfh->ctrl_handler, arg); >> >> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h >> index dbce2b5..e48b9fd 100644 >> --- a/include/uapi/linux/v4l2-subdev.h >> +++ b/include/uapi/linux/v4l2-subdev.h >> @@ -154,9 +154,21 @@ struct v4l2_subdev_selection { >> __u32 reserved[8]; >> }; >> >> +struct v4l2_subdev_capability { >> + __u32 version; >> + __u32 device_caps; > > This is called capabilities in struct v4l2_capability. I'd follow the same > pattern. No, it's called device_caps in struct v4l2_capability. The capabilities field in that same structure contains the capabilities of the device as a whole: so not just of the device node in question, but the union of the capabilities of all device nodes created by the driver. > >> + __u32 pads; >> + __u32 entity_id; > > What's the use case for the entity_id field btw.? Supposing that the user > wouldn't be using the MC interface to obtain it, is the entity_id relevant > in this context? Or is your intent first open the sub-device, and then find > out more information on the entity? Right, the latter. I want to be able to do v4l2-compliance -d /dev/v4l-subdev0 and it should be able to figure everything out from there. > >> + __u32 reserved[48]; > > Why 48? Can't remember :-) > > As memory is typically allocated in powers of two (or n^2 + (n-1)^2), how > about aligning it accordingly? I don't think we lose anything by making this > e.g. 60. Although 28 would probably suffice as well (or 29 with the pads > field removed as discussed). Even that much sounds like a lot. 29 would work fine as well. In general I am more generous with reserved fields these days since we've hit the limits of it too often. Regards, Hans > >> +}; >> + >> +/* This v4l2_subdev is also a media entity and the entity_id field is valid */ >> +#define V4L2_SUBDEV_CAP_ENTITY (1 << 0) >> + >> /* 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) > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index 6359606..2ab1f7d 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -25,6 +25,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> @@ -187,6 +188,24 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg) #endif switch (cmd) { + case VIDIOC_SUBDEV_QUERYCAP: { + struct v4l2_subdev_capability *cap = arg; + + cap->version = LINUX_VERSION_CODE; + cap->device_caps = 0; + cap->pads = 0; + cap->entity_id = 0; +#if defined(CONFIG_MEDIA_CONTROLLER) + if (sd->entity.parent) { + cap->device_caps = V4L2_SUBDEV_CAP_ENTITY; + cap->pads = sd->entity.num_pads; + cap->entity_id = sd->entity.id; + } +#endif + memset(cap->reserved, 0, sizeof(cap->reserved)); + break; + } + case VIDIOC_QUERYCTRL: return v4l2_queryctrl(vfh->ctrl_handler, arg); diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index dbce2b5..e48b9fd 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -154,9 +154,21 @@ struct v4l2_subdev_selection { __u32 reserved[8]; }; +struct v4l2_subdev_capability { + __u32 version; + __u32 device_caps; + __u32 pads; + __u32 entity_id; + __u32 reserved[48]; +}; + +/* This v4l2_subdev is also a media entity and the entity_id field is valid */ +#define V4L2_SUBDEV_CAP_ENTITY (1 << 0) + /* 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)