[PATCH v4 0/5] media: Register read-only sub-dev devnode

[PATCH v4 0/5] media: Register read-only sub-dev devnode

[Jacopo Mondi]

v4 is now rebased on top of latest media master which has moved documentation
around quite a bit.

v4 includes two patches originally from Hans to add support for SUBDEV_QUERYCAP
ioctl. Compared to its initial version only capabilities flags and version are
reported.
https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=subdev-querycap

I chose to report both the RO and RW capabilities flag to make it possible for
userspace to test on both cases, as RW was the 'standard' so far, the flag could
be removed if considered not necessary.

Checkpatch reports:
WARNING: LINUX_VERSION_CODE should be avoided, code should be for the version to which it is merged
#44: FILE: drivers/media/v4l2-core/v4l2-subdev.c:344:
+		cap->version = LINUX_VERSION_CODE;

but I see LINUX_VERSION_CODE being used to version the media controller as well,
so I assume it's a false positive.

Copy of the old cover letter is below reported:

-------------------------------------------------------------------------------
Add new function v4l2_device_register_ro_subdev_nodes() to pair with
v4l2_device_register_subdev_nodes() that allows a bridge driver to register the
device node for its subdevices in read-only mode.

Devnode-centric (aka non-MC) bridge drivers control their subdevices through
direct calls to v4l2 subdev operations and do not want userspace to be able
to control the subdevice configuration by calling ioctls on the sub-device
devnode. For this reason, they usually refrain from registering any devnode at
all for their subdevices.

However it is sometimes required for userspace to access the sub-dev device
nodes to collect information on the actual configuration, without changing
the one currently applied to the device.

This requirement became pressing while working on libcamera on devnode-centric
platforms that do not expose any sub-device for their camera sensor to prevent
userspace from changing their configuration. To allow them to register device
node and being guaranteed to retain control of the subdevice configuration this
series proposes a way to register device nodes in read-only to restrict
access to all ioctls that could potentially affect the sub-dev configuration.
-------------------------------------------------------------------------------


Thanks
   j

v3->v4:
- Rebase v3 on latest media master and new documentation layout
- Add SUBDEV_QUERYCAP support

v2->v3:
- Add Sakari's ack to the series
- Documentation:
  - Address Sakari' and Hans suggestions
- Implementation:
  - Rename V4L2_FL_RO_DEVNODE to V4L2_FL_SUBDEV_RO_DEVNODE
  - Limit the ability to register sub-device video device nodes to
    driver claiming support for CONFIG_VIDEO_V4L2_SUBDEV_API

v1->v2:
- Documentation:
  - Add a new patch using Laurent's suggestion to update the sub-device
    userspace API introduction
  - Take in some of Laurent's suggestions in v4l2-subdev.rst and add a new
    section in dev-subdev.rst
- Implementation:
  - As noted by Andrey, V4L2_FL_* are meant to be used as bitmasks. Use
    test_bit()/set_bit() as the rest of the v4l2 core does. It's a bit an
    overkill compared to use plain BIT() as noted by Sakari but I preferred
    consistency with the rest of the core
  - Make v4l2_device_register_subdev_nodes() and
    v4l2_device_register_ro_subdev_nodes() to v4l2-device.h and make them
    inline functions. Documentation style has been copied from other functions
    with similar implementations, such as __video_register_device() in
    v4l2-dev.h

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

Jacopo Mondi (3):
  Documentation: media: Update sub-device API intro
  Documentation: media: Document read-only subdevice
  media: v4l2-dev: Add v4l2_device_register_ro_subdev_node()

 .../driver-api/media/v4l2-subdev.rst          |  53 +++++++-
 .../userspace-api/media/v4l/dev-subdev.rst    |   5 +
 .../userspace-api/media/v4l/user-func.rst     |   1 +
 .../media/v4l/vidioc-g-dv-timings.rst         |   6 +
 .../userspace-api/media/v4l/vidioc-g-std.rst  |   6 +
 .../media/v4l/vidioc-subdev-g-crop.rst        |   9 ++
 .../media/v4l/vidioc-subdev-g-fmt.rst         |   8 ++
 .../v4l/vidioc-subdev-g-frame-interval.rst    |   8 ++
 .../media/v4l/vidioc-subdev-g-selection.rst   |   8 ++
 .../media/v4l/vidioc-subdev-querycap.rst      | 114 ++++++++++++++++++
 drivers/media/v4l2-core/v4l2-device.c         |   7 +-
 drivers/media/v4l2-core/v4l2-subdev.c         |  31 +++++
 include/media/v4l2-dev.h                      |   7 ++
 include/media/v4l2-device.h                   |  50 +++++++-
 include/uapi/linux/v4l2-subdev.h              |  15 +++
 15 files changed, 320 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst

