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@xxxxxxxxxx> > > >>>>> --- > > >>>>> 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
