[RFCv2 PATCH 0/2] add VIDIOC_SUBDEV_QUERYCAP ioctl

[RFCv2 PATCH 0/2] add VIDIOC_SUBDEV_QUERYCAP ioctl

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

I tried to get this in back in 2015, but that effort stalled.

Trying again, since I really need this in order to add proper v4l-subdev
support to v4l2-ctl and v4l2-compliance. There currently is no way of
unique identifying that the device really is a v4l-subdev device other
than the device name (which can be changed by udev).

So this patch series adds a VIDIOC_SUBDEV_QUERYCAP ioctl that is in
the core so it's guaranteed to be there.

If the subdev is part of an MC then it also gives the corresponding
entity ID of the subdev and the major/minor numbers of the MC device
so v4l2-compliance can relate the subdev device directly to the right
MC device. The reserved array has room enough for more strings should
we need them later, although I think what we have here is sufficient.

Regards,

	Hans

Changes since v1:
- Add name field. Without that it is hard to figure out which subdev
  it is since the entity ID is not very human readable.

Hans Verkuil (2):
  v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
  v4l: document VIDIOC_SUBDEV_QUERYCAP

 Documentation/media/uapi/v4l/user-func.rst         |   1 +
 .../media/uapi/v4l/vidioc-subdev-querycap.rst      | 121 +++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c              |  27 +++++
 include/uapi/linux/v4l2-subdev.h                   |  31 ++++++
 4 files changed, 180 insertions(+)
 create mode 100644 Documentation/media/uapi/v4l/vidioc-subdev-querycap.rst

-- 
2.13.1

[RFCv2 PATCH 1/2] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl

[Hans Verkuil]

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 device_caps of the
subdevice. If the subdev is used as part of a media controller, then
it also returns the entity ID and the major and minor numbers of the
media controller device node.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 27 +++++++++++++++++++++++++++
 include/uapi/linux/v4l2-subdev.h      | 31 +++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 43fefa73e0a3..56cc255171d7 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -20,8 +20,10 @@
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/kdev_t.h>
 #include <linux/videodev2.h>
 #include <linux/export.h>
+#include <linux/version.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -186,6 +188,31 @@ 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;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+		struct media_device *mdev = sd->entity.graph_obj.mdev;
+		struct media_devnode *devnode = mdev ? mdev->devnode : NULL;
+#endif
+
+		cap->version = LINUX_VERSION_CODE;
+		cap->device_caps = 0;
+		strlcpy(cap->name, sd->name, sizeof(cap->name));
+		cap->entity_id = 0;
+		cap->media_node_major = 0;
+		cap->media_node_minor = 0;
+#if defined(CONFIG_MEDIA_CONTROLLER)
+		if (devnode) {
+			cap->device_caps = V4L2_SUBDEV_CAP_ENTITY;
+			cap->entity_id = sd->entity.graph_obj.id;
+			cap->media_node_major = MAJOR(devnode->cdev.dev);
+			cap->media_node_minor = MINOR(devnode->cdev.dev);
+		}
+#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 dbce2b554e02..3dd1c412bf0d 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -154,9 +154,40 @@ struct v4l2_subdev_selection {
 	__u32 reserved[8];
 };
 