--
2.26.1

[PATCH v4 1/5] Documentation: media: Update sub-device API intro

[Jacopo Mondi]

Update the V4L2 sub-device userspace API introduction to provide more
details on why complex devices might want to register devnodes for the
connected subdevices.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 Documentation/driver-api/media/v4l2-subdev.rst | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index 29e07e23f888..41ccb3e5c707 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -275,8 +275,13 @@ system the .unbind() method is called. All three callbacks are optional.
 V4L2 sub-device userspace API
 -----------------------------
 
-Beside exposing a kernel API through the :c:type:`v4l2_subdev_ops` structure,
-V4L2 sub-devices can also be controlled directly by userspace applications.
+Bridge drivers traditionally expose one or multiple video nodes to userspace,
+and control subdevices through the :c:type:`v4l2_subdev_ops` operations in
+response to video node operations. This hides the complexity of the underlying
+hardware from applications. For complex devices, finer-grained control of the
+device than what the video nodes offer may be required. In those cases, bridge
+drivers that implement :ref:`the media controller API <media_controller>` may
+opt for making the subdevice operations directly accessible from userpace.
 
 Device nodes named ``v4l-subdev``\ *X* can be created in ``/dev`` to access
 sub-devices directly. If a sub-device supports direct userspace configuration
-- 
2.26.1

[PATCH v4 2/5] Documentation: media: Document read-only subdevice

[Jacopo Mondi]

Document a new kAPI function to register subdev device nodes in read only
mode and for each affected ioctl report how access is restricted.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../driver-api/media/v4l2-subdev.rst          | 44 +++++++++++++++++++
 .../userspace-api/media/v4l/dev-subdev.rst    |  5 +++
 .../media/v4l/vidioc-g-dv-timings.rst         |  6 +++
 .../userspace-api/media/v4l/vidioc-g-std.rst  |  6 +++
 .../media/v4l/vidioc-subdev-g-crop.rst        |  9 ++++
 .../media/v4l/vidioc-subdev-g-fmt.rst         |  8 ++++
 .../v4l/vidioc-subdev-g-frame-interval.rst    |  8 ++++
 .../media/v4l/vidioc-subdev-g-selection.rst   |  8 ++++
 8 files changed, 94 insertions(+)

diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst
index 41ccb3e5c707..6ced2381952a 100644
--- a/Documentation/driver-api/media/v4l2-subdev.rst
+++ b/Documentation/driver-api/media/v4l2-subdev.rst
@@ -332,6 +332,50 @@ Private ioctls
 	All ioctls not in the above list are passed directly to the sub-device
 	driver through the core::ioctl operation.
 
+Read-only sub-device userspace API
+----------------------------------
+
+Bridge drivers that control their connected subdevices through direct calls to
+the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually
+want userspace to be able to change the same parameters through the subdevice
+device node and thus do not usually register any.
+
+It is sometimes useful to report to userspace the current subdevice
+configuration through a read-only API, that does not permit applications to
+change to the device parameters but allows interfacing to the subdevice device
+node to inspect them.
+
+For instance, to implement cameras based on computational photography, userspace
+needs to know the detailed camera sensor configuration (in terms of skipping,
+binning, cropping and scaling) for each supported output resolution. To support
+such use cases, bridge drivers may expose the subdevice operations to userspace
+through a read-only API.
+
+To create a read-only device node for all the subdevices registered with the
+``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call
+:c:func:`v4l2_device_register_ro_subdev_nodes`.
+
+Access to the following ioctls for userspace applications is restricted on
+sub-device device nodes registered with
+:c:func:`v4l2_device_register_ro_subdev_nodes`.
+
+``VIDIOC_SUBDEV_S_FMT``,
+``VIDIOC_SUBDEV_S_CROP``,
+``VIDIOC_SUBDEV_S_SELECTION``:
+
+	These ioctls are only allowed on a read-only subdevice device node
+	for the :ref:`V4L2_SUBDEV_FORMAT_TRY <v4l2-subdev-format-whence>`
+	formats and selection rectangles.
+
+``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,
+``VIDIOC_SUBDEV_S_DV_TIMINGS``,
+``VIDIOC_SUBDEV_S_STD``:
+
+	These ioctls are not allowed on a read-only subdevice node.
+
+In case the ioctl is not allowed, or the format to modify is set to
+``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and
+the errno variable is set to ``-EPERM``.
 
 I2C sub-device drivers
 ----------------------
diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst
index 0c1a5f50ee21..134d2fb909fa 100644
--- a/Documentation/userspace-api/media/v4l/dev-subdev.rst
+++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst
@@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to
 Sub-device character device nodes, conventionally named
 ``/dev/v4l-subdev*``, use major number 81.
 
+Drivers may opt to limit the sub-device character devices to only expose
+operations that do not modify the device state. In such a case the sub-devices
+are referred to as ``read-only`` in the rest of this documentation, and the
+related restrictions are documented in individual ioctls.
+
 
 Controls
 ========
diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-dv-timings.rst b/Documentation/userspace-api/media/v4l/vidioc-g-dv-timings.rst
index 84806a893cb7..9a035a4ea0f0 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-dv-timings.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-dv-timings.rst
@@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`
 structure as argument. If the ioctl is not supported or the timing
 values are not correct, the driver returns ``EINVAL`` error code.
 
+Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been
+registered in read-only mode is not allowed. An error is returned and the errno
+variable is set to ``-EPERM``.
+
 The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of
 the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If
 the current input or output does not support DV timings (e.g. if
@@ -81,6 +85,8 @@ ENODATA
 EBUSY
     The device is busy and therefore can not change the timings.
 
+EPERM
+    ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.
 
 .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
 
diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-std.rst b/Documentation/userspace-api/media/v4l/vidioc-g-std.rst
index b0bdb719d405..6d8cb7f29ac6 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-g-std.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-g-std.rst
@@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`
 does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is
 returned.
 
+Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered
+in read-only mode is not allowed. An error is returned and the errno variable is
+set to ``-EPERM``.
 
 Return Value
 ============
@@ -79,3 +82,6 @@ EINVAL
 
 ENODATA
     Standard video timings are not supported for this input or output.
+
+EPERM
+    ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst
index 8d9fc13015a6..615e3efdf935 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-crop.rst
@@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two
 applications querying the same sub-device would thus not interact with
 each other.
 
+If the subdev device node has been registered in read-only mode, calls to
+``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to
+``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno
+variable is set to ``-EPERM``.
+
 Drivers must not return an error solely because the requested crop
 rectangle doesn't match the device capabilities. They must instead
 modify the rectangle to match what the hardware can provide. The
@@ -123,3 +128,7 @@ EINVAL
     references a non-existing pad, the ``which`` field references a
     non-existing format, or cropping is not supported on the given
     subdev pad.
+
+EPERM
+    The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice
+    and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst
index 69d60e18664b..909ee9f90867 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst
@@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,
 a low-pass noise filter might crop pixels at the frame boundaries,
 modifying its output frame size.
 
+If the subdev device node has been registered in read-only mode, calls to
+``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to
+``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno
+variable is set to ``-EPERM``.
+
 Drivers must not return an error solely because the requested format
 doesn't match the device capabilities. They must instead modify the
 format to match what the hardware can provide. The modified format
@@ -146,6 +151,9 @@ EINVAL
     ``pad`` references a non-existing pad, or the ``which`` field
     references a non-existing format.
 
+EPERM
+    The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice
+    and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.
 
 ============
 
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst
index b61baaf11624..51e1bff797f0 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-frame-interval.rst
@@ -65,6 +65,10 @@ struct
 contains the current frame interval as would be returned by a
 ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.
 
+Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been
+registered in read-only mode is not allowed. An error is returned and the errno
+variable is set to ``-EPERM``.
+
 Drivers must not return an error solely because the requested interval
 doesn't match the device capabilities. They must instead modify the
 interval to match what the hardware can provide. The modified interval
@@ -118,3 +122,7 @@ EINVAL
     :c:type:`v4l2_subdev_frame_interval`
     ``pad`` references a non-existing pad, or the pad doesn't support
     frame intervals.
+
+EPERM
+    The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only
+    subdevice.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
index 981c95df2dec..06c9553ac48f 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-selection.rst
@@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.
 See :ref:`subdev` for more information on how each selection target
 affects the image processing pipeline inside the subdevice.
 
+If the subdev device node has been registered in read-only mode, calls to
+``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to
+``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno
+variable is set to ``-EPERM``.
 
 Types of selection targets
 --------------------------
@@ -123,3 +127,7 @@ EINVAL
     ``pad`` references a non-existing pad, the ``which`` field
     references a non-existing format, or the selection target is not
     supported on the given subdev pad.
+
+EPERM
+    The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only
+    subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.
-- 
2.26.1

[PATCH v4 3/5] media: v4l2-dev: Add v4l2_device_register_ro_subdev_node()

[Jacopo Mondi]

