| Message ID | 20240416193319.778192-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Generic line based metadata support, internal pads | expand |
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:32:38PM +0300, Sakari Ailus wrote: > Now that metadata mbus formats have been added, it is necessary to define > which fields in struct v4l2_mbus_format are applicable to them (not many). > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > .../userspace-api/media/v4l/subdev-formats.rst | 15 ++++++++------- > include/uapi/linux/v4l2-mediabus.h | 18 ++++++++++++------ > 2 files changed, 20 insertions(+), 13 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst > index d9a5ee954cdd..0547f2733ee3 100644 > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst > @@ -33,7 +33,7 @@ Media Bus Formats > * - __u32 > - ``field`` > - Field order, from enum :c:type:`v4l2_field`. See > - :ref:`field-order` for details. > + :ref:`field-order` for details. Zero on metadata mbus codes. I would write "Zero for" instead of "Zero on". Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > * - __u32 > - ``colorspace`` > - Image colorspace, from enum :c:type:`v4l2_colorspace`. > @@ -45,7 +45,7 @@ Media Bus Formats > conversion is supported by setting the flag > V4L2_SUBDEV_MBUS_CODE_CSC_COLORSPACE in the corresponding struct > :c:type:`v4l2_subdev_mbus_code_enum` during enumeration. > - See :ref:`v4l2-subdev-mbus-code-flags`. > + See :ref:`v4l2-subdev-mbus-code-flags`. Zero on metadata mbus codes. > * - union { > - (anonymous) > * - __u16 > @@ -61,7 +61,7 @@ Media Bus Formats > that ycbcr_enc conversion is supported by setting the flag > V4L2_SUBDEV_MBUS_CODE_CSC_YCBCR_ENC in the corresponding struct > :c:type:`v4l2_subdev_mbus_code_enum` during enumeration. > - See :ref:`v4l2-subdev-mbus-code-flags`. > + See :ref:`v4l2-subdev-mbus-code-flags`. Zero on metadata mbus codes. > * - __u16 > - ``hsv_enc`` > - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`. > @@ -75,7 +75,7 @@ Media Bus Formats > that hsv_enc conversion is supported by setting the flag > V4L2_SUBDEV_MBUS_CODE_CSC_HSV_ENC in the corresponding struct > :c:type:`v4l2_subdev_mbus_code_enum` during enumeration. > - See :ref:`v4l2-subdev-mbus-code-flags` > + See :ref:`v4l2-subdev-mbus-code-flags`. Zero on metadata mbus codes. > * - } > - > * - __u16 > @@ -90,8 +90,8 @@ Media Bus Formats > The driver indicates that quantization conversion is supported by > setting the flag V4L2_SUBDEV_MBUS_CODE_CSC_QUANTIZATION in the > corresponding struct :c:type:`v4l2_subdev_mbus_code_enum` > - during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`. > - > + during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`. Zero on > + metadata mbus codes. > * - __u16 > - ``xfer_func`` > - Transfer function, from enum :c:type:`v4l2_xfer_func`. > @@ -104,7 +104,8 @@ Media Bus Formats > The driver indicates that the transfer function conversion is supported by > setting the flag V4L2_SUBDEV_MBUS_CODE_CSC_XFER_FUNC in the > corresponding struct :c:type:`v4l2_subdev_mbus_code_enum` > - during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`. > + during enumeration. See :ref:`v4l2-subdev-mbus-code-flags`. Zero on > + metadata mbus codes. > * - __u16 > - ``flags`` > - flags See: :ref:v4l2-mbus-framefmt-flags > diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h > index 6b07b73473b5..de1d6161bf62 100644 > --- a/include/uapi/linux/v4l2-mediabus.h > +++ b/include/uapi/linux/v4l2-mediabus.h > @@ -19,12 +19,18 @@ > * @width: image width > * @height: image height > * @code: data format code (from enum v4l2_mbus_pixelcode) > - * @field: used interlacing type (from enum v4l2_field) > - * @colorspace: colorspace of the data (from enum v4l2_colorspace) > - * @ycbcr_enc: YCbCr encoding of the data (from enum v4l2_ycbcr_encoding) > - * @hsv_enc: HSV encoding of the data (from enum v4l2_hsv_encoding) > - * @quantization: quantization of the data (from enum v4l2_quantization) > - * @xfer_func: transfer function of the data (from enum v4l2_xfer_func) > + * @field: used interlacing type (from enum v4l2_field), zero on metadata > + * mbus codes > + * @colorspace: colorspace of the data (from enum v4l2_colorspace), zero on > + * metadata mbus codes > + * @ycbcr_enc: YCbCr encoding of the data (from enum v4l2_ycbcr_encoding), zero > + * on metadata mbus codes > + * @hsv_enc: HSV encoding of the data (from enum v4l2_hsv_encoding), zero on > + * metadata mbus codes > + * @quantization: quantization of the data (from enum v4l2_quantization), zero > + * on metadata mbus codes > + * @xfer_func: transfer function of the data (from enum v4l2_xfer_func), zero > + * on metadata mbus codes > * @flags: flags (V4L2_MBUS_FRAMEFMT_*) > * @reserved: reserved bytes that can be later used > */
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:32:40PM +0300, Sakari Ailus wrote: > Many camera sensors, among other devices, transmit embedded data and image > data for each CSI-2 frame. This embedded data typically contains register > configuration of the sensor that has been used to capture the image data > of the same frame. > > The embedded data is received by the CSI-2 receiver and has the same > properties as the image data, including that it is line based: it has > width, height and bytesperline (stride). > > Add these fields to struct v4l2_meta_format and document them. > > Also add V4L2_FMT_FLAG_META_LINE_BASED to tell a given format is > line-based i.e. these fields of struct v4l2_meta_format are valid for it. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../userspace-api/media/v4l/dev-meta.rst | 21 +++++++++++++++++++ > .../media/v4l/vidioc-enum-fmt.rst | 7 +++++++ > .../media/videodev2.h.rst.exceptions | 1 + > drivers/media/v4l2-core/v4l2-ioctl.c | 5 +++-- > include/uapi/linux/videodev2.h | 10 +++++++++ > 5 files changed, 42 insertions(+), 2 deletions(-) > > diff --git a/Documentation/userspace-api/media/v4l/dev-meta.rst b/Documentation/userspace-api/media/v4l/dev-meta.rst > index 0e7e1ee1471a..5eee9ab60395 100644 > --- a/Documentation/userspace-api/media/v4l/dev-meta.rst > +++ b/Documentation/userspace-api/media/v4l/dev-meta.rst > @@ -47,6 +47,12 @@ member of the ``fmt`` union as needed per the desired operation. Both drivers > and applications must set the remainder of the :c:type:`v4l2_format` structure > to 0. > > +Devices that capture metadata by line have the struct v4l2_fmtdesc > +``V4L2_FMT_FLAG_META_LINE_BASED`` flag set for :c:func:`VIDIOC_ENUM_FMT`. Such > +devices can typically also :ref:`capture image data <capture>`. This primarily > +involves devices that receive the data from a different devices such as a camera > +sensor. > + > .. c:type:: v4l2_meta_format > > .. tabularcolumns:: |p{1.4cm}|p{2.4cm}|p{13.5cm}| > @@ -65,3 +71,18 @@ to 0. > - ``buffersize`` > - Maximum buffer size in bytes required for data. The value is set by the > driver. > + * - __u32 > + - ``width`` > + - Width of a line of metadata in Data Units. Valid when > + :c:type`v4l2_fmtdesc` flag ``V4L2_FMT_FLAG_META_LINE_BASED`` is set, > + otherwise zero. See :c:func:`VIDIOC_ENUM_FMT`. > + * - __u32 > + - ``height`` > + - Number of rows of metadata. Valid when :c:type`v4l2_fmtdesc` flag > + ``V4L2_FMT_FLAG_META_LINE_BASED`` is set, otherwise zero. See > + :c:func:`VIDIOC_ENUM_FMT`. > + * - __u32 > + - ``bytesperline`` > + - Offset in bytes between the beginning of two consecutive lines. Valid > + when :c:type`v4l2_fmtdesc` flag ``V4L2_FMT_FLAG_META_LINE_BASED`` is > + set, otherwise zero. See :c:func:`VIDIOC_ENUM_FMT`. > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > index 000c154b0f98..a439be1b15d1 100644 > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > @@ -227,6 +227,13 @@ the ``mbus_code`` field is handled differently: > The application can ask to configure the quantization of the capture > device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with > :ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set. > + * - ``V4L2_FMT_FLAG_META_LINE_BASED`` > + - 0x0200 > + - The metadata format is line-based. In this case the ``width``, > + ``height`` and ``bytesperline`` fields of :c:type:`v4l2_meta_format` are > + valid. The buffer consists of ``height`` lines, each having ``width`` > + Data Units of data and offset (in bytes) between the beginning of each s/and offset/, and the offset/ > + two consecutive lines is ``bytesperline``. > > Return Value > ============ > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > index 3e58aac4ef0b..bdc628e8c1d6 100644 > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > @@ -215,6 +215,7 @@ replace define V4L2_FMT_FLAG_CSC_XFER_FUNC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > +replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags > > # V4L2 timecode types > replace define V4L2_TC_TYPE_24FPS timecode-type > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > index ae2dca7f2817..2cfc9106857a 100644 > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > @@ -343,8 +343,9 @@ static void v4l_print_format(const void *arg, bool write_only) > case V4L2_BUF_TYPE_META_OUTPUT: > meta = &p->fmt.meta; > pixelformat = meta->dataformat; > - pr_cont(", dataformat=%p4cc, buffersize=%u\n", > - &pixelformat, meta->buffersize); > + pr_cont(", dataformat=%p4cc, buffersize=%u, width=%u, height=%u, bytesperline=%u\n", > + &pixelformat, meta->buffersize, meta->width, > + meta->height, meta->bytesperline); > break; > } > } > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index c7cf20b5da67..37112dfebd0c 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -877,6 +877,7 @@ struct v4l2_fmtdesc { > #define V4L2_FMT_FLAG_CSC_YCBCR_ENC 0x0080 > #define V4L2_FMT_FLAG_CSC_HSV_ENC V4L2_FMT_FLAG_CSC_YCBCR_ENC > #define V4L2_FMT_FLAG_CSC_QUANTIZATION 0x0100 > +#define V4L2_FMT_FLAG_META_LINE_BASED 0x0200 Could the V4L2_FMT_FLAG_META_LINE_BASED flag be set by the V4L2 core for line-based metadata formats, instead of requiring drivers to set it ? It would ensure consistency. > > /* Frame Size and frame rate enumeration */ > /* > @@ -2424,10 +2425,19 @@ struct v4l2_sdr_format { > * struct v4l2_meta_format - metadata format definition > * @dataformat: little endian four character code (fourcc) > * @buffersize: maximum size in bytes required for data > + * @width: number of data units of data per line (valid for line > + * based formats only, see format documentation) > + * @height: number of lines of data per buffer (valid for line based > + * formats only) > + * @bytesperline: offset between the beginnings of two adjacent lines in > + * bytes (valid for line based formats only) > */ > struct v4l2_meta_format { > __u32 dataformat; > __u32 buffersize; > + __u32 width; > + __u32 height; > + __u32 bytesperline; > } __attribute__ ((packed)); > > /**
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:32:44PM +0300, Sakari Ailus wrote: > Document S_ROUTING behaviour for different devices. > > Generally in devices that produce streams the streams are static and some > can be enabled and disabled, whereas in devices that just transport them > or write them to memory, more configurability is allowed. Document this. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Julien Massot <julien.massot@collabora.com> > --- > .../userspace-api/media/v4l/dev-subdev.rst | 24 +++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > index d30dcb9e2537..de8dfd4f11a5 100644 > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > @@ -593,6 +593,30 @@ Any configurations of a stream within a pad, such as format or selections, > are independent of similar configurations on other streams. This is > subject to change in the future. > > +Device types and routing setup > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +Different kinds of sub-devices have differing behaviour for route activation, > +depending on the hardware. In all cases, however, only routes that have the > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag set are active. > + > +Devices generating the streams may allow enabling and disabling some of the > +routes or the configuration is fixed. If the routes can be disabled, not "... some of the routes, or have a fixed routing configuration." > +declaring the routes (or declaring them without > +``VIDIOC_SUBDEV_STREAM_FL_ACTIVE`` flag set) in ``VIDIOC_SUBDEV_S_ROUTING`` will > +disable the routes while the sub-device driver retains the streams and their > +format and selection configuration. I still find the "retains their format and selection configuration" quite unclear :-S > The ``VIDIOC_SUBDEV_S_ROUTING`` will still s/will still/ioctl will still/ > +return such routes back to the user in the routes array, with the > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag unset. > + > +Devices transporting the streams almost always have more configurability with > +respect to routing. Typically any route between the sub-device's sink and source > +pads is possible, and multiple routes (usually up to certain limited number) may > +be active simultaneously. For such devices, no routes are created by the driver > +and user-created routes are fully replaced when ``VIDIOC_SUBDEV_S_ROUTING`` is > +called on the sub-device. Such newly created routes have the device's default > +configuration for format and selection rectangles. > + > Configuring streams > ^^^^^^^^^^^^^^^^^^^ >
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:32:52PM +0300, Sakari Ailus wrote: > Prepare for using ccs_pm_get_init from locations earlier than its the > current place. > > Also add a missing newline while at it. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/ccs/ccs-core.c | 73 ++++++++++++++++---------------- > 1 file changed, 37 insertions(+), 36 deletions(-) > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index 7e5474e38732..d7bc9418eabb 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -1718,6 +1718,43 @@ static int ccs_power_off(struct device *dev) > * Video stream management > */ > > +static int ccs_pm_get_init(struct ccs_sensor *sensor) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > + int rval; > + > + /* > + * It can't use pm_runtime_resume_and_get() here, as the driver > + * relies at the returned value to detect if the device was already > + * active or not. > + */ > + rval = pm_runtime_get_sync(&client->dev); > + if (rval < 0) > + goto error; > + > + /* Device was already active, so don't set controls */ > + if (rval == 1 && !sensor->handler_setup_needed) > + return 0; > + > + sensor->handler_setup_needed = false; > + > + /* Restore V4L2 controls to the previously suspended device */ > + rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > + if (rval) > + goto error; > + > + rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > + if (rval) > + goto error; > + > + /* Keep PM runtime usage_count incremented on success */ > + return 0; > + > +error: > + pm_runtime_put(&client->dev); > + return rval; > +} > + > static int ccs_start_streaming(struct ccs_sensor *sensor) > { > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > @@ -1872,42 +1909,6 @@ static int ccs_stop_streaming(struct ccs_sensor *sensor) > * V4L2 subdev video operations > */ > > -static int ccs_pm_get_init(struct ccs_sensor *sensor) > -{ > - struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > - int rval; > - > - /* > - * It can't use pm_runtime_resume_and_get() here, as the driver > - * relies at the returned value to detect if the device was already > - * active or not. > - */ > - rval = pm_runtime_get_sync(&client->dev); > - if (rval < 0) > - goto error; > - > - /* Device was already active, so don't set controls */ > - if (rval == 1 && !sensor->handler_setup_needed) > - return 0; > - > - sensor->handler_setup_needed = false; > - > - /* Restore V4L2 controls to the previously suspended device */ > - rval = v4l2_ctrl_handler_setup(&sensor->pixel_array->ctrl_handler); > - if (rval) > - goto error; > - > - rval = v4l2_ctrl_handler_setup(&sensor->src->ctrl_handler); > - if (rval) > - goto error; > - > - /* Keep PM runtime usage_count incremented on success */ > - return 0; > -error: > - pm_runtime_put(&client->dev); > - return rval; > -} > - > static int ccs_set_stream(struct v4l2_subdev *subdev, int enable) > { > struct ccs_sensor *sensor = to_ccs_sensor(subdev);
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:32:57PM +0300, Sakari Ailus wrote: > Provide information on the frame layout using frame descriptors. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Julien Massot <julien.massot@collabora.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/ccs/ccs-core.c | 57 ++++++++++++++++++++++++++++++++ > drivers/media/i2c/ccs/ccs.h | 4 +++ > 2 files changed, 61 insertions(+) > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index a711233f6fbf..3ca2415fca3b 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -25,6 +25,7 @@ > #include <linux/slab.h> > #include <linux/smiapp.h> > #include <linux/v4l2-mediabus.h> > +#include <media/mipi-csi2.h> > #include <media/v4l2-cci.h> > #include <media/v4l2-device.h> > #include <media/v4l2-fwnode.h> > @@ -245,6 +246,33 @@ static int ccs_read_all_limits(struct ccs_sensor *sensor) > return ret; > } > > +static u8 ccs_mipi_csi2_data_type(unsigned int bpp) > +{ > + switch (bpp) { > + case 6: > + return MIPI_CSI2_DT_RAW6; > + case 7: > + return MIPI_CSI2_DT_RAW7; > + case 8: > + return MIPI_CSI2_DT_RAW8; > + case 10: > + return MIPI_CSI2_DT_RAW10; > + case 12: > + return MIPI_CSI2_DT_RAW12; > + case 14: > + return MIPI_CSI2_DT_RAW14; > + case 16: > + return MIPI_CSI2_DT_RAW16; > + case 20: > + return MIPI_CSI2_DT_RAW20; > + case 24: > + return MIPI_CSI2_DT_RAW24; > + default: > + WARN_ON(1); > + return 0; > + } > +} > + > static int ccs_read_frame_fmt(struct ccs_sensor *sensor) > { > struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd); > @@ -2633,6 +2661,34 @@ static int ccs_set_selection(struct v4l2_subdev *subdev, > return ret; > } > > +static int ccs_get_frame_desc(struct v4l2_subdev *subdev, unsigned int pad, > + struct v4l2_mbus_frame_desc *desc) > +{ > + struct ccs_sensor *sensor = to_ccs_sensor(subdev); > + struct v4l2_mbus_frame_desc_entry *entry = desc->entry; > + > + switch (sensor->hwcfg.csi_signalling_mode) { > + case CCS_CSI_SIGNALING_MODE_CSI_2_DPHY: > + case CCS_CSI_SIGNALING_MODE_CSI_2_CPHY: > + desc->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > + break; > + default: > + /* FIXME: CCP2 support */ > + return -EINVAL; > + } > + > + entry->pixelcode = sensor->csi_format->code; > + entry->stream = CCS_STREAM_PIXEL; > + entry->bus.csi2.dt = > + sensor->csi_format->width == sensor->csi_format->compressed ? > + ccs_mipi_csi2_data_type(sensor->csi_format->width) : > + CCS_DEFAULT_COMPRESSED_DT; > + entry++; > + desc->num_entries++; > + > + return 0; > +} > + > static int ccs_get_skip_frames(struct v4l2_subdev *subdev, u32 *frames) > { > struct ccs_sensor *sensor = to_ccs_sensor(subdev); > @@ -3055,6 +3111,7 @@ static const struct v4l2_subdev_pad_ops ccs_pad_ops = { > .set_selection = ccs_set_selection, > .enable_streams = ccs_enable_streams, > .disable_streams = ccs_disable_streams, > + .get_frame_desc = ccs_get_frame_desc, > }; > > static const struct v4l2_subdev_sensor_ops ccs_sensor_ops = { > diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h > index 4725e6eca8d0..90b442a3d53e 100644 > --- a/drivers/media/i2c/ccs/ccs.h > +++ b/drivers/media/i2c/ccs/ccs.h > @@ -46,6 +46,8 @@ > > #define CCS_COLOUR_COMPONENTS 4 > > +#define CCS_DEFAULT_COMPRESSED_DT MIPI_CSI2_DT_USER_DEFINED(0) > + > #define SMIAPP_NAME "smiapp" > #define CCS_NAME "ccs" > > @@ -175,6 +177,8 @@ struct ccs_csi_data_format { > #define CCS_PAD_SRC 1 > #define CCS_PADS 2 > > +#define CCS_STREAM_PIXEL 0 > + > struct ccs_binning_subtype { > u8 horizontal:4; > u8 vertical:4;
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:33:01PM +0300, Sakari Ailus wrote: > The CCS embedded data format has multiple aspects (packing, encoding and > the rest, including register addresses and semantics). Explicitly allow > non-compliant embedded data to use the two former to reduce redundant > documentation. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../userspace-api/media/drivers/camera-sensor.rst | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/userspace-api/media/drivers/camera-sensor.rst b/Documentation/userspace-api/media/drivers/camera-sensor.rst > index 9f3b0da3ad0d..dc415b8f6c8e 100644 > --- a/Documentation/userspace-api/media/drivers/camera-sensor.rst > +++ b/Documentation/userspace-api/media/drivers/camera-sensor.rst > @@ -123,3 +123,14 @@ In general, changing the embedded data format from the driver-configured values > is not supported. The height of the metadata is device-specific and the width > is that (or less of that) of the image width, as configured on the pixel data > stream. > + > +CCS and non-CCS embedded data > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +Embedded data which is fully compliant with CCS definitions uses ``CCS embedded > +data format <MEDIA-BUS-FMT-CCS-EMBEDDED>`` media bus code (level > +3). Device-specific embedded data compliant with either MIPI CCS embedded data > +levels 1 or 2 only shall not use CCS embedded data mbus code, but may refer to > +CCS embedded data documentation with the level of conformance specified and omit > +documenting these aspects of the format. The rest of the device-specific > +embedded data format is documented in the context of the data format itself.
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:33:07PM +0300, Sakari Ailus wrote: > Compute scaling configuration from sub-device state instead of storing it > to the driver's device context struct. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/ccs/ccs-core.c | 59 ++++++++++++++++++++++---------- > drivers/media/i2c/ccs/ccs.h | 3 -- > 2 files changed, 40 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/i2c/ccs/ccs-core.c b/drivers/media/i2c/ccs/ccs-core.c > index 08e719d611fb..541faa7d84a6 100644 > --- a/drivers/media/i2c/ccs/ccs-core.c > +++ b/drivers/media/i2c/ccs/ccs-core.c > @@ -531,19 +531,51 @@ ccs_get_binning(struct ccs_sensor *sensor, u8 *binning_mode, u8 *binh, u8 *binv) > *binv = sink_crop->height / sink_comp->height; > } > > +static void ccs_get_scaling(struct ccs_sensor *sensor, u8 *scaling_mode, > + u8 *scale_m) > +{ > + struct v4l2_subdev_state *state = > + v4l2_subdev_get_locked_active_state(&sensor->scaler->sd); > + const struct v4l2_rect *sink_crop = > + v4l2_subdev_state_get_crop(state, CCS_PAD_SINK, > + CCS_STREAM_PIXEL); > + const struct v4l2_rect *sink_comp = > + v4l2_subdev_state_get_compose(state, CCS_PAD_SINK, > + CCS_STREAM_PIXEL); > + > + *scale_m = sink_crop->width * CCS_LIM(sensor, SCALER_N_MIN) / > + sink_comp->width; > + > + if (!scaling_mode) > + return; > + > + if (sink_crop->width == sink_comp->width) > + *scaling_mode = CCS_SCALING_MODE_NO_SCALING; > + else if (sink_crop->height == sink_comp->height) > + *scaling_mode = CCS_SCALING_MODE_HORIZONTAL; > + else > + *scaling_mode = SMIAPP_SCALING_MODE_BOTH; > +} > + > static int ccs_pll_update(struct ccs_sensor *sensor) > { > struct ccs_pll *pll = &sensor->pll; > u8 binh, binv; > + u8 scale_m; > int rval; > > ccs_get_binning(sensor, NULL, &binh, &binv); > > + if (sensor->scaler) > + ccs_get_scaling(sensor, NULL, &scale_m); > + else > + scale_m = CCS_LIM(sensor, SCALER_N_MIN); > + > pll->binning_horizontal = binh; > pll->binning_vertical = binv; > pll->link_freq = > sensor->link_freq->qmenu_int[sensor->link_freq->val]; > - pll->scale_m = sensor->scale_m; > + pll->scale_m = scale_m; > pll->bits_per_pixel = sensor->csi_format->compressed; > > rval = ccs_pll_try(sensor, pll); > @@ -1186,7 +1218,7 @@ static int ccs_get_mbus_formats(struct ccs_sensor *sensor) > /* Figure out which BPP values can be used with which formats. */ > pll->binning_horizontal = 1; > pll->binning_vertical = 1; > - pll->scale_m = sensor->scale_m; > + pll->scale_m = CCS_LIM(sensor, SCALER_N_MIN); > > for (i = 0; i < ARRAY_SIZE(ccs_csi_data_formats); i++) { > sensor->compressed_min_bpp = > @@ -1935,11 +1967,15 @@ static int ccs_enable_streams(struct v4l2_subdev *subdev, > /* Scaling */ > if (CCS_LIM(sensor, SCALING_CAPABILITY) > != CCS_SCALING_CAPABILITY_NONE) { > - rval = ccs_write(sensor, SCALING_MODE, sensor->scaling_mode); > + u8 scaling_mode, scale_m; > + > + ccs_get_scaling(sensor, &scaling_mode, &scale_m); > + > + rval = ccs_write(sensor, SCALING_MODE, scaling_mode); > if (rval < 0) > goto err_pm_put; > > - rval = ccs_write(sensor, SCALE_M, sensor->scale_m); > + rval = ccs_write(sensor, SCALE_M, scale_m); > if (rval < 0) > goto err_pm_put; > } > @@ -2255,7 +2291,6 @@ static void ccs_propagate(struct v4l2_subdev *subdev, > struct v4l2_subdev_state *sd_state, int which, You can now drop the which parameter to this function \o/ With this, and the is_active variable removed from the caller, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > int target) > { > - struct ccs_sensor *sensor = to_ccs_sensor(subdev); > struct ccs_subdev *ssd = to_ccs_subdev(subdev); > struct v4l2_rect *comp, *crop; > struct v4l2_mbus_framefmt *fmt; > @@ -2268,13 +2303,6 @@ static void ccs_propagate(struct v4l2_subdev *subdev, > CCS_STREAM_PIXEL); > comp->width = crop->width; > comp->height = crop->height; > - if (which == V4L2_SUBDEV_FORMAT_ACTIVE) { > - if (ssd == sensor->scaler) { > - sensor->scale_m = CCS_LIM(sensor, SCALER_N_MIN); > - sensor->scaling_mode = > - CCS_SCALING_MODE_NO_SCALING; > - } > - } > fallthrough; > case V4L2_SEL_TGT_COMPOSE: > crop = v4l2_subdev_state_get_crop(sd_state, CCS_PAD_SRC, > @@ -2653,11 +2681,6 @@ static void ccs_set_compose_scaler(struct v4l2_subdev *subdev, > * CCS_LIM(sensor, SCALER_N_MIN)) & ~1; > else > sel->r.height = sink_crop->height; > - > - if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > - sensor->scale_m = scale_m; > - sensor->scaling_mode = mode; > - } > } > /* We're only called on source pads. This function sets scaling. */ > static int ccs_set_compose(struct v4l2_subdev *subdev, > @@ -3763,8 +3786,6 @@ static int ccs_probe(struct i2c_client *client) > sensor->pixel_array = &sensor->ssds[sensor->ssds_used]; > sensor->ssds_used++; > > - sensor->scale_m = CCS_LIM(sensor, SCALER_N_MIN); > - > /* prepare PLL configuration input values */ > sensor->pll.bus_type = CCS_PLL_BUS_TYPE_CSI2_DPHY; > sensor->pll.csi2.lanes = sensor->hwcfg.lanes; > diff --git a/drivers/media/i2c/ccs/ccs.h b/drivers/media/i2c/ccs/ccs.h > index aadbd4302607..1c30fa85bed6 100644 > --- a/drivers/media/i2c/ccs/ccs.h > +++ b/drivers/media/i2c/ccs/ccs.h > @@ -237,9 +237,6 @@ struct ccs_sensor { > u32 embedded_mbus_code; > u8 emb_data_ctrl; > > - u8 scale_m; > - u8 scaling_mode; > - > u8 frame_skip; > u16 embedded_start; /* embedded data start line */ > u16 embedded_end;
Hi Sakari, Thank you for the patch. On Tue, Apr 16, 2024 at 10:33:16PM +0300, Sakari Ailus wrote: > Add support for the G_SELECTION IOCTL in the ov2740 driver. We need to first define and document how G_SELECTION should behave for raw sensors. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/i2c/ov2740.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > index e37d824291fe..6e355e986b88 100644 > --- a/drivers/media/i2c/ov2740.c > +++ b/drivers/media/i2c/ov2740.c > @@ -23,6 +23,11 @@ > #define OV2740_DATA_LANES 2 > #define OV2740_RGB_DEPTH 10 > > +#define OV2740_DUMMY_LINES_PRE 8U > +#define OV2740_DUMMY_LINES_POST 8U > +#define OV2740_ACTIVE_WIDTH 1932U > +#define OV2740_ACTIVE_HEIGHT 1092U > + > #define OV2740_REG_CHIP_ID 0x300a > #define OV2740_CHIP_ID 0x2740 > > @@ -1114,6 +1119,31 @@ static int ov2740_set_format(struct v4l2_subdev *sd, > fmt->pad, fmt->stream); > } > > +static int ov2740_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + struct v4l2_subdev_selection *sel) > +{ > + switch (sel->target) { > + case V4L2_SEL_TGT_NATIVE_SIZE: > + case V4L2_SEL_TGT_CROP_BOUNDS: > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = OV2740_ACTIVE_WIDTH; > + sel->r.height = OV2740_DUMMY_LINES_PRE + OV2740_ACTIVE_HEIGHT + > + OV2740_DUMMY_LINES_POST; > + return 0; > + case V4L2_SEL_TGT_CROP: > + case V4L2_SEL_TGT_CROP_DEFAULT: > + sel->r.top = OV2740_DUMMY_LINES_PRE; > + sel->r.left = 0; > + sel->r.width = OV2740_ACTIVE_WIDTH; > + sel->r.height = OV2740_ACTIVE_HEIGHT; > + return 0; > + } > + > + return -EINVAL; > +} > + > static int ov2740_enum_mbus_code(struct v4l2_subdev *sd, > struct v4l2_subdev_state *sd_state, > struct v4l2_subdev_mbus_code_enum *code) > @@ -1222,6 +1252,7 @@ static const struct v4l2_subdev_video_ops ov2740_video_ops = { > static const struct v4l2_subdev_pad_ops ov2740_pad_ops = { > .get_fmt = v4l2_subdev_get_fmt, > .set_fmt = ov2740_set_format, > + .get_selection = ov2740_get_selection, > .enum_mbus_code = ov2740_enum_mbus_code, > .enum_frame_size = ov2740_enum_frame_size, > .enable_streams = ov2740_enable_streams,
Hi Sakari, Thank you for the patches. This is progressing nicely, and I think we can merge part of this series for v6.10. Patches from 01/46 to 08/46 and from 11/46 to 17/46 are nearly there, just a few of them will need a new (and hopefully final) version. You could additionally merge the CCS-specific patches 18/46 to 24/46 if desired. Some ov2740 rework could go in too. The rest of the patches (and in particular 09/46 and 10/46) depend on internal pads support, which still needs discussions and additional documentation for raw sensors. When posting v10, please move 09/46 and 10/46 after 24/46. On Tue, Apr 16, 2024 at 10:32:33PM +0300, Sakari Ailus wrote: > Hi folks, > > Here are a few patches to add support generic, line based metadata as well > as internal pads. While the amount of code is not very large, to the > contrary it is quite small actually IMO, I presume what this is about and > why it is being proposed requires some explaining. > > Metadata mbus codes and formats have existed for some time in V4L2. They > however have been only used by drivers that produce the data itself and > effectively this metadata has always been statistics of some sort (at > least when it comes to ISPs). What is different here is that we intend to > add support for metadata originating from camera sensors. > > Camera sensors produce different kinds of metadata, embedded data (usually > register address--value pairs used to capture the frame, in a more or less > sensor specific format), histograms (in a very sensor specific format), > dark pixels etc. The number of these formats is probably going to be about > as large as image data formats if not larger, as the image data formats > are much better standardised but a smaller subset of them will be > supported by V4L2, at least initially but possibly much more in the long > run. > > Having this many device specific formats would be a major problem for all > the other drivers along that pipeline (not to mention the users of those > drivers), including bridge (e.g. CSI-2 to parallel) but especially CSI-2 > receiver drivers that have DMA: the poor driver developer would not only > need to know camera sensor specific formats but to choose the specific > packing of that format suitable for the DMA used by the hardware. It is > unlikely many of these would ever get tested while being present on the > driver API. Also adding new sensors with new embedded data formats would > involve updating all bridge and CSI-2 receiver drivers. I don't expect > this to be a workable approach. > > Instead what I'm proposing is to use specific metadata formats on the > sensor devices only, on internal pads (more about those soon) of the > sensors, only visible in the UAPI, and then generic mbus formats along the > pipeline and finally generic V4L2 metadata formats on the DMAs (specific > to bit depth and packing). This would unsnarl the two, defining what data > there is (specific mbus code) and how that is transported and packed > (generic mbus codes and V4L2 formats). > > The user space would be required to "know" the path of that data from the > sensor's internal pad to the V4L2 video node. I do not see this as these > devices require at least some knowledge of the pipeline, i.e. hardware at > hand. Separating what the data means and how it is packed may even be > beneficial: it allows separating code that interprets the data (sensor > internal mbus code) from the code that accesses it (packing). > > These formats are in practice line based, meaning that there may be > padding at the end of the line, depending on the bus as well as the DMA. > If non-line based formats are needed, it is always possible to set the > "height" field to 1. > > The internal sink pads are an alternative to source routes [1]. The source > routes were not universally liked and I do have to say I like re-using > existing interface concepts (pads and everything you can do with pads, > including access format, selections etc.) wherever it makes sense, instead > of duplicating functionality. > > Effectively internal sink pads behave mostly just like sink pads, but they > describe a flow of data that originates from a sub-device instead of > arriving to a sub-device. The SUBDEV_S_ROUTING IOCTLs are used to enable > and disable routes from internal sink pads to sub-device's source pads. > The subdev format IOCTLs are usable, too, so one can find which subdev > format is available on given internal sink pad. > > I've also pushed these here and I'll keep updating the branch, I've also > included untested OV2740 patches: > > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=metadata> > > Questions and comments are most welcome. > > Preliminary media-ctl and yavta patches can be found here: > > <URL:https://git.retiisi.eu/?p=~sailus/yavta.git;a=shortlog;h=refs/heads/metadata> > <URL:https://git.retiisi.eu/?p=~sailus/v4l-utils.git;a=shortlog;h=refs/heads/metadata> > > I have used IMX219 as an example on routing in a sensor driver in this > version. > > The patches are on my master branch > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/>. > > [1] <URL:https://lore.kernel.org/linux-media/20220831141357.1396081-20-tomi.valkeinen@ideasonboard.com/> > > since v8: > > - Move the patch adding internal pad flag past the routing API reworks, as > well as a few other patches, in order to separate the patches to those > that could still be merged for v6.10 (routing changes) and those that > couldn't (sensor API related). The patch on the edge is "media: uapi: > v4l: subdev: Enable streams API". > > - Include Laurent's two patches to address crop API issues wrt. streams. > > - Add two patches to prepare for CCS driver rework (media: ccs: Move > ccs_pm_get_init function up and media: ccs: Rename out label of > ccs_start_streaming). > > - Address issues in the ov2740 driver patches (as well as the driver > itself), 4 more patches towards the end of the set. > > - Improved generic metadata format names, align with other existing > formats. > > - Improved ov2740 embedded data documentation. > > - Reworked streams and camera sensor documentation based on Laurent's > comments mainly. In particular, the contradictory concept of internal > source pads no longer should exist in the patches. > > - Fixed pad numbering in the CCS example. > > - Fixed S_ROUTING behaviour when len_routes is too small and when > S_ROUTING isn't implemented by the driver. > > - Reorder sections in meta-formats.rst alphabetically. > > - Add a note per struct fields that certain struct v4l2_subdev_format are > zero for metadata mbus codes. > > - CCS driver patch cleanups. > > - CCS driver metadata width fix for space-efficient embedded data at 16 > bpp and over. > > - Postpone CCS frame descriptor quirk for now. > > - Use MIPI_CSI2_DT_USER_DEFINED(0) instead of a numerical value for > compressed data datatype. > > since v7: > > - Add embedded data support for the ov2740 driver. > > - Add three patches on top, to add an IMMUTABLE flag to source streams > when they cannot be disabled. > > - Improved documentation of len_routes and num_routes arguments of > [GS]_ROUTING. > > - Remove one inclusion of twice-included media/v4l2-fwnode.h in > drivers/media/i2c/ccs/ccs-core.c . > > - Add missing forward declaration of ccs_internal_ops in > drivers/media/i2c/ccs/ccs-core.c . > > since v6: > > - Improve embedded data UAPI documentation on camera sensors. > > - Improve wording of stream glossary entry. > > - Improve internal pad flag documentation. > > - Fix definition of "data unit" and remove an extra "only" in INTERNAL pad > flag description (1st patch). > > - Use IMX219 driver in examples consistently. > > - Remove the CSI-2 to parallel bridge from the example to simplify the > example. > > - Minor rewording of some parts of the routing examples. > > - Rebase on unified sub-device state information access functions: > <URL:https://lore.kernel.org/linux-media/20231027095913.1010187-1-sakari.ailus@linux.intel.com/T/#t> > > - In CCS driver, do not maintain current active configuration in driver's > device context struct (apart from mbus codes). Rely on sub-device state > locking and clean up the code. (Multiple patches towards the end of the > set.) > > - Arrange the CCS patches early in the set towards the end of the set. > > - Move the patch enabling streams API to the end of the set. > > - Rework IOCTL argument copying condition for [GS]_ROUTING). > > - Handle copying back routes in S_ROUTING, do not rely on G_ROUTING > IOCTL implementation. > > - Rebase on metadata preparation patchset v6: > <URL:https://lore.kernel.org/linux-media/20231106121805.1266696-1-sakari.ailus@linux.intel.com/T/#t>. > > since v5: > > - Rebase on new set of preparation patches. > > - Switch CCS driver from s_stream to enable_streams/disable_streams. Keep > streaming state information --- the sensor remains in streaming state if > any of the streams is enabled. > > - Fix setting mbus code on embedded data in get_frame_desc() op in the CCS > driver. > > since v4: > > - Add a patch to acquire two sub-device states that may use the same lock. > > - Add a patch for CCS driver to remove ccs_get_crop_compose() helper. > > - Add a patch for CCS driver moving acquiring and releasing the mutex to > the s_stream callback. > > - Add a patch for CCS driver to rely on sub-device state locking using a > single driver-provided lock. > > - Fixed calculating minimum number of routes in copying the routes > (thanks, Laurent). > > - Moved a label in S_ROUTING handling to make Clang happy (hopefully). > > - Fixed setting emb_data_ctrl register for CCS embedded data support. > > - Rebase on Laurent's cleanup patches. > > - Wrap a few long lines. > > - Write in embedded data documentation sensor drivers generally don't > allow configuring it. > > since v3: > > - Separate preparation patches from this set. > > - Add a definition for "Data unit", a pixel that is not image data and use > it instead in format documentation. > > - Fix more numbered lists in dev-subdev.rst. > > - Remove a redundant definition for V4L2_META_FMT_GENERIC_CSI2_2_24 --- > V4L2_META_FMT_GENERIC_CSI2_12 can be used instead. > > - Use "X" instead of "p" to denote padding in format documentation. > > - Use IMX219 in examples instead of CCS. > > - Document that the generic V4L2 CSI-2 metadata formats use padding > defined in CSI-2 spec and packing defined in CCS spec. > > - Add patches to align [GS]_ROUTING behaviour with V4L2. This means mainly > returning configured routes as part of S_ROUTING as well. "len_routes" > field is added to denote the length of the array and having more routes > than fits in the array is no longer an error. Also added more reserved > fields. > > - Added trivial support for S_ROUTING (via G_ROUTING implementation) for > use in drivers with static-only routes. > > - Added helper functions to obtain mbus format as well as crop and compose > rectangles that are streams-independent. > > - Added a patch to define generic CSI-2 long packet types. > > - Removed MEDIA_BUS_FMT_IS_META() macro. It didn't seem useful in the end. > > - Use a single CCS embedded data format. The bit depth can be selected > using the meta stream on the source pad. > > - Fix mbus code numbers (there were holes due to removed redundant > formats). > > - Fix generic mbus code documentation (byte was being used instead of > bit). > > - Fix spelling of "length". > > - Added a patch to remove v4l2_subdev_enable_streams_api that disables > streams API. This should be merged once libcamera support for streams > works nicely. > > - Don't use strings in printing frame descriptor flags. > > - Warn on string truncation in printing frame descriptor. > > since v2: > > - Add a better example, with formats. > > - Add CCS static data media bus codes. > > - Added an example demonstrating the use of internal pads. --- Is the > level of detail enough for the purpose? > > - Improved documentation. > > - Added a macro to tell whether a format is a metadata format. > (Documentation could be added.) > > - A small ReST syntax fix in the same section. > > - Drop leftovers of a patch checking for the INTERNAL_SOURCE flag. > > since v1: > > - Make the new pad flag just "INTERNAL", requiring either SINK or SOURCE > pad flag to accompany it. Removed the union in struct v4l2_subdev_route. > > - Add the term "stream" to MC glossary. > > - Improved and fixed documentation (according to comments). > > - Note these formats are little endian. > > - Remove 1X8 from the names of the mbus codes. These formats have generally > 8 bits per pixel. > > - Fix mbus code numbering (had holes in RFC). > > - Add new metadata fields to debug prints. > > - Fix a minor documentation build issue. > > Laurent Pinchart (2): > media: v4l2-subdev: Fix stream handling for crop API > media: v4l2-subdev: Clearly document that the crop API won't be > extended > > Sakari Ailus (44): > media: Documentation: Add "stream" into glossary > media: uapi: Add generic serial metadata mbus formats > media: uapi: Document which mbus format fields are valid for metadata > media: uapi: v4l: Add generic 8-bit metadata format definitions > media: v4l: Support line-based metadata capture > media: Documentation: Additional streams generally don't harm capture > media: Documentation: Document embedded data guidelines for camera > sensors > media: Documentation: v4l: Document internal sink pads > media: Documentation: Document S_ROUTING behaviour > media: v4l: subdev: Add a function to lock two sub-device states, use > it > media: v4l: subdev: Move G_ROUTING handling below S_ROUTING > media: v4l: subdev: Copy argument back to user also for S_ROUTING > media: v4l: subdev: Add len_routes field to struct v4l2_subdev_routing > media: v4l: subdev: Return routes set using S_ROUTING > media: v4l: subdev: Add trivial set_routing support > media: ccs: No need to set streaming to false in power off > media: ccs: Move ccs_pm_get_init function up > media: ccs: Rename out label of ccs_start_streaming > media: ccs: Use {enable,disable}_streams operations > media: ccs: Track streaming state > media: ccs: Move ccs_validate_csi_data_format up > media: ccs: Support frame descriptors > media: uapi: v4l: subdev: Enable streams API > media: mc: Add INTERNAL pad flag > media: uapi: ccs: Add media bus code for MIPI CCS embedded data > media: Documentation: Document non-CCS use of CCS embedded data format > media: Documentation: ccs: Document routing > media: ccs: Add support for embedded data stream > media: ccs: Remove ccs_get_crop_compose helper > media: ccs: Rely on sub-device state locking > media: ccs: Compute binning configuration from sub-device state > media: ccs: Compute scaling configuration from sub-device state > media: ccs: Remove which parameter from ccs_propagate > media: uapi: Add media bus code for ov2740 embedded data > media: ov2740: Fix LINK_FREQ and PIXEL_RATE control value reporting > media: ov2740: Remove shorthand variables > media: ov2740: Switch to {enable,disable}_streams > media: ov2740: Track streaming state > media: ov2740: Add support for embedded data > media: ov2740: Add generic sensor fwnode properties as controls > media: ov2740: Add support for G_SELECTION IOCTL > media: v4l: Add V4L2_SUBDEV_ROUTE_FL_IMMUTABLE sub-device routing flag > media: ccs: Add IMMUTABLE route flag > media: ov2740: Add IMMUTABLE route flag > > .../media/drivers/camera-sensor.rst | 32 + > .../userspace-api/media/drivers/ccs.rst | 38 +- > .../userspace-api/media/glossary.rst | 15 + > .../media/mediactl/media-types.rst | 9 + > .../userspace-api/media/v4l/dev-meta.rst | 21 + > .../userspace-api/media/v4l/dev-subdev.rst | 179 ++- > .../userspace-api/media/v4l/meta-formats.rst | 3 +- > .../media/v4l/metafmt-generic.rst | 328 +++++ > .../media/v4l/subdev-formats.rst | 374 +++++- > .../media/v4l/vidioc-enum-fmt.rst | 7 + > .../media/v4l/vidioc-subdev-g-crop.rst | 6 +- > .../media/v4l/vidioc-subdev-g-routing.rst | 60 +- > .../media/videodev2.h.rst.exceptions | 1 + > drivers/media/i2c/ccs/ccs-core.c | 1050 +++++++++++------ > drivers/media/i2c/ccs/ccs.h | 27 +- > drivers/media/i2c/ov2740.c | 304 +++-- > drivers/media/mc/mc-entity.c | 15 +- > drivers/media/v4l2-core/v4l2-ioctl.c | 25 +- > drivers/media/v4l2-core/v4l2-subdev.c | 118 +- > include/media/v4l2-subdev.h | 42 + > include/uapi/linux/media-bus-format.h | 13 + > include/uapi/linux/media.h | 1 + > include/uapi/linux/v4l2-mediabus.h | 18 +- > include/uapi/linux/v4l2-subdev.h | 18 +- > include/uapi/linux/videodev2.h | 18 + > 25 files changed, 2183 insertions(+), 539 deletions(-) > create mode 100644 Documentation/userspace-api/media/v4l/metafmt-generic.rst
Hi Laurent, On Fri, Apr 19, 2024 at 07:30:09PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. Thanks for the review. > > On Tue, Apr 16, 2024 at 10:32:40PM +0300, Sakari Ailus wrote: > > Many camera sensors, among other devices, transmit embedded data and image > > data for each CSI-2 frame. This embedded data typically contains register > > configuration of the sensor that has been used to capture the image data > > of the same frame. > > > > The embedded data is received by the CSI-2 receiver and has the same > > properties as the image data, including that it is line based: it has > > width, height and bytesperline (stride). > > > > Add these fields to struct v4l2_meta_format and document them. > > > > Also add V4L2_FMT_FLAG_META_LINE_BASED to tell a given format is > > line-based i.e. these fields of struct v4l2_meta_format are valid for it. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../userspace-api/media/v4l/dev-meta.rst | 21 +++++++++++++++++++ > > .../media/v4l/vidioc-enum-fmt.rst | 7 +++++++ > > .../media/videodev2.h.rst.exceptions | 1 + > > drivers/media/v4l2-core/v4l2-ioctl.c | 5 +++-- > > include/uapi/linux/videodev2.h | 10 +++++++++ > > 5 files changed, 42 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-meta.rst b/Documentation/userspace-api/media/v4l/dev-meta.rst > > index 0e7e1ee1471a..5eee9ab60395 100644 > > --- a/Documentation/userspace-api/media/v4l/dev-meta.rst > > +++ b/Documentation/userspace-api/media/v4l/dev-meta.rst > > @@ -47,6 +47,12 @@ member of the ``fmt`` union as needed per the desired operation. Both drivers > > and applications must set the remainder of the :c:type:`v4l2_format` structure > > to 0. > > > > +Devices that capture metadata by line have the struct v4l2_fmtdesc > > +``V4L2_FMT_FLAG_META_LINE_BASED`` flag set for :c:func:`VIDIOC_ENUM_FMT`. Such > > +devices can typically also :ref:`capture image data <capture>`. This primarily > > +involves devices that receive the data from a different devices such as a camera > > +sensor. > > + > > .. c:type:: v4l2_meta_format > > > > .. tabularcolumns:: |p{1.4cm}|p{2.4cm}|p{13.5cm}| > > @@ -65,3 +71,18 @@ to 0. > > - ``buffersize`` > > - Maximum buffer size in bytes required for data. The value is set by the > > driver. > > + * - __u32 > > + - ``width`` > > + - Width of a line of metadata in Data Units. Valid when > > + :c:type`v4l2_fmtdesc` flag ``V4L2_FMT_FLAG_META_LINE_BASED`` is set, > > + otherwise zero. See :c:func:`VIDIOC_ENUM_FMT`. > > + * - __u32 > > + - ``height`` > > + - Number of rows of metadata. Valid when :c:type`v4l2_fmtdesc` flag > > + ``V4L2_FMT_FLAG_META_LINE_BASED`` is set, otherwise zero. See > > + :c:func:`VIDIOC_ENUM_FMT`. > > + * - __u32 > > + - ``bytesperline`` > > + - Offset in bytes between the beginning of two consecutive lines. Valid > > + when :c:type`v4l2_fmtdesc` flag ``V4L2_FMT_FLAG_META_LINE_BASED`` is > > + set, otherwise zero. See :c:func:`VIDIOC_ENUM_FMT`. > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > index 000c154b0f98..a439be1b15d1 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-enum-fmt.rst > > @@ -227,6 +227,13 @@ the ``mbus_code`` field is handled differently: > > The application can ask to configure the quantization of the capture > > device when calling the :ref:`VIDIOC_S_FMT <VIDIOC_G_FMT>` ioctl with > > :ref:`V4L2_PIX_FMT_FLAG_SET_CSC <v4l2-pix-fmt-flag-set-csc>` set. > > + * - ``V4L2_FMT_FLAG_META_LINE_BASED`` > > + - 0x0200 > > + - The metadata format is line-based. In this case the ``width``, > > + ``height`` and ``bytesperline`` fields of :c:type:`v4l2_meta_format` are > > + valid. The buffer consists of ``height`` lines, each having ``width`` > > + Data Units of data and offset (in bytes) between the beginning of each > > s/and offset/, and the offset/ Sounds good. > > > + two consecutive lines is ``bytesperline``. > > > > Return Value > > ============ > > diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > index 3e58aac4ef0b..bdc628e8c1d6 100644 > > --- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > +++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions > > @@ -215,6 +215,7 @@ replace define V4L2_FMT_FLAG_CSC_XFER_FUNC fmtdesc-flags > > replace define V4L2_FMT_FLAG_CSC_YCBCR_ENC fmtdesc-flags > > replace define V4L2_FMT_FLAG_CSC_HSV_ENC fmtdesc-flags > > replace define V4L2_FMT_FLAG_CSC_QUANTIZATION fmtdesc-flags > > +replace define V4L2_FMT_FLAG_META_LINE_BASED fmtdesc-flags > > > > # V4L2 timecode types > > replace define V4L2_TC_TYPE_24FPS timecode-type > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index ae2dca7f2817..2cfc9106857a 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -343,8 +343,9 @@ static void v4l_print_format(const void *arg, bool write_only) > > case V4L2_BUF_TYPE_META_OUTPUT: > > meta = &p->fmt.meta; > > pixelformat = meta->dataformat; > > - pr_cont(", dataformat=%p4cc, buffersize=%u\n", > > - &pixelformat, meta->buffersize); > > + pr_cont(", dataformat=%p4cc, buffersize=%u, width=%u, height=%u, bytesperline=%u\n", > > + &pixelformat, meta->buffersize, meta->width, > > + meta->height, meta->bytesperline); > > break; > > } > > } > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > > index c7cf20b5da67..37112dfebd0c 100644 > > --- a/include/uapi/linux/videodev2.h > > +++ b/include/uapi/linux/videodev2.h > > @@ -877,6 +877,7 @@ struct v4l2_fmtdesc { > > #define V4L2_FMT_FLAG_CSC_YCBCR_ENC 0x0080 > > #define V4L2_FMT_FLAG_CSC_HSV_ENC V4L2_FMT_FLAG_CSC_YCBCR_ENC > > #define V4L2_FMT_FLAG_CSC_QUANTIZATION 0x0100 > > +#define V4L2_FMT_FLAG_META_LINE_BASED 0x0200 > > Could the V4L2_FMT_FLAG_META_LINE_BASED flag be set by the V4L2 core for > line-based metadata formats, instead of requiring drivers to set it ? It > would ensure consistency. This would essentially need to be set after driver callbacks dealing with a format-related IOCTL has returned, drivers don't need this information as such. I can add a separate patch for it. > > > > > /* Frame Size and frame rate enumeration */ > > /* > > @@ -2424,10 +2425,19 @@ struct v4l2_sdr_format { > > * struct v4l2_meta_format - metadata format definition > > * @dataformat: little endian four character code (fourcc) > > * @buffersize: maximum size in bytes required for data > > + * @width: number of data units of data per line (valid for line > > + * based formats only, see format documentation) > > + * @height: number of lines of data per buffer (valid for line based > > + * formats only) > > + * @bytesperline: offset between the beginnings of two adjacent lines in > > + * bytes (valid for line based formats only) > > */ > > struct v4l2_meta_format { > > __u32 dataformat; > > __u32 buffersize; > > + __u32 width; > > + __u32 height; > > + __u32 bytesperline; > > } __attribute__ ((packed)); > > > > /** >
Hi Laurent, On Fri, Apr 19, 2024 at 08:17:20PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tue, Apr 16, 2024 at 10:32:44PM +0300, Sakari Ailus wrote: > > Document S_ROUTING behaviour for different devices. > > > > Generally in devices that produce streams the streams are static and some > > can be enabled and disabled, whereas in devices that just transport them > > or write them to memory, more configurability is allowed. Document this. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Julien Massot <julien.massot@collabora.com> > > --- > > .../userspace-api/media/v4l/dev-subdev.rst | 24 +++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > index d30dcb9e2537..de8dfd4f11a5 100644 > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > @@ -593,6 +593,30 @@ Any configurations of a stream within a pad, such as format or selections, > > are independent of similar configurations on other streams. This is > > subject to change in the future. > > > > +Device types and routing setup > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +Different kinds of sub-devices have differing behaviour for route activation, > > +depending on the hardware. In all cases, however, only routes that have the > > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag set are active. > > + > > +Devices generating the streams may allow enabling and disabling some of the > > +routes or the configuration is fixed. If the routes can be disabled, not > > "... some of the routes, or have a fixed routing configuration." Seems fine. > > > +declaring the routes (or declaring them without > > +``VIDIOC_SUBDEV_STREAM_FL_ACTIVE`` flag set) in ``VIDIOC_SUBDEV_S_ROUTING`` will > > +disable the routes while the sub-device driver retains the streams and their > > +format and selection configuration. > > I still find the "retains their format and selection configuration" > quite unclear :-S Alternatively we could say that the routes are simply not active, without referring to explicitly to formats and selections. I.e.: If the routes can be disabled, not declaring the routes (or declaring them without ``VIDIOC_SUBDEV_STREAM_FL_ACTIVE`` flag set) in ``VIDIOC_SUBDEV_S_ROUTING`` will disable the routes. > > > The ``VIDIOC_SUBDEV_S_ROUTING`` will still > > s/will still/ioctl will still/ Both seem to exist, more common is without "ioctl": $ git grep '[`<]VIDIOC_SUBDEV' -- Documentation/userspace-api/media/|wc -l 84 $ git grep -i "VIDIOC_SUBDEV.*ioctl" -- Documentation/userspace-api/media/|wc -l 34 > > > +return such routes back to the user in the routes array, with the > > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag unset. > > + > > +Devices transporting the streams almost always have more configurability with > > +respect to routing. Typically any route between the sub-device's sink and source > > +pads is possible, and multiple routes (usually up to certain limited number) may > > +be active simultaneously. For such devices, no routes are created by the driver > > +and user-created routes are fully replaced when ``VIDIOC_SUBDEV_S_ROUTING`` is > > +called on the sub-device. Such newly created routes have the device's default > > +configuration for format and selection rectangles. > > + > > Configuring streams > > ^^^^^^^^^^^^^^^^^^^ > > >
Hi Laurent, On Sat, Apr 20, 2024 at 01:45:19AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tue, Apr 16, 2024 at 10:32:48PM +0300, Sakari Ailus wrote: > > The len_routes field is used to tell the size of the routes array in > > struct v4l2_subdev_routing. This way the number of routes returned from > > S_ROUTING IOCTL may be larger than the number of routes provided, in case > > there are more routes returned by the driver. > > > > Note that this uAPI is still disabled in the code, so this change can > > safely be done. Anyone who manually patched the code to enable this uAPI > > must update their code. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../media/v4l/vidioc-subdev-g-routing.rst | 50 +++++++++++++------ > > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++--- > > include/media/v4l2-subdev.h | 2 + > > include/uapi/linux/v4l2-subdev.h | 9 ++-- > > 5 files changed, 52 insertions(+), 25 deletions(-) > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > index 26b5004bfe6d..27eb4c82a0e1 100644 > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > @@ -43,23 +43,42 @@ The routing configuration determines the flows of data inside an entity. > > Drivers report their current routing tables using the > > ``VIDIOC_SUBDEV_G_ROUTING`` ioctl and application may enable or disable routes > > with the ``VIDIOC_SUBDEV_S_ROUTING`` ioctl, by adding or removing routes and > > -setting or clearing flags of the ``flags`` field of a > > -struct :c:type:`v4l2_subdev_route`. > > +setting or clearing flags of the ``flags`` field of a struct > > +:c:type:`v4l2_subdev_route`. > > > > -All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This > > -means that the userspace must reconfigure all streams after calling the ioctl > > -with e.g. ``VIDIOC_SUBDEV_S_FMT``. > > +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. > > +This means that the userspace must reconfigure all stream formats and selections > > +after calling the ioctl with e.g. ``VIDIOC_SUBDEV_S_FMT``. > > > > Only subdevices which have both sink and source pads can support routing. > > > > -When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application > > -provided ``num_routes`` is not big enough to contain all the available routes > > -the subdevice exposes, drivers return the ENOSPC error code and adjust the > > -value of the ``num_routes`` field. Application should then reserve enough memory > > -for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again. > > - > > -On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the > > -``num_routes`` field to reflect the actual number of routes returned. > > +The ``len_routes`` field indicates the number of routes that can fit in the > > +``routes`` array allocated by userspace. It is set by applications for both > > +ioctls to indicate how many routes the kernel can return, and is never modified > > +by the kernel. > > + > > +The ``num_routes`` field, when returned from the kernel on both IOCTLs, > > +indicates the number of routes in the subdevice routing table and when calling > > +``VIDIOC_SUBDEV_S_ROUTING``, it is set by userspace to the number of routes that > > +the application stored in the ``routes`` array. The value returned by the kernel > > +may be smaller or larger than the value of ``num_routes`` set by the application > > +for ``VIDIOC_SUBDEV_S_ROUTING``, as drivers may adjust the requested routing > > +table. > > I still think the proposal I made when reviewing the previous version is > clearer :-) > > ---- > The ``num_routes`` field indicates the number of routes in the subdevice routing > table. For ``VIDIOC_SUBDEV_S_ROUTING``, it is set by userspace to the number of > routes that the application stored in the ``routes`` array. For both ioctls, it > is returned by the kernel and indicates how many routes are stored in the > subdevice routing table. This may be smaller or larger than the value of > ``num_routes`` set by the application for ``VIDIOC_SUBDEV_S_ROUTING``, as > drivers may adjust the requested routing table. > ---- > > You replied that > > > For S_ROUTING this is the number of routes in the IOCTL argument. The > > routing table may contain more (static routes). > > and that's right, but, even when set by userspace for S_ROUTING, the > num_routes fields is the number of routes that userspace tries to set in > the routing table. I think starting with a first sentence that describes > what the field contains, and then explaining how it's used for the > different ioctls by userspace and kernel space, is clearer. The problem with your suggestion is that it's not entirely correct: num_routes is indeed used for two different purposes. Removing " in the subdevice routing table" in the first sentence would be a simple fix. > > > + > > +The kernel can return a ``num_routes`` value larger than ``len_routes`` from > > +both ioctls. This indicates thare are more routes in the routing table than fits > > +the ``routes`` array. In this case, the ``routes`` array is filled by the kernel > > +with the first ``len_routes`` entries of the subdevice routing table. This is > > +not considered to be an error, and the ioctl call succeeds. If the applications > > +wants to retrieve the missing routes, it can issue a new > > +``VIDIOC_SUBDEV_G_ROUTING`` call with a large enough ``routes`` array. > > + > > +indicate there are more routes than fits to the ``routes`` array. In this > > +case first ``len_routes`` were returned back to the userspace in the > > +``routes`` array. This is not considered as an error. > > I think these 3 lines are a leftover. Yes, I'll remove them. > > > + > > +Also ``VIDIOC_SUBDEV_S_ROUTING`` may return more route than the user provided in > > s/Also // > s/route/routes/ Yes. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks! > > > +``num_routes`` field due to e.g. hardware properties. > > > > .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}| > > > > @@ -74,6 +93,9 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the > > - ``which`` > > - Routing table to be accessed, from enum > > :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > > + * - __u32 > > + - ``len_routes`` > > + - The length of the array (as in memory reserved for the array) > > * - struct :c:type:`v4l2_subdev_route` > > - ``routes[]`` > > - Array of struct :c:type:`v4l2_subdev_route` entries > > @@ -81,7 +103,7 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the > > - ``num_routes`` > > - Number of entries of the routes array > > * - __u32 > > - - ``reserved``\ [5] > > + - ``reserved``\ [11] > > - Reserved for future extensions. Applications and drivers must set > > the array to zero. > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > index 1863ecae9ec9..f30f61c008c7 100644 > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > @@ -3185,13 +3185,13 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > case VIDIOC_SUBDEV_S_ROUTING: { > > struct v4l2_subdev_routing *routing = parg; > > > > - if (routing->num_routes > 256) > > + if (routing->len_routes > 256) > > return -E2BIG; > > > > *user_ptr = u64_to_user_ptr(routing->routes); > > *kernel_ptr = (void **)&routing->routes; > > *array_size = sizeof(struct v4l2_subdev_route) > > - * routing->num_routes; > > + * routing->len_routes; > > ret = 1; > > break; > > } > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index 2ba11e5343f0..904378007a79 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -927,6 +927,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) > > return -EPERM; > > > > + if (routing->num_routes > routing->len_routes) > > + return -EINVAL; > > + > > memset(routing->reserved, 0, sizeof(routing->reserved)); > > > > for (i = 0; i < routing->num_routes; ++i) { > > @@ -953,6 +956,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > } > > > > krouting.num_routes = routing->num_routes; > > + krouting.len_routes = routing->len_routes; > > krouting.routes = routes; > > > > return v4l2_subdev_call(sd, pad, set_routing, state, > > @@ -973,14 +977,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > > > krouting = &state->routing; > > > > - if (routing->num_routes < krouting->num_routes) { > > - routing->num_routes = krouting->num_routes; > > - return -ENOSPC; > > - } > > - > > memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, > > krouting->routes, > > - krouting->num_routes * sizeof(*krouting->routes)); > > + min(krouting->num_routes, routing->len_routes) * > > + sizeof(*krouting->routes)); > > routing->num_routes = krouting->num_routes; > > > > return 0; > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 9cce48365975..1df6b963a1c9 100644 > > --- a/include/media/v4l2-subdev.h > > +++ b/include/media/v4l2-subdev.h > > @@ -728,12 +728,14 @@ struct v4l2_subdev_stream_configs { > > /** > > * struct v4l2_subdev_krouting - subdev routing table > > * > > + * @len_routes: length of routes array, in routes > > * @num_routes: number of routes > > * @routes: &struct v4l2_subdev_route > > * > > * This structure contains the routing table for a subdev. > > */ > > struct v4l2_subdev_krouting { > > + unsigned int len_routes; > > unsigned int num_routes; > > struct v4l2_subdev_route *routes; > > }; > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > index 81a24bd38003..6a39128d0606 100644 > > --- a/include/uapi/linux/v4l2-subdev.h > > +++ b/include/uapi/linux/v4l2-subdev.h > > @@ -228,15 +228,18 @@ struct v4l2_subdev_route { > > * struct v4l2_subdev_routing - Subdev routing information > > * > > * @which: configuration type (from enum v4l2_subdev_format_whence) > > - * @num_routes: the total number of routes in the routes array > > + * @len_routes: the length of the routes array, in routes > > * @routes: pointer to the routes array > > + * @num_routes: the total number of routes, possibly more than fits in the > > + * routes array > > * @reserved: drivers and applications must zero this array > > */ > > struct v4l2_subdev_routing { > > __u32 which; > > - __u32 num_routes; > > + __u32 len_routes; > > __u64 routes; > > - __u32 reserved[6]; > > + __u32 num_routes; > > + __u32 reserved[11]; > > }; > > > > /* >
Hi Sakari, On Tue, Apr 23, 2024 at 10:45:54AM +0000, Sakari Ailus wrote: > On Sat, Apr 20, 2024 at 01:45:19AM +0300, Laurent Pinchart wrote: > > On Tue, Apr 16, 2024 at 10:32:48PM +0300, Sakari Ailus wrote: > > > The len_routes field is used to tell the size of the routes array in > > > struct v4l2_subdev_routing. This way the number of routes returned from > > > S_ROUTING IOCTL may be larger than the number of routes provided, in case > > > there are more routes returned by the driver. > > > > > > Note that this uAPI is still disabled in the code, so this change can > > > safely be done. Anyone who manually patched the code to enable this uAPI > > > must update their code. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > .../media/v4l/vidioc-subdev-g-routing.rst | 50 +++++++++++++------ > > > drivers/media/v4l2-core/v4l2-ioctl.c | 4 +- > > > drivers/media/v4l2-core/v4l2-subdev.c | 12 ++--- > > > include/media/v4l2-subdev.h | 2 + > > > include/uapi/linux/v4l2-subdev.h | 9 ++-- > > > 5 files changed, 52 insertions(+), 25 deletions(-) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > > index 26b5004bfe6d..27eb4c82a0e1 100644 > > > --- a/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > > +++ b/Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst > > > @@ -43,23 +43,42 @@ The routing configuration determines the flows of data inside an entity. > > > Drivers report their current routing tables using the > > > ``VIDIOC_SUBDEV_G_ROUTING`` ioctl and application may enable or disable routes > > > with the ``VIDIOC_SUBDEV_S_ROUTING`` ioctl, by adding or removing routes and > > > -setting or clearing flags of the ``flags`` field of a > > > -struct :c:type:`v4l2_subdev_route`. > > > +setting or clearing flags of the ``flags`` field of a struct > > > +:c:type:`v4l2_subdev_route`. > > > > > > -All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. This > > > -means that the userspace must reconfigure all streams after calling the ioctl > > > -with e.g. ``VIDIOC_SUBDEV_S_FMT``. > > > +All stream configurations are reset when ``VIDIOC_SUBDEV_S_ROUTING`` is called. > > > +This means that the userspace must reconfigure all stream formats and selections > > > +after calling the ioctl with e.g. ``VIDIOC_SUBDEV_S_FMT``. > > > > > > Only subdevices which have both sink and source pads can support routing. > > > > > > -When inspecting routes through ``VIDIOC_SUBDEV_G_ROUTING`` and the application > > > -provided ``num_routes`` is not big enough to contain all the available routes > > > -the subdevice exposes, drivers return the ENOSPC error code and adjust the > > > -value of the ``num_routes`` field. Application should then reserve enough memory > > > -for all the route entries and call ``VIDIOC_SUBDEV_G_ROUTING`` again. > > > - > > > -On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the > > > -``num_routes`` field to reflect the actual number of routes returned. > > > +The ``len_routes`` field indicates the number of routes that can fit in the > > > +``routes`` array allocated by userspace. It is set by applications for both > > > +ioctls to indicate how many routes the kernel can return, and is never modified > > > +by the kernel. > > > + > > > +The ``num_routes`` field, when returned from the kernel on both IOCTLs, > > > +indicates the number of routes in the subdevice routing table and when calling > > > +``VIDIOC_SUBDEV_S_ROUTING``, it is set by userspace to the number of routes that > > > +the application stored in the ``routes`` array. The value returned by the kernel > > > +may be smaller or larger than the value of ``num_routes`` set by the application > > > +for ``VIDIOC_SUBDEV_S_ROUTING``, as drivers may adjust the requested routing > > > +table. > > > > I still think the proposal I made when reviewing the previous version is > > clearer :-) > > > > ---- > > The ``num_routes`` field indicates the number of routes in the subdevice routing > > table. For ``VIDIOC_SUBDEV_S_ROUTING``, it is set by userspace to the number of > > routes that the application stored in the ``routes`` array. For both ioctls, it > > is returned by the kernel and indicates how many routes are stored in the > > subdevice routing table. This may be smaller or larger than the value of > > ``num_routes`` set by the application for ``VIDIOC_SUBDEV_S_ROUTING``, as > > drivers may adjust the requested routing table. > > ---- > > > > You replied that > > > > > For S_ROUTING this is the number of routes in the IOCTL argument. The > > > routing table may contain more (static routes). > > > > and that's right, but, even when set by userspace for S_ROUTING, the > > num_routes fields is the number of routes that userspace tries to set in > > the routing table. I think starting with a first sentence that describes > > what the field contains, and then explaining how it's used for the > > different ioctls by userspace and kernel space, is clearer. > > The problem with your suggestion is that it's not entirely correct: > num_routes is indeed used for two different purposes. Removing " in the > subdevice routing table" in the first sentence would be a simple fix. How about dropping just "subdevice", and keeping "in the routing table" ? That should cover both cases. > > > + > > > +The kernel can return a ``num_routes`` value larger than ``len_routes`` from > > > +both ioctls. This indicates thare are more routes in the routing table than fits > > > +the ``routes`` array. In this case, the ``routes`` array is filled by the kernel > > > +with the first ``len_routes`` entries of the subdevice routing table. This is > > > +not considered to be an error, and the ioctl call succeeds. If the applications > > > +wants to retrieve the missing routes, it can issue a new > > > +``VIDIOC_SUBDEV_G_ROUTING`` call with a large enough ``routes`` array. > > > + > > > +indicate there are more routes than fits to the ``routes`` array. In this > > > +case first ``len_routes`` were returned back to the userspace in the > > > +``routes`` array. This is not considered as an error. > > > > I think these 3 lines are a leftover. > > Yes, I'll remove them. > > > > + > > > +Also ``VIDIOC_SUBDEV_S_ROUTING`` may return more route than the user provided in > > > > s/Also // > > s/route/routes/ > > Yes. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks! > > > > > > +``num_routes`` field due to e.g. hardware properties. > > > > > > .. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}| > > > > > > @@ -74,6 +93,9 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the > > > - ``which`` > > > - Routing table to be accessed, from enum > > > :ref:`v4l2_subdev_format_whence <v4l2-subdev-format-whence>`. > > > + * - __u32 > > > + - ``len_routes`` > > > + - The length of the array (as in memory reserved for the array) > > > * - struct :c:type:`v4l2_subdev_route` > > > - ``routes[]`` > > > - Array of struct :c:type:`v4l2_subdev_route` entries > > > @@ -81,7 +103,7 @@ On a successful ``VIDIOC_SUBDEV_G_ROUTING`` call the driver updates the > > > - ``num_routes`` > > > - Number of entries of the routes array > > > * - __u32 > > > - - ``reserved``\ [5] > > > + - ``reserved``\ [11] > > > - Reserved for future extensions. Applications and drivers must set > > > the array to zero. > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c > > > index 1863ecae9ec9..f30f61c008c7 100644 > > > --- a/drivers/media/v4l2-core/v4l2-ioctl.c > > > +++ b/drivers/media/v4l2-core/v4l2-ioctl.c > > > @@ -3185,13 +3185,13 @@ static int check_array_args(unsigned int cmd, void *parg, size_t *array_size, > > > case VIDIOC_SUBDEV_S_ROUTING: { > > > struct v4l2_subdev_routing *routing = parg; > > > > > > - if (routing->num_routes > 256) > > > + if (routing->len_routes > 256) > > > return -E2BIG; > > > > > > *user_ptr = u64_to_user_ptr(routing->routes); > > > *kernel_ptr = (void **)&routing->routes; > > > *array_size = sizeof(struct v4l2_subdev_route) > > > - * routing->num_routes; > > > + * routing->len_routes; > > > ret = 1; > > > break; > > > } > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index 2ba11e5343f0..904378007a79 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -927,6 +927,9 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > > if (routing->which != V4L2_SUBDEV_FORMAT_TRY && ro_subdev) > > > return -EPERM; > > > > > > + if (routing->num_routes > routing->len_routes) > > > + return -EINVAL; > > > + > > > memset(routing->reserved, 0, sizeof(routing->reserved)); > > > > > > for (i = 0; i < routing->num_routes; ++i) { > > > @@ -953,6 +956,7 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > > } > > > > > > krouting.num_routes = routing->num_routes; > > > + krouting.len_routes = routing->len_routes; > > > krouting.routes = routes; > > > > > > return v4l2_subdev_call(sd, pad, set_routing, state, > > > @@ -973,14 +977,10 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg, > > > > > > krouting = &state->routing; > > > > > > - if (routing->num_routes < krouting->num_routes) { > > > - routing->num_routes = krouting->num_routes; > > > - return -ENOSPC; > > > - } > > > - > > > memcpy((struct v4l2_subdev_route *)(uintptr_t)routing->routes, > > > krouting->routes, > > > - krouting->num_routes * sizeof(*krouting->routes)); > > > + min(krouting->num_routes, routing->len_routes) * > > > + sizeof(*krouting->routes)); > > > routing->num_routes = krouting->num_routes; > > > > > > return 0; > > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > > index 9cce48365975..1df6b963a1c9 100644 > > > --- a/include/media/v4l2-subdev.h > > > +++ b/include/media/v4l2-subdev.h > > > @@ -728,12 +728,14 @@ struct v4l2_subdev_stream_configs { > > > /** > > > * struct v4l2_subdev_krouting - subdev routing table > > > * > > > + * @len_routes: length of routes array, in routes > > > * @num_routes: number of routes > > > * @routes: &struct v4l2_subdev_route > > > * > > > * This structure contains the routing table for a subdev. > > > */ > > > struct v4l2_subdev_krouting { > > > + unsigned int len_routes; > > > unsigned int num_routes; > > > struct v4l2_subdev_route *routes; > > > }; > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > index 81a24bd38003..6a39128d0606 100644 > > > --- a/include/uapi/linux/v4l2-subdev.h > > > +++ b/include/uapi/linux/v4l2-subdev.h > > > @@ -228,15 +228,18 @@ struct v4l2_subdev_route { > > > * struct v4l2_subdev_routing - Subdev routing information > > > * > > > * @which: configuration type (from enum v4l2_subdev_format_whence) > > > - * @num_routes: the total number of routes in the routes array > > > + * @len_routes: the length of the routes array, in routes > > > * @routes: pointer to the routes array > > > + * @num_routes: the total number of routes, possibly more than fits in the > > > + * routes array > > > * @reserved: drivers and applications must zero this array > > > */ > > > struct v4l2_subdev_routing { > > > __u32 which; > > > - __u32 num_routes; > > > + __u32 len_routes; > > > __u64 routes; > > > - __u32 reserved[6]; > > > + __u32 num_routes; > > > + __u32 reserved[11]; > > > }; > > > > > > /*
Hi Sakari, On Tue, Apr 23, 2024 at 10:08:05AM +0000, Sakari Ailus wrote: > On Fri, Apr 19, 2024 at 08:17:20PM +0300, Laurent Pinchart wrote: > > On Tue, Apr 16, 2024 at 10:32:44PM +0300, Sakari Ailus wrote: > > > Document S_ROUTING behaviour for different devices. > > > > > > Generally in devices that produce streams the streams are static and some > > > can be enabled and disabled, whereas in devices that just transport them > > > or write them to memory, more configurability is allowed. Document this. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Reviewed-by: Julien Massot <julien.massot@collabora.com> > > > --- > > > .../userspace-api/media/v4l/dev-subdev.rst | 24 +++++++++++++++++++ > > > 1 file changed, 24 insertions(+) > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > > index d30dcb9e2537..de8dfd4f11a5 100644 > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > > @@ -593,6 +593,30 @@ Any configurations of a stream within a pad, such as format or selections, > > > are independent of similar configurations on other streams. This is > > > subject to change in the future. > > > > > > +Device types and routing setup > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > + > > > +Different kinds of sub-devices have differing behaviour for route activation, > > > +depending on the hardware. In all cases, however, only routes that have the > > > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag set are active. > > > + > > > +Devices generating the streams may allow enabling and disabling some of the > > > +routes or the configuration is fixed. If the routes can be disabled, not > > > > "... some of the routes, or have a fixed routing configuration." > > Seems fine. > > > > +declaring the routes (or declaring them without > > > +``VIDIOC_SUBDEV_STREAM_FL_ACTIVE`` flag set) in ``VIDIOC_SUBDEV_S_ROUTING`` will > > > +disable the routes while the sub-device driver retains the streams and their > > > +format and selection configuration. > > > > I still find the "retains their format and selection configuration" > > quite unclear :-S > > Alternatively we could say that the routes are simply not active, without > referring to explicitly to formats and selections. I.e.: > > If the routes can be disabled, not declaring the routes (or declaring them > without ``VIDIOC_SUBDEV_STREAM_FL_ACTIVE`` flag set) in > ``VIDIOC_SUBDEV_S_ROUTING`` will disable the routes. I'm fine with that. > > > The ``VIDIOC_SUBDEV_S_ROUTING`` will still > > > > s/will still/ioctl will still/ > > Both seem to exist, more common is without "ioctl": > > $ git grep '[`<]VIDIOC_SUBDEV' -- Documentation/userspace-api/media/|wc -l > 84 > $ git grep -i "VIDIOC_SUBDEV.*ioctl" -- Documentation/userspace-api/media/|wc -l > 34 You'll often find "ioctl" at the beginning of the next line :-) If you would like to avoid it, you should drop "The" at the beginning of the sentence. > > > +return such routes back to the user in the routes array, with the > > > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag unset. > > > + > > > +Devices transporting the streams almost always have more configurability with > > > +respect to routing. Typically any route between the sub-device's sink and source > > > +pads is possible, and multiple routes (usually up to certain limited number) may > > > +be active simultaneously. For such devices, no routes are created by the driver > > > +and user-created routes are fully replaced when ``VIDIOC_SUBDEV_S_ROUTING`` is > > > +called on the sub-device. Such newly created routes have the device's default > > > +configuration for format and selection rectangles. > > > + > > > Configuring streams > > > ^^^^^^^^^^^^^^^^^^^ > > >
Hi Laurent, On Tue, Apr 23, 2024 at 03:59:44PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Apr 23, 2024 at 10:08:05AM +0000, Sakari Ailus wrote: > > On Fri, Apr 19, 2024 at 08:17:20PM +0300, Laurent Pinchart wrote: > > > On Tue, Apr 16, 2024 at 10:32:44PM +0300, Sakari Ailus wrote: > > > > Document S_ROUTING behaviour for different devices. > > > > > > > > Generally in devices that produce streams the streams are static and some > > > > can be enabled and disabled, whereas in devices that just transport them > > > > or write them to memory, more configurability is allowed. Document this. > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > Reviewed-by: Julien Massot <julien.massot@collabora.com> > > > > --- > > > > .../userspace-api/media/v4l/dev-subdev.rst | 24 +++++++++++++++++++ > > > > 1 file changed, 24 insertions(+) > > > > > > > > diff --git a/Documentation/userspace-api/media/v4l/dev-subdev.rst b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > > > index d30dcb9e2537..de8dfd4f11a5 100644 > > > > --- a/Documentation/userspace-api/media/v4l/dev-subdev.rst > > > > +++ b/Documentation/userspace-api/media/v4l/dev-subdev.rst > > > > @@ -593,6 +593,30 @@ Any configurations of a stream within a pad, such as format or selections, > > > > are independent of similar configurations on other streams. This is > > > > subject to change in the future. > > > > > > > > +Device types and routing setup > > > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > + > > > > +Different kinds of sub-devices have differing behaviour for route activation, > > > > +depending on the hardware. In all cases, however, only routes that have the > > > > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag set are active. > > > > + > > > > +Devices generating the streams may allow enabling and disabling some of the > > > > +routes or the configuration is fixed. If the routes can be disabled, not > > > > > > "... some of the routes, or have a fixed routing configuration." > > > > Seems fine. > > > > > > +declaring the routes (or declaring them without > > > > +``VIDIOC_SUBDEV_STREAM_FL_ACTIVE`` flag set) in ``VIDIOC_SUBDEV_S_ROUTING`` will > > > > +disable the routes while the sub-device driver retains the streams and their > > > > +format and selection configuration. > > > > > > I still find the "retains their format and selection configuration" > > > quite unclear :-S > > > > Alternatively we could say that the routes are simply not active, without > > referring to explicitly to formats and selections. I.e.: > > > > If the routes can be disabled, not declaring the routes (or declaring them > > without ``VIDIOC_SUBDEV_STREAM_FL_ACTIVE`` flag set) in > > ``VIDIOC_SUBDEV_S_ROUTING`` will disable the routes. > > I'm fine with that. > > > > > The ``VIDIOC_SUBDEV_S_ROUTING`` will still > > > > > > s/will still/ioctl will still/ > > > > Both seem to exist, more common is without "ioctl": > > > > $ git grep '[`<]VIDIOC_SUBDEV' -- Documentation/userspace-api/media/|wc -l > > 84 > > $ git grep -i "VIDIOC_SUBDEV.*ioctl" -- Documentation/userspace-api/media/|wc -l > > 34 > > You'll often find "ioctl" at the beginning of the next line :-) If you > would like to avoid it, you should drop "The" at the beginning of the > sentence. Sounds good. > > > > > +return such routes back to the user in the routes array, with the > > > > +``V4L2_SUBDEV_STREAM_FL_ACTIVE`` flag unset. > > > > + > > > > +Devices transporting the streams almost always have more configurability with > > > > +respect to routing. Typically any route between the sub-device's sink and source > > > > +pads is possible, and multiple routes (usually up to certain limited number) may > > > > +be active simultaneously. For such devices, no routes are created by the driver > > > > +and user-created routes are fully replaced when ``VIDIOC_SUBDEV_S_ROUTING`` is > > > > +called on the sub-device. Such newly created routes have the device's default > > > > +configuration for format and selection rectangles. > > > > + > > > > Configuring streams > > > > ^^^^^^^^^^^^^^^^^^^ > > > > >
Hi Laurent, On Tue, Apr 23, 2024 at 03:54:50PM +0300, Laurent Pinchart wrote: > How about dropping just "subdevice", and keeping "in the routing table" > ? That should cover both cases. Sounds good!
Hi Laurent, On Sat, Apr 20, 2024 at 12:42:48PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Tue, Apr 16, 2024 at 10:33:10PM +0300, Sakari Ailus wrote: > > The driver dug the supported link frequency up from the V4L2 fwnode > > s/dug/digs/ > > > endpoint and used it internally, but failed to report this in the > > s/used/uses/ > s/failed/fails/ > > > LINK_FREQ and PIXEL_RATE controls. Fix this. > > > > Fixes: 0677a2d9b735 ("media: ov2740: Add support for 180 MHz link frequency") > > Cc: stable@vger.kernel.org # for v6.8 and later > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > You're missing the tags given by Hans and Bingbu. As this patch is > unrelated to the rest of the series, it should be split off and merged > in v6.10. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks. I think I forgot the patch to this branch as well. It's been merged already so all is well.
Hi folks, Here are a few patches to add support generic, line based metadata as well as internal pads. While the amount of code is not very large, to the contrary it is quite small actually IMO, I presume what this is about and why it is being proposed requires some explaining. Metadata mbus codes and formats have existed for some time in V4L2. They however have been only used by drivers that produce the data itself and effectively this metadata has always been statistics of some sort (at least when it comes to ISPs). What is different here is that we intend to add support for metadata originating from camera sensors. Camera sensors produce different kinds of metadata, embedded data (usually register address--value pairs used to capture the frame, in a more or less sensor specific format), histograms (in a very sensor specific format), dark pixels etc. The number of these formats is probably going to be about as large as image data formats if not larger, as the image data formats are much better standardised but a smaller subset of them will be supported by V4L2, at least initially but possibly much more in the long run. Having this many device specific formats would be a major problem for all the other drivers along that pipeline (not to mention the users of those drivers), including bridge (e.g. CSI-2 to parallel) but especially CSI-2 receiver drivers that have DMA: the poor driver developer would not only need to know camera sensor specific formats but to choose the specific packing of that format suitable for the DMA used by the hardware. It is unlikely many of these would ever get tested while being present on the driver API. Also adding new sensors with new embedded data formats would involve updating all bridge and CSI-2 receiver drivers. I don't expect this to be a workable approach. Instead what I'm proposing is to use specific metadata formats on the sensor devices only, on internal pads (more about those soon) of the sensors, only visible in the UAPI, and then generic mbus formats along the pipeline and finally generic V4L2 metadata formats on the DMAs (specific to bit depth and packing). This would unsnarl the two, defining what data there is (specific mbus code) and how that is transported and packed (generic mbus codes and V4L2 formats). The user space would be required to "know" the path of that data from the sensor's internal pad to the V4L2 video node. I do not see this as these devices require at least some knowledge of the pipeline, i.e. hardware at hand. Separating what the data means and how it is packed may even be beneficial: it allows separating code that interprets the data (sensor internal mbus code) from the code that accesses it (packing). These formats are in practice line based, meaning that there may be padding at the end of the line, depending on the bus as well as the DMA. If non-line based formats are needed, it is always possible to set the "height" field to 1. The internal sink pads are an alternative to source routes [1]. The source routes were not universally liked and I do have to say I like re-using existing interface concepts (pads and everything you can do with pads, including access format, selections etc.) wherever it makes sense, instead of duplicating functionality. Effectively internal sink pads behave mostly just like sink pads, but they describe a flow of data that originates from a sub-device instead of arriving to a sub-device. The SUBDEV_S_ROUTING IOCTLs are used to enable and disable routes from internal sink pads to sub-device's source pads. The subdev format IOCTLs are usable, too, so one can find which subdev format is available on given internal sink pad. I've also pushed these here and I'll keep updating the branch, I've also included untested OV2740 patches: <URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=metadata> Questions and comments are most welcome. Preliminary media-ctl and yavta patches can be found here: <URL:https://git.retiisi.eu/?p=~sailus/yavta.git;a=shortlog;h=refs/heads/metadata> <URL:https://git.retiisi.eu/?p=~sailus/v4l-utils.git;a=shortlog;h=refs/heads/metadata> I have used IMX219 as an example on routing in a sensor driver in this version. The patches are on my master branch <URL:https://git.linuxtv.org/sailus/media_tree.git/log/>. [1] <URL:https://lore.kernel.org/linux-media/20220831141357.1396081-20-tomi.valkeinen@ideasonboard.com/> since v8: - Move the patch adding internal pad flag past the routing API reworks, as well as a few other patches, in order to separate the patches to those that could still be merged for v6.10 (routing changes) and those that couldn't (sensor API related). The patch on the edge is "media: uapi: v4l: subdev: Enable streams API". - Include Laurent's two patches to address crop API issues wrt. streams. - Add two patches to prepare for CCS driver rework (media: ccs: Move ccs_pm_get_init function up and media: ccs: Rename out label of ccs_start_streaming). - Address issues in the ov2740 driver patches (as well as the driver itself), 4 more patches towards the end of the set. - Improved generic metadata format names, align with other existing formats. - Improved ov2740 embedded data documentation. - Reworked streams and camera sensor documentation based on Laurent's comments mainly. In particular, the contradictory concept of internal source pads no longer should exist in the patches. - Fixed pad numbering in the CCS example. - Fixed S_ROUTING behaviour when len_routes is too small and when S_ROUTING isn't implemented by the driver. - Reorder sections in meta-formats.rst alphabetically. - Add a note per struct fields that certain struct v4l2_subdev_format are zero for metadata mbus codes. - CCS driver patch cleanups. - CCS driver metadata width fix for space-efficient embedded data at 16 bpp and over. - Postpone CCS frame descriptor quirk for now. - Use MIPI_CSI2_DT_USER_DEFINED(0) instead of a numerical value for compressed data datatype. since v7: - Add embedded data support for the ov2740 driver. - Add three patches on top, to add an IMMUTABLE flag to source streams when they cannot be disabled. - Improved documentation of len_routes and num_routes arguments of [GS]_ROUTING. - Remove one inclusion of twice-included media/v4l2-fwnode.h in drivers/media/i2c/ccs/ccs-core.c . - Add missing forward declaration of ccs_internal_ops in drivers/media/i2c/ccs/ccs-core.c . since v6: - Improve embedded data UAPI documentation on camera sensors. - Improve wording of stream glossary entry. - Improve internal pad flag documentation. - Fix definition of "data unit" and remove an extra "only" in INTERNAL pad flag description (1st patch). - Use IMX219 driver in examples consistently. - Remove the CSI-2 to parallel bridge from the example to simplify the example. - Minor rewording of some parts of the routing examples. - Rebase on unified sub-device state information access functions: <URL:https://lore.kernel.org/linux-media/20231027095913.1010187-1-sakari.ailus@linux.intel.com/T/#t> - In CCS driver, do not maintain current active configuration in driver's device context struct (apart from mbus codes). Rely on sub-device state locking and clean up the code. (Multiple patches towards the end of the set.) - Arrange the CCS patches early in the set towards the end of the set. - Move the patch enabling streams API to the end of the set. - Rework IOCTL argument copying condition for [GS]_ROUTING). - Handle copying back routes in S_ROUTING, do not rely on G_ROUTING IOCTL implementation. - Rebase on metadata preparation patchset v6: <URL:https://lore.kernel.org/linux-media/20231106121805.1266696-1-sakari.ailus@linux.intel.com/T/#t>. since v5: - Rebase on new set of preparation patches. - Switch CCS driver from s_stream to enable_streams/disable_streams. Keep streaming state information --- the sensor remains in streaming state if any of the streams is enabled. - Fix setting mbus code on embedded data in get_frame_desc() op in the CCS driver. since v4: - Add a patch to acquire two sub-device states that may use the same lock. - Add a patch for CCS driver to remove ccs_get_crop_compose() helper. - Add a patch for CCS driver moving acquiring and releasing the mutex to the s_stream callback. - Add a patch for CCS driver to rely on sub-device state locking using a single driver-provided lock. - Fixed calculating minimum number of routes in copying the routes (thanks, Laurent). - Moved a label in S_ROUTING handling to make Clang happy (hopefully). - Fixed setting emb_data_ctrl register for CCS embedded data support. - Rebase on Laurent's cleanup patches. - Wrap a few long lines. - Write in embedded data documentation sensor drivers generally don't allow configuring it. since v3: - Separate preparation patches from this set. - Add a definition for "Data unit", a pixel that is not image data and use it instead in format documentation. - Fix more numbered lists in dev-subdev.rst. - Remove a redundant definition for V4L2_META_FMT_GENERIC_CSI2_2_24 --- V4L2_META_FMT_GENERIC_CSI2_12 can be used instead. - Use "X" instead of "p" to denote padding in format documentation. - Use IMX219 in examples instead of CCS. - Document that the generic V4L2 CSI-2 metadata formats use padding defined in CSI-2 spec and packing defined in CCS spec. - Add patches to align [GS]_ROUTING behaviour with V4L2. This means mainly returning configured routes as part of S_ROUTING as well. "len_routes" field is added to denote the length of the array and having more routes than fits in the array is no longer an error. Also added more reserved fields. - Added trivial support for S_ROUTING (via G_ROUTING implementation) for use in drivers with static-only routes. - Added helper functions to obtain mbus format as well as crop and compose rectangles that are streams-independent. - Added a patch to define generic CSI-2 long packet types. - Removed MEDIA_BUS_FMT_IS_META() macro. It didn't seem useful in the end. - Use a single CCS embedded data format. The bit depth can be selected using the meta stream on the source pad. - Fix mbus code numbers (there were holes due to removed redundant formats). - Fix generic mbus code documentation (byte was being used instead of bit). - Fix spelling of "length". - Added a patch to remove v4l2_subdev_enable_streams_api that disables streams API. This should be merged once libcamera support for streams works nicely. - Don't use strings in printing frame descriptor flags. - Warn on string truncation in printing frame descriptor. since v2: - Add a better example, with formats. - Add CCS static data media bus codes. - Added an example demonstrating the use of internal pads. --- Is the level of detail enough for the purpose? - Improved documentation. - Added a macro to tell whether a format is a metadata format. (Documentation could be added.) - A small ReST syntax fix in the same section. - Drop leftovers of a patch checking for the INTERNAL_SOURCE flag. since v1: - Make the new pad flag just "INTERNAL", requiring either SINK or SOURCE pad flag to accompany it. Removed the union in struct v4l2_subdev_route. - Add the term "stream" to MC glossary. - Improved and fixed documentation (according to comments). - Note these formats are little endian. - Remove 1X8 from the names of the mbus codes. These formats have generally 8 bits per pixel. - Fix mbus code numbering (had holes in RFC). - Add new metadata fields to debug prints. - Fix a minor documentation build issue. Laurent Pinchart (2): media: v4l2-subdev: Fix stream handling for crop API media: v4l2-subdev: Clearly document that the crop API won't be extended Sakari Ailus (44): media: Documentation: Add "stream" into glossary media: uapi: Add generic serial metadata mbus formats media: uapi: Document which mbus format fields are valid for metadata media: uapi: v4l: Add generic 8-bit metadata format definitions media: v4l: Support line-based metadata capture media: Documentation: Additional streams generally don't harm capture media: Documentation: Document embedded data guidelines for camera sensors media: Documentation: v4l: Document internal sink pads media: Documentation: Document S_ROUTING behaviour media: v4l: subdev: Add a function to lock two sub-device states, use it media: v4l: subdev: Move G_ROUTING handling below S_ROUTING media: v4l: subdev: Copy argument back to user also for S_ROUTING media: v4l: subdev: Add len_routes field to struct v4l2_subdev_routing media: v4l: subdev: Return routes set using S_ROUTING media: v4l: subdev: Add trivial set_routing support media: ccs: No need to set streaming to false in power off media: ccs: Move ccs_pm_get_init function up media: ccs: Rename out label of ccs_start_streaming media: ccs: Use {enable,disable}_streams operations media: ccs: Track streaming state media: ccs: Move ccs_validate_csi_data_format up media: ccs: Support frame descriptors media: uapi: v4l: subdev: Enable streams API media: mc: Add INTERNAL pad flag media: uapi: ccs: Add media bus code for MIPI CCS embedded data media: Documentation: Document non-CCS use of CCS embedded data format media: Documentation: ccs: Document routing media: ccs: Add support for embedded data stream media: ccs: Remove ccs_get_crop_compose helper media: ccs: Rely on sub-device state locking media: ccs: Compute binning configuration from sub-device state media: ccs: Compute scaling configuration from sub-device state media: ccs: Remove which parameter from ccs_propagate media: uapi: Add media bus code for ov2740 embedded data media: ov2740: Fix LINK_FREQ and PIXEL_RATE control value reporting media: ov2740: Remove shorthand variables media: ov2740: Switch to {enable,disable}_streams media: ov2740: Track streaming state media: ov2740: Add support for embedded data media: ov2740: Add generic sensor fwnode properties as controls media: ov2740: Add support for G_SELECTION IOCTL media: v4l: Add V4L2_SUBDEV_ROUTE_FL_IMMUTABLE sub-device routing flag media: ccs: Add IMMUTABLE route flag media: ov2740: Add IMMUTABLE route flag .../media/drivers/camera-sensor.rst | 32 + .../userspace-api/media/drivers/ccs.rst | 38 +- .../userspace-api/media/glossary.rst | 15 + .../media/mediactl/media-types.rst | 9 + .../userspace-api/media/v4l/dev-meta.rst | 21 + .../userspace-api/media/v4l/dev-subdev.rst | 179 ++- .../userspace-api/media/v4l/meta-formats.rst | 3 +- .../media/v4l/metafmt-generic.rst | 328 +++++ .../media/v4l/subdev-formats.rst | 374 +++++- .../media/v4l/vidioc-enum-fmt.rst | 7 + .../media/v4l/vidioc-subdev-g-crop.rst | 6 +- .../media/v4l/vidioc-subdev-g-routing.rst | 60 +- .../media/videodev2.h.rst.exceptions | 1 + drivers/media/i2c/ccs/ccs-core.c | 1050 +++++++++++------ drivers/media/i2c/ccs/ccs.h | 27 +- drivers/media/i2c/ov2740.c | 304 +++-- drivers/media/mc/mc-entity.c | 15 +- drivers/media/v4l2-core/v4l2-ioctl.c | 25 +- drivers/media/v4l2-core/v4l2-subdev.c | 118 +- include/media/v4l2-subdev.h | 42 + include/uapi/linux/media-bus-format.h | 13 + include/uapi/linux/media.h | 1 + include/uapi/linux/v4l2-mediabus.h | 18 +- include/uapi/linux/v4l2-subdev.h | 18 +- include/uapi/linux/videodev2.h | 18 + 25 files changed, 2183 insertions(+), 539 deletions(-) create mode 100644 Documentation/userspace-api/media/v4l/metafmt-generic.rst