| Message ID | 20200327223522.506832-3-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Register read-only sub-dev devnode | expand |
Hi Jacopo, Thanks for the patchset. On Fri, Mar 27, 2020 at 11:35:21PM +0100, Jacopo Mondi wrote: > Document a new kapi function to register subdev device nodes in read only > mode and for each affected ioctl report how access is restricted. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > 8 files changed, 94 insertions(+) > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > index 41ccb3e5c707..6506a673e6a1 100644 > --- a/Documentation/media/kapi/v4l2-subdev.rst > +++ b/Documentation/media/kapi/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 ioclt is not allowed, or the format to modify is set to "IOCTL". > +``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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > index 029bb2d9928a..6082f9c2f8f4 100644 > --- a/Documentation/media/uapi/v4l/dev-subdev.rst > +++ b/Documentation/media/uapi/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 don't 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. Perhaps "IOCTLs". With these, for the set, Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > + > > Controls > ======== > diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > index e36dd2622857..611f94e4510a 100644 > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > index e633e42e3910..e220b38b859f 100644 > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > index 632ee053accc..62f5d9870ca7 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > index 472577bd1745..3a2f64bb00e7 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > index 4b1b4bc78bfe..34aa39096e3d 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > index fc73d27e6d74..abd046cef612 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > +++ b/Documentation/media/uapi/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``.
Hi Jacopo, Some comments below: On 3/27/20 11:35 PM, Jacopo Mondi wrote: > Document a new kapi function to register subdev device nodes in read only kAPI > mode and for each affected ioctl report how access is restricted. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > 8 files changed, 94 insertions(+) > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > index 41ccb3e5c707..6506a673e6a1 100644 > --- a/Documentation/media/kapi/v4l2-subdev.rst > +++ b/Documentation/media/kapi/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`. Should we add something about creating a /dev/media device as well? It's basically required for this functionality. I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order to be able to call this function. Or possibly have an explicit test in __v4l2_device_register_subdev_nodes for the presence of a media device if read_only is true. > + > +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > index 029bb2d9928a..6082f9c2f8f4 100644 > --- a/Documentation/media/uapi/v4l/dev-subdev.rst > +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices don't -> do not ("don't" is a bit too informal) > +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > index e36dd2622857..611f94e4510a 100644 > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > index e633e42e3910..e220b38b859f 100644 > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > index 632ee053accc..62f5d9870ca7 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > +++ b/Documentation/media/uapi/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 mode calls -> mode, calls > +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > index 472577bd1745..3a2f64bb00e7 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > +++ b/Documentation/media/uapi/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 ditto. > +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > index 4b1b4bc78bfe..34aa39096e3d 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > index fc73d27e6d74..abd046cef612 100644 > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > +++ b/Documentation/media/uapi/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 ditto > +``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``. > Regards, Hans
Hi Hans, On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote: > On 3/27/20 11:35 PM, Jacopo Mondi wrote: > > Document a new kapi function to register subdev device nodes in read only > > kAPI > > > mode and for each affected ioctl report how access is restricted. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > > 8 files changed, 94 insertions(+) > > > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > > index 41ccb3e5c707..6506a673e6a1 100644 > > --- a/Documentation/media/kapi/v4l2-subdev.rst > > +++ b/Documentation/media/kapi/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`. > > Should we add something about creating a /dev/media device as well? It's basically > required for this functionality. I'm not opposed to that, but I don't think this should be specific to the read-only API, as it's a shared requirement with thr R/W API. The previous section, "V4L2 sub-device userspace API", doesn't mention media devices. > I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() > under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order > to be able to call this function. Or possibly have an explicit test in > __v4l2_device_register_subdev_nodes for the presence of a media device if > read_only is true. VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify this and make MEDIA_CONTROLLER a requirement for any userspace access from userspace. > > + > > +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > > index 029bb2d9928a..6082f9c2f8f4 100644 > > --- a/Documentation/media/uapi/v4l/dev-subdev.rst > > +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices > > don't -> do not > > ("don't" is a bit too informal) > > > +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > index e36dd2622857..611f94e4510a 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > > index e633e42e3910..e220b38b859f 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > index 632ee053accc..62f5d9870ca7 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > +++ b/Documentation/media/uapi/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 > > mode calls -> mode, calls > > > +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > index 472577bd1745..3a2f64bb00e7 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > +++ b/Documentation/media/uapi/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 > > ditto. > > > +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > index 4b1b4bc78bfe..34aa39096e3d 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > index fc73d27e6d74..abd046cef612 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > +++ b/Documentation/media/uapi/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 > > ditto > > > +``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``. > >
On 4/1/20 1:46 PM, Laurent Pinchart wrote: > Hi Hans, > > On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote: >> On 3/27/20 11:35 PM, Jacopo Mondi wrote: >>> Document a new kapi function to register subdev device nodes in read only >> >> kAPI >> >>> mode and for each affected ioctl report how access is restricted. >>> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ >>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ >>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ >>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ >>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ >>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ >>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ >>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ >>> 8 files changed, 94 insertions(+) >>> >>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst >>> index 41ccb3e5c707..6506a673e6a1 100644 >>> --- a/Documentation/media/kapi/v4l2-subdev.rst >>> +++ b/Documentation/media/kapi/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`. >> >> Should we add something about creating a /dev/media device as well? It's basically >> required for this functionality. > > I'm not opposed to that, but I don't think this should be specific to > the read-only API, as it's a shared requirement with thr R/W API. The > previous section, "V4L2 sub-device userspace API", doesn't mention media > devices. True. > >> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() >> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order >> to be able to call this function. Or possibly have an explicit test in >> __v4l2_device_register_subdev_nodes for the presence of a media device if >> read_only is true. > > VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that > actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify > this and make MEDIA_CONTROLLER a requirement for any userspace access > from userspace. I agree. Regards, Hans > >>> + >>> +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst >>> index 029bb2d9928a..6082f9c2f8f4 100644 >>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst >>> +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices >> >> don't -> do not >> >> ("don't" is a bit too informal) >> >>> +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst >>> index e36dd2622857..611f94e4510a 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst >>> index e633e42e3910..e220b38b859f 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst >>> index 632ee053accc..62f5d9870ca7 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst >>> +++ b/Documentation/media/uapi/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 >> >> mode calls -> mode, calls >> >>> +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst >>> index 472577bd1745..3a2f64bb00e7 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst >>> +++ b/Documentation/media/uapi/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 >> >> ditto. >> >>> +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst >>> index 4b1b4bc78bfe..34aa39096e3d 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst >>> index fc73d27e6d74..abd046cef612 100644 >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst >>> +++ b/Documentation/media/uapi/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 >> >> ditto >> >>> +``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``. >>> >
Hi Hans, Laurent, On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote: > On 4/1/20 1:46 PM, Laurent Pinchart wrote: > > Hi Hans, > > > > On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote: > >> On 3/27/20 11:35 PM, Jacopo Mondi wrote: [snip] > >>> +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`. > >> > >> Should we add something about creating a /dev/media device as well? It's basically > >> required for this functionality. > > > > I'm not opposed to that, but I don't think this should be specific to > > the read-only API, as it's a shared requirement with thr R/W API. The > > previous section, "V4L2 sub-device userspace API", doesn't mention media > > devices. > > True. > > > > >> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() > >> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order > >> to be able to call this function. Or possibly have an explicit test in > >> __v4l2_device_register_subdev_nodes for the presence of a media device if > >> read_only is true. > > > > VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that > > actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify > > this and make MEDIA_CONTROLLER a requirement for any userspace access > > from userspace. > > I agree. I initially looked into making this new function conditional on the presence of VIDEO_V4L2_SUBDEV_API, but then I've noticed only some part of the code in drivers/media/v4l2-core/v4l2-subdev.c is guarded by that flag, and at the same time v4l2_device_register_subdev_nodes() was not. So I assumed registering read-only wihtout V4L2_SUBDEV_API should have been allowed in the same way, even if in case V4L2_SUBDEV_API is not enabled, ro or rw does not actually make any difference. How would you like to proceed forward ? Guarding v4l2_device_register_[ro_]subdev_node() with MEDIA_CONTROLLER flag ? Thanks j > > Regards, > > Hans > > > > >>> + > >>> +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > >>> index 029bb2d9928a..6082f9c2f8f4 100644 > >>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst > >>> +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices > >> > >> don't -> do not > >> > >> ("don't" is a bit too informal) > >> > >>> +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > >>> index e36dd2622857..611f94e4510a 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > >>> index e633e42e3910..e220b38b859f 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > >>> index 632ee053accc..62f5d9870ca7 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > >>> +++ b/Documentation/media/uapi/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 > >> > >> mode calls -> mode, calls > >> > >>> +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > >>> index 472577bd1745..3a2f64bb00e7 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > >>> +++ b/Documentation/media/uapi/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 > >> > >> ditto. > >> > >>> +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > >>> index 4b1b4bc78bfe..34aa39096e3d 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > >>> index fc73d27e6d74..abd046cef612 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > >>> +++ b/Documentation/media/uapi/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 > >> > >> ditto > >> > >>> +``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``. > >>> > > >
Hi Sakari, one small question On Sat, Mar 28, 2020 at 01:33:58AM +0200, Sakari Ailus wrote: > Hi Jacopo, > > Thanks for the patchset. > > On Fri, Mar 27, 2020 at 11:35:21PM +0100, Jacopo Mondi wrote: > > Document a new kapi function to register subdev device nodes in read only > > mode and for each affected ioctl report how access is restricted. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > > 8 files changed, 94 insertions(+) > > > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > > index 41ccb3e5c707..6506a673e6a1 100644 > > --- a/Documentation/media/kapi/v4l2-subdev.rst > > +++ b/Documentation/media/kapi/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 ioclt is not allowed, or the format to modify is set to > > "IOCTL". are you referring to the typo ioclt/ioctl or are you suggesting to spell IOCTL uppercase ? As the rest of the documentation uses lowercase "ioctl" ... > > > +``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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > > index 029bb2d9928a..6082f9c2f8f4 100644 > > --- a/Documentation/media/uapi/v4l/dev-subdev.rst > > +++ b/Documentation/media/uapi/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 don't 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. > > Perhaps "IOCTLs". Same here, the rest of the documentation uses lowercase. Thanks j > > With these, for the set, > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > + > > > > Controls > > ======== > > diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > index e36dd2622857..611f94e4510a 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > > index e633e42e3910..e220b38b859f 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > index 632ee053accc..62f5d9870ca7 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > index 472577bd1745..3a2f64bb00e7 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > index 4b1b4bc78bfe..34aa39096e3d 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > index fc73d27e6d74..abd046cef612 100644 > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > +++ b/Documentation/media/uapi/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``. > > -- > Regards, > > Sakari Ailus
Hi Hans, On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote: > On 4/1/20 1:46 PM, Laurent Pinchart wrote: > > On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote: > >> On 3/27/20 11:35 PM, Jacopo Mondi wrote: > >>> Document a new kapi function to register subdev device nodes in read only > >> > >> kAPI > >> > >>> mode and for each affected ioctl report how access is restricted. > >>> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>> --- > >>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > >>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > >>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > >>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > >>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > >>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > >>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > >>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > >>> 8 files changed, 94 insertions(+) > >>> > >>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > >>> index 41ccb3e5c707..6506a673e6a1 100644 > >>> --- a/Documentation/media/kapi/v4l2-subdev.rst > >>> +++ b/Documentation/media/kapi/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`. > >> > >> Should we add something about creating a /dev/media device as well? It's basically > >> required for this functionality. > > > > I'm not opposed to that, but I don't think this should be specific to > > the read-only API, as it's a shared requirement with thr R/W API. The > > previous section, "V4L2 sub-device userspace API", doesn't mention media > > devices. > > True. > > >> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() > >> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order > >> to be able to call this function. Or possibly have an explicit test in > >> __v4l2_device_register_subdev_nodes for the presence of a media device if > >> read_only is true. > > > > VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that > > actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify > > this and make MEDIA_CONTROLLER a requirement for any userspace access > > from userspace. > > I agree. Does that mean we should rework it as a prerequisite for this series, as part of the series, or separately ? > >>> + > >>> +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > >>> index 029bb2d9928a..6082f9c2f8f4 100644 > >>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst > >>> +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices > >> > >> don't -> do not > >> > >> ("don't" is a bit too informal) > >> > >>> +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > >>> index e36dd2622857..611f94e4510a 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > >>> index e633e42e3910..e220b38b859f 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > >>> index 632ee053accc..62f5d9870ca7 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > >>> +++ b/Documentation/media/uapi/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 > >> > >> mode calls -> mode, calls > >> > >>> +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > >>> index 472577bd1745..3a2f64bb00e7 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > >>> +++ b/Documentation/media/uapi/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 > >> > >> ditto. > >> > >>> +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > >>> index 4b1b4bc78bfe..34aa39096e3d 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > >>> index fc73d27e6d74..abd046cef612 100644 > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > >>> +++ b/Documentation/media/uapi/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 > >> > >> ditto > >> > >>> +``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``.
Hi Jacopo, On Thu, Apr 02, 2020 at 12:10:21AM +0200, Jacopo Mondi wrote: > Hi Sakari, > one small question > > On Sat, Mar 28, 2020 at 01:33:58AM +0200, Sakari Ailus wrote: > > Hi Jacopo, > > > > Thanks for the patchset. > > > > On Fri, Mar 27, 2020 at 11:35:21PM +0100, Jacopo Mondi wrote: > > > Document a new kapi function to register subdev device nodes in read only > > > mode and for each affected ioctl report how access is restricted. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > > > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > > > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > > > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > > > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > > > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > > > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > > > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > > > 8 files changed, 94 insertions(+) > > > > > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > > > index 41ccb3e5c707..6506a673e6a1 100644 > > > --- a/Documentation/media/kapi/v4l2-subdev.rst > > > +++ b/Documentation/media/kapi/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 ioclt is not allowed, or the format to modify is set to > > > > "IOCTL". > > are you referring to the typo ioclt/ioctl or are you suggesting to > spell IOCTL uppercase ? As the rest of the documentation uses > lowercase "ioctl" ... Ok; then just use lower case. > > > > > +``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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > > > index 029bb2d9928a..6082f9c2f8f4 100644 > > > --- a/Documentation/media/uapi/v4l/dev-subdev.rst > > > +++ b/Documentation/media/uapi/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 don't 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. > > > > Perhaps "IOCTLs". > > Same here, the rest of the documentation uses lowercase. > > Thanks > j > > > > With these, for the set, > > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > + > > > > > > Controls > > > ======== > > > diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > > index e36dd2622857..611f94e4510a 100644 > > > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > > > index e633e42e3910..e220b38b859f 100644 > > > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > > index 632ee053accc..62f5d9870ca7 100644 > > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > > index 472577bd1745..3a2f64bb00e7 100644 > > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > > index 4b1b4bc78bfe..34aa39096e3d 100644 > > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > > +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > > index fc73d27e6d74..abd046cef612 100644 > > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > > +++ b/Documentation/media/uapi/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``. > > > > -- > > Regards, > > > > Sakari Ailus
Hi On Sat, Apr 04, 2020 at 04:35:45AM +0300, Laurent Pinchart wrote: > Hi Hans, > > On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote: > > On 4/1/20 1:46 PM, Laurent Pinchart wrote: > > > On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote: > > >> On 3/27/20 11:35 PM, Jacopo Mondi wrote: > > >>> Document a new kapi function to register subdev device nodes in read only > > >> > > >> kAPI > > >> > > >>> mode and for each affected ioctl report how access is restricted. > > >>> > > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>> --- > > >>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > > >>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > > >>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > > >>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > > >>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > > >>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > > >>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > > >>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > > >>> 8 files changed, 94 insertions(+) > > >>> > > >>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > > >>> index 41ccb3e5c707..6506a673e6a1 100644 > > >>> --- a/Documentation/media/kapi/v4l2-subdev.rst > > >>> +++ b/Documentation/media/kapi/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`. > > >> > > >> Should we add something about creating a /dev/media device as well? It's basically > > >> required for this functionality. > > > > > > I'm not opposed to that, but I don't think this should be specific to > > > the read-only API, as it's a shared requirement with thr R/W API. The > > > previous section, "V4L2 sub-device userspace API", doesn't mention media > > > devices. > > > > True. > > > > >> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() > > >> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order > > >> to be able to call this function. Or possibly have an explicit test in > > >> __v4l2_device_register_subdev_nodes for the presence of a media device if > > >> read_only is true. > > > > > > VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that > > > actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify > > > this and make MEDIA_CONTROLLER a requirement for any userspace access > > > from userspace. > > > > I agree. > > Does that mean we should rework it as a prerequisite for this series, as > part of the series, or separately ? I would, for the moment, make v4l2_device_register_ro_subdev_nodes and v4l2_device_register_subdev_nodes dependendent on VIDEO_V4L2_SUBDEV_API, so that they're only available to drivers actually wanting userspace access. Then when moving everything to MEDIA_CONTROLLER, they can be moved to. However, this is not clear to me https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-subdev.c#L338 What is the reason to have some of the subdev ioctls protected by VIDEO_V4L2_SUBDEV_API, while the v4l2-control related ones are not ? Thanks j > > > >>> + > > >>> +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > > >>> index 029bb2d9928a..6082f9c2f8f4 100644 > > >>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst > > >>> +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices > > >> > > >> don't -> do not > > >> > > >> ("don't" is a bit too informal) > > >> > > >>> +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > >>> index e36dd2622857..611f94e4510a 100644 > > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > > >>> index e633e42e3910..e220b38b859f 100644 > > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > >>> index 632ee053accc..62f5d9870ca7 100644 > > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > >>> +++ b/Documentation/media/uapi/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 > > >> > > >> mode calls -> mode, calls > > >> > > >>> +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > >>> index 472577bd1745..3a2f64bb00e7 100644 > > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > >>> +++ b/Documentation/media/uapi/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 > > >> > > >> ditto. > > >> > > >>> +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > >>> index 4b1b4bc78bfe..34aa39096e3d 100644 > > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > >>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > >>> index fc73d27e6d74..abd046cef612 100644 > > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > >>> +++ b/Documentation/media/uapi/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 > > >> > > >> ditto > > >> > > >>> +``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``. > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Mon, Apr 06, 2020 at 01:07:32PM +0200, Jacopo Mondi wrote: > On Sat, Apr 04, 2020 at 04:35:45AM +0300, Laurent Pinchart wrote: > > On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote: > >> On 4/1/20 1:46 PM, Laurent Pinchart wrote: > >>> On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote: > >>>> On 3/27/20 11:35 PM, Jacopo Mondi wrote: > >>>>> Document a new kapi function to register subdev device nodes in read only > >>>> > >>>> kAPI > >>>> > >>>>> mode and for each affected ioctl report how access is restricted. > >>>>> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >>>>> --- > >>>>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > >>>>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > >>>>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > >>>>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > >>>>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > >>>>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > >>>>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > >>>>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > >>>>> 8 files changed, 94 insertions(+) > >>>>> > >>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > >>>>> index 41ccb3e5c707..6506a673e6a1 100644 > >>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst > >>>>> +++ b/Documentation/media/kapi/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`. > >>>> > >>>> Should we add something about creating a /dev/media device as well? It's basically > >>>> required for this functionality. > >>> > >>> I'm not opposed to that, but I don't think this should be specific to > >>> the read-only API, as it's a shared requirement with thr R/W API. The > >>> previous section, "V4L2 sub-device userspace API", doesn't mention media > >>> devices. > >> > >> True. > >> > >>>> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() > >>>> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order > >>>> to be able to call this function. Or possibly have an explicit test in > >>>> __v4l2_device_register_subdev_nodes for the presence of a media device if > >>>> read_only is true. > >>> > >>> VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that > >>> actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify > >>> this and make MEDIA_CONTROLLER a requirement for any userspace access > >>> from userspace. > >> > >> I agree. > > > > Does that mean we should rework it as a prerequisite for this series, as > > part of the series, or separately ? > > I would, for the moment, make v4l2_device_register_ro_subdev_nodes and > v4l2_device_register_subdev_nodes dependendent on > VIDEO_V4L2_SUBDEV_API, so that they're only available to drivers > actually wanting userspace access. Then when moving everything to > MEDIA_CONTROLLER, they can be moved to. > > However, this is not clear to me > https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-subdev.c#L338 > > What is the reason to have some of the subdev ioctls protected by > VIDEO_V4L2_SUBDEV_API, while the v4l2-control related ones are not ? I'd say historical mistake :-) I think we can move everything under VIDEO_V4L2_SUBDEV_API. > >>>>> + > >>>>> +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > >>>>> index 029bb2d9928a..6082f9c2f8f4 100644 > >>>>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst > >>>>> +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices > >>>> > >>>> don't -> do not > >>>> > >>>> ("don't" is a bit too informal) > >>>> > >>>>> +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > >>>>> index e36dd2622857..611f94e4510a 100644 > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > >>>>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > >>>>> index e633e42e3910..e220b38b859f 100644 > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > >>>>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > >>>>> index 632ee053accc..62f5d9870ca7 100644 > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > >>>>> +++ b/Documentation/media/uapi/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 > >>>> > >>>> mode calls -> mode, calls > >>>> > >>>>> +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > >>>>> index 472577bd1745..3a2f64bb00e7 100644 > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > >>>>> +++ b/Documentation/media/uapi/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 > >>>> > >>>> ditto. > >>>> > >>>>> +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > >>>>> index 4b1b4bc78bfe..34aa39096e3d 100644 > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > >>>>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > >>>>> index fc73d27e6d74..abd046cef612 100644 > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > >>>>> +++ b/Documentation/media/uapi/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 > >>>> > >>>> ditto > >>>> > >>>>> +``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``.
Hi Laurent, On Mon, Apr 06, 2020 at 03:39:35PM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > On Mon, Apr 06, 2020 at 01:07:32PM +0200, Jacopo Mondi wrote: > > On Sat, Apr 04, 2020 at 04:35:45AM +0300, Laurent Pinchart wrote: > > > On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote: > > >> On 4/1/20 1:46 PM, Laurent Pinchart wrote: > > >>> On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote: > > >>>> On 3/27/20 11:35 PM, Jacopo Mondi wrote: > > >>>>> Document a new kapi function to register subdev device nodes in read only > > >>>> > > >>>> kAPI > > >>>> > > >>>>> mode and for each affected ioctl report how access is restricted. > > >>>>> > > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > >>>>> --- > > >>>>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ > > >>>>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ > > >>>>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ > > >>>>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ > > >>>>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ > > >>>>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ > > >>>>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ > > >>>>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ > > >>>>> 8 files changed, 94 insertions(+) > > >>>>> > > >>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst > > >>>>> index 41ccb3e5c707..6506a673e6a1 100644 > > >>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst > > >>>>> +++ b/Documentation/media/kapi/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`. > > >>>> > > >>>> Should we add something about creating a /dev/media device as well? It's basically > > >>>> required for this functionality. > > >>> > > >>> I'm not opposed to that, but I don't think this should be specific to > > >>> the read-only API, as it's a shared requirement with thr R/W API. The > > >>> previous section, "V4L2 sub-device userspace API", doesn't mention media > > >>> devices. > > >> > > >> True. > > >> > > >>>> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes() > > >>>> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order > > >>>> to be able to call this function. Or possibly have an explicit test in > > >>>> __v4l2_device_register_subdev_nodes for the presence of a media device if > > >>>> read_only is true. > > >>> > > >>> VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that > > >>> actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify > > >>> this and make MEDIA_CONTROLLER a requirement for any userspace access > > >>> from userspace. > > >> > > >> I agree. > > > > > > Does that mean we should rework it as a prerequisite for this series, as > > > part of the series, or separately ? > > > > I would, for the moment, make v4l2_device_register_ro_subdev_nodes and > > v4l2_device_register_subdev_nodes dependendent on > > VIDEO_V4L2_SUBDEV_API, so that they're only available to drivers > > actually wanting userspace access. Then when moving everything to > > MEDIA_CONTROLLER, they can be moved to. > > > > However, this is not clear to me > > https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-subdev.c#L338 > > > > What is the reason to have some of the subdev ioctls protected by > > VIDEO_V4L2_SUBDEV_API, while the v4l2-control related ones are not ? > > I'd say historical mistake :-) I think we can move everything under > VIDEO_V4L2_SUBDEV_API. > Good, I was afraid that limiting the ability to register subdevices to drivers claiming support for VIDEO_V4L2_SUBDEV_API only would break use cases that depends on the ability to set controls on a subdevice device node without claming subdevice API support (which seems anyway wrong to me). I will send a new versio making resitration of subdevice devnodes dependent on V4L2_SUBDEV_API then. Thanks j > > >>>>> + > > >>>>> +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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst > > >>>>> index 029bb2d9928a..6082f9c2f8f4 100644 > > >>>>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst > > >>>>> +++ b/Documentation/media/uapi/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 don't modify the device state. In such a case the sub-devices > > >>>> > > >>>> don't -> do not > > >>>> > > >>>> ("don't" is a bit too informal) > > >>>> > > >>>>> +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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > >>>>> index e36dd2622857..611f94e4510a 100644 > > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst > > >>>>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst > > >>>>> index e633e42e3910..e220b38b859f 100644 > > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst > > >>>>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > >>>>> index 632ee053accc..62f5d9870ca7 100644 > > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst > > >>>>> +++ b/Documentation/media/uapi/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 > > >>>> > > >>>> mode calls -> mode, calls > > >>>> > > >>>>> +``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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > >>>>> index 472577bd1745..3a2f64bb00e7 100644 > > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst > > >>>>> +++ b/Documentation/media/uapi/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 > > >>>> > > >>>> ditto. > > >>>> > > >>>>> +``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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > >>>>> index 4b1b4bc78bfe..34aa39096e3d 100644 > > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst > > >>>>> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > >>>>> index fc73d27e6d74..abd046cef612 100644 > > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst > > >>>>> +++ b/Documentation/media/uapi/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 > > >>>> > > >>>> ditto > > >>>> > > >>>>> +``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``. > > -- > Regards, > > Laurent Pinchart
diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst index 41ccb3e5c707..6506a673e6a1 100644 --- a/Documentation/media/kapi/v4l2-subdev.rst +++ b/Documentation/media/kapi/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 ioclt 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/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst index 029bb2d9928a..6082f9c2f8f4 100644 --- a/Documentation/media/uapi/v4l/dev-subdev.rst +++ b/Documentation/media/uapi/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 don't 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/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst index e36dd2622857..611f94e4510a 100644 --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst index e633e42e3910..e220b38b859f 100644 --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst index 632ee053accc..62f5d9870ca7 100644 --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst index 472577bd1745..3a2f64bb00e7 100644 --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst index 4b1b4bc78bfe..34aa39096e3d 100644 --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst index fc73d27e6d74..abd046cef612 100644 --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst +++ b/Documentation/media/uapi/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``.
Document a new kapi function to register subdev device nodes in read only mode and for each affected ioctl report how access is restricted. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++ Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++ .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++ Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++ .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++ .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++ .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++ .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++ 8 files changed, 94 insertions(+)