Re: [libcamera-devel] [PATCH 1/4] Documentation: media: Document read-only subdevice

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacopo,

On Wed, Mar 25, 2020 at 11:37:20PM +0200, Laurent Pinchart wrote:
> On Tue, Mar 24, 2020 at 09:28:41PM +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@xxxxxxxxxx>
> > ---
> >  Documentation/media/kapi/v4l2-subdev.rst      | 38 +++++++++++++++++++
> >  .../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 ++++
> >  7 files changed, 83 insertions(+)
> > 
> > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> > index 29e07e23f888..26edb4178eea 100644
> > --- a/Documentation/media/kapi/v4l2-subdev.rst
> > +++ b/Documentation/media/kapi/v4l2-subdev.rst
> > @@ -327,6 +327,44 @@ 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.
> 
> I think part of this belongs to the previous section.
> 
> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst
> index 29e07e23f888..41ccb3e5c707 100644
> --- a/Documentation/media/kapi/v4l2-subdev.rst
> +++ b/Documentation/media/kapi/v4l2-subdev.rst
> @@ -275,8 +275,13 @@ system the .unbind() method is called. All three callbacks are optional.
>  V4L2 sub-device userspace API
>  -----------------------------
>  
> -Beside exposing a kernel API through the :c:type:`v4l2_subdev_ops` structure,
> -V4L2 sub-devices can also be controlled directly by userspace applications.
> +Bridge drivers traditionally expose one or multiple video nodes to userspace,
> +and control subdevices through the :c:type:`v4l2_subdev_ops` operations in
> +response to video node operations. This hides the complexity of the underlying
> +hardware from applications. For complex devices, finer-grained control of the
> +device than what the video nodes offer may be required. In those cases, bridge
> +drivers that implement :ref:`the media controller API <media_controller>` may
> +opt for making the subdevice operations directly accessible from userpace.
>  
>  Device nodes named ``v4l-subdev``\ *X* can be created in ``/dev`` to access
>  sub-devices directly. If a sub-device supports direct userspace configuration
> 
> > +
> > +Although, it is sometime useful to report to userspace the current subdevice
> 
> s/Although/However/
> s/sometime/sometimes/
> 
> > +configuration through a read-only API, that do not allow any change to the
> > +device parameters but allows userspace applications to interface to the
> > +subdevice device node and inspect them. To register a read-only device node for
> > +all the registered sub-devices marked with ``V4L2_SUBDEV_FL_HAS_DEVNODE``
> > +the :c:type:`v4l2_device` driver should call
> > +:c:func:`v4l2_device_register_ro_subdev_nodes`.
> 
> This would become
> 
> It is sometimes useful to report detailed information about subdevice
> configuration to userspace without exposing full direct control of the
> subdevice. 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 opt for
> exposing subdevice operations to userspace in a read-only fashion.
> 
> The subdevice read-only userspace API requires the subdevices to set the
> ``V4L2_SUBDEV_FL_HAS_DEVNODE`` before being registered, exactly as for the
> previously described subdevice userspace API. The :c:type:`v4l2_device`
> driver shall then call :c:func:`v4l2_device_register_ro_subdev_nodes` to
> create the subdevice device nodes.

I would like to publicly apologize for proposing a full rewrite of your
work. I realize how harsh this is, and I want to emphasize that I didn't
mean to imply in any way that your version was unacceptable or to
diminish your work. Please feel free to ignore these comments.

> > +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_FRAME_INTERVAL``
> > +``VIDIOC_SUBDEV_S_SELECTION``
> > +``VIDIOC_SUBDEV_S_DV_TIMINGS``
> > +``VIDIOC_SUBDEV_S_CROP``
> > +``VIDIOC_SUBDEV_S_STD``
> > +
> > +``VIDIOC_SUBDEV_S_FMT``, ``VIDIOC_SUBDEV_S_SELECTION`` and
> > +``VIDIOC_SUBDEV_S_CROP`` are only allowed on a read-only subdevice device node
> > +only if the format to modify is set to ``V4L2_SUBDEV_FORMAT_TRY`` (
> > +:ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`).
> > +
> > +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``, ``VIDIOC_S_DV_TIMINGS`` and
> > +``VIDIOC_SUBDEV_S_STD`` are always disallowed on a read-only subdevice node.
> > +
> > +In case the ioclt is not allowed at all or the format to modify is set to
> > +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and
> > +sets the errno variable to ``-EPERM``.
> 
> I would split the list in two.
> 
> ``VIDIOC_SUBDEV_S_FMT``,
> ``VIDIOC_SUBDEV_S_CROP``,
> ``VIDIOC_SUBDEV_S_SELECTION``:
> 
> 	These ioctls are only allowed for the
> 	:ref:`V4L2_SUBDEV_FORMAT_TRY <v4l2-subdev-format-whence>` formats and
> 	selection rectangles. Any attempt to access the
> 	:ref:`V4L2_SUBDEV_FORMAT_ACTIVE <v4l2-subdev-format-whence>`
> 	configuration results in a ``-EPERM`` error.
> 
> ``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,
> ``VIDIOC_SUBDEV_S_DV_TIMINGS``,
> ``VIDIOC_SUBDEV_S_STD``:
> 
> 	These ioctls are not allowed and result in a ``-EPERM`` error.
> 
> >  
> >  I2C sub-device drivers
> >  ----------------------
> > diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst
> > index 5712bd48e687..435d955aaf85 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``.
> 
> This looks good, but you also need to explain the read-only subdev API
> in dev-subdev.rst.
> 
> 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
>  ========
> 

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux