Re: [RFC PATCH 1/3] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
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
>>
>> 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>>
>> #include
>> #include
>> @@ -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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/3] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
Hi Hans,
Thanks for the patch!
On Fri, May 01, 2015 at 01:33:48PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil
>
> 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
> ---
> 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
> #include
> #include
> +#include
>
> #include
> #include
> @@ -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)
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/3] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
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
>>
>> 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
>> ---
>> 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
>> #include
>> #include
>> +#include
>
> 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
>> #include
>> @@ -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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 1/3] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
Hi Hans,
Thank you for the patch.
On Friday 01 May 2015 13:33:48 Hans Verkuil wrote:
> From: Hans Verkuil
>
> 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
> ---
> 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
> #include
> #include
> +#include
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
> #include
> @@ -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)
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 1/3] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
From: Hans Verkuil
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
---
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
#include
#include
+#include
#include
#include
@@ -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)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