+/**
+ * struct v4l2_subdev_capability - subdev capabilities
+ * @version: the kernel version
+ * @device_caps: the subdev capabilities
+ * @name: the subdev name
+ * @entity_id: the entity ID as assigned by the media controller. Only
+ * valid if V4L2_SUBDEV_CAP_ENTITY is set
+ * @media_node_major: the major number of the media controller device node.
+ * Only valid if V4L2_SUBDEV_CAP_ENTITY is set
+ * @media_node_minor: the minor number of the media controller device node.
+ * Only valid if V4L2_SUBDEV_CAP_ENTITY is set
+ * @reserved: for future use, set to zero for now
+ */
+struct v4l2_subdev_capability {
+	__u32 version;
+	__u32 device_caps;
+	char  name[32];
+	__u32 entity_id;
+	/* Corresponding media controller device node specifications */
+	__u32 media_node_major;
+	__u32 media_node_minor;
+	__u32 reserved[19];
+};
+
+/*
+ * This v4l2_subdev is also a media entity and the entity_id, media_node_major
+ * and media_node_minor fields are 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.13.1

[RFCv2 PATCH 2/2] v4l: document VIDIOC_SUBDEV_QUERYCAP

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add documentation for the new VIDIOC_SUBDEV_QUERYCAP ioctl.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/media/uapi/v4l/user-func.rst         |   1 +
 .../media/uapi/v4l/vidioc-subdev-querycap.rst      | 121 +++++++++++++++++++++
 2 files changed, 122 insertions(+)
 create mode 100644 Documentation/media/uapi/v4l/vidioc-subdev-querycap.rst

diff --git a/Documentation/media/uapi/v4l/user-func.rst b/Documentation/media/uapi/v4l/user-func.rst
index 3e0413b83a33..eda5a01b5228 100644
--- a/Documentation/media/uapi/v4l/user-func.rst
+++ b/Documentation/media/uapi/v4l/user-func.rst
@@ -71,6 +71,7 @@ Function Reference
     vidioc-subdev-g-fmt
     vidioc-subdev-g-frame-interval
     vidioc-subdev-g-selection
+    vidioc-subdev-querycap
     vidioc-subscribe-event
     func-mmap
     func-munmap
diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-querycap.rst b/Documentation/media/uapi/v4l/vidioc-subdev-querycap.rst
new file mode 100644
index 000000000000..6143d201b11e
--- /dev/null
+++ b/Documentation/media/uapi/v4l/vidioc-subdev-querycap.rst
@@ -0,0 +1,121 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _VIDIOC_SUBDEV_QUERYCAP:
+
+****************************
+ioctl VIDIOC_SUBDEV_QUERYCAP
+****************************
+
+Name
+====
+
+VIDIOC_SUBDEV_QUERYCAP - Query sub-device capabilities
+
+
+Synopsis
+========
+
+.. c:function:: int ioctl( int fd, VIDIOC_SUBDEV_QUERYCAP, struct v4l2_subdev_capability *argp )
+    :name: VIDIOC_SUBDEV_QUERYCAP
+
+
+Arguments
+=========
+
+``fd``
+    File descriptor returned by :ref:`open() <func-open>`.
+
+``argp``
+
+
+Description
+===========
+
+All V4L2 sub-devices support the
+``VIDIOC_SUBDEV_QUERYCAP`` ioctl. It is used to identify
+kernel devices compatible with this specification and to obtain
+information about driver and hardware capabilities. The ioctl takes a
+pointer to a struct :c:type:`v4l2_subdev_capability` which is filled by the
+driver. When the driver is not compatible with this specification the ioctl
+returns ``ENOTTY`` error code.
+
+.. tabularcolumns:: |p{1.5cm}|p{2.5cm}|p{13cm}|
+
+.. c:type:: v4l2_subdev_capability
+
+.. flat-table:: struct v4l2_subdev_capability
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       3 4 20
+
+    * - __u32
+      - ``version``
+      - Version number of the driver.
+
+	The version reported is provided by the
+	V4L2 subsystem following the kernel numbering scheme. However, it
+	may not always return the same version as the kernel if, for
+	example, a stable or distribution-modified kernel uses the V4L2
+	stack from a newer kernel.
+
+	The version number is formatted using the ``KERNEL_VERSION()``
+	macro:
+    * - :cspan:`2`
+
+	``#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))``
+
+	``__u32 version = KERNEL_VERSION(0, 8, 1);``
+
+	``printf ("Version: %u.%u.%u\\n",``
+
+	``(version >> 16) & 0xFF, (version >> 8) & 0xFF, version & 0xFF);``
+    * - __u32
+      - ``device_caps``
+      - Sub-device capabilities of the opened device, see
+	:ref:`subdevice-capabilities`.
+    * - char
+      - ``name``\ [32]
+      - NUL-terminated name of the sub-device.
+    * - __u32
+      - ``entity_id``
+      - The media controller entity ID of the sub-device. This is only valid if
+        the ``V4L2_SUBDEV_CAP_ENTITY`` capability is set, it is 0 otherwise.
+    * - __u32
+      - ``media_node_major``
+      - The major number of the media controller device node corresponding sub-device.
+        This is only valid if the ``V4L2_SUBDEV_CAP_ENTITY`` capability is set, it is
+	0 otherwise.
+    * - __u32
+      - ``media_node_minor``
+      - The minor number of the media controller device node corresponding sub-device.
+        This is only valid if the ``V4L2_SUBDEV_CAP_ENTITY`` capability is set, it is
+	0 otherwise.
+    * - __u32
+      - ``reserved``\ [19]
+      - Reserved for future extensions. Applications and drivers must set
+	the array to zero.
+
+.. tabularcolumns:: |p{6cm}|p{2.2cm}|p{8.8cm}|
+
+.. _subdevice-capabilities:
+
+.. cssclass:: longtable
+
+.. flat-table:: Sub-Device Capabilities Flags
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * - ``V4L2_SUBDEV_CAP_ENTITY``
+      - 0x00000001
+      - The sub-device is a media controller entity and the ``entity_id``,
+        ``media_node_major`` and ``media_node_minor`` fields of
+        struct :c:type:`v4l2_subdev_capability` are valid. These fields
+	are 0 if this capability is not set.
+
+Return Value
+============
+
+On success 0 is returned, on error -1 and the ``errno`` variable is set
+appropriately. The generic error codes are described at the
+:ref:`Generic Error Codes <gen-errors>` chapter.
-- 
2.13.1

Re: [RFCv2 PATCH 0/2] add VIDIOC_SUBDEV_QUERYCAP ioctl

[Laurent Pinchart]

Hi Hans,

Thank you for the patches.

On Friday 28 Jul 2017 13:05:27 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> I tried to get this in back in 2015, but that effort stalled.
> 
> Trying again, since I really need this in order to add proper v4l-subdev
> support to v4l2-ctl and v4l2-compliance. There currently is no way of
> unique identifying that the device really is a v4l-subdev device other
> than the device name (which can be changed by udev).
> 
> So this patch series adds a VIDIOC_SUBDEV_QUERYCAP ioctl that is in
> the core so it's guaranteed to be there.
> 
> If the subdev is part of an MC then it also gives the corresponding
> entity ID of the subdev and the major/minor numbers of the MC device
> so v4l2-compliance can relate the subdev device directly to the right
> MC device. The reserved array has room enough for more strings should
> we need them later, although I think what we have here is sufficient.

I still think this is not correct for two reasons.

First of all, the new querycap ioctl uses the same ioctl number as 
VIDIOC_QUERYCAP. The full 32-bit number is different as the structures used 
for the two ioctls currently have different sizes, but that's not guaranteed 
going forward when we'll extend the structures used by the two ioctls with new 
fields.

To solve this, if you really want to identify the type of device node at 
runtime, we should have a single ioctl supported by the two device nodes. 
Given that we"re running out of capabilities bits for VIDIOC_QUERYCAP, this 
could be a good occasion to introduce a new ioctl to query capabilities.

The second reason is that I don't think we should report the media device node 
associated with the subdev. Userspace really needs to become MC-centric, 
starting with the MC device, and going to the video nodes and subdev nodes. 
The other way around just doesn't make sense to me.

For MC-enabled devices, specifying subdev or video nodes by /dev node name is 
painful. When you have multiple video devices on the system, or even when 
you're modifying initialization order in a driver, the devnode names will not 
be stable across boots. I used to type them out manually in the very beginning 
and very quickly switched to retrieving them from the subdev entity name in my 
test scripts.

What I'd like the compliance tools to do is to test all video nodes and subdev 
nodes for a given MC device, with an option to restrict the tests to a subset 
of the video devices and subdevs specified by media entity name. We obviously 
need to keep support for addressing video nodes manually as not all devices 
are MC-enabled, but for subdev we don't have to care about such a backward 
compatibility issue as there's currently no compliance tool.

On a side note, I believe subdev nodes should depend on MC. It has been a 
historical mistake not to do so, and as far as I can see only three drivers 
register subdev nodes without registering a media device. They should be fixed 
if they want to benefit from the compliance tool.

> Changes since v1:
> - Add name field. Without that it is hard to figure out which subdev
>   it is since the entity ID is not very human readable.
> 
> Hans Verkuil (2):
>   v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
>   v4l: document VIDIOC_SUBDEV_QUERYCAP
> 
>  Documentation/media/uapi/v4l/user-func.rst         |   1 +
>  .../media/uapi/v4l/vidioc-subdev-querycap.rst      | 121 +++++++++++++++++
>  drivers/media/v4l2-core/v4l2-subdev.c              |  27 +++++
>  include/uapi/linux/v4l2-subdev.h                   |  31 ++++++
>  4 files changed, 180 insertions(+)
>  create mode 100644 Documentation/media/uapi/v4l/vidioc-subdev-querycap.rst

-- 
Regards,

Laurent Pinchart

Re: [RFCv2 PATCH 0/2] add VIDIOC_SUBDEV_QUERYCAP ioctl

[Hans Verkuil]

On 07/28/2017 03:25 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patches.
> 
> On Friday 28 Jul 2017 13:05:27 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> I tried to get this in back in 2015, but that effort stalled.
>>
>> Trying again, since I really need this in order to add proper v4l-subdev
>> support to v4l2-ctl and v4l2-compliance. There currently is no way of
>> unique identifying that the device really is a v4l-subdev device other
>> than the device name (which can be changed by udev).
>>
>> So this patch series adds a VIDIOC_SUBDEV_QUERYCAP ioctl that is in
>> the core so it's guaranteed to be there.
>>
>> If the subdev is part of an MC then it also gives the corresponding
>> entity ID of the subdev and the major/minor numbers of the MC device
>> so v4l2-compliance can relate the subdev device directly to the right
>> MC device. The reserved array has room enough for more strings should
>> we need them later, although I think what we have here is sufficient.
> 
> I still think this is not correct for two reasons.
> 
> First of all, the new querycap ioctl uses the same ioctl number as 
> VIDIOC_QUERYCAP. The full 32-bit number is different as the structures used 
> for the two ioctls currently have different sizes, but that's not guaranteed 
> going forward when we'll extend the structures used by the two ioctls with new 
> fields.

I think it is extraordinarily unlikely that these two will ever become identical.
And anyway, we control that ourselves.

> To solve this, if you really want to identify the type of device node at 
> runtime, we should have a single ioctl supported by the two device nodes. 
> Given that we"re running out of capabilities bits for VIDIOC_QUERYCAP, this 
> could be a good occasion to introduce a new ioctl to query capabilities.

This makes more sense :-)

> The second reason is that I don't think we should report the media device node 
> associated with the subdev. Userspace really needs to become MC-centric, 
> starting with the MC device, and going to the video nodes and subdev nodes. 
> The other way around just doesn't make sense to me.

It's not for 'regular' applications. It's to easily find out to which media
device a /dev/v4l-subdevX belongs. Primarily for applications like v4l2-compliance.

We have the information, and right now there is no way to take a v4l-subdevX device
and determine of which media device it is part other than scanning the topologies
of all media devices. That's nuts. This is cheap and makes life for a certain
class of applications much easier. Creating good compliance tests is critical
and this is a small but important contribution to that.

> For MC-enabled devices, specifying subdev or video nodes by /dev node name is 
> painful. When you have multiple video devices on the system, or even when 
> you're modifying initialization order in a driver, the devnode names will not 
> be stable across boots. I used to type them out manually in the very beginning 
> and very quickly switched to retrieving them from the subdev entity name in my 
> test scripts.
> 
> What I'd like the compliance tools to do is to test all video nodes and subdev 
> nodes for a given MC device, with an option to restrict the tests to a subset 
> of the video devices and subdevs specified by media entity name. We obviously 
> need to keep support for addressing video nodes manually as not all devices 
> are MC-enabled, but for subdev we don't have to care about such a backward 
> compatibility issue as there's currently no compliance tool.

I want two things:

1) v4l2-compliance to be able to test a v4l-subdevX, just in isolation. And to
   be able to find the corresponding media device and make sure that what the
   v4l-subdev does is compatible with the entity/link information from the MC.

2) make a media-compliance to look at the media topology as a whole.

Having the major/minor numbers are specifically for 1.

Actually, I really want to have the major/minor numbers of the media device for
/dev/videoX as well, but entity ID +  major + minor number would use up the
available space in struct v4l2_capability, so your suggestion of making a new
VIDIOC_EXT_QUERYCAP has merit.

> On a side note, I believe subdev nodes should depend on MC. It has been a 
> historical mistake not to do so, and as far as I can see only three drivers 
> register subdev nodes without registering a media device. They should be fixed 
> if they want to benefit from the compliance tool.

Which ones?

I'm not opposed to that. It would simplify matters quite a bit.

But I very, very strongly believe that a VIDIOC_EXT_QUERYCAP should return the
corresponding entity ID and /dev/mediaX major and minor numbers. It's very useful
information for a certain class of applications.

Heck, I want to do 'v4l2-ctl -d /dev/video0 -D' or 'v4l2-ctl -d /dev/v4l-subdev0'
and see not only the device capabilities, but also the corresponding entity
information. Without having to scan through all /dev/media devices or requiring the
user to pass the /dev/mediaX device separately.

This information is very cheap and I see no reason whatsoever not to implement this.
It also feels much more symmetrical if I have what is effectively a backlink to
the media device to which the subdev belongs.

And yes, normally applications will never need this since they use the media device
and never reference a /dev/v4l-subdevX by name. But for v4l2-ctl and v4l2-compliance
it is very useful indeed.

Regards,

	Hans

> 
>> Changes since v1:
>> - Add name field. Without that it is hard to figure out which subdev
>>   it is since the entity ID is not very human readable.
>>
>> Hans Verkuil (2):
>>   v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
>>   v4l: document VIDIOC_SUBDEV_QUERYCAP
>>
>>  Documentation/media/uapi/v4l/user-func.rst         |   1 +
>>  .../media/uapi/v4l/vidioc-subdev-querycap.rst      | 121 +++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-subdev.c              |  27 +++++
>>  include/uapi/linux/v4l2-subdev.h                   |  31 ++++++
>>  4 files changed, 180 insertions(+)
>>  create mode 100644 Documentation/media/uapi/v4l/vidioc-subdev-querycap.rst
> 

Re: [RFCv2 PATCH 0/2] add VIDIOC_SUBDEV_QUERYCAP ioctl

[Hans Verkuil]

On 07/28/2017 04:04 PM, Hans Verkuil wrote:
> On 07/28/2017 03:25 PM, Laurent Pinchart wrote:
>> To solve this, if you really want to identify the type of device node at 
>> runtime, we should have a single ioctl supported by the two device nodes. 
>> Given that we"re running out of capabilities bits for VIDIOC_QUERYCAP, this 
>> could be a good occasion to introduce a new ioctl to query capabilities.
> 
> This makes more sense :-)

Here is a quick proposal:

struct v4l2_ext_capability {
        char    driver[16];
        char    name[32];
        char    bus_info[32];
        __u64   device_caps;
        __u32   version;
        __u32   entity_id;
        /* Corresponding media controller device node specifications */
        __u32   media_node_major;
        __u32   media_node_minor;
        __u32   reserved[16];
};