Add to the V4L2 core a function to register device nodes for video
subdevices in read-only mode.

Registering a device node in read-only mode is useful to expose to
userspace the current sub-device configuration, without allowing
application to change it by using the V4L2 subdevice ioctls.

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-device.c |  7 ++--
 drivers/media/v4l2-core/v4l2-subdev.c | 19 ++++++++++
 include/media/v4l2-dev.h              |  7 ++++
 include/media/v4l2-device.h           | 50 ++++++++++++++++++++++++---
 4 files changed, 77 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index c69941214bb2..de4287251a89 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -186,7 +186,8 @@ static void v4l2_device_release_subdev_node(struct video_device *vdev)
 	kfree(vdev);
 }
 
-int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+int __v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev,
+					bool read_only)
 {
 	struct video_device *vdev;
 	struct v4l2_subdev *sd;
@@ -215,6 +216,8 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 		vdev->fops = &v4l2_subdev_fops;
 		vdev->release = v4l2_device_release_subdev_node;
 		vdev->ctrl_handler = sd->ctrl_handler;
+		if (read_only)
+			set_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
 		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
 					      sd->owner);
 		if (err < 0) {
@@ -252,7 +255,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 
 	return err;
 }
-EXPORT_SYMBOL_GPL(v4l2_device_register_subdev_nodes);
+EXPORT_SYMBOL_GPL(__v4l2_device_register_subdev_nodes);
 
 void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 {
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index a376b351135f..1dc263c2ca0a 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -331,6 +331,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	struct v4l2_fh *vfh = file->private_data;
 #if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
 	struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
+	bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
 #endif
 	int rval;
 
@@ -477,6 +478,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_S_FMT: {
 		struct v4l2_subdev_format *format = arg;
 
+		if (format->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
+			return -EPERM;
+
 		memset(format->reserved, 0, sizeof(format->reserved));
 		memset(format->format.reserved, 0, sizeof(format->format.reserved));
 		return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
@@ -504,6 +508,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		struct v4l2_subdev_crop *crop = arg;
 		struct v4l2_subdev_selection sel;
 
+		if (crop->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
+			return -EPERM;
+
 		memset(crop->reserved, 0, sizeof(crop->reserved));
 		memset(&sel, 0, sizeof(sel));
 		sel.which = crop->which;
@@ -545,6 +552,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_S_FRAME_INTERVAL: {
 		struct v4l2_subdev_frame_interval *fi = arg;
 
+		if (ro_subdev)
+			return -EPERM;
+
 		memset(fi->reserved, 0, sizeof(fi->reserved));
 		return v4l2_subdev_call(sd, video, s_frame_interval, arg);
 	}
@@ -568,6 +578,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_S_SELECTION: {
 		struct v4l2_subdev_selection *sel = arg;
 
+		if (sel->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev)
+			return -EPERM;
+
 		memset(sel->reserved, 0, sizeof(sel->reserved));
 		return v4l2_subdev_call(
 			sd, pad, set_selection, subdev_fh->pad, sel);
@@ -604,6 +617,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		return v4l2_subdev_call(sd, video, g_dv_timings, arg);
 
 	case VIDIOC_SUBDEV_S_DV_TIMINGS:
+		if (ro_subdev)
+			return -EPERM;
+
 		return v4l2_subdev_call(sd, video, s_dv_timings, arg);
 
 	case VIDIOC_SUBDEV_G_STD:
@@ -612,6 +628,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_SUBDEV_S_STD: {
 		v4l2_std_id *std = arg;
 
+		if (ro_subdev)
+			return -EPERM;
+
 		return v4l2_subdev_call(sd, video, s_std, *std);
 	}
 
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 4602c15ff878..ad2d41952442 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -82,11 +82,18 @@ struct v4l2_ctrl_handler;
  *	but the old crop API will still work as expected in order to preserve
  *	backwards compatibility.
  *	Never set this flag for new drivers.
+ * @V4L2_FL_SUBDEV_RO_DEVNODE:
+ *	indicates that the video device node is registered in read-only mode.
+ *	The flag only applies to device nodes registered for sub-devices, it is
+ *	set by the core when the sub-devices device nodes are registered with
+ *	v4l2_device_register_ro_subdev_nodes() and used by the sub-device ioctl
+ *	handler to restrict access to some ioctl calls.
  */
 enum v4l2_video_device_flags {
 	V4L2_FL_REGISTERED		= 0,
 	V4L2_FL_USES_V4L2_FH		= 1,
 	V4L2_FL_QUIRK_INVERTED_CROP	= 2,
+	V4L2_FL_SUBDEV_RO_DEVNODE	= 3,
 };
 
 /* Priority helper functions */
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 7c912b7d2870..64ec4de948e9 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -174,14 +174,56 @@ int __must_check v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 void v4l2_device_unregister_subdev(struct v4l2_subdev *sd);
 
 /**
- * v4l2_device_register_subdev_nodes - Registers device nodes for all subdevs
- *	of the v4l2 device that are marked with
- *	the %V4L2_SUBDEV_FL_HAS_DEVNODE flag.
+ * __v4l2_device_register_ro_subdev_nodes - Registers device nodes for
+ *      all subdevs of the v4l2 device that are marked with the
+ *      %V4L2_SUBDEV_FL_HAS_DEVNODE flag.
  *
  * @v4l2_dev: pointer to struct v4l2_device
+ * @read_only: subdevices read-only flag. True to register the subdevices
+ *	device nodes in read-only mode, false to allow full access to the
+ *	subdevice userspace API.
  */
 int __must_check
-v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev);
+__v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev,
+				    bool read_only);
+
+/**
+ * v4l2_device_register_subdev_nodes - Registers subdevices device nodes with
+ *	unrestricted access to the subdevice userspace operations
+ *
+ * Internally calls __v4l2_device_register_subdev_nodes(). See its documentation
+ * for more details.
+ *
+ * @v4l2_dev: pointer to struct v4l2_device
+ */
+static inline int __must_check
+v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
+{
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+	return __v4l2_device_register_subdev_nodes(v4l2_dev, false);
+#else
+	return 0;
+#endif
+}
+
+/**
+ * v4l2_device_register_ro_subdev_nodes - Registers subdevices device nodes
+ *	in read-only mode
+ *
+ * Internally calls __v4l2_device_register_subdev_nodes(). See its documentation
+ * for more details.
+ *
+ * @v4l2_dev: pointer to struct v4l2_device
+ */
+static inline int __must_check
+v4l2_device_register_ro_subdev_nodes(struct v4l2_device *v4l2_dev)
+{
+#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
+	return __v4l2_device_register_subdev_nodes(v4l2_dev, true);
+#else
+	return 0;
+#endif
+}
 
 /**
  * v4l2_subdev_notify - Sends a notification to v4l2_device.
-- 
2.26.1

[PATCH v4 4/5] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl

[Jacopo Mondi]

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 1dc263c2ca0a..ca0aa54429c5 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>
@@ -336,6 +337,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..0886f88be193 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		(1 << 0)
+/* The v4l2 sub-device video device node is registered in read/write mode. */
+#define V4L2_SUBDEV_CAP_RW_SUBDEV		(1 << 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)
-- 
2.26.1

[PATCH v4 5/5] v4l: document VIDIOC_SUBDEV_QUERYCAP

[Jacopo Mondi]

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>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../userspace-api/media/v4l/user-func.rst     |   1 +
 .../media/v4l/vidioc-subdev-querycap.rst      | 114 ++++++++++++++++++
 2 files changed, 115 insertions(+)
 create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst

diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
index f235f88efe89..559cce421d41 100644
--- a/Documentation/userspace-api/media/v4l/user-func.rst
+++ b/Documentation/userspace-api/media/v4l/user-func.rst
@@ -78,6 +78,7 @@ Function Reference
     vidioc-subdev-g-fmt
     vidioc-subdev-g-frame-interval
     vidioc-subdev-g-selection
+    vidioc-subdev-querycap.rst
     vidioc-subscribe-event
     func-mmap
     func-munmap
diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
new file mode 100644
index 000000000000..d9b2e19e1339
--- /dev/null
+++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
@@ -0,0 +1,114 @@
+.. Permission is granted to copy, distribute and/or modify this
+.. document under the terms of the GNU Free Documentation License,
+.. Version 1.1 or any later version published by the Free Software
+.. Foundation, with no Invariant Sections, no Front-Cover Texts
+.. and no Back-Cover Texts. A copy of the license is included at
+.. Documentation/userspace-api/media/fdl-appendix.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``
+    Pointer to struct :c:type:`v4l2_subdev_capability`.
+
+
+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
+      - ``subdev_caps``
+      - Sub-device capabilities of the opened device, see
+	:ref:`subdevice-capabilities`.
+
+.. 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_RO_SUBDEV
+      - 0x00000001
+      - The sub-device device node is registered in read-only mode.
+        Access to the sub-device ioctls that modify the device state is
+        restricted. Refer to each individual subdevice ioctl documentation
+        for a description of which restrictions apply to a read-only sub-device.
+
+    * - V4L2_SUBDEV_CAP_RW_SUBDEV
+      - 0x00000002
+      - The sub-device device node is registered in read/write mode, all the
+        subdevice ioctls are accessible from userspace.
+
+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.
+
+ENOTTY
+    The device node is not a V4L2 sub-device.
-- 
2.26.1

Re: [PATCH v4 4/5] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5546 bytes --]

Hi Jacopo,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on linus/master v5.7-rc2 next-20200420]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jacopo-Mondi/media-Register-read-only-sub-dev-devnode/20200421-053419
base:   git://linuxtv.org/media_tree.git master
config: arm-bcm2835_defconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project a9b137f9ffba8cb25dfd7dd1fb613e8aac121b37)
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/media/v4l2-core/v4l2-subdev.c:345:23: error: use of undeclared identifier 'ro_subdev'
                   cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
                                       ^
   1 error generated.

