* [PATCH v5 1/6] Documentation: media: Update sub-device API intro
2020-04-28 21:06 [PATCH v5 0/6] media: Register read-only sub-dev devnode Jacopo Mondi
@ 2020-04-28 21:06 ` Jacopo Mondi
2020-04-28 21:06 ` [PATCH v5 2/6] Documentation: media: Document read-only subdevice Jacopo Mondi
` (4 subsequent siblings)
5 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-28 21:06 UTC (permalink / raw)
To: linux-media, libcamera-devel
Cc: Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 2/6] Documentation: media: Document read-only subdevice
2020-04-28 21:06 [PATCH v5 0/6] media: Register read-only sub-dev devnode Jacopo Mondi
2020-04-28 21:06 ` [PATCH v5 1/6] Documentation: media: Update sub-device API intro Jacopo Mondi
@ 2020-04-28 21:06 ` Jacopo Mondi
2020-04-28 21:06 ` [PATCH v5 3/6] media: v4l2-dev: Add v4l2_device_register_ro_subdev_node() Jacopo Mondi
` (3 subsequent siblings)
5 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-28 21:06 UTC (permalink / raw)
To: linux-media, libcamera-devel
Cc: Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 3/6] media: v4l2-dev: Add v4l2_device_register_ro_subdev_node()
2020-04-28 21:06 [PATCH v5 0/6] media: Register read-only sub-dev devnode Jacopo Mondi
2020-04-28 21:06 ` [PATCH v5 1/6] Documentation: media: Update sub-device API intro Jacopo Mondi
2020-04-28 21:06 ` [PATCH v5 2/6] Documentation: media: Document read-only subdevice Jacopo Mondi
@ 2020-04-28 21:06 ` Jacopo Mondi
2020-04-28 21:06 ` [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected Jacopo Mondi
` (2 subsequent siblings)
5 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-28 21:06 UTC (permalink / raw)
To: linux-media, libcamera-devel
Cc: Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
2020-04-28 21:06 [PATCH v5 0/6] media: Register read-only sub-dev devnode Jacopo Mondi
` (2 preceding siblings ...)
2020-04-28 21:06 ` [PATCH v5 3/6] media: v4l2-dev: Add v4l2_device_register_ro_subdev_node() Jacopo Mondi
@ 2020-04-28 21:06 ` Jacopo Mondi
2020-04-28 21:26 ` Sakari Ailus
` (2 more replies)
2020-04-28 21:06 ` [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl Jacopo Mondi
2020-04-28 21:06 ` [PATCH v5 6/6] v4l: document VIDIOC_SUBDEV_QUERYCAP Jacopo Mondi
5 siblings, 3 replies; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-28 21:06 UTC (permalink / raw)
To: linux-media, libcamera-devel
Cc: Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart
A sub-device device node can be registered in user space only if the
CONFIG_V4L2_SUBDEV_API Kconfig option is selected.
Remove checks from the v4l2-subdev file handle open/close functions and
ioctl handler as they are only accessible if a device node was registered
to user space in first place.
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
drivers/media/v4l2-core/v4l2-subdev.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 1dc263c2ca0a..f3fe515b8ccb 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -24,22 +24,19 @@
static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
{
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
if (sd->entity.num_pads) {
fh->pad = v4l2_subdev_alloc_pad_config(sd);
if (fh->pad == NULL)
return -ENOMEM;
}
-#endif
+
return 0;
}
static void subdev_fh_free(struct v4l2_subdev_fh *fh)
{
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
v4l2_subdev_free_pad_config(fh->pad);
fh->pad = NULL;
-#endif
}
static int subdev_open(struct file *file)
@@ -329,10 +326,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
struct video_device *vdev = video_devdata(file);
struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
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;
switch (cmd) {
@@ -466,7 +461,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
return ret;
}
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
case VIDIOC_SUBDEV_G_FMT: {
struct v4l2_subdev_format *format = arg;
@@ -646,7 +640,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
case VIDIOC_SUBDEV_QUERYSTD:
return v4l2_subdev_call(sd, video, querystd, arg);
-#endif
+
default:
return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
}
--
2.26.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
2020-04-28 21:06 ` [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected Jacopo Mondi
@ 2020-04-28 21:26 ` Sakari Ailus
2020-04-29 7:02 ` Jacopo Mondi
2020-04-28 23:44 ` kbuild test robot
2020-04-29 8:58 ` [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr Jacopo Mondi
2 siblings, 1 reply; 24+ messages in thread
From: Sakari Ailus @ 2020-04-28 21:26 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart
Hi Jacopo,
On Tue, Apr 28, 2020 at 11:06:07PM +0200, Jacopo Mondi wrote:
> A sub-device device node can be registered in user space only if the
> CONFIG_V4L2_SUBDEV_API Kconfig option is selected.
>
> Remove checks from the v4l2-subdev file handle open/close functions and
> ioctl handler as they are only accessible if a device node was registered
> to user space in first place.
Is there other motivation with this than clean up things a little?
The change increases the binary size marginally if the Kconfig option is
disabled.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1dc263c2ca0a..f3fe515b8ccb 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -24,22 +24,19 @@
>
> static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> {
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> if (sd->entity.num_pads) {
> fh->pad = v4l2_subdev_alloc_pad_config(sd);
> if (fh->pad == NULL)
> return -ENOMEM;
> }
> -#endif
> +
> return 0;
> }
>
> static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> {
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> v4l2_subdev_free_pad_config(fh->pad);
> fh->pad = NULL;
> -#endif
> }
>
> static int subdev_open(struct file *file)
> @@ -329,10 +326,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> struct video_device *vdev = video_devdata(file);
> struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> 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;
>
> switch (cmd) {
> @@ -466,7 +461,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> return ret;
> }
>
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> case VIDIOC_SUBDEV_G_FMT: {
> struct v4l2_subdev_format *format = arg;
>
> @@ -646,7 +640,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> case VIDIOC_SUBDEV_QUERYSTD:
> return v4l2_subdev_call(sd, video, querystd, arg);
> -#endif
> +
> default:
> return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> }
> --
> 2.26.1
>
--
Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
2020-04-28 21:26 ` Sakari Ailus
@ 2020-04-29 7:02 ` Jacopo Mondi
2020-04-29 8:27 ` Sakari Ailus
0 siblings, 1 reply; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-29 7:02 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart
Hi Sakari,
On Wed, Apr 29, 2020 at 12:26:43AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Tue, Apr 28, 2020 at 11:06:07PM +0200, Jacopo Mondi wrote:
> > A sub-device device node can be registered in user space only if the
> > CONFIG_V4L2_SUBDEV_API Kconfig option is selected.
> >
> > Remove checks from the v4l2-subdev file handle open/close functions and
> > ioctl handler as they are only accessible if a device node was registered
> > to user space in first place.
>
> Is there other motivation with this than clean up things a little?
>
I had to add yet-another #if defined and I got fed up. If you don't
have a device node registered you won't be able to access any of the
functions where the existing #if defined() where placed.
> The change increases the binary size marginally if the Kconfig option is
> disabled.
>
I see. Should we instead guard the whole file handle operations and
ioctl handler instead of having #if defined() spread inside them ? I
assume they where there as leftover, as I'm still missing the point,
give that, as said, without V4L2_SUBDEV_API, you can't register any
device node to userspace..
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 10 ++--------
> > 1 file changed, 2 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 1dc263c2ca0a..f3fe515b8ccb 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -24,22 +24,19 @@
> >
> > static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> > {
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > if (sd->entity.num_pads) {
> > fh->pad = v4l2_subdev_alloc_pad_config(sd);
> > if (fh->pad == NULL)
> > return -ENOMEM;
> > }
> > -#endif
> > +
> > return 0;
> > }
> >
> > static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> > {
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > v4l2_subdev_free_pad_config(fh->pad);
> > fh->pad = NULL;
> > -#endif
> > }
> >
> > static int subdev_open(struct file *file)
> > @@ -329,10 +326,8 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > struct video_device *vdev = video_devdata(file);
> > struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> > 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;
> >
> > switch (cmd) {
> > @@ -466,7 +461,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > return ret;
> > }
> >
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > case VIDIOC_SUBDEV_G_FMT: {
> > struct v4l2_subdev_format *format = arg;
> >
> > @@ -646,7 +640,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >
> > case VIDIOC_SUBDEV_QUERYSTD:
> > return v4l2_subdev_call(sd, video, querystd, arg);
> > -#endif
> > +
> > default:
> > return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> > }
> > --
> > 2.26.1
> >
>
> --
> Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
2020-04-29 7:02 ` Jacopo Mondi
@ 2020-04-29 8:27 ` Sakari Ailus
2020-04-29 8:43 ` Jacopo Mondi
0 siblings, 1 reply; 24+ messages in thread
From: Sakari Ailus @ 2020-04-29 8:27 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart
Hi Jacopo,
On Wed, Apr 29, 2020 at 09:02:15AM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Wed, Apr 29, 2020 at 12:26:43AM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Tue, Apr 28, 2020 at 11:06:07PM +0200, Jacopo Mondi wrote:
> > > A sub-device device node can be registered in user space only if the
> > > CONFIG_V4L2_SUBDEV_API Kconfig option is selected.
> > >
> > > Remove checks from the v4l2-subdev file handle open/close functions and
> > > ioctl handler as they are only accessible if a device node was registered
> > > to user space in first place.
> >
> > Is there other motivation with this than clean up things a little?
> >
>
> I had to add yet-another #if defined and I got fed up. If you don't
> have a device node registered you won't be able to access any of the
> functions where the existing #if defined() where placed.
>
> > The change increases the binary size marginally if the Kconfig option is
> > disabled.
> >
>
> I see. Should we instead guard the whole file handle operations and
> ioctl handler instead of having #if defined() spread inside them ? I
> assume they where there as leftover, as I'm still missing the point,
> give that, as said, without V4L2_SUBDEV_API, you can't register any
> device node to userspace..
I think that's why those #ifdefs have been originally put there --- it's
just dead code without the subdev nodes, and the compiler won't be able to
figure this out.
But it seems, later on, when people have added code that supports
sub-device nodes, no #ifdefs have been added.
I think I'd make sense to remove the current #ifdefs and add dummy ops for
everything where needed that truly depends on that Kconfig option (i.e.
sub-device nodes). Or just to remove these, as your patch does. It's not a
lot of code.
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
2020-04-29 8:27 ` Sakari Ailus
@ 2020-04-29 8:43 ` Jacopo Mondi
0 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-29 8:43 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart
Hi Sakari
On Wed, Apr 29, 2020 at 11:27:37AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Apr 29, 2020 at 09:02:15AM +0200, Jacopo Mondi wrote:
> > Hi Sakari,
> >
> > On Wed, Apr 29, 2020 at 12:26:43AM +0300, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Tue, Apr 28, 2020 at 11:06:07PM +0200, Jacopo Mondi wrote:
> > > > A sub-device device node can be registered in user space only if the
> > > > CONFIG_V4L2_SUBDEV_API Kconfig option is selected.
> > > >
> > > > Remove checks from the v4l2-subdev file handle open/close functions and
> > > > ioctl handler as they are only accessible if a device node was registered
> > > > to user space in first place.
> > >
> > > Is there other motivation with this than clean up things a little?
> > >
> >
> > I had to add yet-another #if defined and I got fed up. If you don't
> > have a device node registered you won't be able to access any of the
> > functions where the existing #if defined() where placed.
> >
> > > The change increases the binary size marginally if the Kconfig option is
> > > disabled.
> > >
> >
> > I see. Should we instead guard the whole file handle operations and
> > ioctl handler instead of having #if defined() spread inside them ? I
> > assume they where there as leftover, as I'm still missing the point,
> > give that, as said, without V4L2_SUBDEV_API, you can't register any
> > device node to userspace..
>
> I think that's why those #ifdefs have been originally put there --- it's
> just dead code without the subdev nodes, and the compiler won't be able to
> figure this out.
>
> But it seems, later on, when people have added code that supports
> sub-device nodes, no #ifdefs have been added.
>
> I think I'd make sense to remove the current #ifdefs and add dummy ops for
> everything where needed that truly depends on that Kconfig option (i.e.
> sub-device nodes). Or just to remove these, as your patch does. It's not a
> lot of code.
that's what I've done now, as soon as I've run a few compile test, I'll send
a new version.
Thanks
j
>
> --
> Kind regards,
>
> Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
2020-04-28 21:06 ` [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected Jacopo Mondi
@ 2020-04-28 23:44 ` kbuild test robot
2020-04-28 23:44 ` kbuild test robot
2020-04-29 8:58 ` [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr Jacopo Mondi
2 siblings, 0 replies; 24+ messages in thread
From: kbuild test robot @ 2020-04-28 23:44 UTC (permalink / raw)
To: Jacopo Mondi, linux-media, libcamera-devel
Cc: kbuild-all, Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart
[-- Attachment #1: Type: text/plain, Size: 9873 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-rc3 next-20200428]
[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/20200429-062133
base: git://linuxtv.org/media_tree.git master
config: arm-at91_dt_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 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: In function 'subdev_fh_init':
>> drivers/media/v4l2-core/v4l2-subdev.c:28:5: error: 'struct v4l2_subdev_fh' has no member named 'pad'
28 | fh->pad = v4l2_subdev_alloc_pad_config(sd);
| ^~
drivers/media/v4l2-core/v4l2-subdev.c:29:9: error: 'struct v4l2_subdev_fh' has no member named 'pad'
29 | if (fh->pad == NULL)
| ^~
drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_fh_free':
drivers/media/v4l2-core/v4l2-subdev.c:38:32: error: 'struct v4l2_subdev_fh' has no member named 'pad'
38 | v4l2_subdev_free_pad_config(fh->pad);
| ^~
drivers/media/v4l2-core/v4l2-subdev.c:39:4: error: 'struct v4l2_subdev_fh' has no member named 'pad'
39 | fh->pad = NULL;
| ^~
In file included from include/media/v4l2-device.h:13,
from drivers/media/v4l2-core/v4l2-subdev.c:20:
drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_do_ioctl':
drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
494 | sd, pad, get_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
494 | sd, pad, get_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
516 | sd, pad, set_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
516 | sd, pad, set_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:569:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
vim +28 drivers/media/v4l2-core/v4l2-subdev.c
2096a5dcf9704f drivers/media/video/v4l2-subdev.c Laurent Pinchart 2009-12-09 24
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 25 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 26 {
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 27 if (sd->entity.num_pads) {
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 @28 fh->pad = v4l2_subdev_alloc_pad_config(sd);
ae184cda8d0eeb drivers/media/video/v4l2-subdev.c Sakari Ailus 2011-10-14 29 if (fh->pad == NULL)
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 30 return -ENOMEM;
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 31 }
b9e6aad3939a62 drivers/media/v4l2-core/v4l2-subdev.c Jacopo Mondi 2020-04-28 32
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 33 return 0;
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 34 }
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 35
:::::: The code at line 28 was first introduced by commit
:::::: 9b02cbb3ede89b5cd84bbe4ef493bd130d76b070 [media] v4l: subdev: Add pad config allocator and init
:::::: TO: Laurent Pinchart <laurent.pinchart@linaro.org>
:::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
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: 28588 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
@ 2020-04-28 23:44 ` kbuild test robot
0 siblings, 0 replies; 24+ messages in thread
From: kbuild test robot @ 2020-04-28 23:44 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 10027 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-rc3 next-20200428]
[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/20200429-062133
base: git://linuxtv.org/media_tree.git master
config: arm-at91_dt_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 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: In function 'subdev_fh_init':
>> drivers/media/v4l2-core/v4l2-subdev.c:28:5: error: 'struct v4l2_subdev_fh' has no member named 'pad'
28 | fh->pad = v4l2_subdev_alloc_pad_config(sd);
| ^~
drivers/media/v4l2-core/v4l2-subdev.c:29:9: error: 'struct v4l2_subdev_fh' has no member named 'pad'
29 | if (fh->pad == NULL)
| ^~
drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_fh_free':
drivers/media/v4l2-core/v4l2-subdev.c:38:32: error: 'struct v4l2_subdev_fh' has no member named 'pad'
38 | v4l2_subdev_free_pad_config(fh->pad);
| ^~
drivers/media/v4l2-core/v4l2-subdev.c:39:4: error: 'struct v4l2_subdev_fh' has no member named 'pad'
39 | fh->pad = NULL;
| ^~
In file included from include/media/v4l2-device.h:13,
from drivers/media/v4l2-core/v4l2-subdev.c:20:
drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_do_ioctl':
drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
494 | sd, pad, get_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
494 | sd, pad, get_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
516 | sd, pad, set_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
516 | sd, pad, set_selection, subdev_fh->pad, &sel);
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
1111 | __sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
| ^~
include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
1113 | __result = __sd->ops->o->f(__sd, ##args); \
| ^~~~
drivers/media/v4l2-core/v4l2-subdev.c:569:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
vim +28 drivers/media/v4l2-core/v4l2-subdev.c
2096a5dcf9704f drivers/media/video/v4l2-subdev.c Laurent Pinchart 2009-12-09 24
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 25 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 26 {
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 27 if (sd->entity.num_pads) {
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 @28 fh->pad = v4l2_subdev_alloc_pad_config(sd);
ae184cda8d0eeb drivers/media/video/v4l2-subdev.c Sakari Ailus 2011-10-14 29 if (fh->pad == NULL)
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 30 return -ENOMEM;
9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 31 }
b9e6aad3939a62 drivers/media/v4l2-core/v4l2-subdev.c Jacopo Mondi 2020-04-28 32
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 33 return 0;
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 34 }
7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 35
:::::: The code at line 28 was first introduced by commit
:::::: 9b02cbb3ede89b5cd84bbe4ef493bd130d76b070 [media] v4l: subdev: Add pad config allocator and init
:::::: TO: Laurent Pinchart <laurent.pinchart@linaro.org>
:::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 28588 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected
2020-04-28 23:44 ` kbuild test robot
(?)
@ 2020-04-29 7:04 ` Jacopo Mondi
-1 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-29 7:04 UTC (permalink / raw)
To: kbuild test robot
Cc: linux-media, libcamera-devel, kbuild-all, mchehab,
hverkuil-cisco, sakari.ailus, andrey.konovalov, laurent.pinchart
Here it goes -.-'
I was hoping to have this going through kbuild soon
Would guarding the whole file handle operations and the ioctl handler
a best option then ?
On Wed, Apr 29, 2020 at 07:44:30AM +0800, kbuild test robot wrote:
> 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-rc3 next-20200428]
> [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/20200429-062133
> base: git://linuxtv.org/media_tree.git master
> config: arm-at91_dt_defconfig (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 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: In function 'subdev_fh_init':
> >> drivers/media/v4l2-core/v4l2-subdev.c:28:5: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 28 | fh->pad = v4l2_subdev_alloc_pad_config(sd);
> | ^~
> drivers/media/v4l2-core/v4l2-subdev.c:29:9: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 29 | if (fh->pad == NULL)
> | ^~
> drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_fh_free':
> drivers/media/v4l2-core/v4l2-subdev.c:38:32: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 38 | v4l2_subdev_free_pad_config(fh->pad);
> | ^~
> drivers/media/v4l2-core/v4l2-subdev.c:39:4: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 39 | fh->pad = NULL;
> | ^~
> In file included from include/media/v4l2-device.h:13,
> from drivers/media/v4l2-core/v4l2-subdev.c:20:
> drivers/media/v4l2-core/v4l2-subdev.c: In function 'subdev_do_ioctl':
> drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
> | ^~
> include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
> 1111 | __sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:469:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 469 | return v4l2_subdev_call(sd, pad, get_fmt, subdev_fh->pad, format);
> | ^~
> include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
> 1113 | __result = __sd->ops->o->f(__sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
> | ^~
> include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
> 1111 | __sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:480:54: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 480 | return v4l2_subdev_call(sd, pad, set_fmt, subdev_fh->pad, format);
> | ^~
> include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
> 1113 | __result = __sd->ops->o->f(__sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 494 | sd, pad, get_selection, subdev_fh->pad, &sel);
> | ^~
> include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
> 1111 | __sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:494:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 494 | sd, pad, get_selection, subdev_fh->pad, &sel);
> | ^~
> include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
> 1113 | __result = __sd->ops->o->f(__sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 516 | sd, pad, set_selection, subdev_fh->pad, &sel);
> | ^~
> include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
> 1111 | __sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:516:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 516 | sd, pad, set_selection, subdev_fh->pad, &sel);
> | ^~
> include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
> 1113 | __result = __sd->ops->o->f(__sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
> | ^~
> include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
> 1111 | __sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:527:61: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 527 | return v4l2_subdev_call(sd, pad, enum_mbus_code, subdev_fh->pad,
> | ^~
> include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
> 1113 | __result = __sd->ops->o->f(__sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
> | ^~
> include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
> 1111 | __sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:535:62: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 535 | return v4l2_subdev_call(sd, pad, enum_frame_size, subdev_fh->pad,
> | ^~
> include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
> 1113 | __result = __sd->ops->o->f(__sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
> | ^~
> include/media/v4l2-subdev.h:1111:16: note: in definition of macro 'v4l2_subdev_call'
> 1111 | __sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:560:66: error: 'struct v4l2_subdev_fh' has no member named 'pad'
> 560 | return v4l2_subdev_call(sd, pad, enum_frame_interval, subdev_fh->pad,
> | ^~
> include/media/v4l2-subdev.h:1113:39: note: in definition of macro 'v4l2_subdev_call'
> 1113 | __result = __sd->ops->o->f(__sd, ##args); \
> | ^~~~
> drivers/media/v4l2-core/v4l2-subdev.c:569:37: error: 'struct v4l2_subdev_fh' has no member named 'pad'
>
> vim +28 drivers/media/v4l2-core/v4l2-subdev.c
>
> 2096a5dcf9704f drivers/media/video/v4l2-subdev.c Laurent Pinchart 2009-12-09 24
> 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 25 static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 26 {
> 9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 27 if (sd->entity.num_pads) {
> 9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 @28 fh->pad = v4l2_subdev_alloc_pad_config(sd);
> ae184cda8d0eeb drivers/media/video/v4l2-subdev.c Sakari Ailus 2011-10-14 29 if (fh->pad == NULL)
> 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 30 return -ENOMEM;
> 9b02cbb3ede89b drivers/media/v4l2-core/v4l2-subdev.c Laurent Pinchart 2015-04-24 31 }
> b9e6aad3939a62 drivers/media/v4l2-core/v4l2-subdev.c Jacopo Mondi 2020-04-28 32
> 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 33 return 0;
> 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 34 }
> 7cd5a16b22af7d drivers/media/video/v4l2-subdev.c Stanimir Varbanov 2010-05-21 35
>
> :::::: The code at line 28 was first introduced by commit
> :::::: 9b02cbb3ede89b5cd84bbe4ef493bd130d76b070 [media] v4l: subdev: Add pad config allocator and init
>
> :::::: TO: Laurent Pinchart <laurent.pinchart@linaro.org>
> :::::: CC: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr
2020-04-28 21:06 ` [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected Jacopo Mondi
2020-04-28 21:26 ` Sakari Ailus
2020-04-28 23:44 ` kbuild test robot
@ 2020-04-29 8:58 ` Jacopo Mondi
2020-04-29 9:49 ` Sakari Ailus
2 siblings, 1 reply; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-29 8:58 UTC (permalink / raw)
To: linux-media, libcamera-devel
Cc: Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart
A sub-device device node can be registered in user space only if the
CONFIG_V4L2_SUBDEV_API Kconfig option is selected. Currently the
open/close file operations and the ioctl handler have some parts of their
implementations guarded by #if defined(CONFIG_V4L2_SUBDEV_API), while
they are actually not accessible without a video device node registered
to user space.
Guard the whole open, close and ioctl handler and provide stubs if the
V4L2_SUBDEV_API Kconfig option is not selected.
This slightly reduces the kernel size when the option is not selected
and simplifies the file ops and ioctl implementations.
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
A different approach compared to v5, which was anyway buggy and not a proper
solution.
Sending out for comments, while waiting for consensus on v5 [5/6] (reserved
space in the ioctl argument vs versioning based on structure size)
Compile tested with and without V4L2_SUBDEV_API Kconfig option enabled and
with drivers that depends on it built-in or as modules.
---
drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++------
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 1dc263c2ca0a..6fef52880c99 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -22,24 +22,22 @@
#include <media/v4l2-fh.h>
#include <media/v4l2-event.h>
+#if defined(CONFIG_V4L2_SUBDEV_API)
static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
{
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
if (sd->entity.num_pads) {
fh->pad = v4l2_subdev_alloc_pad_config(sd);
if (fh->pad == NULL)
return -ENOMEM;
}
-#endif
+
return 0;
}
static void subdev_fh_free(struct v4l2_subdev_fh *fh)
{
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
v4l2_subdev_free_pad_config(fh->pad);
fh->pad = NULL;
-#endif
}
static int subdev_open(struct file *file)
@@ -111,6 +109,17 @@ static int subdev_close(struct file *file)
return 0;
}
+#else /* CONFIG_V4L2_SUBDEV_API */
+static int subdev_open(struct file *file)
+{
+ return 0;
+}
+
+static int subdev_close(struct file *file)
+{
+ return 0;
+}
+#endif /* CONFIG_V4L2_SUBDEV_API */
static inline int check_which(u32 which)
{
@@ -324,15 +333,14 @@ const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
};
EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
+#if defined(CONFIG_V4L2_SUBDEV_API)
static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
{
struct video_device *vdev = video_devdata(file);
struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
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;
switch (cmd) {
@@ -466,7 +474,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
return ret;
}
-#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
case VIDIOC_SUBDEV_G_FMT: {
struct v4l2_subdev_format *format = arg;
@@ -646,7 +653,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
case VIDIOC_SUBDEV_QUERYSTD:
return v4l2_subdev_call(sd, video, querystd, arg);
-#endif
+
default:
return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
}
@@ -686,6 +693,22 @@ static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
}
#endif
+#else /* CONFIG_V4L2_SUBDEV_API */
+static long subdev_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return 0;
+}
+
+#ifdef CONFIG_COMPAT
+static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return 0;
+}
+#endif
+#endif /* CONFIG_V4L2_SUBDEV_API */
+
static __poll_t subdev_poll(struct file *file, poll_table *wait)
{
struct video_device *vdev = video_devdata(file);
--
2.26.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr
2020-04-29 8:58 ` [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr Jacopo Mondi
@ 2020-04-29 9:49 ` Sakari Ailus
2020-04-29 10:16 ` Jacopo Mondi
0 siblings, 1 reply; 24+ messages in thread
From: Sakari Ailus @ 2020-04-29 9:49 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart
Hi Jacopo,
Thanks for the update.
On Wed, Apr 29, 2020 at 10:58:55AM +0200, Jacopo Mondi wrote:
> A sub-device device node can be registered in user space only if the
> CONFIG_V4L2_SUBDEV_API Kconfig option is selected. Currently the
> open/close file operations and the ioctl handler have some parts of their
> implementations guarded by #if defined(CONFIG_V4L2_SUBDEV_API), while
> they are actually not accessible without a video device node registered
> to user space.
>
> Guard the whole open, close and ioctl handler and provide stubs if the
> V4L2_SUBDEV_API Kconfig option is not selected.
>
> This slightly reduces the kernel size when the option is not selected
> and simplifies the file ops and ioctl implementations.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> A different approach compared to v5, which was anyway buggy and not a proper
> solution.
>
> Sending out for comments, while waiting for consensus on v5 [5/6] (reserved
> space in the ioctl argument vs versioning based on structure size)
>
> Compile tested with and without V4L2_SUBDEV_API Kconfig option enabled and
> with drivers that depends on it built-in or as modules.
>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++------
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1dc263c2ca0a..6fef52880c99 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -22,24 +22,22 @@
> #include <media/v4l2-fh.h>
> #include <media/v4l2-event.h>
>
> +#if defined(CONFIG_V4L2_SUBDEV_API)
> static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> {
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> if (sd->entity.num_pads) {
> fh->pad = v4l2_subdev_alloc_pad_config(sd);
> if (fh->pad == NULL)
> return -ENOMEM;
> }
> -#endif
> +
> return 0;
> }
>
> static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> {
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> v4l2_subdev_free_pad_config(fh->pad);
> fh->pad = NULL;
> -#endif
> }
>
> static int subdev_open(struct file *file)
> @@ -111,6 +109,17 @@ static int subdev_close(struct file *file)
>
> return 0;
> }
> +#else /* CONFIG_V4L2_SUBDEV_API */
> +static int subdev_open(struct file *file)
> +{
> + return 0;
Perhaps:
return -ENODEV;
And I'd use inline functions in the header.
> +}
> +
> +static int subdev_close(struct file *file)
> +{
> + return 0;
> +}
> +#endif /* CONFIG_V4L2_SUBDEV_API */
>
> static inline int check_which(u32 which)
> {
> @@ -324,15 +333,14 @@ const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
> };
> EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
>
> +#if defined(CONFIG_V4L2_SUBDEV_API)
> static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> {
> struct video_device *vdev = video_devdata(file);
> struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> 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;
>
> switch (cmd) {
> @@ -466,7 +474,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> return ret;
> }
>
> -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> case VIDIOC_SUBDEV_G_FMT: {
> struct v4l2_subdev_format *format = arg;
>
> @@ -646,7 +653,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>
> case VIDIOC_SUBDEV_QUERYSTD:
> return v4l2_subdev_call(sd, video, querystd, arg);
> -#endif
> +
> default:
> return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> }
> @@ -686,6 +693,22 @@ static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
> }
> #endif
>
> +#else /* CONFIG_V4L2_SUBDEV_API */
> +static long subdev_ioctl(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + return 0;
return -ENOTTY;
> +}
> +
> +#ifdef CONFIG_COMPAT
> +static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
> + unsigned long arg)
> +{
> + return 0;
Ditto.
> +}
> +#endif
> +#endif /* CONFIG_V4L2_SUBDEV_API */
> +
> static __poll_t subdev_poll(struct file *file, poll_table *wait)
> {
> struct video_device *vdev = video_devdata(file);
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr
2020-04-29 9:49 ` Sakari Ailus
@ 2020-04-29 10:16 ` Jacopo Mondi
2020-04-29 11:00 ` Sakari Ailus
0 siblings, 1 reply; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-29 10:16 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart
Hi Sakari,
On Wed, Apr 29, 2020 at 12:49:49PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> Thanks for the update.
>
> On Wed, Apr 29, 2020 at 10:58:55AM +0200, Jacopo Mondi wrote:
> > A sub-device device node can be registered in user space only if the
> > CONFIG_V4L2_SUBDEV_API Kconfig option is selected. Currently the
> > open/close file operations and the ioctl handler have some parts of their
> > implementations guarded by #if defined(CONFIG_V4L2_SUBDEV_API), while
> > they are actually not accessible without a video device node registered
> > to user space.
> >
> > Guard the whole open, close and ioctl handler and provide stubs if the
> > V4L2_SUBDEV_API Kconfig option is not selected.
> >
> > This slightly reduces the kernel size when the option is not selected
> > and simplifies the file ops and ioctl implementations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > A different approach compared to v5, which was anyway buggy and not a proper
> > solution.
> >
> > Sending out for comments, while waiting for consensus on v5 [5/6] (reserved
> > space in the ioctl argument vs versioning based on structure size)
> >
> > Compile tested with and without V4L2_SUBDEV_API Kconfig option enabled and
> > with drivers that depends on it built-in or as modules.
> >
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++------
> > 1 file changed, 31 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 1dc263c2ca0a..6fef52880c99 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -22,24 +22,22 @@
> > #include <media/v4l2-fh.h>
> > #include <media/v4l2-event.h>
> >
> > +#if defined(CONFIG_V4L2_SUBDEV_API)
> > static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> > {
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > if (sd->entity.num_pads) {
> > fh->pad = v4l2_subdev_alloc_pad_config(sd);
> > if (fh->pad == NULL)
> > return -ENOMEM;
> > }
> > -#endif
> > +
> > return 0;
> > }
> >
> > static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> > {
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > v4l2_subdev_free_pad_config(fh->pad);
> > fh->pad = NULL;
> > -#endif
> > }
> >
> > static int subdev_open(struct file *file)
> > @@ -111,6 +109,17 @@ static int subdev_close(struct file *file)
> >
> > return 0;
> > }
> > +#else /* CONFIG_V4L2_SUBDEV_API */
> > +static int subdev_open(struct file *file)
> > +{
> > + return 0;
>
> Perhaps:
>
> return -ENODEV;
>
> And I'd use inline functions in the header.
There is no way this functions gets called if no device node is
registered to userspace, I think. These are only here to please the
compiler. Am I mistaken ?
Also, these function are not exported by any headers, but only the
file_operations structure that contains them is:
include/media/v4l2-subdev.h
extern const struct v4l2_file_operations v4l2_subdev_fops;
>
> > +}
> > +
> > +static int subdev_close(struct file *file)
> > +{
> > + return 0;
> > +}
> > +#endif /* CONFIG_V4L2_SUBDEV_API */
> >
> > static inline int check_which(u32 which)
> > {
> > @@ -324,15 +333,14 @@ const struct v4l2_subdev_ops v4l2_subdev_call_wrappers = {
> > };
> > EXPORT_SYMBOL(v4l2_subdev_call_wrappers);
> >
> > +#if defined(CONFIG_V4L2_SUBDEV_API)
> > static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > {
> > struct video_device *vdev = video_devdata(file);
> > struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
> > 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;
> >
> > switch (cmd) {
> > @@ -466,7 +474,6 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > return ret;
> > }
> >
> > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > case VIDIOC_SUBDEV_G_FMT: {
> > struct v4l2_subdev_format *format = arg;
> >
> > @@ -646,7 +653,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >
> > case VIDIOC_SUBDEV_QUERYSTD:
> > return v4l2_subdev_call(sd, video, querystd, arg);
> > -#endif
> > +
> > default:
> > return v4l2_subdev_call(sd, core, ioctl, cmd, arg);
> > }
> > @@ -686,6 +693,22 @@ static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
> > }
> > #endif
> >
> > +#else /* CONFIG_V4L2_SUBDEV_API */
> > +static long subdev_ioctl(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return 0;
>
> return -ENOTTY;
>
> > +}
> > +
> > +#ifdef CONFIG_COMPAT
> > +static long subdev_compat_ioctl32(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return 0;
>
> Ditto.
>
> > +}
> > +#endif
> > +#endif /* CONFIG_V4L2_SUBDEV_API */
> > +
> > static __poll_t subdev_poll(struct file *file, poll_table *wait)
> > {
> > struct video_device *vdev = video_devdata(file);
>
> --
> Kind regards,
>
> Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5.1] media: v4l2-subdev: Guard whole fops and ioctl hdlr
2020-04-29 10:16 ` Jacopo Mondi
@ 2020-04-29 11:00 ` Sakari Ailus
0 siblings, 0 replies; 24+ messages in thread
From: Sakari Ailus @ 2020-04-29 11:00 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart
On Wed, Apr 29, 2020 at 12:16:00PM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Wed, Apr 29, 2020 at 12:49:49PM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > Thanks for the update.
> >
> > On Wed, Apr 29, 2020 at 10:58:55AM +0200, Jacopo Mondi wrote:
> > > A sub-device device node can be registered in user space only if the
> > > CONFIG_V4L2_SUBDEV_API Kconfig option is selected. Currently the
> > > open/close file operations and the ioctl handler have some parts of their
> > > implementations guarded by #if defined(CONFIG_V4L2_SUBDEV_API), while
> > > they are actually not accessible without a video device node registered
> > > to user space.
> > >
> > > Guard the whole open, close and ioctl handler and provide stubs if the
> > > V4L2_SUBDEV_API Kconfig option is not selected.
> > >
> > > This slightly reduces the kernel size when the option is not selected
> > > and simplifies the file ops and ioctl implementations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > A different approach compared to v5, which was anyway buggy and not a proper
> > > solution.
> > >
> > > Sending out for comments, while waiting for consensus on v5 [5/6] (reserved
> > > space in the ioctl argument vs versioning based on structure size)
> > >
> > > Compile tested with and without V4L2_SUBDEV_API Kconfig option enabled and
> > > with drivers that depends on it built-in or as modules.
> > >
> > > ---
> > > drivers/media/v4l2-core/v4l2-subdev.c | 39 +++++++++++++++++++++------
> > > 1 file changed, 31 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 1dc263c2ca0a..6fef52880c99 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -22,24 +22,22 @@
> > > #include <media/v4l2-fh.h>
> > > #include <media/v4l2-event.h>
> > >
> > > +#if defined(CONFIG_V4L2_SUBDEV_API)
> > > static int subdev_fh_init(struct v4l2_subdev_fh *fh, struct v4l2_subdev *sd)
> > > {
> > > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > if (sd->entity.num_pads) {
> > > fh->pad = v4l2_subdev_alloc_pad_config(sd);
> > > if (fh->pad == NULL)
> > > return -ENOMEM;
> > > }
> > > -#endif
> > > +
> > > return 0;
> > > }
> > >
> > > static void subdev_fh_free(struct v4l2_subdev_fh *fh)
> > > {
> > > -#if defined(CONFIG_VIDEO_V4L2_SUBDEV_API)
> > > v4l2_subdev_free_pad_config(fh->pad);
> > > fh->pad = NULL;
> > > -#endif
> > > }
> > >
> > > static int subdev_open(struct file *file)
> > > @@ -111,6 +109,17 @@ static int subdev_close(struct file *file)
> > >
> > > return 0;
> > > }
> > > +#else /* CONFIG_V4L2_SUBDEV_API */
> > > +static int subdev_open(struct file *file)
> > > +{
> > > + return 0;
> >
> > Perhaps:
> >
> > return -ENODEV;
> >
> > And I'd use inline functions in the header.
Um, as they're only used here, they indeed should remain here, not in
the header. I missed that while commenting.
>
> There is no way this functions gets called if no device node is
> registered to userspace, I think. These are only here to please the
> compiler. Am I mistaken ?
The fops struct is still there. It shouldn't be called, no, but if that
happens, then providing the right return value is helpful.
>
> Also, these function are not exported by any headers, but only the
> file_operations structure that contains them is:
>
> include/media/v4l2-subdev.h
> extern const struct v4l2_file_operations v4l2_subdev_fops;
Right, agreed.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
2020-04-28 21:06 [PATCH v5 0/6] media: Register read-only sub-dev devnode Jacopo Mondi
` (3 preceding siblings ...)
2020-04-28 21:06 ` [PATCH v5 4/6] media: v4l2-subdev: Assume V4L2_SUBDEV_API is selected Jacopo Mondi
@ 2020-04-28 21:06 ` Jacopo Mondi
2020-04-28 21:28 ` Sakari Ailus
2020-04-28 21:06 ` [PATCH v5 6/6] v4l: document VIDIOC_SUBDEV_QUERYCAP Jacopo Mondi
5 siblings, 1 reply; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-28 21:06 UTC (permalink / raw)
To: linux-media, libcamera-devel
Cc: Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart, 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 subdev_caps of the
subdevice. Define as the initial set of subdev_caps the read-only or
read/write flags, to signal to userspace which set of IOCTLs are
available on the subdevice.
Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++
2 files changed, 27 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f3fe515b8ccb..b8c0071aa4d0 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -15,6 +15,7 @@
#include <linux/types.h>
#include <linux/videodev2.h>
#include <linux/export.h>
+#include <linux/version.h>
#include <media/v4l2-ctrls.h>
#include <media/v4l2-device.h>
@@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
int rval;
switch (cmd) {
+ case VIDIOC_SUBDEV_QUERYCAP: {
+ struct v4l2_subdev_capability *cap = arg;
+
+ memset(cap, 0, sizeof(*cap));
+ cap->version = LINUX_VERSION_CODE;
+ cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
+ : V4L2_SUBDEV_CAP_RW_SUBDEV;
+
+ return 0;
+ }
+
case VIDIOC_QUERYCTRL:
/*
* TODO: this really should be folded into v4l2_queryctrl (this
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
index 03970ce30741..89dc8f2ba6b3 100644
--- a/include/uapi/linux/v4l2-subdev.h
+++ b/include/uapi/linux/v4l2-subdev.h
@@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
__u32 reserved[8];
};
+/**
+ * struct v4l2_subdev_capability - subdev capabilities
+ * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
+ */
+struct v4l2_subdev_capability {
+ __u32 version;
+ __u32 subdev_caps;
+};
+
+/* The v4l2 sub-device video device node is registered in read-only mode. */
+#define V4L2_SUBDEV_CAP_RO_SUBDEV BIT(0)
+/* The v4l2 sub-device video device node is registered in read/write mode. */
+#define V4L2_SUBDEV_CAP_RW_SUBDEV BIT(1)
+
/* Backwards compatibility define --- to be removed */
#define v4l2_subdev_edid v4l2_edid
+#define VIDIOC_SUBDEV_QUERYCAP _IOR('V', 0, struct v4l2_subdev_capability)
#define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format)
#define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format)
#define VIDIOC_SUBDEV_G_FRAME_INTERVAL _IOWR('V', 21, struct v4l2_subdev_frame_interval)
--
2.26.1
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
2020-04-28 21:06 ` [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl Jacopo Mondi
@ 2020-04-28 21:28 ` Sakari Ailus
2020-04-29 8:09 ` Jacopo Mondi
0 siblings, 1 reply; 24+ messages in thread
From: Sakari Ailus @ 2020-04-28 21:28 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart, Hans Verkuil
Hi Jacopo,
On Tue, Apr 28, 2020 at 11:06:08PM +0200, Jacopo Mondi wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl
> that apps can call to determine that it is indeed a V4L2 device, there
> is currently no equivalent for v4l-subdev nodes. Adding this ioctl will
> solve that, and it will allow utilities like v4l2-compliance to be used
> with these devices as well.
>
> SUBDEV_QUERYCAP currently returns the version and subdev_caps of the
> subdevice. Define as the initial set of subdev_caps the read-only or
> read/write flags, to signal to userspace which set of IOCTLs are
> available on the subdevice.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
> include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++
> 2 files changed, 27 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index f3fe515b8ccb..b8c0071aa4d0 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -15,6 +15,7 @@
> #include <linux/types.h>
> #include <linux/videodev2.h>
> #include <linux/export.h>
> +#include <linux/version.h>
>
> #include <media/v4l2-ctrls.h>
> #include <media/v4l2-device.h>
> @@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> int rval;
>
> switch (cmd) {
> + case VIDIOC_SUBDEV_QUERYCAP: {
> + struct v4l2_subdev_capability *cap = arg;
> +
> + memset(cap, 0, sizeof(*cap));
> + cap->version = LINUX_VERSION_CODE;
> + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
> + : V4L2_SUBDEV_CAP_RW_SUBDEV;
> +
> + return 0;
> + }
> +
> case VIDIOC_QUERYCTRL:
> /*
> * TODO: this really should be folded into v4l2_queryctrl (this
> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> index 03970ce30741..89dc8f2ba6b3 100644
> --- a/include/uapi/linux/v4l2-subdev.h
> +++ b/include/uapi/linux/v4l2-subdev.h
> @@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
> __u32 reserved[8];
> };
>
> +/**
> + * struct v4l2_subdev_capability - subdev capabilities
> + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
> + */
> +struct v4l2_subdev_capability {
> + __u32 version;
> + __u32 subdev_caps;
How do you intend to address additional fields being added to the struct in
the future? Something else than what's been done in V4L2 traditionally?
> +};
> +
> +/* The v4l2 sub-device video device node is registered in read-only mode. */
> +#define V4L2_SUBDEV_CAP_RO_SUBDEV BIT(0)
> +/* The v4l2 sub-device video device node is registered in read/write mode. */
> +#define V4L2_SUBDEV_CAP_RW_SUBDEV BIT(1)
> +
> /* Backwards compatibility define --- to be removed */
> #define v4l2_subdev_edid v4l2_edid
>
> +#define VIDIOC_SUBDEV_QUERYCAP _IOR('V', 0, struct v4l2_subdev_capability)
> #define VIDIOC_SUBDEV_G_FMT _IOWR('V', 4, struct v4l2_subdev_format)
> #define VIDIOC_SUBDEV_S_FMT _IOWR('V', 5, struct v4l2_subdev_format)
> #define VIDIOC_SUBDEV_G_FRAME_INTERVAL _IOWR('V', 21, struct v4l2_subdev_frame_interval)
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
2020-04-28 21:28 ` Sakari Ailus
@ 2020-04-29 8:09 ` Jacopo Mondi
2020-04-29 8:18 ` Sakari Ailus
0 siblings, 1 reply; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-29 8:09 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart, Hans Verkuil
Hi Sakari,
On Wed, Apr 29, 2020 at 12:28:58AM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Tue, Apr 28, 2020 at 11:06:08PM +0200, Jacopo Mondi wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl
> > that apps can call to determine that it is indeed a V4L2 device, there
> > is currently no equivalent for v4l-subdev nodes. Adding this ioctl will
> > solve that, and it will allow utilities like v4l2-compliance to be used
> > with these devices as well.
> >
> > SUBDEV_QUERYCAP currently returns the version and subdev_caps of the
> > subdevice. Define as the initial set of subdev_caps the read-only or
> > read/write flags, to signal to userspace which set of IOCTLs are
> > available on the subdevice.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
> > include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++
> > 2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index f3fe515b8ccb..b8c0071aa4d0 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -15,6 +15,7 @@
> > #include <linux/types.h>
> > #include <linux/videodev2.h>
> > #include <linux/export.h>
> > +#include <linux/version.h>
> >
> > #include <media/v4l2-ctrls.h>
> > #include <media/v4l2-device.h>
> > @@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > int rval;
> >
> > switch (cmd) {
> > + case VIDIOC_SUBDEV_QUERYCAP: {
> > + struct v4l2_subdev_capability *cap = arg;
> > +
> > + memset(cap, 0, sizeof(*cap));
> > + cap->version = LINUX_VERSION_CODE;
> > + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
> > + : V4L2_SUBDEV_CAP_RW_SUBDEV;
> > +
> > + return 0;
> > + }
> > +
> > case VIDIOC_QUERYCTRL:
> > /*
> > * TODO: this really should be folded into v4l2_queryctrl (this
> > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > index 03970ce30741..89dc8f2ba6b3 100644
> > --- a/include/uapi/linux/v4l2-subdev.h
> > +++ b/include/uapi/linux/v4l2-subdev.h
> > @@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
> > __u32 reserved[8];
> > };
> >
> > +/**
> > + * struct v4l2_subdev_capability - subdev capabilities
> > + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
> > + */
> > +struct v4l2_subdev_capability {
> > + __u32 version;
> > + __u32 subdev_caps;
>
> How do you intend to address additional fields being added to the struct in
> the future? Something else than what's been done in V4L2 traditionally?
>
I'm not sure I get what you mean here, so I assume I don't know what
"has been done in V4L2 traditionally" and why what I have here goes
against it...
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
2020-04-29 8:09 ` Jacopo Mondi
@ 2020-04-29 8:18 ` Sakari Ailus
2020-05-06 13:29 ` Hans Verkuil
0 siblings, 1 reply; 24+ messages in thread
From: Sakari Ailus @ 2020-04-29 8:18 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, libcamera-devel, mchehab, hverkuil-cisco,
andrey.konovalov, laurent.pinchart, Hans Verkuil
Hi Jacopo,
On Wed, Apr 29, 2020 at 10:09:49AM +0200, Jacopo Mondi wrote:
> Hi Sakari,
>
> On Wed, Apr 29, 2020 at 12:28:58AM +0300, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Tue, Apr 28, 2020 at 11:06:08PM +0200, Jacopo Mondi wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > >
> > > While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl
> > > that apps can call to determine that it is indeed a V4L2 device, there
> > > is currently no equivalent for v4l-subdev nodes. Adding this ioctl will
> > > solve that, and it will allow utilities like v4l2-compliance to be used
> > > with these devices as well.
> > >
> > > SUBDEV_QUERYCAP currently returns the version and subdev_caps of the
> > > subdevice. Define as the initial set of subdev_caps the read-only or
> > > read/write flags, to signal to userspace which set of IOCTLs are
> > > available on the subdevice.
> > >
> > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
> > > include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++
> > > 2 files changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index f3fe515b8ccb..b8c0071aa4d0 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -15,6 +15,7 @@
> > > #include <linux/types.h>
> > > #include <linux/videodev2.h>
> > > #include <linux/export.h>
> > > +#include <linux/version.h>
> > >
> > > #include <media/v4l2-ctrls.h>
> > > #include <media/v4l2-device.h>
> > > @@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> > > int rval;
> > >
> > > switch (cmd) {
> > > + case VIDIOC_SUBDEV_QUERYCAP: {
> > > + struct v4l2_subdev_capability *cap = arg;
> > > +
> > > + memset(cap, 0, sizeof(*cap));
> > > + cap->version = LINUX_VERSION_CODE;
> > > + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
> > > + : V4L2_SUBDEV_CAP_RW_SUBDEV;
> > > +
> > > + return 0;
> > > + }
> > > +
> > > case VIDIOC_QUERYCTRL:
> > > /*
> > > * TODO: this really should be folded into v4l2_queryctrl (this
> > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> > > index 03970ce30741..89dc8f2ba6b3 100644
> > > --- a/include/uapi/linux/v4l2-subdev.h
> > > +++ b/include/uapi/linux/v4l2-subdev.h
> > > @@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
> > > __u32 reserved[8];
> > > };
> > >
> > > +/**
> > > + * struct v4l2_subdev_capability - subdev capabilities
> > > + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
> > > + */
> > > +struct v4l2_subdev_capability {
> > > + __u32 version;
> > > + __u32 subdev_caps;
> >
> > How do you intend to address additional fields being added to the struct in
> > the future? Something else than what's been done in V4L2 traditionally?
> >
>
> I'm not sure I get what you mean here, so I assume I don't know what
> "has been done in V4L2 traditionally" and why what I have here goes
> against it...
I can't help noticing you have no reserved fields in your IOCTL argument
struct. That has generally been the way V4L2 IOCTLs have been extended when
there's been a need to.
Media controller adopted a different approach to that recently, based on
the argument size. We've discussed doing that on V4L2 but I don't think
a decision has been made.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
2020-04-29 8:18 ` Sakari Ailus
@ 2020-05-06 13:29 ` Hans Verkuil
2020-05-06 18:34 ` Sakari Ailus
0 siblings, 1 reply; 24+ messages in thread
From: Hans Verkuil @ 2020-05-06 13:29 UTC (permalink / raw)
To: Sakari Ailus, Jacopo Mondi
Cc: linux-media, libcamera-devel, mchehab, andrey.konovalov,
laurent.pinchart, Hans Verkuil
On 29/04/2020 10:18, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Apr 29, 2020 at 10:09:49AM +0200, Jacopo Mondi wrote:
>> Hi Sakari,
>>
>> On Wed, Apr 29, 2020 at 12:28:58AM +0300, Sakari Ailus wrote:
>>> Hi Jacopo,
>>>
>>> On Tue, Apr 28, 2020 at 11:06:08PM +0200, Jacopo Mondi wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl
>>>> that apps can call to determine that it is indeed a V4L2 device, there
>>>> is currently no equivalent for v4l-subdev nodes. Adding this ioctl will
>>>> solve that, and it will allow utilities like v4l2-compliance to be used
>>>> with these devices as well.
>>>>
>>>> SUBDEV_QUERYCAP currently returns the version and subdev_caps of the
>>>> subdevice. Define as the initial set of subdev_caps the read-only or
>>>> read/write flags, to signal to userspace which set of IOCTLs are
>>>> available on the subdevice.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> ---
>>>> drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
>>>> include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++
>>>> 2 files changed, 27 insertions(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> index f3fe515b8ccb..b8c0071aa4d0 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>> @@ -15,6 +15,7 @@
>>>> #include <linux/types.h>
>>>> #include <linux/videodev2.h>
>>>> #include <linux/export.h>
>>>> +#include <linux/version.h>
>>>>
>>>> #include <media/v4l2-ctrls.h>
>>>> #include <media/v4l2-device.h>
>>>> @@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>> int rval;
>>>>
>>>> switch (cmd) {
>>>> + case VIDIOC_SUBDEV_QUERYCAP: {
>>>> + struct v4l2_subdev_capability *cap = arg;
>>>> +
>>>> + memset(cap, 0, sizeof(*cap));
>>>> + cap->version = LINUX_VERSION_CODE;
>>>> + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
>>>> + : V4L2_SUBDEV_CAP_RW_SUBDEV;
>>>> +
>>>> + return 0;
>>>> + }
>>>> +
>>>> case VIDIOC_QUERYCTRL:
>>>> /*
>>>> * TODO: this really should be folded into v4l2_queryctrl (this
>>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
>>>> index 03970ce30741..89dc8f2ba6b3 100644
>>>> --- a/include/uapi/linux/v4l2-subdev.h
>>>> +++ b/include/uapi/linux/v4l2-subdev.h
>>>> @@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
>>>> __u32 reserved[8];
>>>> };
>>>>
>>>> +/**
>>>> + * struct v4l2_subdev_capability - subdev capabilities
>>>> + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
>>>> + */
>>>> +struct v4l2_subdev_capability {
>>>> + __u32 version;
>>>> + __u32 subdev_caps;
>>>
>>> How do you intend to address additional fields being added to the struct in
>>> the future? Something else than what's been done in V4L2 traditionally?
>>>
>>
>> I'm not sure I get what you mean here, so I assume I don't know what
>> "has been done in V4L2 traditionally" and why what I have here goes
>> against it...
>
> I can't help noticing you have no reserved fields in your IOCTL argument
> struct. That has generally been the way V4L2 IOCTLs have been extended when
> there's been a need to.
>
> Media controller adopted a different approach to that recently, based on
> the argument size. We've discussed doing that on V4L2 but I don't think
> a decision has been made.
>
While I agree that using the argument size to do 'versioning' of the API
is a better solution, the fact is that historically we always used a 'reserved'
field. And I think we need to do that here as well. It's consistent with
the existing subdev ioctls, so I would be in favor of adding a 'u32 reserved[6];'
field.
If there are such strong feelings against it that it wouldn't be merged, then
we can always just leave it as-is. It's not worth blocking this patch just
because of that.
BTW, one thing that should be changed is the name 'subdev_caps': just name it
'capabilities'. It's a field in a struct v4l2_subdev_capability, so it is
already obvious that this is subdev specific.
Regards,
Hans
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
2020-05-06 13:29 ` Hans Verkuil
@ 2020-05-06 18:34 ` Sakari Ailus
2020-05-07 7:14 ` Hans Verkuil
0 siblings, 1 reply; 24+ messages in thread
From: Sakari Ailus @ 2020-05-06 18:34 UTC (permalink / raw)
To: Hans Verkuil
Cc: Jacopo Mondi, linux-media, libcamera-devel, mchehab,
andrey.konovalov, laurent.pinchart, Hans Verkuil
Hi Hans, Jacopo,
On Wed, May 06, 2020 at 03:29:03PM +0200, Hans Verkuil wrote:
> On 29/04/2020 10:18, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Wed, Apr 29, 2020 at 10:09:49AM +0200, Jacopo Mondi wrote:
> >> Hi Sakari,
> >>
> >> On Wed, Apr 29, 2020 at 12:28:58AM +0300, Sakari Ailus wrote:
> >>> Hi Jacopo,
> >>>
> >>> On Tue, Apr 28, 2020 at 11:06:08PM +0200, Jacopo Mondi wrote:
> >>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>
> >>>> While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl
> >>>> that apps can call to determine that it is indeed a V4L2 device, there
> >>>> is currently no equivalent for v4l-subdev nodes. Adding this ioctl will
> >>>> solve that, and it will allow utilities like v4l2-compliance to be used
> >>>> with these devices as well.
> >>>>
> >>>> SUBDEV_QUERYCAP currently returns the version and subdev_caps of the
> >>>> subdevice. Define as the initial set of subdev_caps the read-only or
> >>>> read/write flags, to signal to userspace which set of IOCTLs are
> >>>> available on the subdevice.
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>> drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
> >>>> include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++
> >>>> 2 files changed, 27 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> index f3fe515b8ccb..b8c0071aa4d0 100644
> >>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> >>>> @@ -15,6 +15,7 @@
> >>>> #include <linux/types.h>
> >>>> #include <linux/videodev2.h>
> >>>> #include <linux/export.h>
> >>>> +#include <linux/version.h>
> >>>>
> >>>> #include <media/v4l2-ctrls.h>
> >>>> #include <media/v4l2-device.h>
> >>>> @@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
> >>>> int rval;
> >>>>
> >>>> switch (cmd) {
> >>>> + case VIDIOC_SUBDEV_QUERYCAP: {
> >>>> + struct v4l2_subdev_capability *cap = arg;
> >>>> +
> >>>> + memset(cap, 0, sizeof(*cap));
> >>>> + cap->version = LINUX_VERSION_CODE;
> >>>> + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
> >>>> + : V4L2_SUBDEV_CAP_RW_SUBDEV;
> >>>> +
> >>>> + return 0;
> >>>> + }
> >>>> +
> >>>> case VIDIOC_QUERYCTRL:
> >>>> /*
> >>>> * TODO: this really should be folded into v4l2_queryctrl (this
> >>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
> >>>> index 03970ce30741..89dc8f2ba6b3 100644
> >>>> --- a/include/uapi/linux/v4l2-subdev.h
> >>>> +++ b/include/uapi/linux/v4l2-subdev.h
> >>>> @@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
> >>>> __u32 reserved[8];
> >>>> };
> >>>>
> >>>> +/**
> >>>> + * struct v4l2_subdev_capability - subdev capabilities
> >>>> + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
> >>>> + */
> >>>> +struct v4l2_subdev_capability {
> >>>> + __u32 version;
> >>>> + __u32 subdev_caps;
> >>>
> >>> How do you intend to address additional fields being added to the struct in
> >>> the future? Something else than what's been done in V4L2 traditionally?
> >>>
> >>
> >> I'm not sure I get what you mean here, so I assume I don't know what
> >> "has been done in V4L2 traditionally" and why what I have here goes
> >> against it...
> >
> > I can't help noticing you have no reserved fields in your IOCTL argument
> > struct. That has generally been the way V4L2 IOCTLs have been extended when
> > there's been a need to.
> >
> > Media controller adopted a different approach to that recently, based on
> > the argument size. We've discussed doing that on V4L2 but I don't think
> > a decision has been made.
> >
>
> While I agree that using the argument size to do 'versioning' of the API
> is a better solution, the fact is that historically we always used a 'reserved'
> field. And I think we need to do that here as well. It's consistent with
> the existing subdev ioctls, so I would be in favor of adding a 'u32 reserved[6];'
> field.
Agreed. Could be even 14, in practice it matters little performance-wise.
>
> If there are such strong feelings against it that it wouldn't be merged, then
> we can always just leave it as-is. It's not worth blocking this patch just
> because of that.
I'm not (strongly) pushing either here; just that we need to make a
decision. I'm in favour of the reserved field for the same reason. I was
just wondering whether it was intentional. :-)
>
> BTW, one thing that should be changed is the name 'subdev_caps': just name it
> 'capabilities'. It's a field in a struct v4l2_subdev_capability, so it is
> already obvious that this is subdev specific.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl
2020-05-06 18:34 ` Sakari Ailus
@ 2020-05-07 7:14 ` Hans Verkuil
0 siblings, 0 replies; 24+ messages in thread
From: Hans Verkuil @ 2020-05-07 7:14 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, linux-media, libcamera-devel, mchehab,
andrey.konovalov, laurent.pinchart, Hans Verkuil
On 06/05/2020 20:34, Sakari Ailus wrote:
> Hi Hans, Jacopo,
>
> On Wed, May 06, 2020 at 03:29:03PM +0200, Hans Verkuil wrote:
>> On 29/04/2020 10:18, Sakari Ailus wrote:
>>> Hi Jacopo,
>>>
>>> On Wed, Apr 29, 2020 at 10:09:49AM +0200, Jacopo Mondi wrote:
>>>> Hi Sakari,
>>>>
>>>> On Wed, Apr 29, 2020 at 12:28:58AM +0300, Sakari Ailus wrote:
>>>>> Hi Jacopo,
>>>>>
>>>>> On Tue, Apr 28, 2020 at 11:06:08PM +0200, Jacopo Mondi wrote:
>>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>>
>>>>>> While normal video/radio/vbi/swradio nodes have a proper QUERYCAP ioctl
>>>>>> that apps can call to determine that it is indeed a V4L2 device, there
>>>>>> is currently no equivalent for v4l-subdev nodes. Adding this ioctl will
>>>>>> solve that, and it will allow utilities like v4l2-compliance to be used
>>>>>> with these devices as well.
>>>>>>
>>>>>> SUBDEV_QUERYCAP currently returns the version and subdev_caps of the
>>>>>> subdevice. Define as the initial set of subdev_caps the read-only or
>>>>>> read/write flags, to signal to userspace which set of IOCTLs are
>>>>>> available on the subdevice.
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>> ---
>>>>>> drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++++
>>>>>> include/uapi/linux/v4l2-subdev.h | 15 +++++++++++++++
>>>>>> 2 files changed, 27 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> index f3fe515b8ccb..b8c0071aa4d0 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>>>>> @@ -15,6 +15,7 @@
>>>>>> #include <linux/types.h>
>>>>>> #include <linux/videodev2.h>
>>>>>> #include <linux/export.h>
>>>>>> +#include <linux/version.h>
>>>>>>
>>>>>> #include <media/v4l2-ctrls.h>
>>>>>> #include <media/v4l2-device.h>
>>>>>> @@ -331,6 +332,17 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
>>>>>> int rval;
>>>>>>
>>>>>> switch (cmd) {
>>>>>> + case VIDIOC_SUBDEV_QUERYCAP: {
>>>>>> + struct v4l2_subdev_capability *cap = arg;
>>>>>> +
>>>>>> + memset(cap, 0, sizeof(*cap));
>>>>>> + cap->version = LINUX_VERSION_CODE;
>>>>>> + cap->subdev_caps |= ro_subdev ? V4L2_SUBDEV_CAP_RO_SUBDEV
>>>>>> + : V4L2_SUBDEV_CAP_RW_SUBDEV;
>>>>>> +
>>>>>> + return 0;
>>>>>> + }
>>>>>> +
>>>>>> case VIDIOC_QUERYCTRL:
>>>>>> /*
>>>>>> * TODO: this really should be folded into v4l2_queryctrl (this
>>>>>> diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h
>>>>>> index 03970ce30741..89dc8f2ba6b3 100644
>>>>>> --- a/include/uapi/linux/v4l2-subdev.h
>>>>>> +++ b/include/uapi/linux/v4l2-subdev.h
>>>>>> @@ -155,9 +155,24 @@ struct v4l2_subdev_selection {
>>>>>> __u32 reserved[8];
>>>>>> };
>>>>>>
>>>>>> +/**
>>>>>> + * struct v4l2_subdev_capability - subdev capabilities
>>>>>> + * @device_caps: the subdev capabilities, see V4L2_SUBDEV_CAP_*.
>>>>>> + */
>>>>>> +struct v4l2_subdev_capability {
>>>>>> + __u32 version;
>>>>>> + __u32 subdev_caps;
>>>>>
>>>>> How do you intend to address additional fields being added to the struct in
>>>>> the future? Something else than what's been done in V4L2 traditionally?
>>>>>
>>>>
>>>> I'm not sure I get what you mean here, so I assume I don't know what
>>>> "has been done in V4L2 traditionally" and why what I have here goes
>>>> against it...
>>>
>>> I can't help noticing you have no reserved fields in your IOCTL argument
>>> struct. That has generally been the way V4L2 IOCTLs have been extended when
>>> there's been a need to.
>>>
>>> Media controller adopted a different approach to that recently, based on
>>> the argument size. We've discussed doing that on V4L2 but I don't think
>>> a decision has been made.
>>>
>>
>> While I agree that using the argument size to do 'versioning' of the API
>> is a better solution, the fact is that historically we always used a 'reserved'
>> field. And I think we need to do that here as well. It's consistent with
>> the existing subdev ioctls, so I would be in favor of adding a 'u32 reserved[6];'
>> field.
>
> Agreed. Could be even 14, in practice it matters little performance-wise.
True. Let's make it 14.
Regards,
Hans
>
>>
>> If there are such strong feelings against it that it wouldn't be merged, then
>> we can always just leave it as-is. It's not worth blocking this patch just
>> because of that.
>
> I'm not (strongly) pushing either here; just that we need to make a
> decision. I'm in favour of the reserved field for the same reason. I was
> just wondering whether it was intentional. :-)
>
>>
>> BTW, one thing that should be changed is the name 'subdev_caps': just name it
>> 'capabilities'. It's a field in a struct v4l2_subdev_capability, so it is
>> already obvious that this is subdev specific.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v5 6/6] v4l: document VIDIOC_SUBDEV_QUERYCAP
2020-04-28 21:06 [PATCH v5 0/6] media: Register read-only sub-dev devnode Jacopo Mondi
` (4 preceding siblings ...)
2020-04-28 21:06 ` [PATCH v5 5/6] v4l2-subdev: add VIDIOC_SUBDEV_QUERYCAP ioctl Jacopo Mondi
@ 2020-04-28 21:06 ` Jacopo Mondi
5 siblings, 0 replies; 24+ messages in thread
From: Jacopo Mondi @ 2020-04-28 21:06 UTC (permalink / raw)
To: linux-media, libcamera-devel
Cc: Jacopo Mondi, mchehab, hverkuil-cisco, sakari.ailus,
andrey.konovalov, laurent.pinchart, Hans Verkuil
From: Hans Verkuil <hans.verkuil@cisco.com>
Add documentation for the new VIDIOC_SUBDEV_QUERYCAP ioctl.
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
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..bf77c842718e 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
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..1ab5c808d9b4
--- /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.
+..
+.. TODO: replace it to GFDL-1.1-or-later WITH no-invariant-sections
+
+.. _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
^ permalink raw reply related [flat|nested] 24+ messages in thread