#define V4L2_CAP_SUBDEV                 0x00000008  /* This is a v4l-subdev device */
#define V4L2_CAP_ENTITY                 0x08000000  /* MC entity */

#define VIDIOC_EXT_QUERYCAP             _IOR('V', 104, struct v4l2_ext_capability)

We keep the existing caps, but double the size of the device_caps field.

Add a CAP_SUBDEV to indicate that it is a subdev, and a CAP_ENTITY to indicate
that it is part of the media controller.

I dropped the old 'capabilities' field. In V4L2 that is meant to give the sum
of all the capabilities of all the video/vbi/radio/swradio device nodes, but
it never worked and is inconsistently implemented.

It's really historical so I decided to drop it. I also replaced __u8 by char
for the string fields (__u8 was very, very annoying!).

No driver changes needed for this, it can all be handled in the core.

Regards,

	Hans

Re: [RFCv2 PATCH 0/2] add VIDIOC_SUBDEV_QUERYCAP ioctl

[Laurent Pinchart]

Hi Hans,

On Friday 28 Jul 2017 16:04:48 Hans Verkuil wrote:
> On 07/28/2017 03:25 PM, Laurent Pinchart wrote:
> > On Friday 28 Jul 2017 13:05:27 Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >> 
> >> I tried to get this in back in 2015, but that effort stalled.
> >> 
> >> Trying again, since I really need this in order to add proper v4l-subdev
> >> support to v4l2-ctl and v4l2-compliance. There currently is no way of
> >> unique identifying that the device really is a v4l-subdev device other
> >> than the device name (which can be changed by udev).
> >> 
> >> So this patch series adds a VIDIOC_SUBDEV_QUERYCAP ioctl that is in
> >> the core so it's guaranteed to be there.
> >> 
> >> If the subdev is part of an MC then it also gives the corresponding
> >> entity ID of the subdev and the major/minor numbers of the MC device
> >> so v4l2-compliance can relate the subdev device directly to the right
> >> MC device. The reserved array has room enough for more strings should
> >> we need them later, although I think what we have here is sufficient.
> > 
> > I still think this is not correct for two reasons.
> > 
> > First of all, the new querycap ioctl uses the same ioctl number as
> > VIDIOC_QUERYCAP. The full 32-bit number is different as the structures
> > used for the two ioctls currently have different sizes, but that's not
> > guaranteed going forward when we'll extend the structures used by the two
> > ioctls with new fields.
> 
> I think it is extraordinarily unlikely that these two will ever become
> identical. And anyway, we control that ourselves.
> 
> > To solve this, if you really want to identify the type of device node at
> > runtime, we should have a single ioctl supported by the two device nodes.
> > Given that we"re running out of capabilities bits for VIDIOC_QUERYCAP,
> > this could be a good occasion to introduce a new ioctl to query
> > capabilities.
> 
> This makes more sense :-)
> 
> > The second reason is that I don't think we should report the media device
> > node associated with the subdev. Userspace really needs to become
> > MC-centric, starting with the MC device, and going to the video nodes and
> > subdev nodes. The other way around just doesn't make sense to me.
> 
> It's not for 'regular' applications. It's to easily find out to which media
> device a /dev/v4l-subdevX belongs. Primarily for applications like
> v4l2-compliance.
> 
> We have the information, and right now there is no way to take a v4l-subdevX
> device and determine of which media device it is part other than scanning
> the topologies of all media devices. That's nuts. This is cheap and makes
> life for a certain class of applications much easier. Creating good
> compliance tests is critical and this is a small but important contribution
> to that.