vim +/ro_subdev +345 drivers/media/v4l2-core/v4l2-subdev.c

   327	
   328	static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
   329	{
   330		struct video_device *vdev = video_devdata(file);
   331		struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
   332		struct v4l2_fh *vfh = file->private_data;
   333	#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
   334		struct v4l2_subdev_fh *subdev_fh = to_v4l2_subdev_fh(vfh);
   335		bool ro_subdev = test_bit(V4L2_FL_SUBDEV_RO_DEVNODE, &vdev->flags);
   336	#endif
   337		int rval;
   338	
   339		switch (cmd) {
   340		case VIDIOC_SUBDEV_QUERYCAP: {
   341			struct v4l2_subdev_capability *cap = arg;
   342	
   343			memset(cap, 0, sizeof(*cap));
   344			cap->version = LINUX_VERSION_CODE;
 > 345			cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
   346						      : V4L2_SUBDEV_CAP_RW_SUBDEV;
   347	
   348			return 0;
   349		}
   350	
   351		case VIDIOC_QUERYCTRL:
   352			/*
   353			 * TODO: this really should be folded into v4l2_queryctrl (this
   354			 * currently returns -EINVAL for NULL control handlers).
   355			 * However, v4l2_queryctrl() is still called directly by
   356			 * drivers as well and until that has been addressed I believe
   357			 * it is safer to do the check here. The same is true for the
   358			 * other control ioctls below.
   359			 */
   360			if (!vfh->ctrl_handler)
   361				return -ENOTTY;
   362			return v4l2_queryctrl(vfh->ctrl_handler, arg);
   363	
   364		case VIDIOC_QUERY_EXT_CTRL:
   365			if (!vfh->ctrl_handler)
   366				return -ENOTTY;
   367			return v4l2_query_ext_ctrl(vfh->ctrl_handler, arg);
   368	
   369		case VIDIOC_QUERYMENU:
   370			if (!vfh->ctrl_handler)
   371				return -ENOTTY;
   372			return v4l2_querymenu(vfh->ctrl_handler, arg);
   373	
   374		case VIDIOC_G_CTRL:
   375			if (!vfh->ctrl_handler)
   376				return -ENOTTY;
   377			return v4l2_g_ctrl(vfh->ctrl_handler, arg);
   378	
   379		case VIDIOC_S_CTRL:
   380			if (!vfh->ctrl_handler)
   381				return -ENOTTY;
   382			return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
   383	
   384		case VIDIOC_G_EXT_CTRLS:
   385			if (!vfh->ctrl_handler)
   386				return -ENOTTY;
   387			return v4l2_g_ext_ctrls(vfh->ctrl_handler,
   388						vdev, sd->v4l2_dev->mdev, arg);
   389	
   390		case VIDIOC_S_EXT_CTRLS:
   391			if (!vfh->ctrl_handler)
   392				return -ENOTTY;
   393			return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler,
   394						vdev, sd->v4l2_dev->mdev, arg);
   395	
   396		case VIDIOC_TRY_EXT_CTRLS:
   397			if (!vfh->ctrl_handler)
   398				return -ENOTTY;
   399			return v4l2_try_ext_ctrls(vfh->ctrl_handler,
   400						  vdev, sd->v4l2_dev->mdev, arg);
   401	
   402		case VIDIOC_DQEVENT:
   403			if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
   404				return -ENOIOCTLCMD;
   405	
   406			return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
   407	
   408		case VIDIOC_DQEVENT_TIME32: {
   409			struct v4l2_event_time32 *ev32 = arg;
   410			struct v4l2_event ev = { };
   411	
   412			if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
   413				return -ENOIOCTLCMD;
   414	
   415			rval = v4l2_event_dequeue(vfh, &ev, file->f_flags & O_NONBLOCK);
   416	
   417			*ev32 = (struct v4l2_event_time32) {
   418				.type		= ev.type,
   419				.pending	= ev.pending,
   420				.sequence	= ev.sequence,
   421				.timestamp.tv_sec  = ev.timestamp.tv_sec,
   422				.timestamp.tv_nsec = ev.timestamp.tv_nsec,
   423				.id		= ev.id,
   424			};
   425	
   426			memcpy(&ev32->u, &ev.u, sizeof(ev.u));
   427			memcpy(&ev32->reserved, &ev.reserved, sizeof(ev.reserved));
   428	
   429			return rval;
   430		}
   431	
   432		case VIDIOC_SUBSCRIBE_EVENT:
   433			return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
   434	
   435		case VIDIOC_UNSUBSCRIBE_EVENT:
   436			return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
   437	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27302 bytes --]

