| Message ID | 20220121081810.155500-1-jeanmichel.hautbois@ideasonboard.com |
|---|---|
| Headers | show |
| Series | Add support for BCM2835 camera interface (unicam) | expand |
Hi Jean-Michel, Thank you for the patch. On Fri, Jan 21, 2022 at 09:18:08AM +0100, Jean-Michel Hautbois wrote: > Add both MIPI CSI-2 nodes in the core bcm2711 tree. Use the 3-cells > interrupt declaration, corresponding clocks and default as disabled. > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > arch/arm/boot/dts/bcm2711.dtsi | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi > index dff18fc9a906..077141df7024 100644 > --- a/arch/arm/boot/dts/bcm2711.dtsi > +++ b/arch/arm/boot/dts/bcm2711.dtsi > @@ -3,6 +3,7 @@ > > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/soc/bcm2835-pm.h> > +#include <dt-bindings/power/raspberrypi-power.h> > > / { > compatible = "brcm,bcm2711"; > @@ -293,6 +294,36 @@ hvs: hvs@7e400000 { > interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; > }; > > + csi0: csi1@7e800000 { The node name should be csi@7e800000, not csi1@7e800000. Now, this will probably cause issues with the firmware that looks for csi1 (and csi0 ?) to hand over control of the Unicam CSI-2 receiver to the kernel. I wonder if this is something that could be handled by a firmware update, to also recognize nodes named "csi" ? > + compatible = "brcm,bcm2835-unicam"; > + reg = <0x7e800000 0x800>, > + <0x7e802000 0x4>; > + interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clocks BCM2835_CLOCK_CAM0>, > + <&firmware_clocks 4>; > + clock-names = "lp", "vpu"; > + power-domains = <&power RPI_POWER_DOMAIN_UNICAM0>; > + #address-cells = <1>; > + #size-cells = <0>; > + #clock-cells = <1>; Why do you need #address-cells, #size-cells and #clock-cells ? They're not mentioned in the binding. > + status="disabled"; Missing spaces around the =. Same comment for the next node. > + }; > + > + csi1: csi1@7e801000 { > + compatible = "brcm,bcm2835-unicam"; > + reg = <0x7e801000 0x800>, > + <0x7e802004 0x4>; > + interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clocks BCM2835_CLOCK_CAM1>, > + <&firmware_clocks 4>; > + clock-names = "lp", "vpu"; > + power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>; > + #address-cells = <1>; > + #size-cells = <0>; > + #clock-cells = <1>; > + status="disabled"; > + }; > + > pixelvalve3: pixelvalve@7ec12000 { > compatible = "brcm,bcm2711-pixelvalve3"; > reg = <0x7ec12000 0x100>;
Hi Jean-Michel, Thank you for the patch. On Fri, Jan 21, 2022 at 09:18:06AM +0100, Jean-Michel Hautbois wrote: > Introduce the dt-bindinds documentation for bcm2835 CCP2/CSI2 camera s/bindinds/bindings/ I'd mention "Unicam" somewhere here. > interface. Also add a MAINTAINERS entry for it. > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > Dave: I assumed you were the maintainer for this file, as I based it on the > bcm2835-unicam.txt file. Are you happy to be added directly as the > maintainer, or should this be specified as "Raspberry Pi Kernel > Maintenance <kernel-list@raspberrypi.com>" > - in v2: multiple corrections to pass the bot checking as Rob kindly > told me. > --- > .../bindings/media/brcm,bcm2835-unicam.yaml | 103 ++++++++++++++++++ > MAINTAINERS | 6 + > 2 files changed, 109 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > > diff --git a/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > new file mode 100644 > index 000000000000..1427514142cf > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > @@ -0,0 +1,103 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/brcm,bcm2835-unicam.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Broadcom BCM283x Camera Interface (Unicam) > + > +maintainers: > + - Dave Stevenson <dave.stevenson@raspberrypi.com> > + > +description: |- > + The Unicam block on BCM283x SoCs is the receiver for either > + CSI-2 or CCP2 data from image sensors or similar devices. > + > + The main platform using this SoC is the Raspberry Pi family of boards. > + On the Pi the VideoCore firmware can also control this hardware block, > + and driving it from two different processors will cause issues. > + To avoid this, the firmware checks the device tree configuration > + during boot. If it finds device tree nodes called csi0 or csi1 then > + it will stop the firmware accessing the block, and it can then > + safely be used via the device tree binding. As mentioned in the review of the DT integration, the nodes should ideally be called just "csi", not "csi0" and "csi1" (maybe Rob could confirm this ?). Dave, is there a way the firmware could be updated to also hand over control of the Unicam instances to Linux when a "csi" node is found, not just "csi0" or "csi1" ? Given that the node names are significant, they should be enforced in the YAML schema. > + > +properties: > + compatible: > + const: brcm,bcm2835-unicam > + > + reg: > + description: > + physical base address and length of the register sets for the device. This can be dropped. > + maxItems: 1 There are two items in the example below. How does this validate ? > + > + interrupts: > + description: the IRQ line for this Unicam instance. This can be dropped. > + maxItems: 1 > + > + clocks: > + description: |- > + list of clock specifiers, corresponding to entries in clock-names > + property. clocks: items: - description: The clock for ... - description: The clock for ... (with the two descriptions matching the LP and VPU clocks, I don't know what they are). > + > + clock-names: > + items: > + - const: lp > + - const: vpu > + > + port: > + $ref: /schemas/graph.yaml#/properties/port > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - port > + > +additionalProperties: False > + > +examples: > + - | > + csi1: csi1@7e801000 { > + compatible = "brcm,bcm2835-unicam"; > + reg = <0x7e801000 0x800>, > + <0x7e802004 0x4>; > + interrupts = <2 7>; Let's use the Pi 4 device tree as an example, as that's what we're upstreaming first. > + clocks = <&clocks BCM2835_CLOCK_CAM1>, This will fail to compile without a proper #include, did you get this to pass validation ? > + <&firmware_clocks 4>; > + clock-names = "lp", "vpu"; > + port { > + csi1_ep: endpoint { > + remote-endpoint = <&tc358743_0>; > + data-lanes = <1 2>; > + }; > + }; > + }; > + > + i2c0: i2c@7e205000 { > + tc358743: csi-hdmi-bridge@0f { > + compatible = "toshiba,tc358743"; > + reg = <0x0f>; > + clocks = <&tc358743_clk>; > + clock-names = "refclk"; > + > + tc358743_clk: bridge-clk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <27000000>; > + }; > + > + port { > + tc358743_0: endpoint { > + remote-endpoint = <&csi1_ep>; > + clock-lanes = <0>; > + data-lanes = <1 2>; > + clock-noncontinuous; > + link-frequencies = > + /bits/ 64 <297000000>; > + }; > + }; > + }; > + }; I'd drop this node completely. > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index 33f75892f98e..07f238fd5ff9 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3679,6 +3679,12 @@ F: Documentation/media/v4l-drivers/bcm2835-isp.rst > F: drivers/staging/vc04_services/bcm2835-isp > F: include/uapi/linux/bcm2835-isp.h > > +BROADCOM BCM2835 CAMERA DRIVER > +M: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > + > BROADCOM BCM47XX MIPS ARCHITECTURE > M: Hauke Mehrtens <hauke@hauke-m.de> > M: Rafał Miłecki <zajec5@gmail.com>
Hi Jean-Michel, Thank you for the patch. On Fri, Jan 21, 2022 at 09:18:09AM +0100, Jean-Michel Hautbois wrote: > As of now, imx219 was not supporting anything more than one stream. Add > support for embedded data, based on linux-rpi kernel, and make it work > with multiplexed streams. We have only one source pad with two streams: > stream 0 is the image, and stream 1 is the embedded data. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > in v2: modified the get_format_pad function as it was not correctly > propagating the format in case of sensor flips. > --- > drivers/media/i2c/imx219.c | 452 ++++++++++++++++++++----------------- This is too big, it bundles multiple changes together. It should be split in multiple patches. > 1 file changed, 241 insertions(+), 211 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index e10af3f74b38..d73fe6b8b2fb 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -118,6 +118,16 @@ > #define IMX219_PIXEL_ARRAY_WIDTH 3280U > #define IMX219_PIXEL_ARRAY_HEIGHT 2464U > > +/* Embedded metadata stream structure */ > +#define IMX219_EMBEDDED_LINE_WIDTH 16384 > +#define IMX219_NUM_EMBEDDED_LINES 1 > + > +enum pad_types { > + IMAGE_PAD, > + METADATA_PAD, > + NUM_PADS > +}; No used. > + > struct imx219_reg { > u16 address; > u8 val; > @@ -429,7 +439,7 @@ static const char * const imx219_supply_name[] = { > * - v flip > * - h&v flips > */ > -static const u32 codes[] = { > +static const u32 imx219_mbus_formats[] = { For instance renaming this array should go in a patch of its own. > MEDIA_BUS_FMT_SRGGB10_1X10, > MEDIA_BUS_FMT_SGRBG10_1X10, > MEDIA_BUS_FMT_SGBRG10_1X10, > @@ -655,62 +665,17 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code) > > lockdep_assert_held(&imx219->mutex); > > - for (i = 0; i < ARRAY_SIZE(codes); i++) > - if (codes[i] == code) > + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++) > + if (imx219_mbus_formats[i] == code) > break; > > - if (i >= ARRAY_SIZE(codes)) > + if (i >= ARRAY_SIZE(imx219_mbus_formats)) > i = 0; > > i = (i & ~3) | (imx219->vflip->val ? 2 : 0) | > (imx219->hflip->val ? 1 : 0); > > - return codes[i]; > -} > - > -static void imx219_set_default_format(struct imx219 *imx219) > -{ > - struct v4l2_mbus_framefmt *fmt; > - > - fmt = &imx219->fmt; > - fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10; > - fmt->colorspace = V4L2_COLORSPACE_SRGB; > - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, > - fmt->colorspace, > - fmt->ycbcr_enc); > - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); > - fmt->width = supported_modes[0].width; > - fmt->height = supported_modes[0].height; > - fmt->field = V4L2_FIELD_NONE; > -} > - > -static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh) So should switching from open to init_cfg. Same thing for adding support for the V4L2 subdev active state (v4l2_subdev_init_finalize() and the corresponding changes in the get/set format handlers). This is mainline, not a BSP, patches must be reviewable. > -{ > - struct imx219 *imx219 = to_imx219(sd); > - struct v4l2_mbus_framefmt *try_fmt = > - v4l2_subdev_get_try_format(sd, fh->state, 0); > - struct v4l2_rect *try_crop; > - > - mutex_lock(&imx219->mutex); > - > - /* Initialize try_fmt */ > - try_fmt->width = supported_modes[0].width; > - try_fmt->height = supported_modes[0].height; > - try_fmt->code = imx219_get_format_code(imx219, > - MEDIA_BUS_FMT_SRGGB10_1X10); > - try_fmt->field = V4L2_FIELD_NONE; > - > - /* Initialize try_crop rectangle. */ > - try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0); > - try_crop->top = IMX219_PIXEL_ARRAY_TOP; > - try_crop->left = IMX219_PIXEL_ARRAY_LEFT; > - try_crop->width = IMX219_PIXEL_ARRAY_WIDTH; > - try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT; > - > - mutex_unlock(&imx219->mutex); > - > - return 0; > + return imx219_mbus_formats[i]; > } > > static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > @@ -802,98 +767,148 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = { > .s_ctrl = imx219_set_ctrl, > }; > > -static int imx219_enum_mbus_code(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_mbus_code_enum *code) > +static void imx219_init_formats(struct v4l2_subdev_state *state) > { > - struct imx219 *imx219 = to_imx219(sd); > + struct v4l2_mbus_framefmt *format; > + > + format = v4l2_state_get_stream_format(state, 0, 0); > + format->code = imx219_mbus_formats[0]; > + format->width = supported_modes[0].width; > + format->height = supported_modes[0].height; > + format->field = V4L2_FIELD_NONE; > + format->colorspace = V4L2_COLORSPACE_RAW; > + > + if (state->routing.routes[1].flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE) { > + format = v4l2_state_get_stream_format(state, 0, 1); > + format->code = MEDIA_BUS_FMT_METADATA_8; > + format->width = IMX219_EMBEDDED_LINE_WIDTH; > + format->height = 1; > + format->field = V4L2_FIELD_NONE; > + format->colorspace = V4L2_COLORSPACE_DEFAULT; > + } > +} > > - if (code->index >= (ARRAY_SIZE(codes) / 4)) > - return -EINVAL; > +static int _imx219_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > +{ > + struct v4l2_subdev_route routes[] = { > + { > + .source_pad = 0, > + .source_stream = 0, > + .flags = V4L2_SUBDEV_ROUTE_FL_IMMUTABLE | > + V4L2_SUBDEV_ROUTE_FL_SOURCE | > + V4L2_SUBDEV_ROUTE_FL_ACTIVE, > + }, > + { > + .source_pad = 0, > + .source_stream = 1, > + .flags = V4L2_SUBDEV_ROUTE_FL_SOURCE | > + V4L2_SUBDEV_ROUTE_FL_ACTIVE, > + } > + }; > > - mutex_lock(&imx219->mutex); > - code->code = imx219_get_format_code(imx219, codes[code->index * 4]); > - mutex_unlock(&imx219->mutex); > + struct v4l2_subdev_krouting routing = { > + .num_routes = ARRAY_SIZE(routes), > + .routes = routes, > + }; > + > + int ret; > + > + ret = v4l2_subdev_set_routing(sd, state, &routing); > + if (ret) > + return ret; > + > + imx219_init_formats(state); > > return 0; > } > > -static int imx219_enum_frame_size(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_frame_size_enum *fse) > +static int imx219_set_routing(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state, > + enum v4l2_subdev_format_whence which, > + struct v4l2_subdev_krouting *routing) > { > - struct imx219 *imx219 = to_imx219(sd); > - u32 code; > + int ret; > > - if (fse->index >= ARRAY_SIZE(supported_modes)) > + if (routing->num_routes == 0 || routing->num_routes > 2) > return -EINVAL; > > - mutex_lock(&imx219->mutex); > - code = imx219_get_format_code(imx219, fse->code); > - mutex_unlock(&imx219->mutex); > - if (fse->code != code) > - return -EINVAL; > + v4l2_subdev_lock_state(state); > > - fse->min_width = supported_modes[fse->index].width; > - fse->max_width = fse->min_width; > - fse->min_height = supported_modes[fse->index].height; > - fse->max_height = fse->min_height; > + ret = _imx219_set_routing(sd, state); > > - return 0; > + v4l2_subdev_unlock_state(state); > + > + return ret; > } > > -static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt) > +static int imx219_init_cfg(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *state) > { > - fmt->colorspace = V4L2_COLORSPACE_SRGB; > - fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace); > - fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true, > - fmt->colorspace, > - fmt->ycbcr_enc); > - fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace); > + int ret; > + > + v4l2_subdev_lock_state(state); > + > + ret = _imx219_set_routing(sd, state); > + > + v4l2_subdev_unlock_state(state); > + > + return ret; > } > > -static void imx219_update_pad_format(struct imx219 *imx219, > - const struct imx219_mode *mode, > - struct v4l2_subdev_format *fmt) > +static int imx219_enum_mbus_code(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_mbus_code_enum *code) > { > - fmt->format.width = mode->width; > - fmt->format.height = mode->height; > - fmt->format.field = V4L2_FIELD_NONE; > - imx219_reset_colorspace(&fmt->format); > + if (code->index >= ARRAY_SIZE(imx219_mbus_formats)) > + return -EINVAL; > + > + code->code = imx219_mbus_formats[code->index]; > + > + return 0; > } > > -static int __imx219_get_pad_format(struct imx219 *imx219, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_format *fmt) > +static int imx219_enum_frame_size(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_frame_size_enum *fse) > { > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > - struct v4l2_mbus_framefmt *try_fmt = > - v4l2_subdev_get_try_format(&imx219->sd, sd_state, > - fmt->pad); > - /* update the code which could change due to vflip or hflip: */ > - try_fmt->code = imx219_get_format_code(imx219, try_fmt->code); > - fmt->format = *try_fmt; > + unsigned int i; > + > + if (fse->stream == 0) { > + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); ++i) { > + if (imx219_mbus_formats[i] == fse->code) > + break; > + } > + > + if (i == ARRAY_SIZE(imx219_mbus_formats)) > + return -EINVAL; > + > + if (fse->index >= ARRAY_SIZE(supported_modes)) > + return -EINVAL; > + > + fse->min_width = supported_modes[fse->index].width; > + fse->max_width = fse->min_width; > + fse->max_height = supported_modes[fse->index].height; > + fse->min_height = fse->max_height; > } else { > - imx219_update_pad_format(imx219, imx219->mode, fmt); > - fmt->format.code = imx219_get_format_code(imx219, > - imx219->fmt.code); > + if (fse->code != MEDIA_BUS_FMT_METADATA_8) > + return -EINVAL; > + > + fse->min_width = IMX219_EMBEDDED_LINE_WIDTH; > + fse->max_width = fse->min_width; > + fse->min_height = IMX219_NUM_EMBEDDED_LINES; > + fse->max_height = fse->min_height; > } > > return 0; > } > > -static int imx219_get_pad_format(struct v4l2_subdev *sd, > - struct v4l2_subdev_state *sd_state, > - struct v4l2_subdev_format *fmt) > +static void imx219_update_metadata_pad_format(struct v4l2_subdev_format *fmt) > { > - struct imx219 *imx219 = to_imx219(sd); > - int ret; > - > - mutex_lock(&imx219->mutex); > - ret = __imx219_get_pad_format(imx219, sd_state, fmt); > - mutex_unlock(&imx219->mutex); > - > - return ret; > + fmt->format.width = IMX219_EMBEDDED_LINE_WIDTH; > + fmt->format.height = IMX219_NUM_EMBEDDED_LINES; > + fmt->format.code = MEDIA_BUS_FMT_METADATA_8; > + fmt->format.field = V4L2_FIELD_NONE; > } > > static int imx219_set_pad_format(struct v4l2_subdev *sd, > @@ -901,82 +916,91 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > struct v4l2_subdev_format *fmt) > { > struct imx219 *imx219 = to_imx219(sd); > + struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd); > const struct imx219_mode *mode; > - struct v4l2_mbus_framefmt *framefmt; > - int exposure_max, exposure_def, hblank; > + struct v4l2_mbus_framefmt *format; > unsigned int i; > + int ret = 0; > + int exposure_max, exposure_def, hblank; > > - mutex_lock(&imx219->mutex); > + if (fmt->pad != 0) { > + dev_err(&client->dev, "%s Could not get pad %d\n", __func__, > + fmt->pad); > + return -EINVAL; > + } > > - for (i = 0; i < ARRAY_SIZE(codes); i++) > - if (codes[i] == fmt->format.code) > + if (fmt->stream == 1) { > + /* Only one embedded data mode is supported */ > + imx219_update_metadata_pad_format(fmt); > + return 0; > + } > + > + if (fmt->stream != 0) > + return -EINVAL; > + > + /* > + * Validate the media bus code, defaulting to the first one if the > + * requested code isn't supported. > + */ > + for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); ++i) { > + if (imx219_mbus_formats[i] == fmt->format.code) > break; > - if (i >= ARRAY_SIZE(codes)) > - i = 0; > + } > > - /* Bayer order varies with flips */ > - fmt->format.code = imx219_get_format_code(imx219, codes[i]); > + if (i >= ARRAY_SIZE(imx219_mbus_formats)) > + i = 0; > > mode = v4l2_find_nearest_size(supported_modes, > ARRAY_SIZE(supported_modes), > width, height, > - fmt->format.width, fmt->format.height); > - imx219_update_pad_format(imx219, mode, fmt); > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > - framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad); > - *framefmt = fmt->format; > - } else if (imx219->mode != mode || > - imx219->fmt.code != fmt->format.code) { > - imx219->fmt = fmt->format; > - imx219->mode = mode; > - /* Update limits and set FPS to default */ > - __v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN, > - IMX219_VTS_MAX - mode->height, 1, > - mode->vts_def - mode->height); > - __v4l2_ctrl_s_ctrl(imx219->vblank, > - mode->vts_def - mode->height); > - /* Update max exposure while meeting expected vblanking */ > - exposure_max = mode->vts_def - 4; > - exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > - exposure_max : IMX219_EXPOSURE_DEFAULT; > - __v4l2_ctrl_modify_range(imx219->exposure, > - imx219->exposure->minimum, > - exposure_max, imx219->exposure->step, > - exposure_def); > - /* > - * Currently PPL is fixed to IMX219_PPL_DEFAULT, so hblank > - * depends on mode->width only, and is not changeble in any > - * way other than changing the mode. > - */ > - hblank = IMX219_PPL_DEFAULT - mode->width; > - __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, > - hblank); > - } > + fmt->format.width, > + fmt->format.height); > > - mutex_unlock(&imx219->mutex); > + v4l2_subdev_lock_state(sd_state); > > - return 0; > -} > + /* Update the stored format and return it. */ > + format = v4l2_state_get_stream_format(sd_state, fmt->pad, fmt->stream); > > -static int imx219_set_framefmt(struct imx219 *imx219) > -{ > - switch (imx219->fmt.code) { > - case MEDIA_BUS_FMT_SRGGB8_1X8: > - case MEDIA_BUS_FMT_SGRBG8_1X8: > - case MEDIA_BUS_FMT_SGBRG8_1X8: > - case MEDIA_BUS_FMT_SBGGR8_1X8: > - return imx219_write_regs(imx219, raw8_framefmt_regs, > - ARRAY_SIZE(raw8_framefmt_regs)); > - > - case MEDIA_BUS_FMT_SRGGB10_1X10: > - case MEDIA_BUS_FMT_SGRBG10_1X10: > - case MEDIA_BUS_FMT_SGBRG10_1X10: > - case MEDIA_BUS_FMT_SBGGR10_1X10: > - return imx219_write_regs(imx219, raw10_framefmt_regs, > - ARRAY_SIZE(raw10_framefmt_regs)); > + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && imx219->streaming) { > + ret = -EBUSY; > + goto done; > } > > - return -EINVAL; > + /* Bayer order varies with flips */ > + fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]); > + fmt->format = *format; > + > + /* Update limits and set FPS to default */ > + __v4l2_ctrl_modify_range(imx219->vblank, > + IMX219_VBLANK_MIN, > + IMX219_VTS_MAX - mode->height, > + 1, > + mode->vts_def - mode->height); > + __v4l2_ctrl_s_ctrl(imx219->vblank, mode->vts_def - mode->height); > + /* > + * Update max exposure while meeting > + * expected vblanking > + */ > + exposure_max = mode->vts_def - 4; > + exposure_def = (exposure_max < IMX219_EXPOSURE_DEFAULT) ? > + exposure_max : IMX219_EXPOSURE_DEFAULT; > + __v4l2_ctrl_modify_range(imx219->exposure, > + imx219->exposure->minimum, > + exposure_max, > + imx219->exposure->step, > + exposure_def); > + /* > + * Currently PPL is fixed to IMX219_PPL_DEFAULT, so > + * hblank depends on mode->width only, and is not > + * changeble in any way other than changing the mode. > + */ > + hblank = IMX219_PPL_DEFAULT - mode->width; > + __v4l2_ctrl_modify_range(imx219->hblank, hblank, hblank, 1, hblank); > + > +done: > + v4l2_subdev_unlock_state(sd_state); > + > + return ret; > } > > static const struct v4l2_rect * > @@ -1037,9 +1061,11 @@ static int imx219_start_streaming(struct imx219 *imx219) > const struct imx219_reg_list *reg_list; > int ret; > > - ret = pm_runtime_resume_and_get(&client->dev); > - if (ret < 0) > + ret = pm_runtime_get_sync(&client->dev); > + if (ret < 0) { > + pm_runtime_put_noidle(&client->dev); > return ret; > + } > > /* Apply default values of current mode */ > reg_list = &imx219->mode->reg_list; > @@ -1049,13 +1075,6 @@ static int imx219_start_streaming(struct imx219 *imx219) > goto err_rpm_put; > } > > - ret = imx219_set_framefmt(imx219); > - if (ret) { > - dev_err(&client->dev, "%s failed to set frame format: %d\n", > - __func__, ret); > - goto err_rpm_put; > - } > - > /* Apply customized values from user */ > ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler); > if (ret) > @@ -1133,21 +1152,22 @@ static int imx219_set_stream(struct v4l2_subdev *sd, int enable) > /* Power/clock management functions */ > static int imx219_power_on(struct device *dev) > { > - struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct imx219 *imx219 = to_imx219(sd); > int ret; > > ret = regulator_bulk_enable(IMX219_NUM_SUPPLIES, > imx219->supplies); > if (ret) { > - dev_err(dev, "%s: failed to enable regulators\n", > + dev_err(&client->dev, "%s: failed to enable regulators\n", > __func__); > return ret; > } > > ret = clk_prepare_enable(imx219->xclk); > if (ret) { > - dev_err(dev, "%s: failed to enable clock\n", > + dev_err(&client->dev, "%s: failed to enable clock\n", > __func__); > goto reg_off; > } > @@ -1166,7 +1186,8 @@ static int imx219_power_on(struct device *dev) > > static int imx219_power_off(struct device *dev) > { > - struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct imx219 *imx219 = to_imx219(sd); > > gpiod_set_value_cansleep(imx219->reset_gpio, 0); > @@ -1178,7 +1199,8 @@ static int imx219_power_off(struct device *dev) > > static int __maybe_unused imx219_suspend(struct device *dev) > { > - struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct imx219 *imx219 = to_imx219(sd); > > if (imx219->streaming) > @@ -1189,7 +1211,8 @@ static int __maybe_unused imx219_suspend(struct device *dev) > > static int __maybe_unused imx219_resume(struct device *dev) > { > - struct v4l2_subdev *sd = dev_get_drvdata(dev); > + struct i2c_client *client = to_i2c_client(dev); > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > struct imx219 *imx219 = to_imx219(sd); > int ret; > > @@ -1255,11 +1278,13 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = { > }; > > static const struct v4l2_subdev_pad_ops imx219_pad_ops = { > - .enum_mbus_code = imx219_enum_mbus_code, > - .get_fmt = imx219_get_pad_format, > - .set_fmt = imx219_set_pad_format, > - .get_selection = imx219_get_selection, > - .enum_frame_size = imx219_enum_frame_size, > + .init_cfg = imx219_init_cfg, > + .enum_mbus_code = imx219_enum_mbus_code, > + .get_fmt = v4l2_subdev_get_fmt, > + .set_fmt = imx219_set_pad_format, > + .get_selection = imx219_get_selection, > + .set_routing = imx219_set_routing, > + .enum_frame_size = imx219_enum_frame_size, > }; > > static const struct v4l2_subdev_ops imx219_subdev_ops = { > @@ -1268,10 +1293,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = { > .pad = &imx219_pad_ops, > }; > > -static const struct v4l2_subdev_internal_ops imx219_internal_ops = { > - .open = imx219_open, > -}; > - > /* Initialize control handlers */ > static int imx219_init_controls(struct imx219 *imx219) > { > @@ -1446,6 +1467,7 @@ static int imx219_check_hwcfg(struct device *dev) > static int imx219_probe(struct i2c_client *client) > { > struct device *dev = &client->dev; > + struct v4l2_subdev *sd; > struct imx219 *imx219; > int ret; > > @@ -1453,7 +1475,8 @@ static int imx219_probe(struct i2c_client *client) > if (!imx219) > return -ENOMEM; > > - v4l2_i2c_subdev_init(&imx219->sd, client, &imx219_subdev_ops); > + sd = &imx219->sd; > + v4l2_i2c_subdev_init(sd, client, &imx219_subdev_ops); > > /* Check the hardware configuration in device tree */ > if (imx219_check_hwcfg(dev)) > @@ -1520,27 +1543,29 @@ static int imx219_probe(struct i2c_client *client) > goto error_power_off; > > /* Initialize subdev */ > - imx219->sd.internal_ops = &imx219_internal_ops; > - imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > - V4L2_SUBDEV_FL_HAS_EVENTS; > - imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > + V4L2_SUBDEV_FL_HAS_EVENTS | > + V4L2_SUBDEV_FL_MULTIPLEXED; > > - /* Initialize source pad */ > + /* Initialize the media entity. */ > imx219->pad.flags = MEDIA_PAD_FL_SOURCE; > - > - /* Initialize default format */ > - imx219_set_default_format(imx219); > - > - ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad); > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR; > + ret = media_entity_pads_init(&sd->entity, 1, &imx219->pad); > if (ret) { > dev_err(dev, "failed to init entity pads: %d\n", ret); > goto error_handler_free; > } > > - ret = v4l2_async_register_subdev_sensor(&imx219->sd); > + ret = v4l2_subdev_init_finalize(sd); > + if (ret) { > + dev_err(dev, "failed to finalize sensor init: %d\n", ret); > + goto error_media_entity; > + } > + > + ret = v4l2_async_register_subdev_sensor(sd); > if (ret < 0) { > dev_err(dev, "failed to register sensor sub-device: %d\n", ret); > - goto error_media_entity; > + goto error_free_state; > } > > /* Enable runtime PM and turn off the device */ > @@ -1550,6 +1575,8 @@ static int imx219_probe(struct i2c_client *client) > > return 0; > > +error_free_state: > + v4l2_subdev_cleanup(sd); > error_media_entity: > media_entity_cleanup(&imx219->sd.entity); > > @@ -1568,6 +1595,9 @@ static int imx219_remove(struct i2c_client *client) > struct imx219 *imx219 = to_imx219(sd); > > v4l2_async_unregister_subdev(sd); > + > + v4l2_subdev_cleanup(sd); > + > media_entity_cleanup(&sd->entity); > imx219_free_controls(imx219); >
Hi Laurent, On 22/01/2022 00:27, Laurent Pinchart wrote: > Hi Jean-Michel, > > Thank you for the patch. > > On Fri, Jan 21, 2022 at 09:18:06AM +0100, Jean-Michel Hautbois wrote: >> Introduce the dt-bindinds documentation for bcm2835 CCP2/CSI2 camera > > s/bindinds/bindings/ > > I'd mention "Unicam" somewhere here. > >> interface. Also add a MAINTAINERS entry for it. Oh my, I tis not the right dts bindings patch, I mixed up my trees... :-/ Sorry for this I will send a v2.1 soon... >> >> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> >> Signed-off-by: Naushir Patuck <naush@raspberrypi.com> >> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >> --- >> Dave: I assumed you were the maintainer for this file, as I based it on the >> bcm2835-unicam.txt file. Are you happy to be added directly as the >> maintainer, or should this be specified as "Raspberry Pi Kernel >> Maintenance <kernel-list@raspberrypi.com>" >> - in v2: multiple corrections to pass the bot checking as Rob kindly >> told me. >> --- >> .../bindings/media/brcm,bcm2835-unicam.yaml | 103 ++++++++++++++++++ >> MAINTAINERS | 6 + >> 2 files changed, 109 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> >> diff --git a/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> new file mode 100644 >> index 000000000000..1427514142cf >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> @@ -0,0 +1,103 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/media/brcm,bcm2835-unicam.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Broadcom BCM283x Camera Interface (Unicam) >> + >> +maintainers: >> + - Dave Stevenson <dave.stevenson@raspberrypi.com> >> + >> +description: |- >> + The Unicam block on BCM283x SoCs is the receiver for either >> + CSI-2 or CCP2 data from image sensors or similar devices. >> + >> + The main platform using this SoC is the Raspberry Pi family of boards. >> + On the Pi the VideoCore firmware can also control this hardware block, >> + and driving it from two different processors will cause issues. >> + To avoid this, the firmware checks the device tree configuration >> + during boot. If it finds device tree nodes called csi0 or csi1 then >> + it will stop the firmware accessing the block, and it can then >> + safely be used via the device tree binding. > > As mentioned in the review of the DT integration, the nodes should > ideally be called just "csi", not "csi0" and "csi1" (maybe Rob could > confirm this ?). Dave, is there a way the firmware could be updated to > also hand over control of the Unicam instances to Linux when a "csi" > node is found, not just "csi0" or "csi1" ? > > Given that the node names are significant, they should be enforced in > the YAML schema. > >> + >> +properties: >> + compatible: >> + const: brcm,bcm2835-unicam >> + >> + reg: >> + description: >> + physical base address and length of the register sets for the device. > > This can be dropped. > >> + maxItems: 1 > > There are two items in the example below. How does this validate ? > >> + >> + interrupts: >> + description: the IRQ line for this Unicam instance. > > This can be dropped. > >> + maxItems: 1 >> + >> + clocks: >> + description: |- >> + list of clock specifiers, corresponding to entries in clock-names >> + property. > > clocks: > items: > - description: The clock for ... > - description: The clock for ... > > (with the two descriptions matching the LP and VPU clocks, I don't know > what they are). > >> + >> + clock-names: >> + items: >> + - const: lp >> + - const: vpu >> + >> + port: >> + $ref: /schemas/graph.yaml#/properties/port >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - clocks >> + - clock-names >> + - port >> + >> +additionalProperties: False >> + >> +examples: >> + - | >> + csi1: csi1@7e801000 { >> + compatible = "brcm,bcm2835-unicam"; >> + reg = <0x7e801000 0x800>, >> + <0x7e802004 0x4>; >> + interrupts = <2 7>; > > Let's use the Pi 4 device tree as an example, as that's what we're > upstreaming first. > >> + clocks = <&clocks BCM2835_CLOCK_CAM1>, > > This will fail to compile without a proper #include, did you get this to > pass validation ? > >> + <&firmware_clocks 4>; >> + clock-names = "lp", "vpu"; >> + port { >> + csi1_ep: endpoint { >> + remote-endpoint = <&tc358743_0>; >> + data-lanes = <1 2>; >> + }; >> + }; >> + }; >> + >> + i2c0: i2c@7e205000 { >> + tc358743: csi-hdmi-bridge@0f { >> + compatible = "toshiba,tc358743"; >> + reg = <0x0f>; >> + clocks = <&tc358743_clk>; >> + clock-names = "refclk"; >> + >> + tc358743_clk: bridge-clk { >> + compatible = "fixed-clock"; >> + #clock-cells = <0>; >> + clock-frequency = <27000000>; >> + }; >> + >> + port { >> + tc358743_0: endpoint { >> + remote-endpoint = <&csi1_ep>; >> + clock-lanes = <0>; >> + data-lanes = <1 2>; >> + clock-noncontinuous; >> + link-frequencies = >> + /bits/ 64 <297000000>; >> + }; >> + }; >> + }; >> + }; > > I'd drop this node completely. > >> +... >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 33f75892f98e..07f238fd5ff9 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -3679,6 +3679,12 @@ F: Documentation/media/v4l-drivers/bcm2835-isp.rst >> F: drivers/staging/vc04_services/bcm2835-isp >> F: include/uapi/linux/bcm2835-isp.h >> >> +BROADCOM BCM2835 CAMERA DRIVER >> +M: Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com> >> +L: linux-media@vger.kernel.org >> +S: Maintained >> +F: Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml >> + >> BROADCOM BCM47XX MIPS ARCHITECTURE >> M: Hauke Mehrtens <hauke@hauke-m.de> >> M: Rafał Miłecki <zajec5@gmail.com> >
Hi Laurent and Jean-Michel On Fri, 21 Jan 2022 at 22:45, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Jean-Michel, > > Thank you for the patch. > > On Fri, Jan 21, 2022 at 09:18:08AM +0100, Jean-Michel Hautbois wrote: > > Add both MIPI CSI-2 nodes in the core bcm2711 tree. Use the 3-cells > > interrupt declaration, corresponding clocks and default as disabled. > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > --- > > arch/arm/boot/dts/bcm2711.dtsi | 31 +++++++++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi > > index dff18fc9a906..077141df7024 100644 > > --- a/arch/arm/boot/dts/bcm2711.dtsi > > +++ b/arch/arm/boot/dts/bcm2711.dtsi > > @@ -3,6 +3,7 @@ > > > > #include <dt-bindings/interrupt-controller/arm-gic.h> > > #include <dt-bindings/soc/bcm2835-pm.h> > > +#include <dt-bindings/power/raspberrypi-power.h> > > > > / { > > compatible = "brcm,bcm2711"; > > @@ -293,6 +294,36 @@ hvs: hvs@7e400000 { > > interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>; > > }; > > > > + csi0: csi1@7e800000 { > > The node name should be csi@7e800000, not csi1@7e800000. Now, this will > probably cause issues with the firmware that looks for csi1 (and csi0 ?) > to hand over control of the Unicam CSI-2 receiver to the kernel. I > wonder if this is something that could be handled by a firmware update, > to also recognize nodes named "csi" ? It already looks for any node starting "csi". If you check the downstream DT [1], then the nodes are "csi0: csi@7e800000" and "csi1: csi@7e801000". There is no actual action required to hand the peripheral over to the kernel, it just prevents the firmware from using it and causing problems (it masks out the interrupt, and that's checked as part of the firmware initialising the peripheral). If using imx219 or one of the other sensors supported by the firmware, "vcgencmd get_camera" should report that the sensor isn't detected, and "sudo vcdbg log msg" should have a line similar to "020174.613: camsubs: Ignoring camera 0 as unicam device not available" Dave [1] https://github.com/raspberrypi/linux/blob/rpi-5.10.y/arch/arm/boot/dts/bcm270x.dtsi#L88 > > + compatible = "brcm,bcm2835-unicam"; > > + reg = <0x7e800000 0x800>, > > + <0x7e802000 0x4>; > > + interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clocks BCM2835_CLOCK_CAM0>, > > + <&firmware_clocks 4>; > > + clock-names = "lp", "vpu"; > > + power-domains = <&power RPI_POWER_DOMAIN_UNICAM0>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + #clock-cells = <1>; > > Why do you need #address-cells, #size-cells and #clock-cells ? They're > not mentioned in the binding. > > > + status="disabled"; > > Missing spaces around the =. > > Same comment for the next node. > > > + }; > > + > > + csi1: csi1@7e801000 { > > + compatible = "brcm,bcm2835-unicam"; > > + reg = <0x7e801000 0x800>, > > + <0x7e802004 0x4>; > > + interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clocks BCM2835_CLOCK_CAM1>, > > + <&firmware_clocks 4>; > > + clock-names = "lp", "vpu"; > > + power-domains = <&power RPI_POWER_DOMAIN_UNICAM1>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + #clock-cells = <1>; > > + status="disabled"; > > + }; > > + > > pixelvalve3: pixelvalve@7ec12000 { > > compatible = "brcm,bcm2711-pixelvalve3"; > > reg = <0x7ec12000 0x100>; > > -- > Regards, > > Laurent Pinchart
Hello ! This patch series adds the BCM2835 CCP2/CSI2 camera interface named unicam. This driver is already used in the out-of-tree linux-rpi repository [1], and this work aims to support it in mainline. The series is based on top of: - Rebased on top of 5.16 Tomi Valkeinen's multiplexed stream work, on his multistream/work branch [2] which has been submitted as v10 at the time of this writing. The objective is to demonstrate the use of multiplexed streams on a real world widely used example as well as supporting unicam mainline. - The "Add 12bit and 14bit luma-only formats" series [3] is partly applied (the Y10P format bug) the new formats are now part of this series. The series is made of 7 patches: - 1/7 and 2/7 introduce the new formats needed for the unicam driver - 3/7 introduces dt-bindings documentation and MAINTAINERS entry I have tentatively assigned maintainership, is this fine ? - 4/7 adds the driver support in media/platform - 5/7 introduces the csi nodes in the bcm2711 file, in a disabled state - 6/7 modifies imx219 driver to make it use the multiplexed streams API - 7/7 is the imx219 dtsi file tested on my RPi 4b with the mainline dtb and not the downstream dtb anymore. All those patches are in my tree [4]. Patch 4/7 comes from the linux-rpi work [1] with substantial modifications: - Removed the non-mc API which is deprecated, and not needed upstream - Moved from one video node with two source pads (image and embedded) to two video nodes - Added a subdev between the sensor and the video nodes to properly route the streams - Added support for multiplexed streams API In order to test it, one will need a RPi board and the camv2 (imx219) sensor. An updated v4l-utils is also needed [5] to have support for multiplexed streams. v4l2-compliance passes on both video devices, without streaming testing though. This series in its v2 does integrate the device tree nodes into the upstream BCM2835 or BCM2711 device tree support. The media-ctl topology is: ``` pi@raspberrypi:~ $ media-ctl -d0 -p Media controller API version 5.16.0 Media device information ------------------------ driver unicam model unicam serial bus info platform:fe801000.csi hw revision 0x0 driver version 5.16.0 Device topology - entity 1: unicam-subdev (3 pads, 3 links, 1 route) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev0 routes: 0/0 -> 1/0 [ACTIVE] pad0: Sink [stream:0 fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] <- "imx219 10-0010":0 [ENABLED,IMMUTABLE] pad1: Source [stream:0 fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range] -> "unicam-image":0 [ENABLED,IMMUTABLE] pad2: Source -> "unicam-embedded":0 [ENABLED,IMMUTABLE] - entity 5: imx219 10-0010 (1 pad, 1 link, 2 routes) type V4L2 subdev subtype Sensor flags 0 device node name /dev/v4l-subdev1 routes: 0/0 -> 0/0 [ACTIVE, IMMUTABLE, SOURCE] 0/0 -> 0/1 [ACTIVE, SOURCE] pad0: Source [stream:0 fmt:SRGGB10_1X10/3280x2464 field:none colorspace:raw crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464] [stream:1 fmt:METADATA_8/16384x1 field:none crop.bounds:(8,8)/3280x2464 crop:(8,8)/3280x2464] -> "unicam-subdev":0 [ENABLED,IMMUTABLE] - entity 7: unicam-image (1 pad, 1 link, 0 route) type Node subtype V4L flags 1 device node name /dev/video0 pad0: Sink <- "unicam-subdev":1 [ENABLED,IMMUTABLE] - entity 13: unicam-embedded (1 pad, 1 link, 0 route) type Node subtype V4L flags 0 device node name /dev/video1 pad0: Sink <- "unicam-subdev":2 [ENABLED,IMMUTABLE] ``` In order to properly configure the media pipeline, it is needed to call the usual ioctls, and configure routing in order to send the embedded data from the sensor to the "unicam-embedded" device node : ``` media=0 media-ctl -d${media} -l "'imx219 10-0010':0->'unicam-subdev':0 [1]" media-ctl -d${media} -l "'unicam-subdev':1->'unicam-image':0 [1]" media-ctl -d${media} -v -R "'unicam-subdev' [0/0->1/0[1],0/1->2/0[1]]" media-ctl -d${media} -V "'imx219 10-0010':0/0 [fmt:SRGGB10_1X10/3280x2464 field:none]" v4l2-ctl -d0 --set-fmt-video width=3280,height=2464,pixelformat='pRAA',field=none media-ctl -d${media} -v -V "'imx219 10-0010':0/1 [fmt:METADATA_8/16384x1 field:none]" media-ctl -d${media} -p ``` Be sure to configure the routes before setting the format, as s_routing resets the default format. Then a frame can be capture with yavta: `yavta --capture=1 /dev/video0 -F/tmp/test-#.bin` Side note: My tree [4] also includes a backport for the unicam-isp drivers, it is then possible, and it has been successfully tested, to use libcamera and qcam to display the camera output. [1]: https://github.com/raspberrypi/linux/tree/rpi-5.15.y [2]: https://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git/log/?h=multistream/work [3]: https://patchwork.linuxtv.org/project/linux-media/list/?series=7099 [4]: https://github.com/jhautbois/linux/tree/jmh/tomba/multistream/next/work-v10-with-laurent-isp [5]: https://github.com/tomba/v4l-utils Jean-Michel Hautbois (7): media: v4l: Add V4L2-PIX-FMT-Y12P format media: v4l: Add V4L2-PIX-FMT-Y14P format media: dt-bindings: media: Add bindings for bcm2835-unicam media: bcm2835-unicam: Add support for for CCP2/CSI2 camera interface ARM: dts: bcm2711: Add unicam CSI nodes media: imx219: Add support for multiplexed streams media: bcm283x: Include the imx219 node .../bindings/media/brcm,bcm2835-unicam.yaml | 103 + .../media/v4l/pixfmt-yuv-luma.rst | 44 + MAINTAINERS | 7 + arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 1 + arch/arm/boot/dts/bcm2711.dtsi | 31 + arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi | 103 + drivers/media/i2c/imx219.c | 452 +-- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 2 + drivers/media/platform/bcm2835/Kconfig | 21 + drivers/media/platform/bcm2835/Makefile | 3 + .../media/platform/bcm2835/bcm2835-unicam.c | 2678 +++++++++++++++++ .../media/platform/bcm2835/vc4-regs-unicam.h | 253 ++ 13 files changed, 3488 insertions(+), 211 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml create mode 100644 arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi create mode 100644 drivers/media/platform/bcm2835/Kconfig create mode 100644 drivers/media/platform/bcm2835/Makefile create mode 100644 drivers/media/platform/bcm2835/bcm2835-unicam.c create mode 100644 drivers/media/platform/bcm2835/vc4-regs-unicam.h