I fully agree with the need for compliance tools, but I believe they should go 
from MC to subdev, not the other way around. Let's discuss this below.

> > For MC-enabled devices, specifying subdev or video nodes by /dev node name
> > is painful. When you have multiple video devices on the system, or even
> > when you're modifying initialization order in a driver, the devnode names
> > will not be stable across boots. I used to type them out manually in the
> > very beginning and very quickly switched to retrieving them from the
> > subdev entity name in my test scripts.
> > 
> > What I'd like the compliance tools to do is to test all video nodes and
> > subdev nodes for a given MC device, with an option to restrict the tests
> > to a subset of the video devices and subdevs specified by media entity
> > name. We obviously need to keep support for addressing video nodes
> > manually as not all devices are MC-enabled, but for subdev we don't have
> > to care about such a backward compatibility issue as there's currently no
> > compliance tool.
> 
> I want two things:
> 
> 1) v4l2-compliance to be able to test a v4l-subdevX, just in isolation. And
> to be able to find the corresponding media device and make sure that what
> the v4l-subdev does is compatible with the entity/link information from the
> MC.

Why do you want to test /dev/v4l-subdev$(random) instead of /dev/mediaX + 
"subdev entity name" ?

Furthermore, if you want to test a subdev in complete isolation, why do you 
need to know which media device it belongs to ?