Re: [PATCH v4 4/5] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl

[Sakari Ailus]

Hi Jacopo,

Thanks for the patch.

On Sat, Apr 18, 2020 at 12:32:15PM +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 1dc263c2ca0a..ca0aa54429c5 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>
> @@ -336,6 +337,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..0886f88be193 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;

No reserved fields? Is the intent to extend this later on based on the size
of the IOCTL argument?

> +};
> +
> +/* The v4l2 sub-device video device node is registered in read-only mode. */
> +#define V4L2_SUBDEV_CAP_RO_SUBDEV		(1 << 0)

1U << 0

> +/* The v4l2 sub-device video device node is registered in read/write mode. */
> +#define V4L2_SUBDEV_CAP_RW_SUBDEV		(1 << 1)

1U << 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)

-- 
Regards,

Sakari Ailus

Re: [PATCH v4 5/5] v4l: document VIDIOC_SUBDEV_QUERYCAP

[Sakari Ailus]

Hi Jacopo,

On Sat, Apr 18, 2020 at 12:32:16PM +0200, Jacopo Mondi wrote:
> 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>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../userspace-api/media/v4l/user-func.rst     |   1 +
>  .../media/v4l/vidioc-subdev-querycap.rst      | 114 ++++++++++++++++++
>  2 files changed, 115 insertions(+)
>  create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
> 
> diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> index f235f88efe89..559cce421d41 100644
> --- a/Documentation/userspace-api/media/v4l/user-func.rst
> +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> @@ -78,6 +78,7 @@ Function Reference
>      vidioc-subdev-g-fmt
>      vidioc-subdev-g-frame-interval
>      vidioc-subdev-g-selection
> +    vidioc-subdev-querycap.rst

