mbox series

[RFC,v3,00/11] Add support for BCM2835 camera interface (unicam)

Message ID 20220202175639.149681-1-jeanmichel.hautbois@ideasonboard.com
Headers show
Series Add support for BCM2835 camera interface (unicam) | expand

Message

Jean-Michel Hautbois Feb. 2, 2022, 5:56 p.m. UTC
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 11 patches:
- 1/11 and 2/11 introduce the new formats needed for the unicam driver
- 3/11 introduces dt-bindings documentation and MAINTAINERS entry I have
  tentatively assigned maintainership, is this fine ?
- 4/11 adds the driver support in media/platform
- 5/11 introduces the csi nodes in the bcm2711 file, in a disabled state
- 6/11 to 10/11 modifies imx219 driver to make it use the multiplexed
  streams API
- 11/11 is the imx219 dtsi file tested on my RPi 4b with the mainline
  dtb and not the downstream dtb anymore. *This patch is not intended to
be applied*.

All those patches are in my tree [4].

Patch 4/11 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 with one exception: as the colorspace support is removed in v3,
we now have:

test VIDIOC_G_FMT: OK
  fail: v4l2-test-formats.cpp(363): colorspace >= 0xff
  fail: v4l2-test-formats.cpp(465): testColorspace(!node->is_io_mc,
	pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
test VIDIOC_TRY_FMT: FAIL
  fail: v4l2-test-formats.cpp(363): colorspace >= 0xff
  fail: v4l2-test-formats.cpp(465): testColorspace(!node->is_io_mc,
	pix.pixelformat, pix.colorspace, pix.ycbcr_enc, pix.quantization)
test VIDIOC_S_FMT: FAIL

This series since its v2 does integrate the device tree nodes into the
upstream BCM2835 or BCM2711 device tree support.

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 2-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 2-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 2-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.

The media-ctl topology is:
```
pi@raspberrypi:~ $ media-ctl -d${media} -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, 2 routes)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
	routes:
		0/0 -> 1/0 [ACTIVE]
		0/1 -> 2/0 [ACTIVE]
	pad0: Sink
		[stream:0 fmt:SRGGB10_1X10/3280x2464 field:none colorspace:raw]
		[stream:1 fmt:METADATA_8/16384x1 field:none]
		<- "imx219 2-0010":0 [ENABLED,IMMUTABLE]
	pad1: Source
		[stream:0 fmt:SRGGB10_1X10/3280x2464 field:none colorspace:raw]
		-> "unicam-image":0 [ENABLED,IMMUTABLE]
	pad2: Source
		[stream:0 fmt:METADATA_8/16384x1 field:none]
		-> "unicam-embedded":0 [ENABLED,IMMUTABLE]

- entity 5: imx219 2-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 9: 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 15: 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]

```

Then a frame can be capture with yavta:
`yavta --capture=1 /dev/video0 -F/tmp/test-#.bin`

In v3, capture on the metadata node (/dev/video1 in my case) can't be
started if capture on image node (/dev/video0) is not already started.
This can be tested using yavta, letting it capture frames indefinitely
and start another yavta instance on the /dev/video1 node.


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-rpi/tree/jmh/tomba/multistream/next/work-v10-with-laurent-isp-v3
[5]: https://github.com/jhautbois/v4l-utils/tree/jmh/tomi-multiplexed-streams

Jean-Michel Hautbois (11):
  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 CCP2/CSI2 camera interface
  ARM: dts: bcm2711: Add unicam CSI nodes
  media: imx219: Rename mbus codes array
  media: imx219: Switch from open to init_cfg
  media: imx219: Introduce the set_routing operation
  media: imx219: use a local v4l2_subdev to simplify reading
  media: imx219: Add support for the V4L2 subdev active state
  media: bcm283x: Include the imx219 node

 .../bindings/media/brcm,bcm2835-unicam.yaml   |  107 +
 .../media/v4l/pixfmt-yuv-luma.rst             |   44 +
 MAINTAINERS                                   |    8 +
 arch/arm/boot/dts/bcm2711-rpi-4-b.dts         |    1 +
 arch/arm/boot/dts/bcm2711.dtsi                |   25 +
 arch/arm/boot/dts/bcm283x-rpi-imx219.dtsi     |  102 +
 drivers/media/i2c/imx219.c                    |  344 ++-
 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   | 2586 +++++++++++++++++
 .../media/platform/bcm2835/vc4-regs-unicam.h  |  253 ++
 13 files changed, 3349 insertions(+), 148 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

Comments

Dave Stevenson Feb. 3, 2022, 11:36 a.m. UTC | #1
Hi Jean-Michel and Laurent

n Wed, 2 Feb 2022 at 22:36, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jean-Michel,
>
> On Wed, Feb 02, 2022 at 11:09:20PM +0100, Jean-Michel Hautbois wrote:
> > On 02/02/2022 19:33, Stefan Wahren wrote:
> > > Hi Jean-Michel,
> > >
> > > please drop the first "media:" before dt-bindings.
> > >
> > > Am 02.02.22 um 18:56 schrieb Jean-Michel Hautbois:
> > >> Introduce the dt-bindings documentation for bcm2835 CCP2/CSI2 Unicam
> > >> camera 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>"

Probably easiest to switch to "Raspberry Pi Kernel Maintenance
<kernel-list@raspberrypi.com>".
That list didn't exist when I originally wrote the doc, and it just
makes life easier should I decide to move on (not planning it). Naush
is on that list too.

> > >> ---
> > >>   .../bindings/media/brcm,bcm2835-unicam.yaml   | 107 ++++++++++++++++++
> > >>   MAINTAINERS                                   |   7 ++
> > >>   2 files changed, 114 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..5bf41a8834fa
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml
> > >> @@ -0,0 +1,107 @@
> > >> +# 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 starting by csi then
> > >> +  it will stop the firmware accessing the block, and it can then
> > >> +  safely be used via the device tree binding.
> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    const: brcm,bcm2835-unicam
> > >> +
> > >> +  reg:
> > >> +    maxItems: 2
> > >
> > > I would be nice to have reg-names here similar to the clocks.
> >
> > Sure, I just don't know what the names are ;-).
>
> Please discuss this with the Rasperry Pi developers to figure out then.

It's the "Unicam" and "Clock Manager Image" (CMI) blocks respectively.

CMI is only 4 registers. It provides high speed clock source selection
for the two Unicam blocks, a camera test block that has never been
used, and one of the USB controllers. Each peripheral is controlled by
a separate register.
It was discussed previously and viewed as not worthwhile creating a
full clock driver for it.

> > >> +
> > >> +  interrupts:
> > >> +    maxItems: 1
> > >> +
> > >> +  clocks:
> > >> +    items:
> > >> +      - description: Clock for the camera.
>
> This also seems weird, as far as I know the SoC doesn't output a clock
> for the camera sensor (and it should be specified in the camera sensor
> DT node if it did anyway).

It's the clocks to Unicam, not to the camera / sensor.

The LP clock drives the LP state machine of Unicam for the relevant
DPHY state transitions.
The VPU or core clock is needed to ensure that the other bus systems
are running fast enough for the data generated.

  Dave

> > >> +      - description: Clock for the vpu.
> > >> +
> > >> +  clock-names:
> > >> +    items:
> > >> +      - const: lp
> > >> +      - const: vpu
> > >> +
> > >> +  power-domains:
> > >> +    items:
> > >> +      - description: Unicam power domain
> > >> +
> > >> +  num-data-lanes:
>
> This is a vendor-specific property and thus requires a vendor prefix.
>
> > >> +    items:
> > >> +      - enum: [ 2, 4 ]
> > >> +
> > >> +  port:
> > >> +    additionalProperties: false
> > >> +    $ref: /schemas/graph.yaml#/$defs/port-base
> > >> +
> > >> +    properties:
> > >> +      endpoint:
> > >> +        $ref: /schemas/media/video-interfaces.yaml#
> > >> +        unevaluatedProperties: false
> > >> +
> > >> +        properties:
> > >> +          data-lanes: true
> > >> +          link-frequencies: true
> > >> +
> > >> +        required:
> > >> +          - data-lanes
> > >> +          - link-frequencies
> > >> +
> > >> +    required:
> > >> +      - endpoint
> > >> +
> > >> +required:
> > >> +  - compatible
> > >> +  - reg
> > >> +  - interrupts
> > >> +  - clocks
> > >> +  - clock-names
> > >> +  - power-domains
> > >> +  - num-data-lanes
> > >> +  - port
> > >> +
> > >> +additionalProperties: False
> > >> +
> > >> +examples:
> > >> +  - |
> > >> +    #include <dt-bindings/clock/bcm2835.h>
> > >> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > >> +    #include <dt-bindings/power/raspberrypi-power.h>
> > >> +    csi1: csi@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>;
> > >> +        num-data-lanes = <2>;
> > >> +        port {
> > >> +                csi1_ep: endpoint {
> > >> +                        remote-endpoint = <&imx219_0>;
> > >> +                        data-lanes = <1 2>;
> > >> +                        link-frequencies = /bits/ 64 <456000000>;
> > >> +                };
> > >> +        };
> > >> +    };
> > >> +...
> > >> diff --git a/MAINTAINERS b/MAINTAINERS
> > >> index a0770a861ca4..29344ea86847 100644
> > >> --- a/MAINTAINERS
> > >> +++ b/MAINTAINERS
> > >> @@ -3670,6 +3670,13 @@ N:  bcm113*
> > >>   N:       bcm216*
> > >>   N:       kona
> > >>
> > >> +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
> > >> +F:        arch/arm/boot/dts/bcm283x*
> > >> +
> > >
> > > I suggest to make the MAINTAINERS changes a single separate patch
> > > instead of small incremental changes.
> >
> > I can make it a separate patch, indeed.
> >
> > >>   BROADCOM BCM47XX MIPS ARCHITECTURE
> > >>   M:       Hauke Mehrtens <hauke@hauke-m.de>
> > >>   M:       Rafał Miłecki <zajec5@gmail.com>
>
> --
> Regards,
>
> Laurent Pinchart