By the way, I'd like Sakari to join the discussion, but he won't be back 
before the end of next week.

> 2) make a media-compliance to look at the media topology as a whole.
> 
> Having the major/minor numbers are specifically for 1.
> 
> Actually, I really want to have the major/minor numbers of the media device
> for /dev/videoX as well, but entity ID +  major + minor number would use up
> the available space in struct v4l2_capability, so your suggestion of making
> a new VIDIOC_EXT_QUERYCAP has merit.
> 
> > On a side note, I believe subdev nodes should depend on MC. It has been a
> > historical mistake not to do so, and as far as I can see only three
> > drivers register subdev nodes without registering a media device. They
> > should be fixed if they want to benefit from the compliance tool.
> 
> Which ones?

atmel-isc, cobalt and rcar_drif.

> I'm not opposed to that. It would simplify matters quite a bit.
> 
> But I very, very strongly believe that a VIDIOC_EXT_QUERYCAP should return
> the corresponding entity ID and /dev/mediaX major and minor numbers. It's
> very useful information for a certain class of applications.

Do you have any application in mind other than the compliance tools ?

> Heck, I want to do 'v4l2-ctl -d /dev/video0 -D' or 'v4l2-ctl -d
> /dev/v4l-subdev0' and see not only the device capabilities, but also the
> corresponding entity information. Without having to scan through all
> /dev/media devices or requiring the user to pass the /dev/mediaX device
> separately.
> 
> This information is very cheap and I see no reason whatsoever not to
> implement this. It also feels much more symmetrical if I have what is
> effectively a backlink to the media device to which the subdev belongs.
> 
> And yes, normally applications will never need this since they use the media
> device and never reference a /dev/v4l-subdevX by name. But for v4l2-ctl and
> v4l2-compliance it is very useful indeed.