.rst looks like extra.

>      vidioc-subscribe-event
>      func-mmap
>      func-munmap
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
> new file mode 100644
> index 000000000000..d9b2e19e1339
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
> @@ -0,0 +1,114 @@
> +.. Permission is granted to copy, distribute and/or modify this
> +.. document under the terms of the GNU Free Documentation License,
> +.. Version 1.1 or any later version published by the Free Software
> +.. Foundation, with no Invariant Sections, no Front-Cover Texts
> +.. and no Back-Cover Texts. A copy of the license is included at
> +.. Documentation/userspace-api/media/fdl-appendix.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``
> +    Pointer to struct :c:type:`v4l2_subdev_capability`.
> +
> +
> +Description
> +===========
> +
> +All V4L2 sub-devices support the
> +``VIDIOC_SUBDEV_QUERYCAP`` ioctl. It is used to identify

You could rewrap the paragraph.

> +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
> +      - ``subdev_caps``
> +      - Sub-device capabilities of the opened device, see
> +	:ref:`subdevice-capabilities`.
> +
> +.. 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_RO_SUBDEV
> +      - 0x00000001
> +      - The sub-device device node is registered in read-only mode.
> +        Access to the sub-device ioctls that modify the device state is
> +        restricted. Refer to each individual subdevice ioctl documentation
> +        for a description of which restrictions apply to a read-only sub-device.

Tabs would be nice. Same below (and above, too).

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +
> +    * - V4L2_SUBDEV_CAP_RW_SUBDEV
> +      - 0x00000002
> +      - The sub-device device node is registered in read/write mode, all the
> +        subdevice ioctls are accessible from userspace.
> +
> +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.
> +
> +ENOTTY
> +    The device node is not a V4L2 sub-device.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 4/5] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl

[Laurent Pinchart]

Hi Sakari,

On Wed, Apr 22, 2020 at 12:49:58AM +0300, Sakari Ailus wrote:
> On Sat, Apr 18, 2020 at 12:32:15PM +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 1dc263c2ca0a..ca0aa54429c5 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>
> > @@ -336,6 +337,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..0886f88be193 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;
> 
> No reserved fields? Is the intent to extend this later on based on the size
> of the IOCTL argument?

