[{"id":4350,"web_url":"https://patchwork.libcamera.org/comment/4350/","msgid":"<20200327233358.GD23713@kekkonen.localdomain>","date":"2020-03-27T23:33:58","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Jacopo,\n\nThanks for the patchset.\n\nOn Fri, Mar 27, 2020 at 11:35:21PM +0100, Jacopo Mondi wrote:\n> Document a new kapi function to register subdev device nodes in read only\n> mode and for each affected ioctl report how access is restricted.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> 8 files changed, 94 insertions(+)\n> \n> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> index 41ccb3e5c707..6506a673e6a1 100644\n> --- a/Documentation/media/kapi/v4l2-subdev.rst\n> +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> @@ -332,6 +332,50 @@ Private ioctls\n> \tAll ioctls not in the above list are passed directly to the sub-device\n> \tdriver through the core::ioctl operation.\n> \n> +Read-only sub-device userspace API\n> +----------------------------------\n> +\n> +Bridge drivers that control their connected subdevices through direct calls to\n> +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> +want userspace to be able to change the same parameters through the subdevice\n> +device node and thus do not usually register any.\n> +\n> +It is sometimes useful to report to userspace the current subdevice\n> +configuration through a read-only API, that does not permit applications to\n> +change to the device parameters but allows interfacing to the subdevice device\n> +node to inspect them.\n> +\n> +For instance, to implement cameras based on computational photography, userspace\n> +needs to know the detailed camera sensor configuration (in terms of skipping,\n> +binning, cropping and scaling) for each supported output resolution. To support\n> +such use cases, bridge drivers may expose the subdevice operations to userspace\n> +through a read-only API.\n> +\n> +To create a read-only device node for all the subdevices registered with the\n> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> +\n> +Access to the following ioctls for userspace applications is restricted on\n> +sub-device device nodes registered with\n> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> +\n> +``VIDIOC_SUBDEV_S_FMT``,\n> +``VIDIOC_SUBDEV_S_CROP``,\n> +``VIDIOC_SUBDEV_S_SELECTION``:\n> +\n> +\tThese ioctls are only allowed on a read-only subdevice device node\n> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> +\tformats and selection rectangles.\n> +\n> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> +``VIDIOC_SUBDEV_S_STD``:\n> +\n> +\tThese ioctls are not allowed on a read-only subdevice node.\n> +\n> +In case the ioclt is not allowed, or the format to modify is set to\n\n\"IOCTL\".\n\n> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> +the errno variable is set to ``-EPERM``.\n> \n> I2C sub-device drivers\n> ----------------------\n> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> index 029bb2d9928a..6082f9c2f8f4 100644\n> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> Sub-device character device nodes, conventionally named\n> ``/dev/v4l-subdev*``, use major number 81.\n> \n> +Drivers may opt to limit the sub-device character devices to only expose\n> +operations that don't modify the device state. In such a case the sub-devices\n> +are referred to as ``read-only`` in the rest of this documentation, and the\n> +related restrictions are documented in individual ioctls.\n\nPerhaps \"IOCTLs\".\n\nWith these, for the set,\n\nAcked-by: Sakari Ailus \n\n> +\n> \n> Controls\n> ========\n> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> index e36dd2622857..611f94e4510a 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> structure as argument. If the ioctl is not supported or the timing\n> values are not correct, the driver returns ``EINVAL`` error code.\n> \n> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> +registered in read-only mode is not allowed. An error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> the current input or output does not support DV timings (e.g. if\n> @@ -81,6 +85,8 @@ ENODATA\n> EBUSY\n> The device is busy and therefore can not change the timings.\n> \n> +EPERM\n> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> \n> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> \n> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> index e633e42e3910..e220b38b859f 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> returned.\n> \n> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> +in read-only mode is not allowed. An error is returned and the errno variable is\n> +set to ``-EPERM``.\n> \n> Return Value\n> ============\n> @@ -79,3 +82,6 @@ EINVAL\n> \n> ENODATA\n> Standard video timings are not supported for this input or output.\n> +\n> +EPERM\n> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> index 632ee053accc..62f5d9870ca7 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> applications querying the same sub-device would thus not interact with\n> each other.\n> \n> +If the subdev device node has been registered in read-only mode calls to\n> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> Drivers must not return an error solely because the requested crop\n> rectangle doesn't match the device capabilities. They must instead\n> modify the rectangle to match what the hardware can provide. The\n> @@ -123,3 +128,7 @@ EINVAL\n> references a non-existing pad, the ``which`` field references a\n> non-existing format, or cropping is not supported on the given\n> subdev pad.\n> +\n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> index 472577bd1745..3a2f64bb00e7 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> a low-pass noise filter might crop pixels at the frame boundaries,\n> modifying its output frame size.\n> \n> +If the subdev device node has been registered in read-only mode calls to\n> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> Drivers must not return an error solely because the requested format\n> doesn't match the device capabilities. They must instead modify the\n> format to match what the hardware can provide. The modified format\n> @@ -146,6 +151,9 @@ EINVAL\n> ``pad`` references a non-existing pad, or the ``which`` field\n> references a non-existing format.\n> \n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> \n> ============\n> \n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> index 4b1b4bc78bfe..34aa39096e3d 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> @@ -65,6 +65,10 @@ struct\n> contains the current frame interval as would be returned by a\n> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> \n> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> +registered in read-only mode is not allowed. An error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> Drivers must not return an error solely because the requested interval\n> doesn't match the device capabilities. They must instead modify the\n> interval to match what the hardware can provide. The modified interval\n> @@ -118,3 +122,7 @@ EINVAL\n> :c:type:`v4l2_subdev_frame_interval`\n> ``pad`` references a non-existing pad, or the pad doesn't support\n> frame intervals.\n> +\n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> + subdevice.\n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> index fc73d27e6d74..abd046cef612 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> See :ref:`subdev` for more information on how each selection target\n> affects the image processing pipeline inside the subdevice.\n> \n> +If the subdev device node has been registered in read-only mode calls to\n> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> +variable is set to ``-EPERM``.\n> \n> Types of selection targets\n> --------------------------\n> @@ -123,3 +127,7 @@ EINVAL\n> ``pad`` references a non-existing pad, the ``which`` field\n> references a non-existing format, or the selection target is not\n> supported on the given subdev pad.\n> +\n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.","headers":{"Return-Path":"","Received":["from mga01.intel.com (mga01.intel.com [192.55.52.88])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83D1B60412\n\tfor ;\n\tSat, 28 Mar 2020 00:34:09 +0100 (CET)","from orsmga002.jf.intel.com ([10.7.209.21])\n\tby fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t27 Mar 2020 16:34:06 -0700","from jmikkola-mobl1.ger.corp.intel.com (HELO kekkonen.fi.intel.com)\n\t([10.252.32.179])\n\tby orsmga002.jf.intel.com with ESMTP; 27 Mar 2020 16:34:03 -0700","by kekkonen.fi.intel.com (Postfix, from userid 1000)\n\tid 1477721FB9; Sat, 28 Mar 2020 01:33:59 +0200 (EET)"],"IronPort-SDR":["k0bMOl5+Sed+Ps/OMEzZE14xDAbr5S0PNUPNvAdOMg3biL2ArQxNFUbq32HCZ4M9dHjxSjgKe0\n\tAV9Hg1ZD/0BQ==","wP5Tm2FTD9v/uun686jt+X6X++PQMezfxIhkc4DjzqYTO608VJwdKbIwrpB4/u0DL0iYXJ8xlP\n\t57cgyhfEJq9Q=="],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.72,314,1580803200\"; d=\"scan'208\";a=\"266362538\"","Date":"Sat, 28 Mar 2020 01:33:58 +0200","From":"Sakari Ailus ","To":"Jacopo Mondi ","Cc":"linux-media@vger.kernel.org, libcamera-devel@lists.libcamera.org,\n\tmchehab@kernel.org, hverkuil-cisco@xs4all.nl,\n\tandrey.konovalov@linaro.org, laurent.pinchart@ideasonboard.com","Message-ID":"<20200327233358.GD23713@kekkonen.localdomain>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20200327223522.506832-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 27 Mar 2020 23:34:10 -0000"}},{"id":4358,"web_url":"https://patchwork.libcamera.org/comment/4358/","msgid":"<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>","date":"2020-04-01T11:19:00","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":43,"url":"https://patchwork.libcamera.org/api/people/43/","name":"Hans Verkuil","email":"hverkuil-cisco@xs4all.nl"},"content":"Hi Jacopo,\n\nSome comments below:\n\nOn 3/27/20 11:35 PM, Jacopo Mondi wrote:\n> Document a new kapi function to register subdev device nodes in read only\n\nkAPI\n\n> mode and for each affected ioctl report how access is restricted.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> 8 files changed, 94 insertions(+)\n> \n> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> index 41ccb3e5c707..6506a673e6a1 100644\n> --- a/Documentation/media/kapi/v4l2-subdev.rst\n> +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> @@ -332,6 +332,50 @@ Private ioctls\n> \tAll ioctls not in the above list are passed directly to the sub-device\n> \tdriver through the core::ioctl operation.\n> \n> +Read-only sub-device userspace API\n> +----------------------------------\n> +\n> +Bridge drivers that control their connected subdevices through direct calls to\n> +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> +want userspace to be able to change the same parameters through the subdevice\n> +device node and thus do not usually register any.\n> +\n> +It is sometimes useful to report to userspace the current subdevice\n> +configuration through a read-only API, that does not permit applications to\n> +change to the device parameters but allows interfacing to the subdevice device\n> +node to inspect them.\n> +\n> +For instance, to implement cameras based on computational photography, userspace\n> +needs to know the detailed camera sensor configuration (in terms of skipping,\n> +binning, cropping and scaling) for each supported output resolution. To support\n> +such use cases, bridge drivers may expose the subdevice operations to userspace\n> +through a read-only API.\n> +\n> +To create a read-only device node for all the subdevices registered with the\n> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n\nShould we add something about creating a /dev/media device as well? It's basically\nrequired for this functionality.\n\nI think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\nunder #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\nto be able to call this function. Or possibly have an explicit test in\n__v4l2_device_register_subdev_nodes for the presence of a media device if\nread_only is true.\n\n> +\n> +Access to the following ioctls for userspace applications is restricted on\n> +sub-device device nodes registered with\n> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> +\n> +``VIDIOC_SUBDEV_S_FMT``,\n> +``VIDIOC_SUBDEV_S_CROP``,\n> +``VIDIOC_SUBDEV_S_SELECTION``:\n> +\n> +\tThese ioctls are only allowed on a read-only subdevice device node\n> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> +\tformats and selection rectangles.\n> +\n> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> +``VIDIOC_SUBDEV_S_STD``:\n> +\n> +\tThese ioctls are not allowed on a read-only subdevice node.\n> +\n> +In case the ioclt is not allowed, or the format to modify is set to\n> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> +the errno variable is set to ``-EPERM``.\n> \n> I2C sub-device drivers\n> ----------------------\n> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> index 029bb2d9928a..6082f9c2f8f4 100644\n> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> Sub-device character device nodes, conventionally named\n> ``/dev/v4l-subdev*``, use major number 81.\n> \n> +Drivers may opt to limit the sub-device character devices to only expose\n> +operations that don't modify the device state. In such a case the sub-devices\n\ndon't -> do not\n\n(\"don't\" is a bit too informal)\n\n> +are referred to as ``read-only`` in the rest of this documentation, and the\n> +related restrictions are documented in individual ioctls.\n> +\n> \n> Controls\n> ========\n> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> index e36dd2622857..611f94e4510a 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> structure as argument. If the ioctl is not supported or the timing\n> values are not correct, the driver returns ``EINVAL`` error code.\n> \n> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> +registered in read-only mode is not allowed. An error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> the current input or output does not support DV timings (e.g. if\n> @@ -81,6 +85,8 @@ ENODATA\n> EBUSY\n> The device is busy and therefore can not change the timings.\n> \n> +EPERM\n> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> \n> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> \n> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> index e633e42e3910..e220b38b859f 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> returned.\n> \n> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> +in read-only mode is not allowed. An error is returned and the errno variable is\n> +set to ``-EPERM``.\n> \n> Return Value\n> ============\n> @@ -79,3 +82,6 @@ EINVAL\n> \n> ENODATA\n> Standard video timings are not supported for this input or output.\n> +\n> +EPERM\n> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> index 632ee053accc..62f5d9870ca7 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> applications querying the same sub-device would thus not interact with\n> each other.\n> \n> +If the subdev device node has been registered in read-only mode calls to\n\nmode calls -> mode, calls\n\n> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> Drivers must not return an error solely because the requested crop\n> rectangle doesn't match the device capabilities. They must instead\n> modify the rectangle to match what the hardware can provide. The\n> @@ -123,3 +128,7 @@ EINVAL\n> references a non-existing pad, the ``which`` field references a\n> non-existing format, or cropping is not supported on the given\n> subdev pad.\n> +\n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> index 472577bd1745..3a2f64bb00e7 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> a low-pass noise filter might crop pixels at the frame boundaries,\n> modifying its output frame size.\n> \n> +If the subdev device node has been registered in read-only mode calls to\n\nditto.\n\n> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> Drivers must not return an error solely because the requested format\n> doesn't match the device capabilities. They must instead modify the\n> format to match what the hardware can provide. The modified format\n> @@ -146,6 +151,9 @@ EINVAL\n> ``pad`` references a non-existing pad, or the ``which`` field\n> references a non-existing format.\n> \n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> \n> ============\n> \n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> index 4b1b4bc78bfe..34aa39096e3d 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> @@ -65,6 +65,10 @@ struct\n> contains the current frame interval as would be returned by a\n> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> \n> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> +registered in read-only mode is not allowed. An error is returned and the errno\n> +variable is set to ``-EPERM``.\n> +\n> Drivers must not return an error solely because the requested interval\n> doesn't match the device capabilities. They must instead modify the\n> interval to match what the hardware can provide. The modified interval\n> @@ -118,3 +122,7 @@ EINVAL\n> :c:type:`v4l2_subdev_frame_interval`\n> ``pad`` references a non-existing pad, or the pad doesn't support\n> frame intervals.\n> +\n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> + subdevice.\n> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> index fc73d27e6d74..abd046cef612 100644\n> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> See :ref:`subdev` for more information on how each selection target\n> affects the image processing pipeline inside the subdevice.\n> \n> +If the subdev device node has been registered in read-only mode calls to\n\nditto\n\n> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> +variable is set to ``-EPERM``.\n> \n> Types of selection targets\n> --------------------------\n> @@ -123,3 +127,7 @@ EINVAL\n> ``pad`` references a non-existing pad, the ``which`` field\n> references a non-existing format, or the selection target is not\n> supported on the given subdev pad.\n> +\n> +EPERM\n> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> \n\nRegards,\n\n\tHans","headers":{"Return-Path":"","Received":["from lb3-smtp-cloud9.xs4all.net (lb3-smtp-cloud9.xs4all.net\n\t[194.109.24.30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 776D960105\n\tfor ;\n\tWed, 1 Apr 2020 13:19:07 +0200 (CEST)","from [192.168.2.10] ([46.9.234.233])\n\tby smtp-cloud9.xs4all.net with ESMTPA\n\tid JbOejEaAtfHuvJbOijE93z; Wed, 01 Apr 2020 13:19:07 +0200"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=xs4all.nl header.i=@xs4all.nl\n\theader.b=\"mhztBS+r\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s1;\n\tt=1585739947; bh=uqPLRrALNQNMHOAuPfLNfqgMziZM77nKqeFI6Bd3NgI=;\n\th=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type:From:\n\tSubject;\n\tb=mhztBS+rIMq+6gI8ni8NF/q3gz1wtv8ZvFGmmRejBT51SB9qE8a/zf5DMZBN3coLg\n\tZogH+Gyv6rsP9z8r9WY8FScWvMPm0gNSz9Whp/gbFmhMaWR9xkcf4ASjHfuQ9GfArr\n\tPdW2JoRV2uGpBEi5vlEBji53AbB62yVw46VHyWzSj8ydNMd1VH4+Qx3HLIBxVFd0ZF\n\t+6RB1Gv7EcJ+cw3U3NkvxQT3qMp4hvllzlaTVt7paaZSB1YK93Vi2DmrTqBf2L+bUI\n\t5aeVrwabe3ncfDL4N7ZaUOa0fyD44fnsiW2oM1iZBKJptSqE5IGGMdn8rlHgccePz3\n\tG4cnI/kBzOJnw==","To":"Jacopo Mondi , linux-media@vger.kernel.org,\n\tlibcamera-devel@lists.libcamera.org","Cc":"mchehab@kernel.org, sakari.ailus@linux.intel.com,\n\tandrey.konovalov@linaro.org, laurent.pinchart@ideasonboard.com","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>","From":"Hans Verkuil ","Message-ID":"<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>","Date":"Wed, 1 Apr 2020 13:19:00 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.5.0","MIME-Version":"1.0","In-Reply-To":"<20200327223522.506832-3-jacopo@jmondi.org>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfKH56yuZr98uEsWjD2WaD7WNZXxX4ACSScW3p055c9dh5ZVpMyrRfTcSy65jWiJS8DdUc3kAuHAibSlTKdG0TPIfow10TU5N5gzlWQKboRgVwOS/CPgd\n\tk6SE5gWGOGiu7fQlbjbcqpQUBveKOOVoijiWOdEhjDje5khjrBzL8CcUorMMB52EG4ABc5RYcG59o+BYffhWvQrswnGhJcMY6sN5tHU+hBNQsFF9Ihy36fhg\n\taZ7CqVfKFJHjbjtYgBwDz0V+B2QOJ6mmDUYubr/6UaamURw+u9Ql8suSuD/i94dm/SIqzgh+L1wZX4LDB5ukaeAb9QamGDOHFuc6hidFNHA0qnnEpK5g642/\n\tYhzY8m+JSrv80iO96KlQRK6ztaW2mj8XVr3g/UHZQmKKLjutFIHPa8Gh+k5aEKAEovwmNou/","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 01 Apr 2020 11:19:07 -0000"}},{"id":4361,"web_url":"https://patchwork.libcamera.org/comment/4361/","msgid":"<20200401114611.GC4876@pendragon.ideasonboard.com>","date":"2020-04-01T11:46:11","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote:\n> On 3/27/20 11:35 PM, Jacopo Mondi wrote:\n> > Document a new kapi function to register subdev device nodes in read only\n> \n> kAPI\n> \n> > mode and for each affected ioctl report how access is restricted.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> > 8 files changed, 94 insertions(+)\n> > \n> > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> > index 41ccb3e5c707..6506a673e6a1 100644\n> > --- a/Documentation/media/kapi/v4l2-subdev.rst\n> > +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> > @@ -332,6 +332,50 @@ Private ioctls\n> > \tAll ioctls not in the above list are passed directly to the sub-device\n> > \tdriver through the core::ioctl operation.\n> > \n> > +Read-only sub-device userspace API\n> > +----------------------------------\n> > +\n> > +Bridge drivers that control their connected subdevices through direct calls to\n> > +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> > +want userspace to be able to change the same parameters through the subdevice\n> > +device node and thus do not usually register any.\n> > +\n> > +It is sometimes useful to report to userspace the current subdevice\n> > +configuration through a read-only API, that does not permit applications to\n> > +change to the device parameters but allows interfacing to the subdevice device\n> > +node to inspect them.\n> > +\n> > +For instance, to implement cameras based on computational photography, userspace\n> > +needs to know the detailed camera sensor configuration (in terms of skipping,\n> > +binning, cropping and scaling) for each supported output resolution. To support\n> > +such use cases, bridge drivers may expose the subdevice operations to userspace\n> > +through a read-only API.\n> > +\n> > +To create a read-only device node for all the subdevices registered with the\n> > +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> > +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> \n> Should we add something about creating a /dev/media device as well? It's basically\n> required for this functionality.\n\nI'm not opposed to that, but I don't think this should be specific to\nthe read-only API, as it's a shared requirement with thr R/W API. The\nprevious section, \"V4L2 sub-device userspace API\", doesn't mention media\ndevices.\n\n> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\n> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\n> to be able to call this function. Or possibly have an explicit test in\n> __v4l2_device_register_subdev_nodes for the presence of a media device if\n> read_only is true.\n\nVIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that\nactually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify\nthis and make MEDIA_CONTROLLER a requirement for any userspace access\nfrom userspace.\n\n> > +\n> > +Access to the following ioctls for userspace applications is restricted on\n> > +sub-device device nodes registered with\n> > +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > +\n> > +``VIDIOC_SUBDEV_S_FMT``,\n> > +``VIDIOC_SUBDEV_S_CROP``,\n> > +``VIDIOC_SUBDEV_S_SELECTION``:\n> > +\n> > +\tThese ioctls are only allowed on a read-only subdevice device node\n> > +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> > +\tformats and selection rectangles.\n> > +\n> > +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> > +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> > +``VIDIOC_SUBDEV_S_STD``:\n> > +\n> > +\tThese ioctls are not allowed on a read-only subdevice node.\n> > +\n> > +In case the ioclt is not allowed, or the format to modify is set to\n> > +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> > +the errno variable is set to ``-EPERM``.\n> > \n> > I2C sub-device drivers\n> > ----------------------\n> > diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > index 029bb2d9928a..6082f9c2f8f4 100644\n> > --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> > +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> > Sub-device character device nodes, conventionally named\n> > ``/dev/v4l-subdev*``, use major number 81.\n> > \n> > +Drivers may opt to limit the sub-device character devices to only expose\n> > +operations that don't modify the device state. In such a case the sub-devices\n> \n> don't -> do not\n> \n> (\"don't\" is a bit too informal)\n> \n> > +are referred to as ``read-only`` in the rest of this documentation, and the\n> > +related restrictions are documented in individual ioctls.\n> > +\n> > \n> > Controls\n> > ========\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > index e36dd2622857..611f94e4510a 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> > structure as argument. If the ioctl is not supported or the timing\n> > values are not correct, the driver returns ``EINVAL`` error code.\n> > \n> > +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> > +registered in read-only mode is not allowed. An error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> > the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> > the current input or output does not support DV timings (e.g. if\n> > @@ -81,6 +85,8 @@ ENODATA\n> > EBUSY\n> > The device is busy and therefore can not change the timings.\n> > \n> > +EPERM\n> > + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> > \n> > .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> > \n> > diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > index e633e42e3910..e220b38b859f 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> > does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> > returned.\n> > \n> > +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> > +in read-only mode is not allowed. An error is returned and the errno variable is\n> > +set to ``-EPERM``.\n> > \n> > Return Value\n> > ============\n> > @@ -79,3 +82,6 @@ EINVAL\n> > \n> > ENODATA\n> > Standard video timings are not supported for this input or output.\n> > +\n> > +EPERM\n> > + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > index 632ee053accc..62f5d9870ca7 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> > applications querying the same sub-device would thus not interact with\n> > each other.\n> > \n> > +If the subdev device node has been registered in read-only mode calls to\n> \n> mode calls -> mode, calls\n> \n> > +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > Drivers must not return an error solely because the requested crop\n> > rectangle doesn't match the device capabilities. They must instead\n> > modify the rectangle to match what the hardware can provide. The\n> > @@ -123,3 +128,7 @@ EINVAL\n> > references a non-existing pad, the ``which`` field references a\n> > non-existing format, or cropping is not supported on the given\n> > subdev pad.\n> > +\n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> > + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > index 472577bd1745..3a2f64bb00e7 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> > a low-pass noise filter might crop pixels at the frame boundaries,\n> > modifying its output frame size.\n> > \n> > +If the subdev device node has been registered in read-only mode calls to\n> \n> ditto.\n> \n> > +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > Drivers must not return an error solely because the requested format\n> > doesn't match the device capabilities. They must instead modify the\n> > format to match what the hardware can provide. The modified format\n> > @@ -146,6 +151,9 @@ EINVAL\n> > ``pad`` references a non-existing pad, or the ``which`` field\n> > references a non-existing format.\n> > \n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> > + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > \n> > ============\n> > \n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > index 4b1b4bc78bfe..34aa39096e3d 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > @@ -65,6 +65,10 @@ struct\n> > contains the current frame interval as would be returned by a\n> > ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> > \n> > +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> > +registered in read-only mode is not allowed. An error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > Drivers must not return an error solely because the requested interval\n> > doesn't match the device capabilities. They must instead modify the\n> > interval to match what the hardware can provide. The modified interval\n> > @@ -118,3 +122,7 @@ EINVAL\n> > :c:type:`v4l2_subdev_frame_interval`\n> > ``pad`` references a non-existing pad, or the pad doesn't support\n> > frame intervals.\n> > +\n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> > + subdevice.\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > index fc73d27e6d74..abd046cef612 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> > See :ref:`subdev` for more information on how each selection target\n> > affects the image processing pipeline inside the subdevice.\n> > \n> > +If the subdev device node has been registered in read-only mode calls to\n> \n> ditto\n> \n> > +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > \n> > Types of selection targets\n> > --------------------------\n> > @@ -123,3 +127,7 @@ EINVAL\n> > ``pad`` references a non-existing pad, the ``which`` field\n> > references a non-existing format, or the selection target is not\n> > supported on the given subdev pad.\n> > +\n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> > + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DE23260105\n\tfor ;\n\tWed, 1 Apr 2020 13:46:18 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 489C7A2A;\n\tWed, 1 Apr 2020 13:46:18 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TI2DNOsv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585741578;\n\tbh=ZfiQkL/LHC1gQxjdC+orexbm24E25wFMP1kj2SJpKKo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TI2DNOsvF6UP1fxK9s1gIgpio60lV1ugpZ7mjzMdklV4JlcRdO1CRqkK330IecSL+\n\tBZ8VOYqALOVQqcM050uUI8Em18MX8sTG8BeZNHEjHAS8c8r6cZDjYmwzkdQPT4kJDV\n\t+sTIvxEZDl8FcHdWjKdYSmag3mB+KgUeU/WTqt/w=","Date":"Wed, 1 Apr 2020 14:46:11 +0300","From":"Laurent Pinchart ","To":"Hans Verkuil ","Cc":"Jacopo Mondi , linux-media@vger.kernel.org,\n\tlibcamera-devel@lists.libcamera.org, mchehab@kernel.org,\n\tsakari.ailus@linux.intel.com, andrey.konovalov@linaro.org","Message-ID":"<20200401114611.GC4876@pendragon.ideasonboard.com>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 01 Apr 2020 11:46:19 -0000"}},{"id":4362,"web_url":"https://patchwork.libcamera.org/comment/4362/","msgid":"","date":"2020-04-01T11:54:13","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":43,"url":"https://patchwork.libcamera.org/api/people/43/","name":"Hans Verkuil","email":"hverkuil-cisco@xs4all.nl"},"content":"On 4/1/20 1:46 PM, Laurent Pinchart wrote:\n> Hi Hans,\n> \n> On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote:\n>> On 3/27/20 11:35 PM, Jacopo Mondi wrote:\n>>> Document a new kapi function to register subdev device nodes in read only\n>>\n>> kAPI\n>>\n>>> mode and for each affected ioctl report how access is restricted.\n>>>\n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n>>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n>>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n>>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n>>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n>>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n>>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n>>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n>>> 8 files changed, 94 insertions(+)\n>>>\n>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n>>> index 41ccb3e5c707..6506a673e6a1 100644\n>>> --- a/Documentation/media/kapi/v4l2-subdev.rst\n>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst\n>>> @@ -332,6 +332,50 @@ Private ioctls\n>>> \tAll ioctls not in the above list are passed directly to the sub-device\n>>> \tdriver through the core::ioctl operation.\n>>> \n>>> +Read-only sub-device userspace API\n>>> +----------------------------------\n>>> +\n>>> +Bridge drivers that control their connected subdevices through direct calls to\n>>> +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n>>> +want userspace to be able to change the same parameters through the subdevice\n>>> +device node and thus do not usually register any.\n>>> +\n>>> +It is sometimes useful to report to userspace the current subdevice\n>>> +configuration through a read-only API, that does not permit applications to\n>>> +change to the device parameters but allows interfacing to the subdevice device\n>>> +node to inspect them.\n>>> +\n>>> +For instance, to implement cameras based on computational photography, userspace\n>>> +needs to know the detailed camera sensor configuration (in terms of skipping,\n>>> +binning, cropping and scaling) for each supported output resolution. To support\n>>> +such use cases, bridge drivers may expose the subdevice operations to userspace\n>>> +through a read-only API.\n>>> +\n>>> +To create a read-only device node for all the subdevices registered with the\n>>> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n>>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n>>\n>> Should we add something about creating a /dev/media device as well? It's basically\n>> required for this functionality.\n> \n> I'm not opposed to that, but I don't think this should be specific to\n> the read-only API, as it's a shared requirement with thr R/W API. The\n> previous section, \"V4L2 sub-device userspace API\", doesn't mention media\n> devices.\n\nTrue.\n\n> \n>> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\n>> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\n>> to be able to call this function. Or possibly have an explicit test in\n>> __v4l2_device_register_subdev_nodes for the presence of a media device if\n>> read_only is true.\n> \n> VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that\n> actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify\n> this and make MEDIA_CONTROLLER a requirement for any userspace access\n> from userspace.\n\nI agree.\n\nRegards,\n\n\tHans\n\n> \n>>> +\n>>> +Access to the following ioctls for userspace applications is restricted on\n>>> +sub-device device nodes registered with\n>>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n>>> +\n>>> +``VIDIOC_SUBDEV_S_FMT``,\n>>> +``VIDIOC_SUBDEV_S_CROP``,\n>>> +``VIDIOC_SUBDEV_S_SELECTION``:\n>>> +\n>>> +\tThese ioctls are only allowed on a read-only subdevice device node\n>>> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n>>> +\tformats and selection rectangles.\n>>> +\n>>> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n>>> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n>>> +``VIDIOC_SUBDEV_S_STD``:\n>>> +\n>>> +\tThese ioctls are not allowed on a read-only subdevice node.\n>>> +\n>>> +In case the ioclt is not allowed, or the format to modify is set to\n>>> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n>>> +the errno variable is set to ``-EPERM``.\n>>> \n>>> I2C sub-device drivers\n>>> ----------------------\n>>> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n>>> index 029bb2d9928a..6082f9c2f8f4 100644\n>>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n>>> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n>>> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n>>> Sub-device character device nodes, conventionally named\n>>> ``/dev/v4l-subdev*``, use major number 81.\n>>> \n>>> +Drivers may opt to limit the sub-device character devices to only expose\n>>> +operations that don't modify the device state. In such a case the sub-devices\n>>\n>> don't -> do not\n>>\n>> (\"don't\" is a bit too informal)\n>>\n>>> +are referred to as ``read-only`` in the rest of this documentation, and the\n>>> +related restrictions are documented in individual ioctls.\n>>> +\n>>> \n>>> Controls\n>>> ========\n>>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n>>> index e36dd2622857..611f94e4510a 100644\n>>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n>>> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n>>> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n>>> structure as argument. If the ioctl is not supported or the timing\n>>> values are not correct, the driver returns ``EINVAL`` error code.\n>>> \n>>> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n>>> +registered in read-only mode is not allowed. An error is returned and the errno\n>>> +variable is set to ``-EPERM``.\n>>> +\n>>> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n>>> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n>>> the current input or output does not support DV timings (e.g. if\n>>> @@ -81,6 +85,8 @@ ENODATA\n>>> EBUSY\n>>> The device is busy and therefore can not change the timings.\n>>> \n>>> +EPERM\n>>> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n>>> \n>>> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n>>> \n>>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n>>> index e633e42e3910..e220b38b859f 100644\n>>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n>>> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n>>> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n>>> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n>>> returned.\n>>> \n>>> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n>>> +in read-only mode is not allowed. An error is returned and the errno variable is\n>>> +set to ``-EPERM``.\n>>> \n>>> Return Value\n>>> ============\n>>> @@ -79,3 +82,6 @@ EINVAL\n>>> \n>>> ENODATA\n>>> Standard video timings are not supported for this input or output.\n>>> +\n>>> +EPERM\n>>> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n>>> index 632ee053accc..62f5d9870ca7 100644\n>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n>>> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n>>> applications querying the same sub-device would thus not interact with\n>>> each other.\n>>> \n>>> +If the subdev device node has been registered in read-only mode calls to\n>>\n>> mode calls -> mode, calls\n>>\n>>> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n>>> +variable is set to ``-EPERM``.\n>>> +\n>>> Drivers must not return an error solely because the requested crop\n>>> rectangle doesn't match the device capabilities. They must instead\n>>> modify the rectangle to match what the hardware can provide. The\n>>> @@ -123,3 +128,7 @@ EINVAL\n>>> references a non-existing pad, the ``which`` field references a\n>>> non-existing format, or cropping is not supported on the given\n>>> subdev pad.\n>>> +\n>>> +EPERM\n>>> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n>>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n>>> index 472577bd1745..3a2f64bb00e7 100644\n>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n>>> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n>>> a low-pass noise filter might crop pixels at the frame boundaries,\n>>> modifying its output frame size.\n>>> \n>>> +If the subdev device node has been registered in read-only mode calls to\n>>\n>> ditto.\n>>\n>>> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n>>> +variable is set to ``-EPERM``.\n>>> +\n>>> Drivers must not return an error solely because the requested format\n>>> doesn't match the device capabilities. They must instead modify the\n>>> format to match what the hardware can provide. The modified format\n>>> @@ -146,6 +151,9 @@ EINVAL\n>>> ``pad`` references a non-existing pad, or the ``which`` field\n>>> references a non-existing format.\n>>> \n>>> +EPERM\n>>> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n>>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n>>> \n>>> ============\n>>> \n>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n>>> index 4b1b4bc78bfe..34aa39096e3d 100644\n>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n>>> @@ -65,6 +65,10 @@ struct\n>>> contains the current frame interval as would be returned by a\n>>> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n>>> \n>>> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n>>> +registered in read-only mode is not allowed. An error is returned and the errno\n>>> +variable is set to ``-EPERM``.\n>>> +\n>>> Drivers must not return an error solely because the requested interval\n>>> doesn't match the device capabilities. They must instead modify the\n>>> interval to match what the hardware can provide. The modified interval\n>>> @@ -118,3 +122,7 @@ EINVAL\n>>> :c:type:`v4l2_subdev_frame_interval`\n>>> ``pad`` references a non-existing pad, or the pad doesn't support\n>>> frame intervals.\n>>> +\n>>> +EPERM\n>>> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n>>> + subdevice.\n>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n>>> index fc73d27e6d74..abd046cef612 100644\n>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n>>> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n>>> See :ref:`subdev` for more information on how each selection target\n>>> affects the image processing pipeline inside the subdevice.\n>>> \n>>> +If the subdev device node has been registered in read-only mode calls to\n>>\n>> ditto\n>>\n>>> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n>>> +variable is set to ``-EPERM``.\n>>> \n>>> Types of selection targets\n>>> --------------------------\n>>> @@ -123,3 +127,7 @@ EINVAL\n>>> ``pad`` references a non-existing pad, the ``which`` field\n>>> references a non-existing format, or the selection target is not\n>>> supported on the given subdev pad.\n>>> +\n>>> +EPERM\n>>> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n>>> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n>>>\n>","headers":{"Return-Path":"","Received":["from lb1-smtp-cloud9.xs4all.net (lb1-smtp-cloud9.xs4all.net\n\t[194.109.24.22])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6DE0A60105\n\tfor ;\n\tWed, 1 Apr 2020 13:54:17 +0200 (CEST)","from [192.168.2.10] ([46.9.234.233])\n\tby smtp-cloud9.xs4all.net with ESMTPA\n\tid JbwjjEpESfHuvJbwmjEJEG; Wed, 01 Apr 2020 13:54:17 +0200"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=xs4all.nl header.i=@xs4all.nl\n\theader.b=\"KuGd6+GF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=xs4all.nl; s=s1;\n\tt=1585742057; bh=KgVbGDRdYxJ2JpcxMu/Q4D4PaorqPgt09eoLqZv61wY=;\n\th=Subject:To:From:Message-ID:Date:MIME-Version:Content-Type:From:\n\tSubject;\n\tb=KuGd6+GFEGhUpCib3Sj3wVS8RPs4g+AYVPtkN6YZmLazQK2OLKkmR2BagibsevPnD\n\t55pGKiUNAaLh0+cvmFBG1VSgo5Pe3Wg8+qzz5C5GOUD0IdIYM/NSTMmwkq9KeN/y4d\n\te64NWTfNl/UwZbZ6igMygaWagH3jbteW44NGn+IPuw9V5TPrDEMmbpylmVT8xDISHG\n\twojfvTEHgB6TVwJ6ylnsuEahqNhfh6IPd/5J/wbOnatbs49LV2gWaEO++2h7dRaZC5\n\tU+1yVhBgIHEtuAWzJHx5Qw/biigmhvjSM0DZb5W+dKWrD3MKPZsRzN5d5nv3nOQoPY\n\tw7wNP6vPJ1OlA==","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , linux-media@vger.kernel.org,\n\tlibcamera-devel@lists.libcamera.org, mchehab@kernel.org,\n\tsakari.ailus@linux.intel.com, andrey.konovalov@linaro.org","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>\n\t<20200401114611.GC4876@pendragon.ideasonboard.com>","From":"Hans Verkuil ","Message-ID":"","Date":"Wed, 1 Apr 2020 13:54:13 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.5.0","MIME-Version":"1.0","In-Reply-To":"<20200401114611.GC4876@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-CMAE-Envelope":"MS4wfNc8xWzycQv7Zqy7PgYWNpTpHCti8O6a4t8I2gdxMjKkpf+4biVF6shYT4EG7QKEmxvowc87C3Fo4CSxkdPfh8Uq1WdX8779A033z5lbW3X0MLdM56Wg\n\tYoAdPr2BRUjmw6UFR9jr53NLHRyAdzMJMwn0d6dYUC5uCDANnpJVwJJ9aGUFXqeCpSlnNtAMzuIU6LafIpP4qruS71BUpgTmrfVCG1+uhIAtqfpf7rRHwWML\n\tShE+vAxfn0szVz/+c7yGECUSRFcppPTKGzGz5xk0Dcvas/Fa1sP908VT61OI3PpNeCYO5GwAYyGZOWSPgF386rGFB85C9mEWthIlhm/3++Fv41hLOQOfKSTb\n\tIkNRc7PEMNDkNKss535emNaphRnwTsj4YnAp1UAS6oeWgBTt/igDt1Y+XHhymu2ta3/NmFG1","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 01 Apr 2020 11:54:17 -0000"}},{"id":4364,"web_url":"https://patchwork.libcamera.org/comment/4364/","msgid":"<20200401123949.uv2wyhcnruikmqpf@uno.localdomain>","date":"2020-04-01T12:39:49","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hans, Laurent,\n\nOn Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote:\n> On 4/1/20 1:46 PM, Laurent Pinchart wrote:\n> > Hi Hans,\n> >\n> > On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote:\n> >> On 3/27/20 11:35 PM, Jacopo Mondi wrote:\n\n[snip]\n\n> >>> +To create a read-only device node for all the subdevices registered with the\n> >>> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> >>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> >>\n> >> Should we add something about creating a /dev/media device as well? It's basically\n> >> required for this functionality.\n> >\n> > I'm not opposed to that, but I don't think this should be specific to\n> > the read-only API, as it's a shared requirement with thr R/W API. The\n> > previous section, \"V4L2 sub-device userspace API\", doesn't mention media\n> > devices.\n>\n> True.\n>\n> >\n> >> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\n> >> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\n> >> to be able to call this function. Or possibly have an explicit test in\n> >> __v4l2_device_register_subdev_nodes for the presence of a media device if\n> >> read_only is true.\n> >\n> > VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that\n> > actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify\n> > this and make MEDIA_CONTROLLER a requirement for any userspace access\n> > from userspace.\n>\n> I agree.\n\nI initially looked into making this new function conditional on the\npresence of VIDEO_V4L2_SUBDEV_API, but then I've noticed only some part\nof the code in drivers/media/v4l2-core/v4l2-subdev.c is guarded by\nthat flag, and at the same time v4l2_device_register_subdev_nodes() was\nnot. So I assumed registering read-only wihtout V4L2_SUBDEV_API should\nhave been allowed in the same way, even if in case V4L2_SUBDEV_API is\nnot enabled, ro or rw does not actually make any difference.\n\nHow would you like to proceed forward ? Guarding\nv4l2_device_register_[ro_]subdev_node() with MEDIA_CONTROLLER flag ?\n\nThanks\n j\n\n>\n> Regards,\n>\n> \tHans\n>\n> >\n> >>> +\n> >>> +Access to the following ioctls for userspace applications is restricted on\n> >>> +sub-device device nodes registered with\n> >>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> >>> +\n> >>> +``VIDIOC_SUBDEV_S_FMT``,\n> >>> +``VIDIOC_SUBDEV_S_CROP``,\n> >>> +``VIDIOC_SUBDEV_S_SELECTION``:\n> >>> +\n> >>> +\tThese ioctls are only allowed on a read-only subdevice device node\n> >>> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> >>> +\tformats and selection rectangles.\n> >>> +\n> >>> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> >>> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> >>> +``VIDIOC_SUBDEV_S_STD``:\n> >>> +\n> >>> +\tThese ioctls are not allowed on a read-only subdevice node.\n> >>> +\n> >>> +In case the ioclt is not allowed, or the format to modify is set to\n> >>> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> >>> +the errno variable is set to ``-EPERM``.\n> >>>\n> >>> I2C sub-device drivers\n> >>> ----------------------\n> >>> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>> index 029bb2d9928a..6082f9c2f8f4 100644\n> >>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> >>> Sub-device character device nodes, conventionally named\n> >>> ``/dev/v4l-subdev*``, use major number 81.\n> >>>\n> >>> +Drivers may opt to limit the sub-device character devices to only expose\n> >>> +operations that don't modify the device state. In such a case the sub-devices\n> >>\n> >> don't -> do not\n> >>\n> >> (\"don't\" is a bit too informal)\n> >>\n> >>> +are referred to as ``read-only`` in the rest of this documentation, and the\n> >>> +related restrictions are documented in individual ioctls.\n> >>> +\n> >>>\n> >>> Controls\n> >>> ========\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>> index e36dd2622857..611f94e4510a 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> >>> structure as argument. If the ioctl is not supported or the timing\n> >>> values are not correct, the driver returns ``EINVAL`` error code.\n> >>>\n> >>> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> >>> +registered in read-only mode is not allowed. An error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> >>> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> >>> the current input or output does not support DV timings (e.g. if\n> >>> @@ -81,6 +85,8 @@ ENODATA\n> >>> EBUSY\n> >>> The device is busy and therefore can not change the timings.\n> >>>\n> >>> +EPERM\n> >>> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> >>>\n> >>> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> >>>\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>> index e633e42e3910..e220b38b859f 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> >>> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> >>> returned.\n> >>>\n> >>> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> >>> +in read-only mode is not allowed. An error is returned and the errno variable is\n> >>> +set to ``-EPERM``.\n> >>>\n> >>> Return Value\n> >>> ============\n> >>> @@ -79,3 +82,6 @@ EINVAL\n> >>>\n> >>> ENODATA\n> >>> Standard video timings are not supported for this input or output.\n> >>> +\n> >>> +EPERM\n> >>> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>> index 632ee053accc..62f5d9870ca7 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> >>> applications querying the same sub-device would thus not interact with\n> >>> each other.\n> >>>\n> >>> +If the subdev device node has been registered in read-only mode calls to\n> >>\n> >> mode calls -> mode, calls\n> >>\n> >>> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> Drivers must not return an error solely because the requested crop\n> >>> rectangle doesn't match the device capabilities. They must instead\n> >>> modify the rectangle to match what the hardware can provide. The\n> >>> @@ -123,3 +128,7 @@ EINVAL\n> >>> references a non-existing pad, the ``which`` field references a\n> >>> non-existing format, or cropping is not supported on the given\n> >>> subdev pad.\n> >>> +\n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> >>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>> index 472577bd1745..3a2f64bb00e7 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> >>> a low-pass noise filter might crop pixels at the frame boundaries,\n> >>> modifying its output frame size.\n> >>>\n> >>> +If the subdev device node has been registered in read-only mode calls to\n> >>\n> >> ditto.\n> >>\n> >>> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> Drivers must not return an error solely because the requested format\n> >>> doesn't match the device capabilities. They must instead modify the\n> >>> format to match what the hardware can provide. The modified format\n> >>> @@ -146,6 +151,9 @@ EINVAL\n> >>> ``pad`` references a non-existing pad, or the ``which`` field\n> >>> references a non-existing format.\n> >>>\n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> >>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >>>\n> >>> ============\n> >>>\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>> index 4b1b4bc78bfe..34aa39096e3d 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>> @@ -65,6 +65,10 @@ struct\n> >>> contains the current frame interval as would be returned by a\n> >>> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> >>>\n> >>> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> >>> +registered in read-only mode is not allowed. An error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> Drivers must not return an error solely because the requested interval\n> >>> doesn't match the device capabilities. They must instead modify the\n> >>> interval to match what the hardware can provide. The modified interval\n> >>> @@ -118,3 +122,7 @@ EINVAL\n> >>> :c:type:`v4l2_subdev_frame_interval`\n> >>> ``pad`` references a non-existing pad, or the pad doesn't support\n> >>> frame intervals.\n> >>> +\n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> >>> + subdevice.\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>> index fc73d27e6d74..abd046cef612 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> >>> See :ref:`subdev` for more information on how each selection target\n> >>> affects the image processing pipeline inside the subdevice.\n> >>>\n> >>> +If the subdev device node has been registered in read-only mode calls to\n> >>\n> >> ditto\n> >>\n> >>> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>>\n> >>> Types of selection targets\n> >>> --------------------------\n> >>> @@ -123,3 +127,7 @@ EINVAL\n> >>> ``pad`` references a non-existing pad, the ``which`` field\n> >>> references a non-existing format, or the selection target is not\n> >>> supported on the given subdev pad.\n> >>> +\n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> >>> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >>>\n> >\n>","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0A18060105\n\tfor ;\n\tWed, 1 Apr 2020 14:36:50 +0200 (CEST)","from uno.localdomain (93-34-114-233.ip49.fastwebnet.it\n\t[93.34.114.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 273FA40004;\n\tWed, 1 Apr 2020 12:36:47 +0000 (UTC)"],"X-Originating-IP":"93.34.114.233","Date":"Wed, 1 Apr 2020 14:39:49 +0200","From":"Jacopo Mondi ","To":"Hans Verkuil ","Cc":"Laurent Pinchart ,\n\tlinux-media@vger.kernel.org, libcamera-devel@lists.libcamera.org,\n\tmchehab@kernel.org, sakari.ailus@linux.intel.com,\n\tandrey.konovalov@linaro.org","Message-ID":"<20200401123949.uv2wyhcnruikmqpf@uno.localdomain>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>\n\t<20200401114611.GC4876@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 01 Apr 2020 12:36:50 -0000"}},{"id":4365,"web_url":"https://patchwork.libcamera.org/comment/4365/","msgid":"<20200401221021.kbdmurcrs2amsezs@uno.localdomain>","date":"2020-04-01T22:10:21","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Sakari,\n one small question\n\nOn Sat, Mar 28, 2020 at 01:33:58AM +0200, Sakari Ailus wrote:\n> Hi Jacopo,\n>\n> Thanks for the patchset.\n>\n> On Fri, Mar 27, 2020 at 11:35:21PM +0100, Jacopo Mondi wrote:\n> > Document a new kapi function to register subdev device nodes in read only\n> > mode and for each affected ioctl report how access is restricted.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> > 8 files changed, 94 insertions(+)\n> >\n> > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> > index 41ccb3e5c707..6506a673e6a1 100644\n> > --- a/Documentation/media/kapi/v4l2-subdev.rst\n> > +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> > @@ -332,6 +332,50 @@ Private ioctls\n> > \tAll ioctls not in the above list are passed directly to the sub-device\n> > \tdriver through the core::ioctl operation.\n> >\n> > +Read-only sub-device userspace API\n> > +----------------------------------\n> > +\n> > +Bridge drivers that control their connected subdevices through direct calls to\n> > +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> > +want userspace to be able to change the same parameters through the subdevice\n> > +device node and thus do not usually register any.\n> > +\n> > +It is sometimes useful to report to userspace the current subdevice\n> > +configuration through a read-only API, that does not permit applications to\n> > +change to the device parameters but allows interfacing to the subdevice device\n> > +node to inspect them.\n> > +\n> > +For instance, to implement cameras based on computational photography, userspace\n> > +needs to know the detailed camera sensor configuration (in terms of skipping,\n> > +binning, cropping and scaling) for each supported output resolution. To support\n> > +such use cases, bridge drivers may expose the subdevice operations to userspace\n> > +through a read-only API.\n> > +\n> > +To create a read-only device node for all the subdevices registered with the\n> > +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> > +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > +\n> > +Access to the following ioctls for userspace applications is restricted on\n> > +sub-device device nodes registered with\n> > +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > +\n> > +``VIDIOC_SUBDEV_S_FMT``,\n> > +``VIDIOC_SUBDEV_S_CROP``,\n> > +``VIDIOC_SUBDEV_S_SELECTION``:\n> > +\n> > +\tThese ioctls are only allowed on a read-only subdevice device node\n> > +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> > +\tformats and selection rectangles.\n> > +\n> > +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> > +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> > +``VIDIOC_SUBDEV_S_STD``:\n> > +\n> > +\tThese ioctls are not allowed on a read-only subdevice node.\n> > +\n> > +In case the ioclt is not allowed, or the format to modify is set to\n>\n> \"IOCTL\".\n\nare you referring to the typo ioclt/ioctl or are you suggesting to\nspell IOCTL uppercase ? As the rest of the documentation uses\nlowercase \"ioctl\" ...\n>\n> > +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> > +the errno variable is set to ``-EPERM``.\n> >\n> > I2C sub-device drivers\n> > ----------------------\n> > diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > index 029bb2d9928a..6082f9c2f8f4 100644\n> > --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> > +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> > Sub-device character device nodes, conventionally named\n> > ``/dev/v4l-subdev*``, use major number 81.\n> >\n> > +Drivers may opt to limit the sub-device character devices to only expose\n> > +operations that don't modify the device state. In such a case the sub-devices\n> > +are referred to as ``read-only`` in the rest of this documentation, and the\n> > +related restrictions are documented in individual ioctls.\n>\n> Perhaps \"IOCTLs\".\n\nSame here, the rest of the documentation uses lowercase.\n\nThanks\n j\n>\n> With these, for the set,\n>\n> Acked-by: Sakari Ailus \n>\n> > +\n> >\n> > Controls\n> > ========\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > index e36dd2622857..611f94e4510a 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> > structure as argument. If the ioctl is not supported or the timing\n> > values are not correct, the driver returns ``EINVAL`` error code.\n> >\n> > +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> > +registered in read-only mode is not allowed. An error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> > the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> > the current input or output does not support DV timings (e.g. if\n> > @@ -81,6 +85,8 @@ ENODATA\n> > EBUSY\n> > The device is busy and therefore can not change the timings.\n> >\n> > +EPERM\n> > + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> >\n> > .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> >\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > index e633e42e3910..e220b38b859f 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> > does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> > returned.\n> >\n> > +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> > +in read-only mode is not allowed. An error is returned and the errno variable is\n> > +set to ``-EPERM``.\n> >\n> > Return Value\n> > ============\n> > @@ -79,3 +82,6 @@ EINVAL\n> >\n> > ENODATA\n> > Standard video timings are not supported for this input or output.\n> > +\n> > +EPERM\n> > + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > index 632ee053accc..62f5d9870ca7 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> > applications querying the same sub-device would thus not interact with\n> > each other.\n> >\n> > +If the subdev device node has been registered in read-only mode calls to\n> > +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > Drivers must not return an error solely because the requested crop\n> > rectangle doesn't match the device capabilities. They must instead\n> > modify the rectangle to match what the hardware can provide. The\n> > @@ -123,3 +128,7 @@ EINVAL\n> > references a non-existing pad, the ``which`` field references a\n> > non-existing format, or cropping is not supported on the given\n> > subdev pad.\n> > +\n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> > + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > index 472577bd1745..3a2f64bb00e7 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> > a low-pass noise filter might crop pixels at the frame boundaries,\n> > modifying its output frame size.\n> >\n> > +If the subdev device node has been registered in read-only mode calls to\n> > +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > Drivers must not return an error solely because the requested format\n> > doesn't match the device capabilities. They must instead modify the\n> > format to match what the hardware can provide. The modified format\n> > @@ -146,6 +151,9 @@ EINVAL\n> > ``pad`` references a non-existing pad, or the ``which`` field\n> > references a non-existing format.\n> >\n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> > + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >\n> > ============\n> >\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > index 4b1b4bc78bfe..34aa39096e3d 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > @@ -65,6 +65,10 @@ struct\n> > contains the current frame interval as would be returned by a\n> > ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> >\n> > +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> > +registered in read-only mode is not allowed. An error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> > +\n> > Drivers must not return an error solely because the requested interval\n> > doesn't match the device capabilities. They must instead modify the\n> > interval to match what the hardware can provide. The modified interval\n> > @@ -118,3 +122,7 @@ EINVAL\n> > :c:type:`v4l2_subdev_frame_interval`\n> > ``pad`` references a non-existing pad, or the pad doesn't support\n> > frame intervals.\n> > +\n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> > + subdevice.\n> > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > index fc73d27e6d74..abd046cef612 100644\n> > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> > See :ref:`subdev` for more information on how each selection target\n> > affects the image processing pipeline inside the subdevice.\n> >\n> > +If the subdev device node has been registered in read-only mode calls to\n> > +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > +variable is set to ``-EPERM``.\n> >\n> > Types of selection targets\n> > --------------------------\n> > @@ -123,3 +127,7 @@ EINVAL\n> > ``pad`` references a non-existing pad, the ``which`` field\n> > references a non-existing format, or the selection target is not\n> > supported on the given subdev pad.\n> > +\n> > +EPERM\n> > + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> > + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n>\n> --\n> Regards,\n>\n> Sakari Ailus","headers":{"Return-Path":"","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6387B6040B\n\tfor ;\n\tThu, 2 Apr 2020 00:07:22 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id A4677240009;\n\tWed, 1 Apr 2020 22:07:19 +0000 (UTC)"],"Date":"Thu, 2 Apr 2020 00:10:21 +0200","From":"Jacopo Mondi ","To":"Sakari Ailus ","Cc":"linux-media@vger.kernel.org, libcamera-devel@lists.libcamera.org,\n\tmchehab@kernel.org, hverkuil-cisco@xs4all.nl,\n\tandrey.konovalov@linaro.org, laurent.pinchart@ideasonboard.com","Message-ID":"<20200401221021.kbdmurcrs2amsezs@uno.localdomain>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<20200327233358.GD23713@kekkonen.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200327233358.GD23713@kekkonen.localdomain>","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 01 Apr 2020 22:07:22 -0000"}},{"id":4383,"web_url":"https://patchwork.libcamera.org/comment/4383/","msgid":"<20200404013545.GJ9690@pendragon.ideasonboard.com>","date":"2020-04-04T01:35:45","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hans,\n\nOn Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote:\n> On 4/1/20 1:46 PM, Laurent Pinchart wrote:\n> > On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote:\n> >> On 3/27/20 11:35 PM, Jacopo Mondi wrote:\n> >>> Document a new kapi function to register subdev device nodes in read only\n> >>\n> >> kAPI\n> >>\n> >>> mode and for each affected ioctl report how access is restricted.\n> >>>\n> >>> Signed-off-by: Jacopo Mondi \n> >>> ---\n> >>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> >>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> >>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> >>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> >>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> >>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> >>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> >>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> >>> 8 files changed, 94 insertions(+)\n> >>>\n> >>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> >>> index 41ccb3e5c707..6506a673e6a1 100644\n> >>> --- a/Documentation/media/kapi/v4l2-subdev.rst\n> >>> +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> >>> @@ -332,6 +332,50 @@ Private ioctls\n> >>> \tAll ioctls not in the above list are passed directly to the sub-device\n> >>> \tdriver through the core::ioctl operation.\n> >>> \n> >>> +Read-only sub-device userspace API\n> >>> +----------------------------------\n> >>> +\n> >>> +Bridge drivers that control their connected subdevices through direct calls to\n> >>> +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> >>> +want userspace to be able to change the same parameters through the subdevice\n> >>> +device node and thus do not usually register any.\n> >>> +\n> >>> +It is sometimes useful to report to userspace the current subdevice\n> >>> +configuration through a read-only API, that does not permit applications to\n> >>> +change to the device parameters but allows interfacing to the subdevice device\n> >>> +node to inspect them.\n> >>> +\n> >>> +For instance, to implement cameras based on computational photography, userspace\n> >>> +needs to know the detailed camera sensor configuration (in terms of skipping,\n> >>> +binning, cropping and scaling) for each supported output resolution. To support\n> >>> +such use cases, bridge drivers may expose the subdevice operations to userspace\n> >>> +through a read-only API.\n> >>> +\n> >>> +To create a read-only device node for all the subdevices registered with the\n> >>> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> >>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> >>\n> >> Should we add something about creating a /dev/media device as well? It's basically\n> >> required for this functionality.\n> > \n> > I'm not opposed to that, but I don't think this should be specific to\n> > the read-only API, as it's a shared requirement with thr R/W API. The\n> > previous section, \"V4L2 sub-device userspace API\", doesn't mention media\n> > devices.\n> \n> True.\n> \n> >> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\n> >> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\n> >> to be able to call this function. Or possibly have an explicit test in\n> >> __v4l2_device_register_subdev_nodes for the presence of a media device if\n> >> read_only is true.\n> > \n> > VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that\n> > actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify\n> > this and make MEDIA_CONTROLLER a requirement for any userspace access\n> > from userspace.\n> \n> I agree.\n\nDoes that mean we should rework it as a prerequisite for this series, as\npart of the series, or separately ?\n\n> >>> +\n> >>> +Access to the following ioctls for userspace applications is restricted on\n> >>> +sub-device device nodes registered with\n> >>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> >>> +\n> >>> +``VIDIOC_SUBDEV_S_FMT``,\n> >>> +``VIDIOC_SUBDEV_S_CROP``,\n> >>> +``VIDIOC_SUBDEV_S_SELECTION``:\n> >>> +\n> >>> +\tThese ioctls are only allowed on a read-only subdevice device node\n> >>> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> >>> +\tformats and selection rectangles.\n> >>> +\n> >>> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> >>> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> >>> +``VIDIOC_SUBDEV_S_STD``:\n> >>> +\n> >>> +\tThese ioctls are not allowed on a read-only subdevice node.\n> >>> +\n> >>> +In case the ioclt is not allowed, or the format to modify is set to\n> >>> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> >>> +the errno variable is set to ``-EPERM``.\n> >>> \n> >>> I2C sub-device drivers\n> >>> ----------------------\n> >>> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>> index 029bb2d9928a..6082f9c2f8f4 100644\n> >>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> >>> Sub-device character device nodes, conventionally named\n> >>> ``/dev/v4l-subdev*``, use major number 81.\n> >>> \n> >>> +Drivers may opt to limit the sub-device character devices to only expose\n> >>> +operations that don't modify the device state. In such a case the sub-devices\n> >>\n> >> don't -> do not\n> >>\n> >> (\"don't\" is a bit too informal)\n> >>\n> >>> +are referred to as ``read-only`` in the rest of this documentation, and the\n> >>> +related restrictions are documented in individual ioctls.\n> >>> +\n> >>> \n> >>> Controls\n> >>> ========\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>> index e36dd2622857..611f94e4510a 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> >>> structure as argument. If the ioctl is not supported or the timing\n> >>> values are not correct, the driver returns ``EINVAL`` error code.\n> >>> \n> >>> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> >>> +registered in read-only mode is not allowed. An error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> >>> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> >>> the current input or output does not support DV timings (e.g. if\n> >>> @@ -81,6 +85,8 @@ ENODATA\n> >>> EBUSY\n> >>> The device is busy and therefore can not change the timings.\n> >>> \n> >>> +EPERM\n> >>> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> >>> \n> >>> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> >>> \n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>> index e633e42e3910..e220b38b859f 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> >>> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> >>> returned.\n> >>> \n> >>> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> >>> +in read-only mode is not allowed. An error is returned and the errno variable is\n> >>> +set to ``-EPERM``.\n> >>> \n> >>> Return Value\n> >>> ============\n> >>> @@ -79,3 +82,6 @@ EINVAL\n> >>> \n> >>> ENODATA\n> >>> Standard video timings are not supported for this input or output.\n> >>> +\n> >>> +EPERM\n> >>> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>> index 632ee053accc..62f5d9870ca7 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> >>> applications querying the same sub-device would thus not interact with\n> >>> each other.\n> >>> \n> >>> +If the subdev device node has been registered in read-only mode calls to\n> >>\n> >> mode calls -> mode, calls\n> >>\n> >>> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> Drivers must not return an error solely because the requested crop\n> >>> rectangle doesn't match the device capabilities. They must instead\n> >>> modify the rectangle to match what the hardware can provide. The\n> >>> @@ -123,3 +128,7 @@ EINVAL\n> >>> references a non-existing pad, the ``which`` field references a\n> >>> non-existing format, or cropping is not supported on the given\n> >>> subdev pad.\n> >>> +\n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> >>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>> index 472577bd1745..3a2f64bb00e7 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> >>> a low-pass noise filter might crop pixels at the frame boundaries,\n> >>> modifying its output frame size.\n> >>> \n> >>> +If the subdev device node has been registered in read-only mode calls to\n> >>\n> >> ditto.\n> >>\n> >>> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> Drivers must not return an error solely because the requested format\n> >>> doesn't match the device capabilities. They must instead modify the\n> >>> format to match what the hardware can provide. The modified format\n> >>> @@ -146,6 +151,9 @@ EINVAL\n> >>> ``pad`` references a non-existing pad, or the ``which`` field\n> >>> references a non-existing format.\n> >>> \n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> >>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >>> \n> >>> ============\n> >>> \n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>> index 4b1b4bc78bfe..34aa39096e3d 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>> @@ -65,6 +65,10 @@ struct\n> >>> contains the current frame interval as would be returned by a\n> >>> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> >>> \n> >>> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> >>> +registered in read-only mode is not allowed. An error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> +\n> >>> Drivers must not return an error solely because the requested interval\n> >>> doesn't match the device capabilities. They must instead modify the\n> >>> interval to match what the hardware can provide. The modified interval\n> >>> @@ -118,3 +122,7 @@ EINVAL\n> >>> :c:type:`v4l2_subdev_frame_interval`\n> >>> ``pad`` references a non-existing pad, or the pad doesn't support\n> >>> frame intervals.\n> >>> +\n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> >>> + subdevice.\n> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>> index fc73d27e6d74..abd046cef612 100644\n> >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> >>> See :ref:`subdev` for more information on how each selection target\n> >>> affects the image processing pipeline inside the subdevice.\n> >>> \n> >>> +If the subdev device node has been registered in read-only mode calls to\n> >>\n> >> ditto\n> >>\n> >>> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>> +variable is set to ``-EPERM``.\n> >>> \n> >>> Types of selection targets\n> >>> --------------------------\n> >>> @@ -123,3 +127,7 @@ EINVAL\n> >>> ``pad`` references a non-existing pad, the ``which`` field\n> >>> references a non-existing format, or the selection target is not\n> >>> supported on the given subdev pad.\n> >>> +\n> >>> +EPERM\n> >>> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> >>> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0BF7360409\n\tfor ;\n\tSat, 4 Apr 2020 03:35:55 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 57F9A321;\n\tSat, 4 Apr 2020 03:35:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"d9bW742Z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585964154;\n\tbh=jVLEJpUmUAn5AwcH/9Q25FtO3j22NbjQineu++LA2d4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d9bW742ZrJ7wY5m2PN0UxIdUDDt2vtkJ5sm/STyJwGRPu3vwNylPzo7ToI4NiwS4L\n\tzwjImW8eaijHPaJ8ElvHqKliJi5GqXjmQ0ovI8cySnH8WwYQ9K1mn3oN74NUBDpqWu\n\tXIVLd94/JTKD33hnGYl5OKeL9qLtavU7o2v/EAv8=","Date":"Sat, 4 Apr 2020 04:35:45 +0300","From":"Laurent Pinchart ","To":"Hans Verkuil ","Cc":"Jacopo Mondi , linux-media@vger.kernel.org,\n\tlibcamera-devel@lists.libcamera.org, mchehab@kernel.org,\n\tsakari.ailus@linux.intel.com, andrey.konovalov@linaro.org","Message-ID":"<20200404013545.GJ9690@pendragon.ideasonboard.com>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>\n\t<20200401114611.GC4876@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 04 Apr 2020 01:35:55 -0000"}},{"id":4384,"web_url":"https://patchwork.libcamera.org/comment/4384/","msgid":"<20200406070036.GA5835@kekkonen.localdomain>","date":"2020-04-06T07:00:36","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":37,"url":"https://patchwork.libcamera.org/api/people/37/","name":"Sakari Ailus","email":"sakari.ailus@linux.intel.com"},"content":"Hi Jacopo,\n\nOn Thu, Apr 02, 2020 at 12:10:21AM +0200, Jacopo Mondi wrote:\n> Hi Sakari,\n> one small question\n> \n> On Sat, Mar 28, 2020 at 01:33:58AM +0200, Sakari Ailus wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for the patchset.\n> >\n> > On Fri, Mar 27, 2020 at 11:35:21PM +0100, Jacopo Mondi wrote:\n> > > Document a new kapi function to register subdev device nodes in read only\n> > > mode and for each affected ioctl report how access is restricted.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> > > Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> > > .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> > > Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> > > .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> > > .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> > > .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> > > .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> > > 8 files changed, 94 insertions(+)\n> > >\n> > > diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> > > index 41ccb3e5c707..6506a673e6a1 100644\n> > > --- a/Documentation/media/kapi/v4l2-subdev.rst\n> > > +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> > > @@ -332,6 +332,50 @@ Private ioctls\n> > > \tAll ioctls not in the above list are passed directly to the sub-device\n> > > \tdriver through the core::ioctl operation.\n> > >\n> > > +Read-only sub-device userspace API\n> > > +----------------------------------\n> > > +\n> > > +Bridge drivers that control their connected subdevices through direct calls to\n> > > +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> > > +want userspace to be able to change the same parameters through the subdevice\n> > > +device node and thus do not usually register any.\n> > > +\n> > > +It is sometimes useful to report to userspace the current subdevice\n> > > +configuration through a read-only API, that does not permit applications to\n> > > +change to the device parameters but allows interfacing to the subdevice device\n> > > +node to inspect them.\n> > > +\n> > > +For instance, to implement cameras based on computational photography, userspace\n> > > +needs to know the detailed camera sensor configuration (in terms of skipping,\n> > > +binning, cropping and scaling) for each supported output resolution. To support\n> > > +such use cases, bridge drivers may expose the subdevice operations to userspace\n> > > +through a read-only API.\n> > > +\n> > > +To create a read-only device node for all the subdevices registered with the\n> > > +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> > > +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > > +\n> > > +Access to the following ioctls for userspace applications is restricted on\n> > > +sub-device device nodes registered with\n> > > +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > > +\n> > > +``VIDIOC_SUBDEV_S_FMT``,\n> > > +``VIDIOC_SUBDEV_S_CROP``,\n> > > +``VIDIOC_SUBDEV_S_SELECTION``:\n> > > +\n> > > +\tThese ioctls are only allowed on a read-only subdevice device node\n> > > +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> > > +\tformats and selection rectangles.\n> > > +\n> > > +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> > > +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> > > +``VIDIOC_SUBDEV_S_STD``:\n> > > +\n> > > +\tThese ioctls are not allowed on a read-only subdevice node.\n> > > +\n> > > +In case the ioclt is not allowed, or the format to modify is set to\n> >\n> > \"IOCTL\".\n> \n> are you referring to the typo ioclt/ioctl or are you suggesting to\n> spell IOCTL uppercase ? As the rest of the documentation uses\n> lowercase \"ioctl\" ...\n\nOk; then just use lower case.\n\n> >\n> > > +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> > > +the errno variable is set to ``-EPERM``.\n> > >\n> > > I2C sub-device drivers\n> > > ----------------------\n> > > diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > > index 029bb2d9928a..6082f9c2f8f4 100644\n> > > --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> > > +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > > @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> > > Sub-device character device nodes, conventionally named\n> > > ``/dev/v4l-subdev*``, use major number 81.\n> > >\n> > > +Drivers may opt to limit the sub-device character devices to only expose\n> > > +operations that don't modify the device state. In such a case the sub-devices\n> > > +are referred to as ``read-only`` in the rest of this documentation, and the\n> > > +related restrictions are documented in individual ioctls.\n> >\n> > Perhaps \"IOCTLs\".\n> \n> Same here, the rest of the documentation uses lowercase.\n> \n> Thanks\n> j\n> >\n> > With these, for the set,\n> >\n> > Acked-by: Sakari Ailus \n> >\n> > > +\n> > >\n> > > Controls\n> > > ========\n> > > diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > > index e36dd2622857..611f94e4510a 100644\n> > > --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > > +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > > @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> > > structure as argument. If the ioctl is not supported or the timing\n> > > values are not correct, the driver returns ``EINVAL`` error code.\n> > >\n> > > +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> > > +registered in read-only mode is not allowed. An error is returned and the errno\n> > > +variable is set to ``-EPERM``.\n> > > +\n> > > The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> > > the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> > > the current input or output does not support DV timings (e.g. if\n> > > @@ -81,6 +85,8 @@ ENODATA\n> > > EBUSY\n> > > The device is busy and therefore can not change the timings.\n> > >\n> > > +EPERM\n> > > + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> > >\n> > > .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> > >\n> > > diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > > index e633e42e3910..e220b38b859f 100644\n> > > --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > > +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > > @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> > > does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> > > returned.\n> > >\n> > > +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> > > +in read-only mode is not allowed. An error is returned and the errno variable is\n> > > +set to ``-EPERM``.\n> > >\n> > > Return Value\n> > > ============\n> > > @@ -79,3 +82,6 @@ EINVAL\n> > >\n> > > ENODATA\n> > > Standard video timings are not supported for this input or output.\n> > > +\n> > > +EPERM\n> > > + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> > > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > > index 632ee053accc..62f5d9870ca7 100644\n> > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > > @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> > > applications querying the same sub-device would thus not interact with\n> > > each other.\n> > >\n> > > +If the subdev device node has been registered in read-only mode calls to\n> > > +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> > > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > > +variable is set to ``-EPERM``.\n> > > +\n> > > Drivers must not return an error solely because the requested crop\n> > > rectangle doesn't match the device capabilities. They must instead\n> > > modify the rectangle to match what the hardware can provide. The\n> > > @@ -123,3 +128,7 @@ EINVAL\n> > > references a non-existing pad, the ``which`` field references a\n> > > non-existing format, or cropping is not supported on the given\n> > > subdev pad.\n> > > +\n> > > +EPERM\n> > > + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> > > + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > > index 472577bd1745..3a2f64bb00e7 100644\n> > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > > @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> > > a low-pass noise filter might crop pixels at the frame boundaries,\n> > > modifying its output frame size.\n> > >\n> > > +If the subdev device node has been registered in read-only mode calls to\n> > > +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> > > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > > +variable is set to ``-EPERM``.\n> > > +\n> > > Drivers must not return an error solely because the requested format\n> > > doesn't match the device capabilities. They must instead modify the\n> > > format to match what the hardware can provide. The modified format\n> > > @@ -146,6 +151,9 @@ EINVAL\n> > > ``pad`` references a non-existing pad, or the ``which`` field\n> > > references a non-existing format.\n> > >\n> > > +EPERM\n> > > + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> > > + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > >\n> > > ============\n> > >\n> > > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > > index 4b1b4bc78bfe..34aa39096e3d 100644\n> > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > > @@ -65,6 +65,10 @@ struct\n> > > contains the current frame interval as would be returned by a\n> > > ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> > >\n> > > +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> > > +registered in read-only mode is not allowed. An error is returned and the errno\n> > > +variable is set to ``-EPERM``.\n> > > +\n> > > Drivers must not return an error solely because the requested interval\n> > > doesn't match the device capabilities. They must instead modify the\n> > > interval to match what the hardware can provide. The modified interval\n> > > @@ -118,3 +122,7 @@ EINVAL\n> > > :c:type:`v4l2_subdev_frame_interval`\n> > > ``pad`` references a non-existing pad, or the pad doesn't support\n> > > frame intervals.\n> > > +\n> > > +EPERM\n> > > + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> > > + subdevice.\n> > > diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > > index fc73d27e6d74..abd046cef612 100644\n> > > --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > > +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > > @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> > > See :ref:`subdev` for more information on how each selection target\n> > > affects the image processing pipeline inside the subdevice.\n> > >\n> > > +If the subdev device node has been registered in read-only mode calls to\n> > > +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> > > +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > > +variable is set to ``-EPERM``.\n> > >\n> > > Types of selection targets\n> > > --------------------------\n> > > @@ -123,3 +127,7 @@ EINVAL\n> > > ``pad`` references a non-existing pad, the ``which`` field\n> > > references a non-existing format, or the selection target is not\n> > > supported on the given subdev pad.\n> > > +\n> > > +EPERM\n> > > + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> > > + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >\n> > --\n> > Regards,\n> >\n> > Sakari Ailus","headers":{"Return-Path":"","Received":["from mga03.intel.com (mga03.intel.com [134.134.136.65])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30E8660409\n\tfor ;\n\tMon, 6 Apr 2020 09:00:50 +0200 (CEST)","from orsmga001.jf.intel.com ([10.7.209.18])\n\tby orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; \n\t06 Apr 2020 00:00:43 -0700","from unknown (HELO kekkonen.fi.intel.com) ([10.252.48.155])\n\tby orsmga001.jf.intel.com with ESMTP; 06 Apr 2020 00:00:41 -0700","by kekkonen.fi.intel.com (Postfix, from userid 1000)\n\tid D06AD21D18; Mon, 6 Apr 2020 10:00:36 +0300 (EEST)"],"IronPort-SDR":["G4FWpjbFAgIIi3DRdgfq4eabb6nMZgF2Gf3jVGoAz6UfB28El215X8ZnJcdHWpP4m3Xthnbx0/\n\tBqxtb5+tCwWw==","khY08lmqRwrvtjiDAROfs1IydXTqkp+hpw8eqyu3YM5NIimz9/GpEQt2wji/aLyS2xZds6DQGm\n\t55W3ShLJNmfQ=="],"X-Amp-Result":"SKIPPED(no attachment in message)","X-Amp-File-Uploaded":"False","X-ExtLoop1":"1","X-IronPort-AV":"E=Sophos;i=\"5.72,350,1580803200\"; d=\"scan'208\";a=\"329786793\"","Date":"Mon, 6 Apr 2020 10:00:36 +0300","From":"Sakari Ailus ","To":"Jacopo Mondi ","Cc":"linux-media@vger.kernel.org, libcamera-devel@lists.libcamera.org,\n\tmchehab@kernel.org, hverkuil-cisco@xs4all.nl,\n\tandrey.konovalov@linaro.org, laurent.pinchart@ideasonboard.com","Message-ID":"<20200406070036.GA5835@kekkonen.localdomain>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<20200327233358.GD23713@kekkonen.localdomain>\n\t<20200401221021.kbdmurcrs2amsezs@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20200401221021.kbdmurcrs2amsezs@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 06 Apr 2020 07:00:52 -0000"}},{"id":4385,"web_url":"https://patchwork.libcamera.org/comment/4385/","msgid":"<20200406110732.cfxepbhllle6vsrw@uno.localdomain>","date":"2020-04-06T11:07:32","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi\n\nOn Sat, Apr 04, 2020 at 04:35:45AM +0300, Laurent Pinchart wrote:\n> Hi Hans,\n>\n> On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote:\n> > On 4/1/20 1:46 PM, Laurent Pinchart wrote:\n> > > On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote:\n> > >> On 3/27/20 11:35 PM, Jacopo Mondi wrote:\n> > >>> Document a new kapi function to register subdev device nodes in read only\n> > >>\n> > >> kAPI\n> > >>\n> > >>> mode and for each affected ioctl report how access is restricted.\n> > >>>\n> > >>> Signed-off-by: Jacopo Mondi \n> > >>> ---\n> > >>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> > >>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> > >>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> > >>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> > >>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> > >>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> > >>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> > >>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> > >>> 8 files changed, 94 insertions(+)\n> > >>>\n> > >>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> > >>> index 41ccb3e5c707..6506a673e6a1 100644\n> > >>> --- a/Documentation/media/kapi/v4l2-subdev.rst\n> > >>> +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> > >>> @@ -332,6 +332,50 @@ Private ioctls\n> > >>> \tAll ioctls not in the above list are passed directly to the sub-device\n> > >>> \tdriver through the core::ioctl operation.\n> > >>>\n> > >>> +Read-only sub-device userspace API\n> > >>> +----------------------------------\n> > >>> +\n> > >>> +Bridge drivers that control their connected subdevices through direct calls to\n> > >>> +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> > >>> +want userspace to be able to change the same parameters through the subdevice\n> > >>> +device node and thus do not usually register any.\n> > >>> +\n> > >>> +It is sometimes useful to report to userspace the current subdevice\n> > >>> +configuration through a read-only API, that does not permit applications to\n> > >>> +change to the device parameters but allows interfacing to the subdevice device\n> > >>> +node to inspect them.\n> > >>> +\n> > >>> +For instance, to implement cameras based on computational photography, userspace\n> > >>> +needs to know the detailed camera sensor configuration (in terms of skipping,\n> > >>> +binning, cropping and scaling) for each supported output resolution. To support\n> > >>> +such use cases, bridge drivers may expose the subdevice operations to userspace\n> > >>> +through a read-only API.\n> > >>> +\n> > >>> +To create a read-only device node for all the subdevices registered with the\n> > >>> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> > >>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > >>\n> > >> Should we add something about creating a /dev/media device as well? It's basically\n> > >> required for this functionality.\n> > >\n> > > I'm not opposed to that, but I don't think this should be specific to\n> > > the read-only API, as it's a shared requirement with thr R/W API. The\n> > > previous section, \"V4L2 sub-device userspace API\", doesn't mention media\n> > > devices.\n> >\n> > True.\n> >\n> > >> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\n> > >> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\n> > >> to be able to call this function. Or possibly have an explicit test in\n> > >> __v4l2_device_register_subdev_nodes for the presence of a media device if\n> > >> read_only is true.\n> > >\n> > > VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that\n> > > actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify\n> > > this and make MEDIA_CONTROLLER a requirement for any userspace access\n> > > from userspace.\n> >\n> > I agree.\n>\n> Does that mean we should rework it as a prerequisite for this series, as\n> part of the series, or separately ?\n\nI would, for the moment, make v4l2_device_register_ro_subdev_nodes and\nv4l2_device_register_subdev_nodes dependendent on\nVIDEO_V4L2_SUBDEV_API, so that they're only available to drivers\nactually wanting userspace access. Then when moving everything to\nMEDIA_CONTROLLER, they can be moved to.\n\nHowever, this is not clear to me\nhttps://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-subdev.c#L338\n\nWhat is the reason to have some of the subdev ioctls protected by\nVIDEO_V4L2_SUBDEV_API, while the v4l2-control related ones are not ?\n\nThanks\n j\n\n>\n> > >>> +\n> > >>> +Access to the following ioctls for userspace applications is restricted on\n> > >>> +sub-device device nodes registered with\n> > >>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > >>> +\n> > >>> +``VIDIOC_SUBDEV_S_FMT``,\n> > >>> +``VIDIOC_SUBDEV_S_CROP``,\n> > >>> +``VIDIOC_SUBDEV_S_SELECTION``:\n> > >>> +\n> > >>> +\tThese ioctls are only allowed on a read-only subdevice device node\n> > >>> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> > >>> +\tformats and selection rectangles.\n> > >>> +\n> > >>> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> > >>> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> > >>> +``VIDIOC_SUBDEV_S_STD``:\n> > >>> +\n> > >>> +\tThese ioctls are not allowed on a read-only subdevice node.\n> > >>> +\n> > >>> +In case the ioclt is not allowed, or the format to modify is set to\n> > >>> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> > >>> +the errno variable is set to ``-EPERM``.\n> > >>>\n> > >>> I2C sub-device drivers\n> > >>> ----------------------\n> > >>> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > >>> index 029bb2d9928a..6082f9c2f8f4 100644\n> > >>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> > >>> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > >>> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> > >>> Sub-device character device nodes, conventionally named\n> > >>> ``/dev/v4l-subdev*``, use major number 81.\n> > >>>\n> > >>> +Drivers may opt to limit the sub-device character devices to only expose\n> > >>> +operations that don't modify the device state. In such a case the sub-devices\n> > >>\n> > >> don't -> do not\n> > >>\n> > >> (\"don't\" is a bit too informal)\n> > >>\n> > >>> +are referred to as ``read-only`` in the rest of this documentation, and the\n> > >>> +related restrictions are documented in individual ioctls.\n> > >>> +\n> > >>>\n> > >>> Controls\n> > >>> ========\n> > >>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > >>> index e36dd2622857..611f94e4510a 100644\n> > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > >>> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > >>> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> > >>> structure as argument. If the ioctl is not supported or the timing\n> > >>> values are not correct, the driver returns ``EINVAL`` error code.\n> > >>>\n> > >>> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> > >>> +registered in read-only mode is not allowed. An error is returned and the errno\n> > >>> +variable is set to ``-EPERM``.\n> > >>> +\n> > >>> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> > >>> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> > >>> the current input or output does not support DV timings (e.g. if\n> > >>> @@ -81,6 +85,8 @@ ENODATA\n> > >>> EBUSY\n> > >>> The device is busy and therefore can not change the timings.\n> > >>>\n> > >>> +EPERM\n> > >>> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> > >>>\n> > >>> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> > >>>\n> > >>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > >>> index e633e42e3910..e220b38b859f 100644\n> > >>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > >>> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > >>> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> > >>> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> > >>> returned.\n> > >>>\n> > >>> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> > >>> +in read-only mode is not allowed. An error is returned and the errno variable is\n> > >>> +set to ``-EPERM``.\n> > >>>\n> > >>> Return Value\n> > >>> ============\n> > >>> @@ -79,3 +82,6 @@ EINVAL\n> > >>>\n> > >>> ENODATA\n> > >>> Standard video timings are not supported for this input or output.\n> > >>> +\n> > >>> +EPERM\n> > >>> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> > >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > >>> index 632ee053accc..62f5d9870ca7 100644\n> > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > >>> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> > >>> applications querying the same sub-device would thus not interact with\n> > >>> each other.\n> > >>>\n> > >>> +If the subdev device node has been registered in read-only mode calls to\n> > >>\n> > >> mode calls -> mode, calls\n> > >>\n> > >>> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> > >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > >>> +variable is set to ``-EPERM``.\n> > >>> +\n> > >>> Drivers must not return an error solely because the requested crop\n> > >>> rectangle doesn't match the device capabilities. They must instead\n> > >>> modify the rectangle to match what the hardware can provide. The\n> > >>> @@ -123,3 +128,7 @@ EINVAL\n> > >>> references a non-existing pad, the ``which`` field references a\n> > >>> non-existing format, or cropping is not supported on the given\n> > >>> subdev pad.\n> > >>> +\n> > >>> +EPERM\n> > >>> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> > >>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > >>> index 472577bd1745..3a2f64bb00e7 100644\n> > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > >>> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> > >>> a low-pass noise filter might crop pixels at the frame boundaries,\n> > >>> modifying its output frame size.\n> > >>>\n> > >>> +If the subdev device node has been registered in read-only mode calls to\n> > >>\n> > >> ditto.\n> > >>\n> > >>> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> > >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > >>> +variable is set to ``-EPERM``.\n> > >>> +\n> > >>> Drivers must not return an error solely because the requested format\n> > >>> doesn't match the device capabilities. They must instead modify the\n> > >>> format to match what the hardware can provide. The modified format\n> > >>> @@ -146,6 +151,9 @@ EINVAL\n> > >>> ``pad`` references a non-existing pad, or the ``which`` field\n> > >>> references a non-existing format.\n> > >>>\n> > >>> +EPERM\n> > >>> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> > >>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > >>>\n> > >>> ============\n> > >>>\n> > >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > >>> index 4b1b4bc78bfe..34aa39096e3d 100644\n> > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > >>> @@ -65,6 +65,10 @@ struct\n> > >>> contains the current frame interval as would be returned by a\n> > >>> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> > >>>\n> > >>> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> > >>> +registered in read-only mode is not allowed. An error is returned and the errno\n> > >>> +variable is set to ``-EPERM``.\n> > >>> +\n> > >>> Drivers must not return an error solely because the requested interval\n> > >>> doesn't match the device capabilities. They must instead modify the\n> > >>> interval to match what the hardware can provide. The modified interval\n> > >>> @@ -118,3 +122,7 @@ EINVAL\n> > >>> :c:type:`v4l2_subdev_frame_interval`\n> > >>> ``pad`` references a non-existing pad, or the pad doesn't support\n> > >>> frame intervals.\n> > >>> +\n> > >>> +EPERM\n> > >>> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> > >>> + subdevice.\n> > >>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > >>> index fc73d27e6d74..abd046cef612 100644\n> > >>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > >>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > >>> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> > >>> See :ref:`subdev` for more information on how each selection target\n> > >>> affects the image processing pipeline inside the subdevice.\n> > >>>\n> > >>> +If the subdev device node has been registered in read-only mode calls to\n> > >>\n> > >> ditto\n> > >>\n> > >>> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> > >>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > >>> +variable is set to ``-EPERM``.\n> > >>>\n> > >>> Types of selection targets\n> > >>> --------------------------\n> > >>> @@ -123,3 +127,7 @@ EINVAL\n> > >>> ``pad`` references a non-existing pad, the ``which`` field\n> > >>> references a non-existing format, or the selection target is not\n> > >>> supported on the given subdev pad.\n> > >>> +\n> > >>> +EPERM\n> > >>> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> > >>> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 366EA6040A\n\tfor ;\n\tMon, 6 Apr 2020 13:04:32 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id EA7ACE0012;\n\tMon, 6 Apr 2020 11:04:29 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 6 Apr 2020 13:07:32 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Hans Verkuil , linux-media@vger.kernel.org,\n\tlibcamera-devel@lists.libcamera.org, mchehab@kernel.org,\n\tsakari.ailus@linux.intel.com, andrey.konovalov@linaro.org","Message-ID":"<20200406110732.cfxepbhllle6vsrw@uno.localdomain>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>\n\t<20200401114611.GC4876@pendragon.ideasonboard.com>\n\t\n\t<20200404013545.GJ9690@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200404013545.GJ9690@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 06 Apr 2020 11:04:32 -0000"}},{"id":4386,"web_url":"https://patchwork.libcamera.org/comment/4386/","msgid":"<20200406123935.GA14971@pendragon.ideasonboard.com>","date":"2020-04-06T12:39:35","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Apr 06, 2020 at 01:07:32PM +0200, Jacopo Mondi wrote:\n> On Sat, Apr 04, 2020 at 04:35:45AM +0300, Laurent Pinchart wrote:\n> > On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote:\n> >> On 4/1/20 1:46 PM, Laurent Pinchart wrote:\n> >>> On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote:\n> >>>> On 3/27/20 11:35 PM, Jacopo Mondi wrote:\n> >>>>> Document a new kapi function to register subdev device nodes in read only\n> >>>>\n> >>>> kAPI\n> >>>>\n> >>>>> mode and for each affected ioctl report how access is restricted.\n> >>>>>\n> >>>>> Signed-off-by: Jacopo Mondi \n> >>>>> ---\n> >>>>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> >>>>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> >>>>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> >>>>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> >>>>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> >>>>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> >>>>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> >>>>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> >>>>> 8 files changed, 94 insertions(+)\n> >>>>>\n> >>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> >>>>> index 41ccb3e5c707..6506a673e6a1 100644\n> >>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst\n> >>>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> >>>>> @@ -332,6 +332,50 @@ Private ioctls\n> >>>>> \tAll ioctls not in the above list are passed directly to the sub-device\n> >>>>> \tdriver through the core::ioctl operation.\n> >>>>>\n> >>>>> +Read-only sub-device userspace API\n> >>>>> +----------------------------------\n> >>>>> +\n> >>>>> +Bridge drivers that control their connected subdevices through direct calls to\n> >>>>> +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> >>>>> +want userspace to be able to change the same parameters through the subdevice\n> >>>>> +device node and thus do not usually register any.\n> >>>>> +\n> >>>>> +It is sometimes useful to report to userspace the current subdevice\n> >>>>> +configuration through a read-only API, that does not permit applications to\n> >>>>> +change to the device parameters but allows interfacing to the subdevice device\n> >>>>> +node to inspect them.\n> >>>>> +\n> >>>>> +For instance, to implement cameras based on computational photography, userspace\n> >>>>> +needs to know the detailed camera sensor configuration (in terms of skipping,\n> >>>>> +binning, cropping and scaling) for each supported output resolution. To support\n> >>>>> +such use cases, bridge drivers may expose the subdevice operations to userspace\n> >>>>> +through a read-only API.\n> >>>>> +\n> >>>>> +To create a read-only device node for all the subdevices registered with the\n> >>>>> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> >>>>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> >>>>\n> >>>> Should we add something about creating a /dev/media device as well? It's basically\n> >>>> required for this functionality.\n> >>>\n> >>> I'm not opposed to that, but I don't think this should be specific to\n> >>> the read-only API, as it's a shared requirement with thr R/W API. The\n> >>> previous section, \"V4L2 sub-device userspace API\", doesn't mention media\n> >>> devices.\n> >>\n> >> True.\n> >>\n> >>>> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\n> >>>> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\n> >>>> to be able to call this function. Or possibly have an explicit test in\n> >>>> __v4l2_device_register_subdev_nodes for the presence of a media device if\n> >>>> read_only is true.\n> >>>\n> >>> VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that\n> >>> actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify\n> >>> this and make MEDIA_CONTROLLER a requirement for any userspace access\n> >>> from userspace.\n> >>\n> >> I agree.\n> >\n> > Does that mean we should rework it as a prerequisite for this series, as\n> > part of the series, or separately ?\n> \n> I would, for the moment, make v4l2_device_register_ro_subdev_nodes and\n> v4l2_device_register_subdev_nodes dependendent on\n> VIDEO_V4L2_SUBDEV_API, so that they're only available to drivers\n> actually wanting userspace access. Then when moving everything to\n> MEDIA_CONTROLLER, they can be moved to.\n> \n> However, this is not clear to me\n> https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-subdev.c#L338\n> \n> What is the reason to have some of the subdev ioctls protected by\n> VIDEO_V4L2_SUBDEV_API, while the v4l2-control related ones are not ?\n\nI'd say historical mistake :-) I think we can move everything under\nVIDEO_V4L2_SUBDEV_API.\n\n> >>>>> +\n> >>>>> +Access to the following ioctls for userspace applications is restricted on\n> >>>>> +sub-device device nodes registered with\n> >>>>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> >>>>> +\n> >>>>> +``VIDIOC_SUBDEV_S_FMT``,\n> >>>>> +``VIDIOC_SUBDEV_S_CROP``,\n> >>>>> +``VIDIOC_SUBDEV_S_SELECTION``:\n> >>>>> +\n> >>>>> +\tThese ioctls are only allowed on a read-only subdevice device node\n> >>>>> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> >>>>> +\tformats and selection rectangles.\n> >>>>> +\n> >>>>> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> >>>>> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> >>>>> +``VIDIOC_SUBDEV_S_STD``:\n> >>>>> +\n> >>>>> +\tThese ioctls are not allowed on a read-only subdevice node.\n> >>>>> +\n> >>>>> +In case the ioclt is not allowed, or the format to modify is set to\n> >>>>> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> >>>>> +the errno variable is set to ``-EPERM``.\n> >>>>>\n> >>>>> I2C sub-device drivers\n> >>>>> ----------------------\n> >>>>> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>>>> index 029bb2d9928a..6082f9c2f8f4 100644\n> >>>>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>>>> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> >>>>> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> >>>>> Sub-device character device nodes, conventionally named\n> >>>>> ``/dev/v4l-subdev*``, use major number 81.\n> >>>>>\n> >>>>> +Drivers may opt to limit the sub-device character devices to only expose\n> >>>>> +operations that don't modify the device state. In such a case the sub-devices\n> >>>>\n> >>>> don't -> do not\n> >>>>\n> >>>> (\"don't\" is a bit too informal)\n> >>>>\n> >>>>> +are referred to as ``read-only`` in the rest of this documentation, and the\n> >>>>> +related restrictions are documented in individual ioctls.\n> >>>>> +\n> >>>>>\n> >>>>> Controls\n> >>>>> ========\n> >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>>>> index e36dd2622857..611f94e4510a 100644\n> >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> >>>>> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> >>>>> structure as argument. If the ioctl is not supported or the timing\n> >>>>> values are not correct, the driver returns ``EINVAL`` error code.\n> >>>>>\n> >>>>> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> >>>>> +registered in read-only mode is not allowed. An error is returned and the errno\n> >>>>> +variable is set to ``-EPERM``.\n> >>>>> +\n> >>>>> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> >>>>> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> >>>>> the current input or output does not support DV timings (e.g. if\n> >>>>> @@ -81,6 +85,8 @@ ENODATA\n> >>>>> EBUSY\n> >>>>> The device is busy and therefore can not change the timings.\n> >>>>>\n> >>>>> +EPERM\n> >>>>> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> >>>>>\n> >>>>> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> >>>>>\n> >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>>>> index e633e42e3910..e220b38b859f 100644\n> >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> >>>>> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> >>>>> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> >>>>> returned.\n> >>>>>\n> >>>>> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> >>>>> +in read-only mode is not allowed. An error is returned and the errno variable is\n> >>>>> +set to ``-EPERM``.\n> >>>>>\n> >>>>> Return Value\n> >>>>> ============\n> >>>>> @@ -79,3 +82,6 @@ EINVAL\n> >>>>>\n> >>>>> ENODATA\n> >>>>> Standard video timings are not supported for this input or output.\n> >>>>> +\n> >>>>> +EPERM\n> >>>>> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>>>> index 632ee053accc..62f5d9870ca7 100644\n> >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> >>>>> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> >>>>> applications querying the same sub-device would thus not interact with\n> >>>>> each other.\n> >>>>>\n> >>>>> +If the subdev device node has been registered in read-only mode calls to\n> >>>>\n> >>>> mode calls -> mode, calls\n> >>>>\n> >>>>> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> >>>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>>>> +variable is set to ``-EPERM``.\n> >>>>> +\n> >>>>> Drivers must not return an error solely because the requested crop\n> >>>>> rectangle doesn't match the device capabilities. They must instead\n> >>>>> modify the rectangle to match what the hardware can provide. The\n> >>>>> @@ -123,3 +128,7 @@ EINVAL\n> >>>>> references a non-existing pad, the ``which`` field references a\n> >>>>> non-existing format, or cropping is not supported on the given\n> >>>>> subdev pad.\n> >>>>> +\n> >>>>> +EPERM\n> >>>>> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> >>>>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>>>> index 472577bd1745..3a2f64bb00e7 100644\n> >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> >>>>> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> >>>>> a low-pass noise filter might crop pixels at the frame boundaries,\n> >>>>> modifying its output frame size.\n> >>>>>\n> >>>>> +If the subdev device node has been registered in read-only mode calls to\n> >>>>\n> >>>> ditto.\n> >>>>\n> >>>>> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> >>>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>>>> +variable is set to ``-EPERM``.\n> >>>>> +\n> >>>>> Drivers must not return an error solely because the requested format\n> >>>>> doesn't match the device capabilities. They must instead modify the\n> >>>>> format to match what the hardware can provide. The modified format\n> >>>>> @@ -146,6 +151,9 @@ EINVAL\n> >>>>> ``pad`` references a non-existing pad, or the ``which`` field\n> >>>>> references a non-existing format.\n> >>>>>\n> >>>>> +EPERM\n> >>>>> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> >>>>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> >>>>>\n> >>>>> ============\n> >>>>>\n> >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>>>> index 4b1b4bc78bfe..34aa39096e3d 100644\n> >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> >>>>> @@ -65,6 +65,10 @@ struct\n> >>>>> contains the current frame interval as would be returned by a\n> >>>>> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> >>>>>\n> >>>>> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> >>>>> +registered in read-only mode is not allowed. An error is returned and the errno\n> >>>>> +variable is set to ``-EPERM``.\n> >>>>> +\n> >>>>> Drivers must not return an error solely because the requested interval\n> >>>>> doesn't match the device capabilities. They must instead modify the\n> >>>>> interval to match what the hardware can provide. The modified interval\n> >>>>> @@ -118,3 +122,7 @@ EINVAL\n> >>>>> :c:type:`v4l2_subdev_frame_interval`\n> >>>>> ``pad`` references a non-existing pad, or the pad doesn't support\n> >>>>> frame intervals.\n> >>>>> +\n> >>>>> +EPERM\n> >>>>> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> >>>>> + subdevice.\n> >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>>>> index fc73d27e6d74..abd046cef612 100644\n> >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> >>>>> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> >>>>> See :ref:`subdev` for more information on how each selection target\n> >>>>> affects the image processing pipeline inside the subdevice.\n> >>>>>\n> >>>>> +If the subdev device node has been registered in read-only mode calls to\n> >>>>\n> >>>> ditto\n> >>>>\n> >>>>> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> >>>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> >>>>> +variable is set to ``-EPERM``.\n> >>>>>\n> >>>>> Types of selection targets\n> >>>>> --------------------------\n> >>>>> @@ -123,3 +127,7 @@ EINVAL\n> >>>>> ``pad`` references a non-existing pad, the ``which`` field\n> >>>>> references a non-existing format, or the selection target is not\n> >>>>> supported on the given subdev pad.\n> >>>>> +\n> >>>>> +EPERM\n> >>>>> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> >>>>> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E7826040A\n\tfor ;\n\tMon, 6 Apr 2020 14:39:45 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7EF5A80E;\n\tMon, 6 Apr 2020 14:39:44 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ur8cESAN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1586176784;\n\tbh=APNOqMQJtQEGVkij7e24KueddBuIdLkdt7tO56bnPGM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ur8cESANHbolA3eUk1aIb85K5q8sCOPBnY802vcKKTNaEut42u6zFkf6Eol4AAzk8\n\tuqrdUAXKzqxYTDPrfPqehFu/zZi5DkVaCSNTroqDcvtLYNfLZR//R2VqdRALC0ZGo5\n\tH/YaaBqAsiBxXW4ut5TI7Q8b/bgH6enZTIHWA5/c=","Date":"Mon, 6 Apr 2020 15:39:35 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Hans Verkuil , linux-media@vger.kernel.org,\n\tlibcamera-devel@lists.libcamera.org, mchehab@kernel.org,\n\tsakari.ailus@linux.intel.com, andrey.konovalov@linaro.org","Message-ID":"<20200406123935.GA14971@pendragon.ideasonboard.com>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>\n\t<20200401114611.GC4876@pendragon.ideasonboard.com>\n\t\n\t<20200404013545.GJ9690@pendragon.ideasonboard.com>\n\t<20200406110732.cfxepbhllle6vsrw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200406110732.cfxepbhllle6vsrw@uno.localdomain>","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 06 Apr 2020 12:39:45 -0000"}},{"id":4387,"web_url":"https://patchwork.libcamera.org/comment/4387/","msgid":"<20200406124758.6dykddwckon262og@uno.localdomain>","date":"2020-04-06T12:47:58","subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Apr 06, 2020 at 03:39:35PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Apr 06, 2020 at 01:07:32PM +0200, Jacopo Mondi wrote:\n> > On Sat, Apr 04, 2020 at 04:35:45AM +0300, Laurent Pinchart wrote:\n> > > On Wed, Apr 01, 2020 at 01:54:13PM +0200, Hans Verkuil wrote:\n> > >> On 4/1/20 1:46 PM, Laurent Pinchart wrote:\n> > >>> On Wed, Apr 01, 2020 at 01:19:00PM +0200, Hans Verkuil wrote:\n> > >>>> On 3/27/20 11:35 PM, Jacopo Mondi wrote:\n> > >>>>> Document a new kapi function to register subdev device nodes in read only\n> > >>>>\n> > >>>> kAPI\n> > >>>>\n> > >>>>> mode and for each affected ioctl report how access is restricted.\n> > >>>>>\n> > >>>>> Signed-off-by: Jacopo Mondi \n> > >>>>> ---\n> > >>>>> Documentation/media/kapi/v4l2-subdev.rst | 44 +++++++++++++++++++\n> > >>>>> Documentation/media/uapi/v4l/dev-subdev.rst | 5 +++\n> > >>>>> .../media/uapi/v4l/vidioc-g-dv-timings.rst | 6 +++\n> > >>>>> Documentation/media/uapi/v4l/vidioc-g-std.rst | 6 +++\n> > >>>>> .../media/uapi/v4l/vidioc-subdev-g-crop.rst | 9 ++++\n> > >>>>> .../media/uapi/v4l/vidioc-subdev-g-fmt.rst | 8 ++++\n> > >>>>> .../v4l/vidioc-subdev-g-frame-interval.rst | 8 ++++\n> > >>>>> .../uapi/v4l/vidioc-subdev-g-selection.rst | 8 ++++\n> > >>>>> 8 files changed, 94 insertions(+)\n> > >>>>>\n> > >>>>> diff --git a/Documentation/media/kapi/v4l2-subdev.rst b/Documentation/media/kapi/v4l2-subdev.rst\n> > >>>>> index 41ccb3e5c707..6506a673e6a1 100644\n> > >>>>> --- a/Documentation/media/kapi/v4l2-subdev.rst\n> > >>>>> +++ b/Documentation/media/kapi/v4l2-subdev.rst\n> > >>>>> @@ -332,6 +332,50 @@ Private ioctls\n> > >>>>> \tAll ioctls not in the above list are passed directly to the sub-device\n> > >>>>> \tdriver through the core::ioctl operation.\n> > >>>>>\n> > >>>>> +Read-only sub-device userspace API\n> > >>>>> +----------------------------------\n> > >>>>> +\n> > >>>>> +Bridge drivers that control their connected subdevices through direct calls to\n> > >>>>> +the kernel API realized by :c:type:`v4l2_subdev_ops` structure do not usually\n> > >>>>> +want userspace to be able to change the same parameters through the subdevice\n> > >>>>> +device node and thus do not usually register any.\n> > >>>>> +\n> > >>>>> +It is sometimes useful to report to userspace the current subdevice\n> > >>>>> +configuration through a read-only API, that does not permit applications to\n> > >>>>> +change to the device parameters but allows interfacing to the subdevice device\n> > >>>>> +node to inspect them.\n> > >>>>> +\n> > >>>>> +For instance, to implement cameras based on computational photography, userspace\n> > >>>>> +needs to know the detailed camera sensor configuration (in terms of skipping,\n> > >>>>> +binning, cropping and scaling) for each supported output resolution. To support\n> > >>>>> +such use cases, bridge drivers may expose the subdevice operations to userspace\n> > >>>>> +through a read-only API.\n> > >>>>> +\n> > >>>>> +To create a read-only device node for all the subdevices registered with the\n> > >>>>> +``V4L2_SUBDEV_FL_HAS_DEVNODE`` set, the :c:type:`v4l2_device` driver should call\n> > >>>>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > >>>>\n> > >>>> Should we add something about creating a /dev/media device as well? It's basically\n> > >>>> required for this functionality.\n> > >>>\n> > >>> I'm not opposed to that, but I don't think this should be specific to\n> > >>> the read-only API, as it's a shared requirement with thr R/W API. The\n> > >>> previous section, \"V4L2 sub-device userspace API\", doesn't mention media\n> > >>> devices.\n> > >>\n> > >> True.\n> > >>\n> > >>>> I think it might be a good idea to put v4l2_device_register_ro_subdev_nodes()\n> > >>>> under #ifdef CONFIG_MEDIA_CONTROLLER so that this config *has* to be set in order\n> > >>>> to be able to call this function. Or possibly have an explicit test in\n> > >>>> __v4l2_device_register_subdev_nodes for the presence of a media device if\n> > >>>> read_only is true.\n> > >>>\n> > >>> VIDEO_V4L2_SUBDEV_API depends on MEDIA_CONTROLLER, but only part of that\n> > >>> actually depends on MEDIA_CONTROLLER in the code. I'd vote to simplify\n> > >>> this and make MEDIA_CONTROLLER a requirement for any userspace access\n> > >>> from userspace.\n> > >>\n> > >> I agree.\n> > >\n> > > Does that mean we should rework it as a prerequisite for this series, as\n> > > part of the series, or separately ?\n> >\n> > I would, for the moment, make v4l2_device_register_ro_subdev_nodes and\n> > v4l2_device_register_subdev_nodes dependendent on\n> > VIDEO_V4L2_SUBDEV_API, so that they're only available to drivers\n> > actually wanting userspace access. Then when moving everything to\n> > MEDIA_CONTROLLER, they can be moved to.\n> >\n> > However, this is not clear to me\n> > https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-subdev.c#L338\n> >\n> > What is the reason to have some of the subdev ioctls protected by\n> > VIDEO_V4L2_SUBDEV_API, while the v4l2-control related ones are not ?\n>\n> I'd say historical mistake :-) I think we can move everything under\n> VIDEO_V4L2_SUBDEV_API.\n>\n\nGood, I was afraid that limiting the ability to register subdevices to\ndrivers claiming support for VIDEO_V4L2_SUBDEV_API only would break\nuse cases that depends on the ability to set controls on a subdevice\ndevice node without claming subdevice API support (which seems anyway\nwrong to me).\n\nI will send a new versio making resitration of subdevice devnodes\ndependent on V4L2_SUBDEV_API then.\n\nThanks\n j\n\n> > >>>>> +\n> > >>>>> +Access to the following ioctls for userspace applications is restricted on\n> > >>>>> +sub-device device nodes registered with\n> > >>>>> +:c:func:`v4l2_device_register_ro_subdev_nodes`.\n> > >>>>> +\n> > >>>>> +``VIDIOC_SUBDEV_S_FMT``,\n> > >>>>> +``VIDIOC_SUBDEV_S_CROP``,\n> > >>>>> +``VIDIOC_SUBDEV_S_SELECTION``:\n> > >>>>> +\n> > >>>>> +\tThese ioctls are only allowed on a read-only subdevice device node\n> > >>>>> +\tfor the :ref:`V4L2_SUBDEV_FORMAT_TRY `\n> > >>>>> +\tformats and selection rectangles.\n> > >>>>> +\n> > >>>>> +``VIDIOC_SUBDEV_S_FRAME_INTERVAL``,\n> > >>>>> +``VIDIOC_SUBDEV_S_DV_TIMINGS``,\n> > >>>>> +``VIDIOC_SUBDEV_S_STD``:\n> > >>>>> +\n> > >>>>> +\tThese ioctls are not allowed on a read-only subdevice node.\n> > >>>>> +\n> > >>>>> +In case the ioclt is not allowed, or the format to modify is set to\n> > >>>>> +``V4L2_SUBDEV_FORMAT_ACTIVE``, the core returns a negative error code and\n> > >>>>> +the errno variable is set to ``-EPERM``.\n> > >>>>>\n> > >>>>> I2C sub-device drivers\n> > >>>>> ----------------------\n> > >>>>> diff --git a/Documentation/media/uapi/v4l/dev-subdev.rst b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > >>>>> index 029bb2d9928a..6082f9c2f8f4 100644\n> > >>>>> --- a/Documentation/media/uapi/v4l/dev-subdev.rst\n> > >>>>> +++ b/Documentation/media/uapi/v4l/dev-subdev.rst\n> > >>>>> @@ -39,6 +39,11 @@ will feature a character device node on which ioctls can be called to\n> > >>>>> Sub-device character device nodes, conventionally named\n> > >>>>> ``/dev/v4l-subdev*``, use major number 81.\n> > >>>>>\n> > >>>>> +Drivers may opt to limit the sub-device character devices to only expose\n> > >>>>> +operations that don't modify the device state. In such a case the sub-devices\n> > >>>>\n> > >>>> don't -> do not\n> > >>>>\n> > >>>> (\"don't\" is a bit too informal)\n> > >>>>\n> > >>>>> +are referred to as ``read-only`` in the rest of this documentation, and the\n> > >>>>> +related restrictions are documented in individual ioctls.\n> > >>>>> +\n> > >>>>>\n> > >>>>> Controls\n> > >>>>> ========\n> > >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > >>>>> index e36dd2622857..611f94e4510a 100644\n> > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-g-dv-timings.rst\n> > >>>>> @@ -57,6 +57,10 @@ pointer to the struct :c:type:`v4l2_dv_timings`\n> > >>>>> structure as argument. If the ioctl is not supported or the timing\n> > >>>>> values are not correct, the driver returns ``EINVAL`` error code.\n> > >>>>>\n> > >>>>> +Calling ``VIDIOC_SUBDEV_S_DV_TIMINGS`` on a subdev device node that has been\n> > >>>>> +registered in read-only mode is not allowed. An error is returned and the errno\n> > >>>>> +variable is set to ``-EPERM``.\n> > >>>>> +\n> > >>>>> The ``linux/v4l2-dv-timings.h`` header can be used to get the timings of\n> > >>>>> the formats in the :ref:`cea861` and :ref:`vesadmt` standards. If\n> > >>>>> the current input or output does not support DV timings (e.g. if\n> > >>>>> @@ -81,6 +85,8 @@ ENODATA\n> > >>>>> EBUSY\n> > >>>>> The device is busy and therefore can not change the timings.\n> > >>>>>\n> > >>>>> +EPERM\n> > >>>>> + ``VIDIOC_SUBDEV_S_DV_TIMINGS`` has been called on a read-only subdevice.\n> > >>>>>\n> > >>>>> .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|\n> > >>>>>\n> > >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-std.rst b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > >>>>> index e633e42e3910..e220b38b859f 100644\n> > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-g-std.rst\n> > >>>>> @@ -66,6 +66,9 @@ video timings (e.g. if :ref:`VIDIOC_ENUMINPUT`\n> > >>>>> does not set the ``V4L2_IN_CAP_STD`` flag), then ``ENODATA`` error code is\n> > >>>>> returned.\n> > >>>>>\n> > >>>>> +Calling ``VIDIOC_SUBDEV_S_STD`` on a subdev device node that has been registered\n> > >>>>> +in read-only mode is not allowed. An error is returned and the errno variable is\n> > >>>>> +set to ``-EPERM``.\n> > >>>>>\n> > >>>>> Return Value\n> > >>>>> ============\n> > >>>>> @@ -79,3 +82,6 @@ EINVAL\n> > >>>>>\n> > >>>>> ENODATA\n> > >>>>> Standard video timings are not supported for this input or output.\n> > >>>>> +\n> > >>>>> +EPERM\n> > >>>>> + ``VIDIOC_SUBDEV_S_STD`` has been called on a read-only subdevice.\n> > >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > >>>>> index 632ee053accc..62f5d9870ca7 100644\n> > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-crop.rst\n> > >>>>> @@ -73,6 +73,11 @@ crop rectangles and stored in the sub-device file handle. Two\n> > >>>>> applications querying the same sub-device would thus not interact with\n> > >>>>> each other.\n> > >>>>>\n> > >>>>> +If the subdev device node has been registered in read-only mode calls to\n> > >>>>\n> > >>>> mode calls -> mode, calls\n> > >>>>\n> > >>>>> +``VIDIOC_SUBDEV_S_CROP`` are only valid if the ``which`` field is set to\n> > >>>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > >>>>> +variable is set to ``-EPERM``.\n> > >>>>> +\n> > >>>>> Drivers must not return an error solely because the requested crop\n> > >>>>> rectangle doesn't match the device capabilities. They must instead\n> > >>>>> modify the rectangle to match what the hardware can provide. The\n> > >>>>> @@ -123,3 +128,7 @@ EINVAL\n> > >>>>> references a non-existing pad, the ``which`` field references a\n> > >>>>> non-existing format, or cropping is not supported on the given\n> > >>>>> subdev pad.\n> > >>>>> +\n> > >>>>> +EPERM\n> > >>>>> + The ``VIDIOC_SUBDEV_S_CROP`` ioctl has been called on a read-only subdevice\n> > >>>>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > >>>>> index 472577bd1745..3a2f64bb00e7 100644\n> > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-fmt.rst\n> > >>>>> @@ -78,6 +78,11 @@ current links configuration or sub-device controls value. For instance,\n> > >>>>> a low-pass noise filter might crop pixels at the frame boundaries,\n> > >>>>> modifying its output frame size.\n> > >>>>>\n> > >>>>> +If the subdev device node has been registered in read-only mode calls to\n> > >>>>\n> > >>>> ditto.\n> > >>>>\n> > >>>>> +``VIDIOC_SUBDEV_S_FMT`` are only valid if the ``which`` field is set to\n> > >>>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > >>>>> +variable is set to ``-EPERM``.\n> > >>>>> +\n> > >>>>> Drivers must not return an error solely because the requested format\n> > >>>>> doesn't match the device capabilities. They must instead modify the\n> > >>>>> format to match what the hardware can provide. The modified format\n> > >>>>> @@ -146,6 +151,9 @@ EINVAL\n> > >>>>> ``pad`` references a non-existing pad, or the ``which`` field\n> > >>>>> references a non-existing format.\n> > >>>>>\n> > >>>>> +EPERM\n> > >>>>> + The ``VIDIOC_SUBDEV_S_FMT`` ioctl has been called on a read-only subdevice\n> > >>>>> + and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n> > >>>>>\n> > >>>>> ============\n> > >>>>>\n> > >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > >>>>> index 4b1b4bc78bfe..34aa39096e3d 100644\n> > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-frame-interval.rst\n> > >>>>> @@ -65,6 +65,10 @@ struct\n> > >>>>> contains the current frame interval as would be returned by a\n> > >>>>> ``VIDIOC_SUBDEV_G_FRAME_INTERVAL`` call.\n> > >>>>>\n> > >>>>> +Calling ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` on a subdev device node that has been\n> > >>>>> +registered in read-only mode is not allowed. An error is returned and the errno\n> > >>>>> +variable is set to ``-EPERM``.\n> > >>>>> +\n> > >>>>> Drivers must not return an error solely because the requested interval\n> > >>>>> doesn't match the device capabilities. They must instead modify the\n> > >>>>> interval to match what the hardware can provide. The modified interval\n> > >>>>> @@ -118,3 +122,7 @@ EINVAL\n> > >>>>> :c:type:`v4l2_subdev_frame_interval`\n> > >>>>> ``pad`` references a non-existing pad, or the pad doesn't support\n> > >>>>> frame intervals.\n> > >>>>> +\n> > >>>>> +EPERM\n> > >>>>> + The ``VIDIOC_SUBDEV_S_FRAME_INTERVAL`` ioctl has been called on a read-only\n> > >>>>> + subdevice.\n> > >>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > >>>>> index fc73d27e6d74..abd046cef612 100644\n> > >>>>> --- a/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > >>>>> +++ b/Documentation/media/uapi/v4l/vidioc-subdev-g-selection.rst\n> > >>>>> @@ -53,6 +53,10 @@ function of the crop API, and more, are supported by the selections API.\n> > >>>>> See :ref:`subdev` for more information on how each selection target\n> > >>>>> affects the image processing pipeline inside the subdevice.\n> > >>>>>\n> > >>>>> +If the subdev device node has been registered in read-only mode calls to\n> > >>>>\n> > >>>> ditto\n> > >>>>\n> > >>>>> +``VIDIOC_SUBDEV_S_SELECTION`` are only valid if the ``which`` field is set to\n> > >>>>> +``V4L2_SUBDEV_FORMAT_TRY``, otherwise an error is returned and the errno\n> > >>>>> +variable is set to ``-EPERM``.\n> > >>>>>\n> > >>>>> Types of selection targets\n> > >>>>> --------------------------\n> > >>>>> @@ -123,3 +127,7 @@ EINVAL\n> > >>>>> ``pad`` references a non-existing pad, the ``which`` field\n> > >>>>> references a non-existing format, or the selection target is not\n> > >>>>> supported on the given subdev pad.\n> > >>>>> +\n> > >>>>> +EPERM\n> > >>>>> + The ``VIDIOC_SUBDEV_S_SELECTION`` ioctl has been called on a read-only\n> > >>>>> + subdevice and the ``which`` field is set to ``V4L2_SUBDEV_FORMAT_ACTIVE``.\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ADDE16040A\n\tfor ;\n\tMon, 6 Apr 2020 14:44:57 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id D4CBA1C0006;\n\tMon, 6 Apr 2020 12:44:55 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 6 Apr 2020 14:47:58 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Hans Verkuil , linux-media@vger.kernel.org,\n\tlibcamera-devel@lists.libcamera.org, mchehab@kernel.org,\n\tsakari.ailus@linux.intel.com, andrey.konovalov@linaro.org","Message-ID":"<20200406124758.6dykddwckon262og@uno.localdomain>","References":"<20200327223522.506832-1-jacopo@jmondi.org>\n\t<20200327223522.506832-3-jacopo@jmondi.org>\n\t<1a51e639-de91-0460-bdac-5183380e9f9d@xs4all.nl>\n\t<20200401114611.GC4876@pendragon.ideasonboard.com>\n\t\n\t<20200404013545.GJ9690@pendragon.ideasonboard.com>\n\t<20200406110732.cfxepbhllle6vsrw@uno.localdomain>\n\t<20200406123935.GA14971@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200406123935.GA14971@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [v2 2/3] Documentation: media: Document\n\tread-only subdevice","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 06 Apr 2020 12:44:57 -0000"}}]