-- 
Regards,

Laurent Pinchart

Re: [RFCv2 PATCH 0/2] add VIDIOC_SUBDEV_QUERYCAP ioctl

[Hans Verkuil]

On 07/28/2017 04:28 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 28 Jul 2017 16:04:48 Hans Verkuil wrote:
>> On 07/28/2017 03:25 PM, Laurent Pinchart wrote:
>>> On Friday 28 Jul 2017 13:05:27 Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> I tried to get this in back in 2015, but that effort stalled.
>>>>
>>>> Trying again, since I really need this in order to add proper v4l-subdev
>>>> support to v4l2-ctl and v4l2-compliance. There currently is no way of
>>>> unique identifying that the device really is a v4l-subdev device other
>>>> than the device name (which can be changed by udev).
>>>>
>>>> So this patch series adds a VIDIOC_SUBDEV_QUERYCAP ioctl that is in
>>>> the core so it's guaranteed to be there.
>>>>
>>>> If the subdev is part of an MC then it also gives the corresponding
>>>> entity ID of the subdev and the major/minor numbers of the MC device
>>>> so v4l2-compliance can relate the subdev device directly to the right
>>>> MC device. The reserved array has room enough for more strings should
>>>> we need them later, although I think what we have here is sufficient.
>>>
>>> I still think this is not correct for two reasons.
>>>
>>> First of all, the new querycap ioctl uses the same ioctl number as
>>> VIDIOC_QUERYCAP. The full 32-bit number is different as the structures
>>> used for the two ioctls currently have different sizes, but that's not
>>> guaranteed going forward when we'll extend the structures used by the two
>>> ioctls with new fields.
>>
>> I think it is extraordinarily unlikely that these two will ever become
>> identical. And anyway, we control that ourselves.
>>
>>> To solve this, if you really want to identify the type of device node at
>>> runtime, we should have a single ioctl supported by the two device nodes.
>>> Given that we"re running out of capabilities bits for VIDIOC_QUERYCAP,
>>> this could be a good occasion to introduce a new ioctl to query
>>> capabilities.
>>
>> This makes more sense :-)
>>
>>> The second reason is that I don't think we should report the media device
>>> node associated with the subdev. Userspace really needs to become
>>> MC-centric, starting with the MC device, and going to the video nodes and
>>> subdev nodes. The other way around just doesn't make sense to me.
>>
>> It's not for 'regular' applications. It's to easily find out to which media
>> device a /dev/v4l-subdevX belongs. Primarily for applications like
>> v4l2-compliance.
>>
>> We have the information, and right now there is no way to take a v4l-subdevX
>> device and determine of which media device it is part other than scanning
>> the topologies of all media devices. That's nuts. This is cheap and makes
>> life for a certain class of applications much easier. Creating good
>> compliance tests is critical and this is a small but important contribution
>> to that.
> 
> I fully agree with the need for compliance tools, but I believe they should go 
> from MC to subdev, not the other way around. Let's discuss this below.
> 
>>> For MC-enabled devices, specifying subdev or video nodes by /dev node name
>>> is painful. When you have multiple video devices on the system, or even
>>> when you're modifying initialization order in a driver, the devnode names
>>> will not be stable across boots. I used to type them out manually in the
>>> very beginning and very quickly switched to retrieving them from the
>>> subdev entity name in my test scripts.
>>>
>>> What I'd like the compliance tools to do is to test all video nodes and
>>> subdev nodes for a given MC device, with an option to restrict the tests
>>> to a subset of the video devices and subdevs specified by media entity
>>> name. We obviously need to keep support for addressing video nodes
>>> manually as not all devices are MC-enabled, but for subdev we don't have
>>> to care about such a backward compatibility issue as there's currently no
>>> compliance tool.
>>
>> I want two things:
>>
>> 1) v4l2-compliance to be able to test a v4l-subdevX, just in isolation. And
>> to be able to find the corresponding media device and make sure that what
>> the v4l-subdev does is compatible with the entity/link information from the
>> MC.
> 
> Why do you want to test /dev/v4l-subdev$(random) instead of /dev/mediaX + 
> "subdev entity name" ?
> 
> Furthermore, if you want to test a subdev in complete isolation, why do you 
> need to know which media device it belongs to ?
> 
> By the way, I'd like Sakari to join the discussion, but he won't be back 
> before the end of next week.
> 
>> 2) make a media-compliance to look at the media topology as a whole.
>>
>> Having the major/minor numbers are specifically for 1.
>>
>> Actually, I really want to have the major/minor numbers of the media device
>> for /dev/videoX as well, but entity ID +  major + minor number would use up
>> the available space in struct v4l2_capability, so your suggestion of making
>> a new VIDIOC_EXT_QUERYCAP has merit.
>>
>>> On a side note, I believe subdev nodes should depend on MC. It has been a
>>> historical mistake not to do so, and as far as I can see only three
>>> drivers register subdev nodes without registering a media device. They
>>> should be fixed if they want to benefit from the compliance tool.
>>
>> Which ones?
> 
> atmel-isc, cobalt and rcar_drif.