That would be my preferred option.

> > +};
> > +
> > +/* The v4l2 sub-device video device node is registered in read-only mode. */
> > +#define V4L2_SUBDEV_CAP_RO_SUBDEV		(1 << 0)
> 
> 1U << 0

BIT(0)

> 
> > +/* The v4l2 sub-device video device node is registered in read/write mode. */
> > +#define V4L2_SUBDEV_CAP_RW_SUBDEV		(1 << 1)
> 
> 1U << 1

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)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/5] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl

[Jacopo Mondi]

Hi Sakari,

On Wed, Apr 22, 2020 at 02:42:32AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Wed, Apr 22, 2020 at 12:49:58AM +0300, Sakari Ailus wrote:
> > On Sat, Apr 18, 2020 at 12:32:15PM +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 1dc263c2ca0a..ca0aa54429c5 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>
> > > @@ -336,6 +337,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..0886f88be193 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;
> >
> > No reserved fields? Is the intent to extend this later on based on the size
> > of the IOCTL argument?
>
> That would be my preferred option.
>

That would be my preferred opinion as well, we would have versioning
and would not be tied to the available reserved fields.

Re: [PATCH v4 5/5] v4l: document VIDIOC_SUBDEV_QUERYCAP

[Jacopo Mondi]

Hi Sakari,

On Wed, Apr 22, 2020 at 12:55:41AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Sat, Apr 18, 2020 at 12:32:16PM +0200, Jacopo Mondi wrote:
> > 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>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../userspace-api/media/v4l/user-func.rst     |   1 +
> >  .../media/v4l/vidioc-subdev-querycap.rst      | 114 ++++++++++++++++++
> >  2 files changed, 115 insertions(+)
> >  create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
> >
> > diff --git a/Documentation/userspace-api/media/v4l/user-func.rst b/Documentation/userspace-api/media/v4l/user-func.rst
> > index f235f88efe89..559cce421d41 100644
> > --- a/Documentation/userspace-api/media/v4l/user-func.rst
> > +++ b/Documentation/userspace-api/media/v4l/user-func.rst
> > @@ -78,6 +78,7 @@ Function Reference
> >      vidioc-subdev-g-fmt
> >      vidioc-subdev-g-frame-interval
> >      vidioc-subdev-g-selection
> > +    vidioc-subdev-querycap.rst
>
> .rst looks like extra.
>
> >      vidioc-subscribe-event
> >      func-mmap
> >      func-munmap
> > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
> > new file mode 100644
> > index 000000000000..d9b2e19e1339
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-querycap.rst
> > @@ -0,0 +1,114 @@
> > +.. Permission is granted to copy, distribute and/or modify this
> > +.. document under the terms of the GNU Free Documentation License,
> > +.. Version 1.1 or any later version published by the Free Software
> > +.. Foundation, with no Invariant Sections, no Front-Cover Texts
> > +.. and no Back-Cover Texts. A copy of the license is included at
> > +.. Documentation/userspace-api/media/fdl-appendix.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``
> > +    Pointer to struct :c:type:`v4l2_subdev_capability`.
> > +
> > +
> > +Description
> > +===========
> > +
> > +All V4L2 sub-devices support the
> > +``VIDIOC_SUBDEV_QUERYCAP`` ioctl. It is used to identify
>
> You could rewrap the paragraph.
>
> > +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
> > +      - ``subdev_caps``
> > +      - Sub-device capabilities of the opened device, see
> > +	:ref:`subdevice-capabilities`.
> > +
> > +.. 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_RO_SUBDEV
> > +      - 0x00000001
> > +      - The sub-device device node is registered in read-only mode.
> > +        Access to the sub-device ioctls that modify the device state is
> > +        restricted. Refer to each individual subdevice ioctl documentation
> > +        for a description of which restrictions apply to a read-only sub-device.
>
> Tabs would be nice. Same below (and above, too).

I'm not sure what is the rule here, but mixing tabs and spaces doesn't
seem nice to me :)

>
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>

Thanks
  j

> > +
> > +    * - V4L2_SUBDEV_CAP_RW_SUBDEV
> > +      - 0x00000002
> > +      - The sub-device device node is registered in read/write mode, all the
> > +        subdevice ioctls are accessible from userspace.
> > +
> > +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.
> > +
> > +ENOTTY
> > +    The device node is not a V4L2 sub-device.
>
> --
> Kind regards,
>
> Sakari Ailus