Huh, I thought I added media device support to cobalt. I probably started
that but never finished it. I can certainly take care of that one, it should
not be hard.

> 
>> I'm not opposed to that. It would simplify matters quite a bit.
>>
>> But I very, very strongly believe that a VIDIOC_EXT_QUERYCAP should return
>> the corresponding entity ID and /dev/mediaX major and minor numbers. It's
>> very useful information for a certain class of applications.
> 
> Do you have any application in mind other than the compliance tools ?

No. v4l2-ctl, v4l2-compliance and perhaps a future media-compliance. Those
are the primary use-cases.

But even more important it is against all my 'good API sense' that I can't
find the corresponding media device from a v4l-subdev/video device. If I
can get to them from the media device through major/minor numbers then
consistent API design requires that I can also go through the other
direction. Especially since I see no downside to this.

It's nuts that I can't just pass /dev/v4l-subdevX to v4l2-ctl/compliance
and have it find out everything about it automatically.

It's all about self-discovery in an API.

Sorry, I feel very strongly about this.

Regards,

	Hans

> 
>> Heck, I want to do 'v4l2-ctl -d /dev/video0 -D' or 'v4l2-ctl -d
>> /dev/v4l-subdev0' and see not only the device capabilities, but also the
>> corresponding entity information. Without having to scan through all
>> /dev/media devices or requiring the user to pass the /dev/mediaX device
>> separately.
>>
>> This information is very cheap and I see no reason whatsoever not to
>> implement this. It also feels much more symmetrical if I have what is
>> effectively a backlink to the media device to which the subdev belongs.
>>
>> And yes, normally applications will never need this since they use the media
>> device and never reference a /dev/v4l-subdevX by name. But for v4l2-ctl and
>> v4l2-compliance it is very useful indeed.
>