| Message ID | 20240301213231.10340-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: Add driver for the Raspberry Pi <5 CSI-2 receiver | expand |
The subject line should obviously have read 'PATCH v6', and I should have updated Jean-Michel's e-mail address instead of blindly relying on get-maintainer.pl. Maybe sending patches on a Friday evening isn't he best idea after all. Sorry about that. On Fri, Mar 01, 2024 at 11:32:15PM +0200, Laurent Pinchart wrote: > Hello everybody, > > This patch series adds a new driver for the BCM2835 (and derivative) > CCP2/CSI2 camera interface named Unicam. This IP core is found in the > VC4-based Raspberry Pi, namely the Pi Zero, Pi 3 and Pi 4. > > Camera support for Raspberry Pi 4 currently relies on a downstream > Unicam driver that live in the Raspberry Pi kernel tree ([1]). The > driver uses the V4L2 API, but works around the lack of features in V4L2 > to properly support sensor embedded data. Since the Unicam driver > development by Raspberry Pi, some of those features have been merged in > the kernel (namely the V4L2 streams API) or are being developed (namely > generic metadata formats and subdev internal pads), with patches posted > for review on the linux-media mailing list ([2]). > > This new upstream driver is based on the downstream code, extensively > reworked to use the new V4L2 APIs. > > The series is based on top of a merge of > > - v7 of the generic metadata and internal pads, rebased on v6.8-rc5 ([3]) > - the downstream ISP driver ported to mainline ([4]) > > For convenience, it can be found in [5]. Note that the ISP driver is > getting upstreamed separately. > > The series starts with five patches that add support for streams and > embedded data to the imx219 driver (01/15 to 05/15). Patches 06/15 to > 09/15 then add the Unicam driver, with new V4L2 pixel formats (06/15 and > 07/15) and DT bindings (08/15) The remaining patches cover DT > integration (10/15 to 14/15) with a sample DT overlay for the IMX219 > camera module (15/15). > > The patches have been tested on a Raspberry Pi 4 using an IMX219 camera > module (the Raspberry Pi camera v2), with libcamera. Updates are needed > to libcamera to use the new V4L2 APIs, patches have been posted to [6]. > For manual testing with media-ctl, corresponding API updates to > v4l-utils are available at [7]. > > The changes to the imx219 driver effectively define the interface that > raw sensors should expose to userspace. This needs to be documented > explicitly. I would like this series to first get a review round, which > will likely raise API questions. I will then work on the documentation. > > As a summary, more work is needed, but I believe we're nearing > completion. This series, along with the libcamera support, help > validating the new kernel APIs. We have in my opinion reached a point > where we can start converting other sensor drivers from the downstream > Raspberry Pi kernel to the standard APIs for embedded data, as well as > integrating the APIs in the Raspberry Pi 5 CFE driver. > > [1] https://github.com/raspberrypi/linux/tree/rpi-6.1.y/drivers/media/platform/bcm2835 > [2] https://lore.kernel.org/linux-media/20231106122539.1268265-1-sakari.ailus@linux.intel.com > [3] https://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git/log/?h=rpi/v6.8/metadata/v7 > [4] https://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git/log/?h=rpi/v6.8/isp/v2 > [5] https://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git/log/?h=rpi/v6.8/unicam/next > [6] https://lists.libcamera.org/pipermail/libcamera-devel/2024-March/040711.html > [7] https://git.linuxtv.org/pinchartl/v4l-utils.git/log/?h=metadata > > Here's the mandatory v4l2-compliance report. There are 5 errors, which > are caused by v4l2-compliance not supporting the new embedded data APIs > yet. This will be fixed and patches for v4l2-compliance will be > submitted (alongside patches to media-ctl). > > v4l2-compliance 1.27.0-5180, 64 bits, 64-bit time_t > v4l2-compliance SHA: c14579f7e5f5 2024-03-01 20:29:24 > > Compliance test for unicam device /dev/media0: > > Media Driver Info: > Driver name : unicam > Model : unicam > Serial : > Bus info : platform:fe801000.csi > Media version : 6.8.0 > Hardware revision: 0x00000000 (0) > Driver version : 6.8.0 > > Required ioctls: > test MEDIA_IOC_DEVICE_INFO: OK > test invalid ioctls: OK > > Allow for multiple opens: > test second /dev/media0 open: OK > test MEDIA_IOC_DEVICE_INFO: OK > test for unlimited opens: OK > > Media Controller ioctls: > test MEDIA_IOC_G_TOPOLOGY: OK > Entities: 4 Interfaces: 4 Pads: 8 Links: 7 > test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK > test MEDIA_IOC_SETUP_LINK: OK > > Total for unicam device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 > -------------------------------------------------------------------------------- > Compliance test for unicam device /dev/video0: > > Driver Info: > Driver name : unicam > Card type : unicam > Bus info : platform:fe801000.csi > Driver version : 6.8.0 > Capabilities : 0xa4a00001 > Video Capture > Metadata Capture > I/O MC > Streaming > Extended Pix Format > Device Capabilities > Device Caps : 0x24200001 > Video Capture > I/O MC > Streaming > Extended Pix Format > Media Driver Info: > Driver name : unicam > Model : unicam > Serial : > Bus info : platform:fe801000.csi > Media version : 6.8.0 > Hardware revision: 0x00000000 (0) > Driver version : 6.8.0 > Interface Info: > ID : 0x0300000d > Type : V4L Video > Entity Info: > ID : 0x0000000b (11) > Name : unicam-image > Function : V4L2 I/O > Flags : default > Pad 0x0100000c : 0: Sink > Link 0x0200000f: from remote pad 0x1000003 of entity 'unicam' (Video Interface Bridge): Data, Enabled, Immutable > > Required ioctls: > test MC information (see 'Media Driver Info' above): OK > test VIDIOC_QUERYCAP: OK > test invalid ioctls: OK > > Allow for multiple opens: > test second /dev/video0 open: OK > test VIDIOC_QUERYCAP: OK > test VIDIOC_G/S_PRIORITY: OK > test for unlimited opens: OK > > Debug ioctls: > test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) > test VIDIOC_LOG_STATUS: OK > > Input ioctls: > test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) > test VIDIOC_ENUMAUDIO: OK (Not Supported) > test VIDIOC_G/S/ENUMINPUT: OK > test VIDIOC_G/S_AUDIO: OK (Not Supported) > Inputs: 1 Audio Inputs: 0 Tuners: 0 > > Output ioctls: > test VIDIOC_G/S_MODULATOR: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_ENUMAUDOUT: OK (Not Supported) > test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) > test VIDIOC_G/S_AUDOUT: OK (Not Supported) > Outputs: 0 Audio Outputs: 0 Modulators: 0 > > Input/Output configuration ioctls: > test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) > test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) > test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) > test VIDIOC_G/S_EDID: OK (Not Supported) > > Control ioctls (Input 0): > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) > test VIDIOC_QUERYCTRL: OK (Not Supported) > test VIDIOC_G/S_CTRL: OK (Not Supported) > test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) > test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) > test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) > Standard Controls: 0 Private Controls: 0 > > Format ioctls (Input 0): > test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK > test VIDIOC_G/S_PARM: OK (Not Supported) > test VIDIOC_G_FBUF: OK (Not Supported) > test VIDIOC_G_FMT: OK > test VIDIOC_TRY_FMT: OK > test VIDIOC_S_FMT: OK > test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) > test Cropping: OK (Not Supported) > test Composing: OK (Not Supported) > test Scaling: OK > > Codec ioctls (Input 0): > test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) > test VIDIOC_G_ENC_INDEX: OK (Not Supported) > test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) > > Buffer ioctls (Input 0): > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK > test CREATE_BUFS maximum buffers: OK > test VIDIOC_EXPBUF: OK > test Requests: OK (Not Supported) > > Total for unicam device /dev/video0: 47, Succeeded: 47, Failed: 0, Warnings: 0 > -------------------------------------------------------------------------------- > Compliance test for unicam device /dev/video1: > > Driver Info: > Driver name : unicam > Card type : unicam > Bus info : platform:fe801000.csi > Driver version : 6.8.0 > Capabilities : 0xa4a00001 > Video Capture > Metadata Capture > I/O MC > Streaming > Extended Pix Format > Device Capabilities > Device Caps : 0x24a00000 > Metadata Capture > I/O MC > Streaming > Extended Pix Format > Media Driver Info: > Driver name : unicam > Model : unicam > Serial : > Bus info : platform:fe801000.csi > Media version : 6.8.0 > Hardware revision: 0x00000000 (0) > Driver version : 6.8.0 > Interface Info: > ID : 0x03000013 > Type : V4L Video > Entity Info: > ID : 0x00000011 (17) > Name : unicam-embedded > Function : V4L2 I/O > Pad 0x01000012 : 0: Sink > Link 0x02000015: from remote pad 0x1000004 of entity 'unicam' (Video Interface Bridge): Data, Enabled, Immutable > > Required ioctls: > test MC information (see 'Media Driver Info' above): OK > test VIDIOC_QUERYCAP: OK > test invalid ioctls: OK > > Allow for multiple opens: > test second /dev/video1 open: OK > test VIDIOC_QUERYCAP: OK > test VIDIOC_G/S_PRIORITY: OK > test for unlimited opens: OK > > Debug ioctls: > test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) > test VIDIOC_LOG_STATUS: OK > > Input ioctls: > test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) > test VIDIOC_ENUMAUDIO: OK (Not Supported) > test VIDIOC_G/S/ENUMINPUT: OK > test VIDIOC_G/S_AUDIO: OK (Not Supported) > Inputs: 1 Audio Inputs: 0 Tuners: 0 > > Output ioctls: > test VIDIOC_G/S_MODULATOR: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_ENUMAUDOUT: OK (Not Supported) > test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) > test VIDIOC_G/S_AUDOUT: OK (Not Supported) > Outputs: 0 Audio Outputs: 0 Modulators: 0 > > Input/Output configuration ioctls: > test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) > test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) > test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) > test VIDIOC_G/S_EDID: OK (Not Supported) > > Control ioctls (Input 0): > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) > test VIDIOC_QUERYCTRL: OK (Not Supported) > test VIDIOC_G/S_CTRL: OK (Not Supported) > test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) > test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) > test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) > Standard Controls: 0 Private Controls: 0 > > Format ioctls (Input 0): > test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK > test VIDIOC_G/S_PARM: OK (Not Supported) > test VIDIOC_G_FBUF: OK (Not Supported) > test VIDIOC_G_FMT: OK > fail: v4l2-test-formats.cpp(590): dataformat 00003ff8 (x?) for buftype 13 not reported by ENUM_FMT > test VIDIOC_TRY_FMT: FAIL > test VIDIOC_S_FMT: OK > test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) > test Cropping: OK (Not Supported) > test Composing: OK (Not Supported) > test Scaling: OK (Not Supported) > > Codec ioctls (Input 0): > test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) > test VIDIOC_G_ENC_INDEX: OK (Not Supported) > test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) > > Buffer ioctls (Input 0): > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK > test CREATE_BUFS maximum buffers: OK > test VIDIOC_EXPBUF: OK > test Requests: OK (Not Supported) > > Total for unicam device /dev/video1: 47, Succeeded: 46, Failed: 1, Warnings: 0 > -------------------------------------------------------------------------------- > Compliance test for unicam device /dev/v4l-subdev0: > > Driver Info: > Driver version : 6.8.0 > Capabilities : 0x00000002 > Streams Support > Client Capabilities: 0x0000000000000003 > streams interval-uses-which Media Driver Info: > Driver name : unicam > Model : unicam > Serial : > Bus info : platform:fe801000.csi > Media version : 6.8.0 > Hardware revision: 0x00000000 (0) > Driver version : 6.8.0 > Interface Info: > ID : 0x03000017 > Type : V4L Sub-Device > Entity Info: > ID : 0x00000001 (1) > Name : unicam > Function : Video Interface Bridge > Pad 0x01000002 : 0: Sink > Link 0x02000009: from remote pad 0x1000006 of entity 'imx219 5-0010' (Camera Sensor): Data, Enabled, Immutable > Pad 0x01000003 : 1: Source > Link 0x0200000f: to remote pad 0x100000c of entity 'unicam-image' (V4L2 I/O): Data, Enabled, Immutable > Pad 0x01000004 : 2: Source > Link 0x02000015: to remote pad 0x1000012 of entity 'unicam-embedded' (V4L2 I/O): Data, Enabled, Immutable > > Required ioctls: > test MC information (see 'Media Driver Info' above): OK > test VIDIOC_SUDBEV_QUERYCAP: OK > test invalid ioctls: OK > > Allow for multiple opens: > test second /dev/v4l-subdev0 open: OK > test VIDIOC_SUBDEV_QUERYCAP: OK > test for unlimited opens: OK > > Debug ioctls: > test VIDIOC_LOG_STATUS: OK (Not Supported) > > Input ioctls: > test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) > test VIDIOC_ENUMAUDIO: OK (Not Supported) > test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) > test VIDIOC_G/S_AUDIO: OK (Not Supported) > Inputs: 0 Audio Inputs: 0 Tuners: 0 > > Output ioctls: > test VIDIOC_G/S_MODULATOR: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_ENUMAUDOUT: OK (Not Supported) > test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) > test VIDIOC_G/S_AUDOUT: OK (Not Supported) > Outputs: 0 Audio Outputs: 0 Modulators: 0 > > Input/Output configuration ioctls: > test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) > test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) > test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) > test VIDIOC_G/S_EDID: OK (Not Supported) > > Sub-Device routing ioctls: > fail: v4l2-test-subdevs.cpp(631): !(source->flags & MEDIA_PAD_FL_SOURCE) > test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL > fail: v4l2-test-subdevs.cpp(631): !(source->flags & MEDIA_PAD_FL_SOURCE) > test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL > > Sub-Device ioctls (Sink Pad 0): > > Sub-Device ioctls (Source Pad 1): > > Sub-Device ioctls (Source Pad 2): > > Control ioctls: > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) > test VIDIOC_QUERYCTRL: OK (Not Supported) > test VIDIOC_G/S_CTRL: OK (Not Supported) > test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) > test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) > test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) > Standard Controls: 0 Private Controls: 0 > > Format ioctls: > test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) > test VIDIOC_G/S_PARM: OK (Not Supported) > test VIDIOC_G_FBUF: OK (Not Supported) > test VIDIOC_G_FMT: OK (Not Supported) > test VIDIOC_TRY_FMT: OK (Not Supported) > test VIDIOC_S_FMT: OK (Not Supported) > test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) > test Cropping: OK (Not Supported) > test Composing: OK (Not Supported) > test Scaling: OK (Not Supported) > > Codec ioctls: > test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) > test VIDIOC_G_ENC_INDEX: OK (Not Supported) > test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) > > Buffer ioctls: > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) > test CREATE_BUFS maximum buffers: OK > test VIDIOC_EXPBUF: OK (Not Supported) > test Requests: OK (Not Supported) > > Total for unicam device /dev/v4l-subdev0: 47, Succeeded: 45, Failed: 2, Warnings: 0 > -------------------------------------------------------------------------------- > Compliance test for unicam device /dev/v4l-subdev1: > > Driver Info: > Driver version : 6.8.0 > Capabilities : 0x00000002 > Streams Support > Client Capabilities: 0x0000000000000003 > streams interval-uses-which Media Driver Info: > Driver name : unicam > Model : unicam > Serial : > Bus info : platform:fe801000.csi > Media version : 6.8.0 > Hardware revision: 0x00000000 (0) > Driver version : 6.8.0 > Interface Info: > ID : 0x03000019 > Type : V4L Sub-Device > Entity Info: > ID : 0x00000005 (5) > Name : imx219 5-0010 > Function : Camera Sensor > Pad 0x01000006 : 0: Source > Link 0x02000009: to remote pad 0x1000002 of entity 'unicam' (Video Interface Bridge): Data, Enabled, Immutable > Pad 0x01000007 : 1: Sink, 00000008 > Pad 0x01000008 : 2: Sink, 00000008 > > Required ioctls: > test MC information (see 'Media Driver Info' above): OK > test VIDIOC_SUDBEV_QUERYCAP: OK > test invalid ioctls: OK > > Allow for multiple opens: > test second /dev/v4l-subdev1 open: OK > test VIDIOC_SUBDEV_QUERYCAP: OK > test for unlimited opens: OK > > Debug ioctls: > test VIDIOC_LOG_STATUS: OK (Not Supported) > > Input ioctls: > test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) > test VIDIOC_ENUMAUDIO: OK (Not Supported) > test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) > test VIDIOC_G/S_AUDIO: OK (Not Supported) > Inputs: 0 Audio Inputs: 0 Tuners: 0 > > Output ioctls: > test VIDIOC_G/S_MODULATOR: OK (Not Supported) > test VIDIOC_G/S_FREQUENCY: OK (Not Supported) > test VIDIOC_ENUMAUDOUT: OK (Not Supported) > test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) > test VIDIOC_G/S_AUDOUT: OK (Not Supported) > Outputs: 0 Audio Outputs: 0 Modulators: 0 > > Input/Output configuration ioctls: > test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) > test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) > test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) > test VIDIOC_G/S_EDID: OK (Not Supported) > > Sub-Device routing ioctls: > fail: v4l2-test-subdevs.cpp(630): !(sink->flags & MEDIA_PAD_FL_SINK) > test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL > fail: v4l2-test-subdevs.cpp(630): !(sink->flags & MEDIA_PAD_FL_SINK) > test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL > > Sub-Device ioctls (Source Pad 0): > > Sub-Device ioctls (Sink Pad 1): > > Sub-Device ioctls (Sink Pad 2): > > Control ioctls: > test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK > test VIDIOC_QUERYCTRL: OK > test VIDIOC_G/S_CTRL: OK > test VIDIOC_G/S/TRY_EXT_CTRLS: OK > test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK > test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) > Standard Controls: 20 Private Controls: 0 > > Format ioctls: > test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) > test VIDIOC_G/S_PARM: OK (Not Supported) > test VIDIOC_G_FBUF: OK (Not Supported) > test VIDIOC_G_FMT: OK (Not Supported) > test VIDIOC_TRY_FMT: OK (Not Supported) > test VIDIOC_S_FMT: OK (Not Supported) > test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) > test Cropping: OK (Not Supported) > test Composing: OK (Not Supported) > test Scaling: OK (Not Supported) > > Codec ioctls: > test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) > test VIDIOC_G_ENC_INDEX: OK (Not Supported) > test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) > > Buffer ioctls: > test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) > test CREATE_BUFS maximum buffers: OK > test VIDIOC_EXPBUF: OK (Not Supported) > test Requests: OK (Not Supported) > > Total for unicam device /dev/v4l-subdev1: 47, Succeeded: 45, Failed: 2, Warnings: 0 > > Grand Total for unicam device /dev/media0: 196, Succeeded: 191, Failed: 5, Warnings: 0 > > > Dave Stevenson (2): > dt-bindings: media: Add bindings for bcm2835-unicam > media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface > > Jean-Michel Hautbois (3): > media: v4l: Add V4L2-PIX-FMT-Y12P format > media: v4l: Add V4L2-PIX-FMT-Y14P format > ARM: dts: bcm2835: Add Unicam CSI nodes > > Laurent Pinchart (8): > media: i2c: imx219: Inline imx219_update_pad_format() in its caller > media: i2c: imx219: Add internal image sink pad > media: i2c: imx219: Report internal routes to userspace > media: i2c: imx219: Report streams using frame descriptors > media: i2c: imx219: Add embedded data support > ARM: dts: bcm2835-rpi: Move firmware-clocks from bcm2711 to bcm2835 > ARM: dts: bcm2711-rpi-4-b: Add CAM1 regulator > [DNI] arm64: dts: broadcom: Add overlay for Raspberry Pi 4B IMX219 > camera > > Uwe Kleine-König (2): > ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0 > ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0 > > .../bindings/media/brcm,bcm2835-unicam.yaml | 117 + > .../media/v4l/pixfmt-yuv-luma.rst | 48 + > MAINTAINERS | 7 + > .../arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts | 7 + > .../boot/dts/broadcom/bcm2711-rpi-cm4-io.dts | 17 + > arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi | 36 +- > arch/arm/boot/dts/broadcom/bcm2711.dtsi | 8 + > arch/arm/boot/dts/broadcom/bcm2835-rpi.dtsi | 19 + > arch/arm/boot/dts/broadcom/bcm283x.dtsi | 24 + > arch/arm64/boot/dts/broadcom/Makefile | 2 + > .../dts/broadcom/bcm2711-rpi-4-b-imx219.dtso | 65 + > drivers/media/i2c/imx219.c | 419 ++- > drivers/media/platform/Kconfig | 1 + > drivers/media/platform/Makefile | 1 + > drivers/media/platform/broadcom/Kconfig | 23 + > drivers/media/platform/broadcom/Makefile | 3 + > .../platform/broadcom/bcm2835-unicam-regs.h | 255 ++ > .../media/platform/broadcom/bcm2835-unicam.c | 2607 +++++++++++++++++ > drivers/media/v4l2-core/v4l2-ioctl.c | 2 + > include/uapi/linux/videodev2.h | 2 + > 20 files changed, 3588 insertions(+), 75 deletions(-) > create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml > create mode 100644 arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b-imx219.dtso > create mode 100644 drivers/media/platform/broadcom/Kconfig > create mode 100644 drivers/media/platform/broadcom/Makefile > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c > > > base-commit: 384a3aa6b48b29b4c71130c028db78484a0f0ab4
Hi Laurent On Fri, Mar 01, 2024 at 11:32:17PM +0200, Laurent Pinchart wrote: > Use the newly added internal pad API to expose the internal > configuration of the sensor to userspace in a standard manner. This also > paves the way for adding support for embedded data with an additional > internal pad. > > To maintain compatibility with existing userspace that may operate on > pad 0 unconditionally, keep the source pad numbered 0 and number the > internal image pad 1. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/imx219.c | 169 +++++++++++++++++++++++++++++-------- > 1 file changed, 133 insertions(+), 36 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 3878da50860e..817bf192e4d9 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -140,6 +140,7 @@ > #define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 > > /* IMX219 native and active pixel array size. */ > +#define IMX219_NATIVE_FORMAT MEDIA_BUS_FMT_SRGGB10_1X10 > #define IMX219_NATIVE_WIDTH 3296U > #define IMX219_NATIVE_HEIGHT 2480U > #define IMX219_PIXEL_ARRAY_LEFT 8U > @@ -312,9 +313,15 @@ static const struct imx219_mode supported_modes[] = { > }, > }; > > +enum imx219_pad_ids { > + IMX219_PAD_SOURCE, > + IMX219_PAD_IMAGE, Feels a bit weird for the internal source pad to be named "Image" as it will (if I understand correctly) host both the image and metadata streams. However, I have an hard time proposing a different name, but we should find something that should be used in all drivers ported to this new API. Enough with bikeshedding anyway, this is a tiny detail. > + IMX219_NUM_PADS, > +}; > + > struct imx219 { > struct v4l2_subdev sd; > - struct media_pad pad; > + struct media_pad pads[IMX219_NUM_PADS]; > > struct regmap *regmap; > struct clk *xclk; /* system clock to IMX219 */ > @@ -374,7 +381,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > int ret = 0; > > state = v4l2_subdev_get_locked_active_state(&imx219->sd); > - format = v4l2_subdev_state_get_format(state, 0); > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE); > > if (ctrl->id == V4L2_CID_VBLANK) { > int exposure_max, exposure_def; > @@ -593,8 +600,8 @@ static int imx219_set_framefmt(struct imx219 *imx219, > u64 bin_h, bin_v; > int ret = 0; > > - format = v4l2_subdev_state_get_format(state, 0); > - crop = v4l2_subdev_state_get_crop(state, 0); > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE); > + crop = v4l2_subdev_state_get_crop(state, IMX219_PAD_IMAGE); > > switch (format->code) { > case MEDIA_BUS_FMT_SRGGB8_1X8: > @@ -764,10 +771,25 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd, > { > struct imx219 *imx219 = to_imx219(sd); > > - if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4)) > - return -EINVAL; > + if (code->pad == IMX219_PAD_IMAGE) { > + /* The internal image pad is hardwired to the native format. */ > + if (code->index) > + return -EINVAL; > > - code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]); > + code->code = IMX219_NATIVE_FORMAT; If you return 0 here you can spare the else {} branch. Same in the function below. > + } else { > + /* > + * On the source pad, the sensor supports multiple raw formats > + * with different bit depths. > + */ > + u32 format; > + > + if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4)) > + return -EINVAL; > + > + format = imx219_mbus_formats[code->index * 4]; > + code->code = imx219_get_format_code(imx219, format); > + } > > return 0; > } > @@ -777,19 +799,25 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd, > struct v4l2_subdev_frame_size_enum *fse) > { > struct imx219 *imx219 = to_imx219(sd); > - u32 code; > > - if (fse->index >= ARRAY_SIZE(supported_modes)) > - return -EINVAL; > + if (fse->pad == IMX219_PAD_IMAGE) { > + if (fse->code != IMX219_NATIVE_FORMAT || fse->index > 0) > + return -EINVAL; > > - code = imx219_get_format_code(imx219, fse->code); > - if (fse->code != code) > - return -EINVAL; > + fse->min_width = IMX219_NATIVE_WIDTH; > + fse->max_width = IMX219_NATIVE_WIDTH; > + fse->min_height = IMX219_NATIVE_HEIGHT; > + fse->max_height = IMX219_NATIVE_HEIGHT; > + } else { > + if (fse->code != imx219_get_format_code(imx219, fse->code) || > + fse->index >= ARRAY_SIZE(supported_modes)) > + return -EINVAL; > > - 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; > + 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; > + } > > return 0; > } > @@ -801,9 +829,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > struct imx219 *imx219 = to_imx219(sd); > const struct imx219_mode *mode; > struct v4l2_mbus_framefmt *format; > + struct v4l2_rect *compose; > struct v4l2_rect *crop; > unsigned int bin_h, bin_v; > > + /* > + * The driver is mode-based, the format can be set on the source pad > + * only. > + */ > + if (fmt->pad != IMX219_PAD_SOURCE) > + return v4l2_subdev_get_fmt(sd, state, fmt); > + > /* > * Adjust the requested format to match the closest mode. The Bayer > * order varies with flips. > @@ -822,22 +858,51 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE; > fmt->format.xfer_func = V4L2_XFER_FUNC_NONE; > > - format = v4l2_subdev_state_get_format(state, 0); > + /* Propagate the format through the sensor. */ > + > + /* The image pad models the pixel array, and thus has a fixed size. */ > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_IMAGE); > *format = fmt->format; Is this assignment needed ? Isn't 'fmt' meant to be applied at pad #0 ? Also, you overwrite the code and size below, do the rest of the 'struct v4l2_mbus_framefmt' fields apply to pad #1 (field, colorspace, quantization etc ? If pad #1 represents the pixel array, I don't think they do). Also, can't the pad #1 format be initialized by .init_state() and never touched again ? > + format->code = IMX219_NATIVE_FORMAT; > + format->width = IMX219_NATIVE_WIDTH; > + format->height = IMX219_NATIVE_HEIGHT; > > /* > * Use binning to maximize the crop rectangle size, and centre it in the > * sensor. > */ > - bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U); > - bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U); > + bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / fmt->format.width, 2U); > + bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / fmt->format.height, 2U); > > - crop = v4l2_subdev_state_get_crop(state, 0); > - crop->width = format->width * bin_h; > - crop->height = format->height * bin_v; > + crop = v4l2_subdev_state_get_crop(state, IMX219_PAD_IMAGE); > + crop->width = fmt->format.width * bin_h; > + crop->height = fmt->format.height * bin_v; > crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2; > crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2; This is no different than before, but I now wonder if VGA is actually obtained by cropping down to 1280x960 and then by a 2x2 binning, or is there a 4x4 binning mode. I presume this is correct though, as this has been validated by many people. > > + /* > + * The compose rectangle models binning, its size is the sensor output > + * size. > + */ > + compose = v4l2_subdev_state_get_compose(state, IMX219_PAD_IMAGE); > + compose->left = 0; > + compose->top = 0; > + compose->width = fmt->format.width; > + compose->height = fmt->format.height; > + > + /* > + * No mode use digital crop, the source pad crop rectangle size and > + * format are thus identical to the image pad compose rectangle. > + */ > + crop = v4l2_subdev_state_get_crop(state, IMX219_PAD_SOURCE); > + crop->left = 0; > + crop->top = 0; > + crop->width = fmt->format.width; > + crop->height = fmt->format.height; > + > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE); > + *format = fmt->format; > + > if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > int exposure_max; > int exposure_def; > @@ -874,13 +939,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > struct v4l2_subdev_state *state, > struct v4l2_subdev_selection *sel) > { > - switch (sel->target) { > - case V4L2_SEL_TGT_CROP: { > - sel->r = *v4l2_subdev_state_get_crop(state, 0); > - return 0; > - } > + struct v4l2_rect *compose; > > + switch (sel->target) { > case V4L2_SEL_TGT_NATIVE_SIZE: > + if (sel->pad != IMX219_PAD_IMAGE) > + return -EINVAL; > + > sel->r.top = 0; > sel->r.left = 0; > sel->r.width = IMX219_NATIVE_WIDTH; > @@ -890,11 +955,35 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > > case V4L2_SEL_TGT_CROP_DEFAULT: > case V4L2_SEL_TGT_CROP_BOUNDS: > - sel->r.top = IMX219_PIXEL_ARRAY_TOP; > - sel->r.left = IMX219_PIXEL_ARRAY_LEFT; > - sel->r.width = IMX219_PIXEL_ARRAY_WIDTH; > - sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT; > + switch (sel->pad) { > + case IMX219_PAD_IMAGE: > + sel->r.top = IMX219_PIXEL_ARRAY_TOP; > + sel->r.left = IMX219_PIXEL_ARRAY_LEFT; > + sel->r.width = IMX219_PIXEL_ARRAY_WIDTH; > + sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT; > + return 0; > > + case IMX219_PAD_SOURCE: > + compose = v4l2_subdev_state_get_compose(state, > + IMX219_PAD_IMAGE); > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = compose->width; > + sel->r.height = compose->height; > + return 0; > + } > + > + break; > + > + case V4L2_SEL_TGT_CROP: > + sel->r = *v4l2_subdev_state_get_crop(state, sel->pad); > + return 0; > + > + case V4L2_SEL_TGT_COMPOSE: > + if (sel->pad != IMX219_PAD_IMAGE) > + return -EINVAL; > + > + sel->r = *v4l2_subdev_state_get_compose(state, sel->pad); > return 0; Do we need to support TGT_COMPOSE_BOUNDS for PAD_IMAGE ? I would have suggested V4L2_SEL_TGT_COMPOSE_DEFAULT too, but according to the spec it does not apply to subdevices. > } > > @@ -906,7 +995,7 @@ static int imx219_init_state(struct v4l2_subdev *sd, > { > struct v4l2_subdev_format fmt = { > .which = V4L2_SUBDEV_FORMAT_TRY, > - .pad = 0, > + .pad = IMX219_PAD_SOURCE, > .format = { > .code = MEDIA_BUS_FMT_SRGGB10_1X10, > .width = supported_modes[0].width, Why not intialize pad#1 format here an never touch it again ? Should also the pad#1 crop and compose rectangles and pad#0 crop be initialized here ? > @@ -1174,10 +1263,18 @@ static int imx219_probe(struct i2c_client *client) > V4L2_SUBDEV_FL_HAS_EVENTS; > imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > - /* Initialize source pad */ > - imx219->pad.flags = MEDIA_PAD_FL_SOURCE; > + /* > + * Initialize the pads. To preserve backward compatibility with > + * userspace that used the sensor before the introduction of the > + * internal image pad, the external source pad is numbered 0 and the > + * internal image pad numbered 1. > + */ > + imx219->pads[IMX219_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > + imx219->pads[IMX219_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK > + | MEDIA_PAD_FL_INTERNAL; > > - ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad); > + ret = media_entity_pads_init(&imx219->sd.entity, > + ARRAY_SIZE(imx219->pads), imx219->pads); > if (ret) { > dev_err(dev, "failed to init entity pads: %d\n", ret); > goto error_handler_free; > -- > Regards, > > Laurent Pinchart > >
Hi Laurent On Mon, Mar 04, 2024 at 10:13:47AM +0100, Jacopo Mondi wrote: > Hi Laurent > > On Fri, Mar 01, 2024 at 11:32:17PM +0200, Laurent Pinchart wrote: > > Use the newly added internal pad API to expose the internal > > configuration of the sensor to userspace in a standard manner. This also > > paves the way for adding support for embedded data with an additional > > internal pad. > > > > To maintain compatibility with existing userspace that may operate on > > pad 0 unconditionally, keep the source pad numbered 0 and number the > > internal image pad 1. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/i2c/imx219.c | 169 +++++++++++++++++++++++++++++-------- > > 1 file changed, 133 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 3878da50860e..817bf192e4d9 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -140,6 +140,7 @@ > > #define IMX219_DEFAULT_LINK_FREQ_4LANE 363000000 > > > > /* IMX219 native and active pixel array size. */ > > +#define IMX219_NATIVE_FORMAT MEDIA_BUS_FMT_SRGGB10_1X10 > > #define IMX219_NATIVE_WIDTH 3296U > > #define IMX219_NATIVE_HEIGHT 2480U > > #define IMX219_PIXEL_ARRAY_LEFT 8U > > @@ -312,9 +313,15 @@ static const struct imx219_mode supported_modes[] = { > > }, > > }; > > > > +enum imx219_pad_ids { > > + IMX219_PAD_SOURCE, > > + IMX219_PAD_IMAGE, > > Feels a bit weird for the internal source pad to be named "Image" as > it will (if I understand correctly) host both the image and metadata > streams. > > However, I have an hard time proposing a different name, but we should > find something that should be used in all drivers ported to this new > API. > > Enough with bikeshedding anyway, this is a tiny detail. > Now that I've read 5/15 I now understand why this is called "IMAGE" :) > > + IMX219_NUM_PADS, > > +}; > > + > > struct imx219 { > > struct v4l2_subdev sd; > > - struct media_pad pad; > > + struct media_pad pads[IMX219_NUM_PADS]; > > > > struct regmap *regmap; > > struct clk *xclk; /* system clock to IMX219 */ > > @@ -374,7 +381,7 @@ static int imx219_set_ctrl(struct v4l2_ctrl *ctrl) > > int ret = 0; > > > > state = v4l2_subdev_get_locked_active_state(&imx219->sd); > > - format = v4l2_subdev_state_get_format(state, 0); > > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE); > > > > if (ctrl->id == V4L2_CID_VBLANK) { > > int exposure_max, exposure_def; > > @@ -593,8 +600,8 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > u64 bin_h, bin_v; > > int ret = 0; > > > > - format = v4l2_subdev_state_get_format(state, 0); > > - crop = v4l2_subdev_state_get_crop(state, 0); > > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE); > > + crop = v4l2_subdev_state_get_crop(state, IMX219_PAD_IMAGE); > > > > switch (format->code) { > > case MEDIA_BUS_FMT_SRGGB8_1X8: > > @@ -764,10 +771,25 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd, > > { > > struct imx219 *imx219 = to_imx219(sd); > > > > - if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4)) > > - return -EINVAL; > > + if (code->pad == IMX219_PAD_IMAGE) { > > + /* The internal image pad is hardwired to the native format. */ > > + if (code->index) > > + return -EINVAL; > > > > - code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]); > > + code->code = IMX219_NATIVE_FORMAT; > > If you return 0 here you can spare the else {} branch. Same in the > function below. > You're reworking this in 5/15, so no need to change this and cause rebase conflicts. > > + } else { > > + /* > > + * On the source pad, the sensor supports multiple raw formats > > + * with different bit depths. > > + */ > > + u32 format; > > + > > + if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4)) > > + return -EINVAL; > > + > > + format = imx219_mbus_formats[code->index * 4]; > > + code->code = imx219_get_format_code(imx219, format); > > + } > > > > return 0; > > } > > @@ -777,19 +799,25 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd, > > struct v4l2_subdev_frame_size_enum *fse) > > { > > struct imx219 *imx219 = to_imx219(sd); > > - u32 code; > > > > - if (fse->index >= ARRAY_SIZE(supported_modes)) > > - return -EINVAL; > > + if (fse->pad == IMX219_PAD_IMAGE) { > > + if (fse->code != IMX219_NATIVE_FORMAT || fse->index > 0) > > + return -EINVAL; > > > > - code = imx219_get_format_code(imx219, fse->code); > > - if (fse->code != code) > > - return -EINVAL; > > + fse->min_width = IMX219_NATIVE_WIDTH; > > + fse->max_width = IMX219_NATIVE_WIDTH; > > + fse->min_height = IMX219_NATIVE_HEIGHT; > > + fse->max_height = IMX219_NATIVE_HEIGHT; > > + } else { > > + if (fse->code != imx219_get_format_code(imx219, fse->code) || > > + fse->index >= ARRAY_SIZE(supported_modes)) > > + return -EINVAL; > > > > - 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; > > + 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; > > + } > > > > return 0; > > } > > @@ -801,9 +829,17 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > struct imx219 *imx219 = to_imx219(sd); > > const struct imx219_mode *mode; > > struct v4l2_mbus_framefmt *format; > > + struct v4l2_rect *compose; > > struct v4l2_rect *crop; > > unsigned int bin_h, bin_v; > > > > + /* > > + * The driver is mode-based, the format can be set on the source pad > > + * only. > > + */ > > + if (fmt->pad != IMX219_PAD_SOURCE) > > + return v4l2_subdev_get_fmt(sd, state, fmt); > > + > > /* > > * Adjust the requested format to match the closest mode. The Bayer > > * order varies with flips. > > @@ -822,22 +858,51 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd, > > fmt->format.quantization = V4L2_QUANTIZATION_FULL_RANGE; > > fmt->format.xfer_func = V4L2_XFER_FUNC_NONE; > > > > - format = v4l2_subdev_state_get_format(state, 0); > > + /* Propagate the format through the sensor. */ > > + > > + /* The image pad models the pixel array, and thus has a fixed size. */ > > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_IMAGE); > > *format = fmt->format; > > Is this assignment needed ? > > Isn't 'fmt' meant to be applied at pad #0 ? Also, you overwrite the > code and size below, do the rest of the 'struct v4l2_mbus_framefmt' > fields apply to pad #1 (field, colorspace, quantization etc ? If pad > #1 represents the pixel array, I don't think they do). > > Also, can't the pad #1 format be initialized by .init_state() and never > touched again ? > > > > + format->code = IMX219_NATIVE_FORMAT; > > + format->width = IMX219_NATIVE_WIDTH; > > + format->height = IMX219_NATIVE_HEIGHT; > > > > /* > > * Use binning to maximize the crop rectangle size, and centre it in the > > * sensor. > > */ > > - bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / format->width, 2U); > > - bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / format->height, 2U); > > + bin_h = min(IMX219_PIXEL_ARRAY_WIDTH / fmt->format.width, 2U); > > + bin_v = min(IMX219_PIXEL_ARRAY_HEIGHT / fmt->format.height, 2U); > > > > - crop = v4l2_subdev_state_get_crop(state, 0); > > - crop->width = format->width * bin_h; > > - crop->height = format->height * bin_v; > > + crop = v4l2_subdev_state_get_crop(state, IMX219_PAD_IMAGE); > > + crop->width = fmt->format.width * bin_h; > > + crop->height = fmt->format.height * bin_v; > > crop->left = (IMX219_NATIVE_WIDTH - crop->width) / 2; > > crop->top = (IMX219_NATIVE_HEIGHT - crop->height) / 2; > > This is no different than before, but I now wonder if VGA is actually > obtained by cropping down to 1280x960 and then by a 2x2 binning, or is > there a 4x4 binning mode. I presume this is correct though, as this has been > validated by many people. > > > > > + /* > > + * The compose rectangle models binning, its size is the sensor output > > + * size. > > + */ > > + compose = v4l2_subdev_state_get_compose(state, IMX219_PAD_IMAGE); > > + compose->left = 0; > > + compose->top = 0; > > + compose->width = fmt->format.width; > > + compose->height = fmt->format.height; > > + > > + /* > > + * No mode use digital crop, the source pad crop rectangle size and > > + * format are thus identical to the image pad compose rectangle. > > + */ > > + crop = v4l2_subdev_state_get_crop(state, IMX219_PAD_SOURCE); > > + crop->left = 0; > > + crop->top = 0; > > + crop->width = fmt->format.width; > > + crop->height = fmt->format.height; > > + > > + format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE); > > + *format = fmt->format; > > + > > if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > int exposure_max; > > int exposure_def; > > @@ -874,13 +939,13 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > > struct v4l2_subdev_state *state, > > struct v4l2_subdev_selection *sel) > > { > > - switch (sel->target) { > > - case V4L2_SEL_TGT_CROP: { > > - sel->r = *v4l2_subdev_state_get_crop(state, 0); > > - return 0; > > - } > > + struct v4l2_rect *compose; > > > > + switch (sel->target) { > > case V4L2_SEL_TGT_NATIVE_SIZE: > > + if (sel->pad != IMX219_PAD_IMAGE) > > + return -EINVAL; > > + > > sel->r.top = 0; > > sel->r.left = 0; > > sel->r.width = IMX219_NATIVE_WIDTH; > > @@ -890,11 +955,35 @@ static int imx219_get_selection(struct v4l2_subdev *sd, > > > > case V4L2_SEL_TGT_CROP_DEFAULT: > > case V4L2_SEL_TGT_CROP_BOUNDS: > > - sel->r.top = IMX219_PIXEL_ARRAY_TOP; > > - sel->r.left = IMX219_PIXEL_ARRAY_LEFT; > > - sel->r.width = IMX219_PIXEL_ARRAY_WIDTH; > > - sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT; > > + switch (sel->pad) { > > + case IMX219_PAD_IMAGE: > > + sel->r.top = IMX219_PIXEL_ARRAY_TOP; > > + sel->r.left = IMX219_PIXEL_ARRAY_LEFT; > > + sel->r.width = IMX219_PIXEL_ARRAY_WIDTH; > > + sel->r.height = IMX219_PIXEL_ARRAY_HEIGHT; > > + return 0; > > > > + case IMX219_PAD_SOURCE: > > + compose = v4l2_subdev_state_get_compose(state, > > + IMX219_PAD_IMAGE); > > + sel->r.top = 0; > > + sel->r.left = 0; > > + sel->r.width = compose->width; > > + sel->r.height = compose->height; > > + return 0; > > + } > > + > > + break; > > + > > + case V4L2_SEL_TGT_CROP: > > + sel->r = *v4l2_subdev_state_get_crop(state, sel->pad); > > + return 0; > > + > > + case V4L2_SEL_TGT_COMPOSE: > > + if (sel->pad != IMX219_PAD_IMAGE) > > + return -EINVAL; > > + > > + sel->r = *v4l2_subdev_state_get_compose(state, sel->pad); > > return 0; > > Do we need to support TGT_COMPOSE_BOUNDS for PAD_IMAGE ? I would have > suggested V4L2_SEL_TGT_COMPOSE_DEFAULT too, but according to the spec > it does not apply to subdevices. > > > } > > > > @@ -906,7 +995,7 @@ static int imx219_init_state(struct v4l2_subdev *sd, > > { > > struct v4l2_subdev_format fmt = { > > .which = V4L2_SUBDEV_FORMAT_TRY, > > - .pad = 0, > > + .pad = IMX219_PAD_SOURCE, > > .format = { > > .code = MEDIA_BUS_FMT_SRGGB10_1X10, > > .width = supported_modes[0].width, > > Why not intialize pad#1 format here an never touch it again ? Should > also the pad#1 crop and compose rectangles and pad#0 crop be > initialized here ? > > > @@ -1174,10 +1263,18 @@ static int imx219_probe(struct i2c_client *client) > > V4L2_SUBDEV_FL_HAS_EVENTS; > > imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > > > - /* Initialize source pad */ > > - imx219->pad.flags = MEDIA_PAD_FL_SOURCE; > > + /* > > + * Initialize the pads. To preserve backward compatibility with > > + * userspace that used the sensor before the introduction of the > > + * internal image pad, the external source pad is numbered 0 and the > > + * internal image pad numbered 1. > > + */ > > + imx219->pads[IMX219_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE; > > + imx219->pads[IMX219_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK > > + | MEDIA_PAD_FL_INTERNAL; > > > > - ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad); > > + ret = media_entity_pads_init(&imx219->sd.entity, > > + ARRAY_SIZE(imx219->pads), imx219->pads); > > if (ret) { > > dev_err(dev, "failed to init entity pads: %d\n", ret); > > goto error_handler_free; > > -- > > Regards, > > > > Laurent Pinchart > > > > >
On Mon, Mar 04, 2024 at 10:33:37AM +0100, Jacopo Mondi wrote: > Hi Laurent > > On Fri, Mar 01, 2024 at 11:32:19PM +0200, Laurent Pinchart wrote: > > Implement the .get_frame_desc() subdev operation to report information > > about streams to the connected CSI-2 receiver. This is required to let > > the CSI-2 receiver driver know about virtual channels and data types for > > each stream. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/i2c/imx219.c | 67 ++++++++++++++++++++++++++++---------- > > 1 file changed, 50 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 52afb821f667..6e0232b6772b 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -23,6 +23,7 @@ > > #include <linux/pm_runtime.h> > > #include <linux/regulator/consumer.h> > > > > +#include <media/mipi-csi2.h> > > #include <media/v4l2-cci.h> > > #include <media/v4l2-ctrls.h> > > #include <media/v4l2-device.h> > > @@ -594,6 +595,24 @@ static void imx219_free_controls(struct imx219 *imx219) > > * Subdev operations > > */ > > > > +static int imx219_format_bpp(u32 code) > > This can be unsigned as the functions does not return errrors I'll change that. > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > +{ > > + switch (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 8; > > + > > + case MEDIA_BUS_FMT_SRGGB10_1X10: > > + case MEDIA_BUS_FMT_SGRBG10_1X10: > > + case MEDIA_BUS_FMT_SGBRG10_1X10: > > + case MEDIA_BUS_FMT_SBGGR10_1X10: > > + default: > > + return 10; > > + } > > +} > > + > > static int imx219_set_framefmt(struct imx219 *imx219, > > struct v4l2_subdev_state *state) > > { > > @@ -605,23 +624,7 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > > > format = v4l2_subdev_state_get_format(state, IMX219_PAD_SOURCE); > > crop = v4l2_subdev_state_get_crop(state, IMX219_PAD_IMAGE); > > - > > - switch (format->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: > > - bpp = 8; > > - break; > > - > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > - default: > > - bpp = 10; > > - break; > > - } > > + bpp = imx219_format_bpp(format->code); > > > > cci_write(imx219->regmap, IMX219_REG_X_ADD_STA_A, > > crop->left - IMX219_PIXEL_ARRAY_LEFT, &ret); > > @@ -1031,6 +1034,35 @@ static int imx219_init_state(struct v4l2_subdev *sd, > > return 0; > > } > > > > +static int imx219_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad, > > + struct v4l2_mbus_frame_desc *fd) > > +{ > > + const struct v4l2_mbus_framefmt *fmt; > > + struct v4l2_subdev_state *state; > > + u32 code; > > + > > + if (pad != IMX219_PAD_SOURCE) > > + return -EINVAL; > > + > > + state = v4l2_subdev_lock_and_get_active_state(sd); > > + fmt = v4l2_subdev_state_get_stream_format(state, IMX219_PAD_SOURCE, 0); > > + code = fmt->code; > > + v4l2_subdev_unlock_state(state); > > + > > + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2; > > + fd->num_entries = 1; > > + > > + memset(fd->entry, 0, sizeof(fd->entry)); > > + > > + fd->entry[0].pixelcode = code; > > + fd->entry[0].stream = 0; > > + fd->entry[0].bus.csi2.vc = 0; > > + fd->entry[0].bus.csi2.dt = imx219_format_bpp(code) == 8 > > + ? MIPI_CSI2_DT_RAW8 : MIPI_CSI2_DT_RAW10; > > + > > + return 0; > > +} > > + > > static const struct v4l2_subdev_core_ops imx219_core_ops = { > > .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > @@ -1046,6 +1078,7 @@ static const struct v4l2_subdev_pad_ops imx219_pad_ops = { > > .set_fmt = imx219_set_pad_format, > > .get_selection = imx219_get_selection, > > .enum_frame_size = imx219_enum_frame_size, > > + .get_frame_desc = imx219_get_frame_desc, > > }; > > > > static const struct v4l2_subdev_ops imx219_subdev_ops = {
Hi Jacopo, On Mon, Mar 04, 2024 at 06:12:21PM +0100, Jacopo Mondi wrote: > On Fri, Mar 01, 2024 at 11:32:24PM +0200, Laurent Pinchart wrote: > > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Add a driver for the Unicam camera receiver block on BCM283x processors. > > It is represented as two video device nodes: unicam-image and > > unicam-embedded which are connected to an internal subdev (named > > unicam-subdev) in order to manage streams routing. > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > Co-developed-by: Naushir Patuck <naush@raspberrypi.com> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Changes since v5: > > > > - Move to drivers/media/platform/broadcom/ > > - Port to the upstream V4L2 streams API > > - Rebase on latest metadata API proposal > > - Add missing error message > > - Drop unneeded documentation block for unicam_isr() > > - Drop unneeded dev_dbg() and dev_err() messages > > - Drop unneeded streams_mask and fmt checks > > - Drop unused unicam_sd_pad_is_sink() > > - Drop unneeded includes > > - Drop v4l2_ctrl_subscribe_event() call > > - Use pm_runtime_resume_and_get() > > - Indentation and line wrap fixes > > - Let the framework set bus_info > > - Use v4l2_fwnode_endpoint_parse() > > - Fix media device cleanup > > - Drop lane reordering checks > > - Fix subdev state locking > > - Drop extra debug messages > > - Move clock handling to runtime PM handlers > > - Reorder functions > > - Rename init functions for more clarity > > - Initialize runtime PM earlier > > - Clarify error messages > > - Simplify subdev init with local variable > > - Fix subdev cleanup > > - Fix typos and indentation > > - Don't initialize local variables needlessly > > - Simplify num lanes check > > - Fix metadata handling in subdev set_fmt > > - Drop manual fallback to .s_stream() > > - Pass v4l2_pix_format to unicam_calc_format_size_bpl() > > - Simplify unicam_set_default_format() > > - Fix default format settings > > - Add busy check in unicam_s_fmt_meta() > > - Add missing \n at end of format strings > > - Fix metadata handling in subdev set_fmt > > - Fix locking when starting streaming > > - Return buffers from start streaming fails > > - Fix format validation for metadata node > > - Use video_device_pipeline_{start,stop}() helpers > > - Simplify format enumeration > > - Drop unset variable > > - Update MAINTAINERS entry > > - Update to the upstream v4l2_async_nf API > > - Update to the latest subdev routing API > > - Update to the latest subdev state API > > - Move from subdev .init_cfg() to .init_state() > > - Update to the latest videobuf2 API > > - Fix v4l2_subdev_enable_streams() error check > > - Use correct pad for the connected subdev > > - Return buffers to vb2 when start streaming fails > > - Improve debugging in start streaming handler > > - Simplify DMA address management > > - Drop comment about bcm2835-camera driver > > - Clarify comments that explain min/max sizes > > - Pass v4l2_pix_format to unicam_try_fmt() > > - Drop unneeded local variables > > - Rename image-related constants and functions > > - Turn unicam_fmt.metadata_fmt into bool > > - Rename unicam_fmt to unicam_format_info > > - Rename unicam_format_info variables to fmtinfo > > - Rename unicam_node.v_fmt to fmt > > - Add metadata formats for RAW10, RAW12 and RAW14 > > - Make metadata formats line-based > > - Validate format on metadata video device > > - Add Co-devlopped-by tags > > > > Changes since v3: > > > > - Add the vendor prefix for DT name > > - Use the reg-names in DT parsing > > - Remove MAINTAINERS entry > > > > Changes since v2: > > > > - Change code organization > > - Remove unused variables > > - Correct the fmt_meta functions > > - Rewrite the start/stop streaming > > - You can now start the image node alone, but not the metadata one > > - The buffers are allocated per-node > > - only the required stream is started, if the route exists and is > > enabled > > - Prefix the macros with UNICAM_ to not have too generic names > > - Drop colorspace support > > > > Changes since v1: > > > > - Replace the unicam_{info,debug,error} macros with dev_*() > > --- > > MAINTAINERS | 1 + > > drivers/media/platform/Kconfig | 1 + > > drivers/media/platform/Makefile | 1 + > > drivers/media/platform/broadcom/Kconfig | 23 + > > drivers/media/platform/broadcom/Makefile | 3 + > > .../platform/broadcom/bcm2835-unicam-regs.h | 255 ++ > > .../media/platform/broadcom/bcm2835-unicam.c | 2607 +++++++++++++++++ > > 7 files changed, 2891 insertions(+) > > create mode 100644 drivers/media/platform/broadcom/Kconfig > > create mode 100644 drivers/media/platform/broadcom/Makefile > > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h > > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c [snip] > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c > > new file mode 100644 > > index 000000000000..716c89b8a217 > > --- /dev/null > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c > > @@ -0,0 +1,2607 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * BCM283x / BCM271x Unicam Capture Driver > > + * > > + * Copyright (C) 2017-2020 - Raspberry Pi (Trading) Ltd. > > + * > > + * Dave Stevenson <dave.stevenson@raspberrypi.com> > > + * > > + * Based on TI am437x driver by > > + * Benoit Parrot <bparrot@ti.com> > > + * Lad, Prabhakar <prabhakar.csengg@gmail.com> > > + * > > + * and TI CAL camera interface driver by > > + * Benoit Parrot <bparrot@ti.com> > > + * > > + * > > + * There are two camera drivers in the kernel for BCM283x - this one and > > + * bcm2835-camera (currently in staging). > > + * > > + * This driver directly controls the Unicam peripheral - there is no > > + * involvement with the VideoCore firmware. Unicam receives CSI-2 or CCP2 data > > + * and writes it into SDRAM. The only potential processing options are to > > + * repack Bayer data into an alternate format, and applying windowing. The > > + * repacking does not shift the data, so can repack V4L2_PIX_FMT_Sxxxx10P to > > + * V4L2_PIX_FMT_Sxxxx10, or V4L2_PIX_FMT_Sxxxx12P to V4L2_PIX_FMT_Sxxxx12, but > > + * not generically up to V4L2_PIX_FMT_Sxxxx16. The driver will add both formats > > + * where the relevant formats are defined, and will automatically configure the > > + * repacking as required. Support for windowing may be added later. > > Does this last paragraph applies ? I see all the supported packed raw > format having an 'unpacked_fourcc' associated (all but the 8 bit ones > ofc) Maybe I'm tired, but I'm not sure to understand why you think it may not apply. > > + * > > + * It should be possible to connect this driver to any sensor with a suitable > > + * output interface and V4L2 subdevice driver. > > + */ > > + > > +#include <linux/clk.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/dma-mapping.h> > > +#include <linux/err.h> > > +#include <linux/interrupt.h> > > +#include <linux/io.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/platform_device.h> > > +#include <linux/pm_runtime.h> > > +#include <linux/slab.h> > > +#include <linux/videodev2.h> > > + > > +#include <media/v4l2-async.h> > > +#include <media/v4l2-common.h> > > +#include <media/v4l2-dev.h> > > +#include <media/v4l2-device.h> > > +#include <media/v4l2-event.h> > > +#include <media/v4l2-ioctl.h> > > +#include <media/v4l2-fwnode.h> > > +#include <media/v4l2-mc.h> > > +#include <media/videobuf2-dma-contig.h> > > + > > +#include "bcm2835-unicam-regs.h" > > + > > +#define UNICAM_MODULE_NAME "unicam" > > + > > +/* > > + * Unicam must request a minimum of 250Mhz from the VPU clock. > > + * Otherwise the input FIFOs overrun and cause image corruption. > > + */ > > +#define UNICAM_MIN_VPU_CLOCK_RATE (250 * 1000 * 1000) > > Shouldn't this be set in DT ? With assigned-clock-rates ? Then the clock would always run at 250MHz (or higher, depending on the rate in the DT), while the driver ensures it runs at at least 250MHz only when the device is being used. > > + > > +/* Unicam has an internal DMA alignment constraint of 16 bytes for each line. */ > > +#define UNICAM_DMA_BPL_ALIGNMENT 16 > > + > > +/* > > + * The image stride is stored in a 16 bit register, and needs to be aligned to > > + * the DMA constraint. As the ISP in the same SoC has a 32 bytes alignment > > + * constraint on its input, set the image stride alignment to 32 bytes here as > > + * well to avoid incompatible configurations. > > + */ > > +#define UNICAM_IMAGE_BPL_ALIGNMENT 32 > > +#define UNICAM_IMAGE_MAX_BPL ((1 << 16) - UNICAM_IMAGE_BPL_ALIGNMENT) > > + > > +/* > > + * Max width is therefore determined by the max stride divided by the number of > > + * bits per pixel. Take 32bpp as a worst case. No imposed limit on the height, > > + * so adopt a square image for want of anything better. > > + */ > > +#define UNICAM_IMAGE_MIN_WIDTH 16 > > +#define UNICAM_IMAGE_MIN_HEIGHT 16 > > +#define UNICAM_IMAGE_MAX_WIDTH (UNICAM_IMAGE_MAX_BPL / 4) > > +#define UNICAM_IMAGE_MAX_HEIGHT UNICAM_IMAGE_MAX_WIDTH > > + > > +/* > > + * There's no intrinsic limits on the width and height for embedded dat. Use > > + * the same maximum values as for the image, to avoid overflows in the image > > + * size computation. > > + */ > > +#define UNICAM_META_MIN_WIDTH 1 > > +#define UNICAM_META_MIN_HEIGHT 1 > > +#define UNICAM_META_MAX_WIDTH UNICAM_IMAGE_MAX_WIDTH > > +#define UNICAM_META_MAX_HEIGHT UNICAM_IMAGE_MAX_HEIGHT > > + > > +/* > > + * Size of the dummy buffer. Can be any size really, but the DMA > > + * allocation works in units of page sizes. > > + */ > > +#define UNICAM_DUMMY_BUF_SIZE PAGE_SIZE > > + > > +#define UNICAM_SD_PAD_SINK 0 > > +#define UNICAM_SD_PAD_SOURCE_IMAGE 1 > > +#define UNICAM_SD_PAD_SOURCE_METADATA 2 > > +#define UNICAM_SD_NUM_PADS (1 + UNICAM_SD_PAD_SOURCE_METADATA) > > How about an enum then ? OK. > > + > > +enum unicam_node_type { > > + UNICAM_IMAGE_NODE, > > + UNICAM_METADATA_NODE, > > + UNICAM_MAX_NODES > > +}; > > + > > +/* > > + * struct unicam_format_info - Unicam media bus format information > > + * @fourcc: V4L2 pixel format FCC identifier. 0 if n/a. > > + * @unpacked_fourcc: V4L2 pixel format FCC identifier if the data is expanded > > + * out to 16bpp. 0 if n/a. > > + * @code: V4L2 media bus format code. > > + * @depth: Bits per pixel as delivered from the source. > > + * @csi_dt: CSI data type. > > + * @metadata_fmt: This format only applies to the metadata pad. > > + */ > > +struct unicam_format_info { > > + u32 fourcc; > > + u32 unpacked_fourcc; > > + u32 code; > > + u8 depth; > > + u8 csi_dt; > > + bool metadata_fmt; > > +}; > > + > > +struct unicam_buffer { > > + struct vb2_v4l2_buffer vb; > > + struct list_head list; > > + dma_addr_t dma_addr; > > + unsigned int size; > > +}; > > + > > +static inline struct unicam_buffer *to_unicam_buffer(struct vb2_buffer *vb) > > +{ > > + return container_of(vb, struct unicam_buffer, vb.vb2_buf); > > +} > > + > > +struct unicam_node { > > + bool registered; > > + bool streaming; > > + unsigned int id; > > + > > + /* Pointer to the current v4l2_buffer */ > > + struct unicam_buffer *cur_frm; > > + /* Pointer to the next v4l2_buffer */ > > + struct unicam_buffer *next_frm; > > + /* video capture */ > > + const struct unicam_format_info *fmtinfo; > > + /* Used to store current pixel format */ > > + struct v4l2_format fmt; > > + /* Buffer queue used in video-buf */ > > + struct vb2_queue buffer_queue; > > + /* Queue of filled frames */ > > + struct list_head dma_queue; > > + /* IRQ lock for DMA queue */ > > + spinlock_t dma_queue_lock; > > + /* lock used to access this structure */ > > + struct mutex lock; > > + /* Identifies video device for this channel */ > > + struct video_device video_dev; > > + /* Pointer to the parent handle */ > > + struct unicam_device *dev; > > + struct media_pad pad; > > + /* > > + * Dummy buffer intended to be used by unicam > > + * if we have no other queued buffers to swap to. > > + */ > > + struct unicam_buffer dummy_buf; > > + void *dummy_buf_cpu_addr; > > +}; > > + > > +struct unicam_device { > > + struct kref kref; > > + > > + /* peripheral base address */ > > + void __iomem *base; > > + /* clock gating base address */ > > + void __iomem *clk_gate_base; > > Is the clock gating part of unicam or should this be expressed as a > clock provide and be referenced in DT through a phandle ? No idea :-) Dave, Naush ? > > + /* lp clock handle */ > > + struct clk *clock; > > + /* vpu clock handle */ > > + struct clk *vpu_clock; > > + /* V4l2 device */ > > + struct v4l2_device v4l2_dev; > > + struct media_device mdev; > > + > > + /* parent device */ > > + struct device *dev; > > + /* subdevice async Notifier */ > > s/Notifier/notifier > > > + struct v4l2_async_notifier notifier; > > + unsigned int sequence; > > + > > + /* Sensor node */ > > + struct { > > + struct v4l2_subdev *subdev; > > + struct media_pad *pad; > > + } sensor; > > + > > + /* Internal subdev */ > > + struct { > > + struct v4l2_subdev sd; > > + struct media_pad pads[UNICAM_SD_NUM_PADS]; > > + bool streaming; > > + } subdev; > > + > > + enum v4l2_mbus_type bus_type; > > + /* > > + * Stores bus.mipi_csi2.flags for CSI2 sensors, or > > + * bus.mipi_csi1.strobe for CCP2. > > + */ > > + unsigned int bus_flags; > > + unsigned int max_data_lanes; > > + unsigned int active_data_lanes; > > + > > + struct media_pipeline pipe; > > + > > + struct unicam_node node[UNICAM_MAX_NODES]; > > +}; > > + > > +static inline struct unicam_device * > > +notifier_to_unicam_device(struct v4l2_async_notifier *notifier) > > +{ > > + return container_of(notifier, struct unicam_device, notifier); > > +} > > + > > +static inline struct unicam_device * > > I thought using inline is disouraged as the compiler knows what to do For this kind of wrapper around container_of() the kernel usually has an explicit inline keyword. I can drop it if needed. > > +sd_to_unicam_device(struct v4l2_subdev *sd) > > +{ > > + return container_of(sd, struct unicam_device, subdev.sd); > > You can also use container_of_const() if you need it There's no const subdev anywhere. > > +} > > + > > +static void unicam_release(struct kref *kref) > > +{ > > + struct unicam_device *unicam = > > + container_of(kref, struct unicam_device, kref); > > + > > + if (unicam->mdev.dev) > > + media_device_cleanup(&unicam->mdev); > > + > > + kfree(unicam); > > +} > > + > > +static struct unicam_device *unicam_get(struct unicam_device *unicam) > > +{ > > + kref_get(&unicam->kref); > > + return unicam; > > +} > > + > > +static void unicam_put(struct unicam_device *unicam) > > +{ > > + kref_put(&unicam->kref, unicam_release); > > +} > > + > > Does this handle a bit more gracefully the pesky lifetime management > issue in v4l2 ? That's supposed to be the idea :-) > > +/* ----------------------------------------------------------------------------- > > + * Misc helper functions > > + */ > > + > > +static inline bool unicam_sd_pad_is_source(u32 pad) > > +{ > > + /* Camera RX has 1 sink pad, and N source pads */ > > + return pad != UNICAM_SD_PAD_SINK; > > +} > > + > > +static inline bool is_metadata_node(struct unicam_node *node) > > +{ > > + return node->video_dev.device_caps & V4L2_CAP_META_CAPTURE; > > +} > > + > > +static inline bool is_image_node(struct unicam_node *node) > > +{ > > + return node->video_dev.device_caps & V4L2_CAP_VIDEO_CAPTURE; > > +} [snip] > > +/* ----------------------------------------------------------------------------- > > + * Videobuf2 queue operations > > + */ > > + > > +static int unicam_queue_setup(struct vb2_queue *vq, > > + unsigned int *nbuffers, > > + unsigned int *nplanes, > > + unsigned int sizes[], > > + struct device *alloc_devs[]) > > +{ > > + struct unicam_node *node = vb2_get_drv_priv(vq); > > + u32 size = is_image_node(node) ? node->fmt.fmt.pix.sizeimage > > + : node->fmt.fmt.meta.buffersize; > > + > > + if (vq->num_buffers + *nbuffers < 3) > > Why 3 ? Are these dummies ? This may be a remnant of old code. Dave, Naush, any comment ? > > + *nbuffers = 3 - vq->num_buffers; > > + > > + if (*nplanes) { > > + if (sizes[0] < size) { > > + dev_dbg(node->dev->dev, "sizes[0] %i < size %u\n", > > + sizes[0], size); > > + return -EINVAL; > > + } > > + size = sizes[0]; > > + } > > + > > + *nplanes = 1; > > + sizes[0] = size; > > + > > + return 0; > > +} > > + > > +static int unicam_buffer_prepare(struct vb2_buffer *vb) > > +{ > > + struct unicam_node *node = vb2_get_drv_priv(vb->vb2_queue); > > + struct unicam_buffer *buf = to_unicam_buffer(vb); > > + u32 size = is_image_node(node) ? node->fmt.fmt.pix.sizeimage > > + : node->fmt.fmt.meta.buffersize; > > + > > + if (vb2_plane_size(vb, 0) < size) { > > + dev_dbg(node->dev->dev, > > + "data will not fit into plane (%lu < %u)\n", > > + vb2_plane_size(vb, 0), size); > > + return -EINVAL; > > + } > > + > > + buf->dma_addr = vb2_dma_contig_plane_dma_addr(&buf->vb.vb2_buf, 0); > > + buf->size = size; > > + > > + vb2_set_plane_payload(&buf->vb.vb2_buf, 0, size); > > + > > + return 0; > > +} > > + > > +static void unicam_return_buffers(struct unicam_node *node, > > + enum vb2_buffer_state state) > > +{ > > + struct unicam_buffer *buf, *tmp; > > + > > + list_for_each_entry_safe(buf, tmp, &node->dma_queue, list) { > > + list_del(&buf->list); > > + vb2_buffer_done(&buf->vb.vb2_buf, state); > > + } > > + > > + if (node->cur_frm) > > + vb2_buffer_done(&node->cur_frm->vb.vb2_buf, > > + state); > > + if (node->next_frm && node->cur_frm != node->next_frm) > > + vb2_buffer_done(&node->next_frm->vb.vb2_buf, > > + state); > > + > > + node->cur_frm = NULL; > > + node->next_frm = NULL; > > +} > > + > > +static int unicam_start_streaming(struct vb2_queue *vq, unsigned int count) > > +{ > > + struct unicam_node *node = vb2_get_drv_priv(vq); > > + struct unicam_device *unicam = node->dev; > > + struct v4l2_subdev_state *state; > > + struct unicam_buffer *buf; > > + unsigned long flags; > > + int ret; > > + u32 pad, stream; > > + u32 remote_pad = is_image_node(node) ? UNICAM_SD_PAD_SOURCE_IMAGE > > + : UNICAM_SD_PAD_SOURCE_METADATA; > > + > > + /* Look for the route for the given pad and stream. */ > > + state = v4l2_subdev_lock_and_get_active_state(&unicam->subdev.sd); > > + ret = v4l2_subdev_routing_find_opposite_end(&state->routing, > > + remote_pad, 0, > > + &pad, &stream); > > + v4l2_subdev_unlock_state(state); > > + > > + if (ret) > > + goto err_return_buffers; > > + > > + dev_dbg(unicam->dev, "Starting stream on %s: %u/%u -> %u/%u (%s)\n", > > + unicam->subdev.sd.name, pad, stream, remote_pad, 0, > > + is_metadata_node(node) ? "metadata" : "image"); > > + > > + /* The metadata node can't be started alone. */ > > + if (is_metadata_node(node)) { > > + if (!unicam->node[UNICAM_IMAGE_NODE].streaming) { > > Does it mean the metadata node has to be started second, or should > this be made a nop and the metadata node gets started once the image > node is started too ? I'm fine with the constraint of having the > metadata node being started second fwiw I think it would be nice to change this indeed. Dave, Naush, any objection ? > > + dev_err(unicam->dev, > > + "Can't start metadata without image\n"); > > + ret = -EINVAL; > > + goto err_return_buffers; > > + } > > + > > + spin_lock_irqsave(&node->dma_queue_lock, flags); > > + buf = list_first_entry(&node->dma_queue, > > + struct unicam_buffer, list); > > + dev_dbg(unicam->dev, "buffer %p\n", buf); > > Is this useful ? Probably not much. I'll drop it. > > + node->cur_frm = buf; > > + node->next_frm = buf; > > + list_del(&buf->list); > > + spin_unlock_irqrestore(&node->dma_queue_lock, flags); > > + > > + unicam_start_metadata(unicam, buf); > > + node->streaming = true; > > + return 0; > > + } > > + > > + ret = pm_runtime_resume_and_get(unicam->dev); > > + if (ret < 0) { > > + dev_err(unicam->dev, "PM runtime resume failed: %d\n", ret); > > + goto err_return_buffers; > > + } > > + > > + ret = video_device_pipeline_start(&node->video_dev, &unicam->pipe); > > + if (ret < 0) { > > + dev_dbg(unicam->dev, "Failed to start media pipeline: %d\n", ret); > > Isn't this an err ? The main cause of failure is a pipeline validation error, triggered by userspace, hence the debug level. > > + goto err_pm_put; > > + } > > + > > + spin_lock_irqsave(&node->dma_queue_lock, flags); > > + buf = list_first_entry(&node->dma_queue, > > + struct unicam_buffer, list); > > + dev_dbg(unicam->dev, "buffer %p\n", buf); > > + node->cur_frm = buf; > > + node->next_frm = buf; > > + list_del(&buf->list); > > + spin_unlock_irqrestore(&node->dma_queue_lock, flags); > > + > > + unicam_start_rx(unicam, buf); > > + > > + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd, remote_pad, BIT(0)); > > A bit confused by the API here, shouldn't we also do this for embedded > data ? Not here, as the two streams go over different pads, but likely above, as part of the change you proposed regarding stream start on the metadata device. I'll wait for Dave and Naush to reply, and I'll rework this function. > > + if (ret < 0) { > > + dev_err(unicam->dev, "stream on failed in subdev\n"); > > + goto error_pipeline; > > + } > > + > > + node->streaming = true; > > + > > + return 0; > > + > > +error_pipeline: > > + video_device_pipeline_stop(&node->video_dev); > > +err_pm_put: > > + pm_runtime_put_sync(unicam->dev); > > +err_return_buffers: > > + unicam_return_buffers(node, VB2_BUF_STATE_QUEUED); > > + return ret; > > +} > > + > > +static void unicam_stop_streaming(struct vb2_queue *vq) > > +{ > > + struct unicam_node *node = vb2_get_drv_priv(vq); > > + struct unicam_device *unicam = node->dev; > > + u32 remote_pad_index = is_image_node(node) ? UNICAM_SD_PAD_SOURCE_IMAGE > > + : UNICAM_SD_PAD_SOURCE_METADATA; > > + > > + node->streaming = false; > > + > > + v4l2_subdev_disable_streams(&unicam->subdev.sd, remote_pad_index, > > + BIT(0)); > > + > > + /* We can stream only with the image node. */ > > + if (is_metadata_node(node)) { > > + /* > > + * Allow the hardware to spin in the dummy buffer. > > + * This is only really needed if the embedded data pad is > > + * disabled before the image pad. > > + */ > > + unicam_wr_dma_addr(node, &node->dummy_buf); > > + goto dequeue_buffers; > > + } > > + > > + unicam_disable(unicam); > > + > > + video_device_pipeline_stop(&node->video_dev); > > + pm_runtime_put(unicam->dev); > > + > > +dequeue_buffers: > > + /* Clear all queued buffers for the node */ > > + unicam_return_buffers(node, VB2_BUF_STATE_ERROR); > > +} > > + > > +static void unicam_buffer_queue(struct vb2_buffer *vb) > > +{ > > + struct unicam_node *node = vb2_get_drv_priv(vb->vb2_queue); > > + struct unicam_buffer *buf = to_unicam_buffer(vb); > > + > > + spin_lock_irq(&node->dma_queue_lock); > > + list_add_tail(&buf->list, &node->dma_queue); > > + spin_unlock_irq(&node->dma_queue_lock); > > +} > > + > > +static const struct vb2_ops unicam_video_qops = { > > + .queue_setup = unicam_queue_setup, > > + .wait_prepare = vb2_ops_wait_prepare, > > + .wait_finish = vb2_ops_wait_finish, > > + .buf_prepare = unicam_buffer_prepare, > > + .start_streaming = unicam_start_streaming, > > + .stop_streaming = unicam_stop_streaming, > > + .buf_queue = unicam_buffer_queue, > > +}; > > + > > +/* ----------------------------------------------------------------------------- > > + * V4L2 video device operations > > + */ > > + > > +static int unicam_querycap(struct file *file, void *priv, > > + struct v4l2_capability *cap) > > +{ > > + strscpy(cap->driver, UNICAM_MODULE_NAME, sizeof(cap->driver)); > > + strscpy(cap->card, UNICAM_MODULE_NAME, sizeof(cap->card)); > > + > > + cap->capabilities |= V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_META_CAPTURE; > > + > > + return 0; > > +} > > + > > +static int unicam_enum_fmt_vid(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + unsigned int index; > > + unsigned int i; > > + > > + for (i = 0, index = 0; i < ARRAY_SIZE(unicam_image_formats); i++) { > > + if (f->mbus_code && unicam_image_formats[i].code != f->mbus_code) > > + continue; > > + > > + if (index == f->index) { > > + f->pixelformat = unicam_image_formats[i].fourcc; > > + return 0; > > + } > > + > > + index++; > > + > > + if (!unicam_image_formats[i].unpacked_fourcc) > > + continue; > > + > > + if (index == f->index) { > > + f->pixelformat = unicam_image_formats[i].unpacked_fourcc; > > + return 0; > > + } > > + > > + index++; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int unicam_g_fmt_vid(struct file *file, void *priv, > > + struct v4l2_format *f) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + > > + *f = node->fmt; > > + > > + return 0; > > +} > > + > > +static const struct unicam_format_info * > > +__unicam_try_fmt_vid(struct unicam_node *node, struct v4l2_pix_format *pix) > > +{ > > + const struct unicam_format_info *fmtinfo; > > + > > + /* > > + * Default to the first format if the requested pixel format code isn't > > + * supported. > > + */ > > + fmtinfo = unicam_find_format_by_fourcc(pix->pixelformat, > > + UNICAM_SD_PAD_SOURCE_IMAGE); > > + if (!fmtinfo) { > > + fmtinfo = &unicam_image_formats[0]; > > + pix->pixelformat = fmtinfo->fourcc; > > + } > > + > > + unicam_calc_image_size_bpl(node->dev, fmtinfo, pix); > > + > > + if (pix->field == V4L2_FIELD_ANY) > > + pix->field = V4L2_FIELD_NONE; > > + > > + return fmtinfo; > > +} > > + > > +static int unicam_try_fmt_vid(struct file *file, void *priv, > > + struct v4l2_format *f) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + > > + __unicam_try_fmt_vid(node, &f->fmt.pix); > > + return 0; > > +} > > + > > +static int unicam_s_fmt_vid(struct file *file, void *priv, > > + struct v4l2_format *f) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + > > + if (vb2_is_busy(&node->buffer_queue)) > > + return -EBUSY; > > + > > + node->fmtinfo = __unicam_try_fmt_vid(node, &f->fmt.pix); > > + node->fmt = *f; > > + > > + return 0; > > +} > > + > > +static int unicam_enum_fmt_meta(struct file *file, void *priv, > > + struct v4l2_fmtdesc *f) > > +{ > > + unsigned int i, index; > > + > > + for (i = 0, index = 0; i < ARRAY_SIZE(unicam_meta_formats); i++) { > > + if (f->mbus_code && unicam_meta_formats[i].code != f->mbus_code) > > Do we want to allow mbus_code filtering for metadata ? There's a > 1-to-1 relationship between mbus codes and pixel formats Is there a reason not to allow it ? I think it's good for the API to be consistent. Generally speaking, a CSI-2 receiver could receive META_CSI2_10 and convert it to the META_8 pixel format. Filtering as a concept thus make sense I think. > > + continue; > > + if (!unicam_meta_formats[i].metadata_fmt) > > + continue; > > How can this be false if we're iterating on unicam_meta_formats[] ? True. I'll drop the metadata_fmt field. > > + > > + if (index == f->index) { > > + f->pixelformat = unicam_meta_formats[i].fourcc; > > + f->type = V4L2_BUF_TYPE_META_CAPTURE; > > + return 0; > > + } > > + index++; > > + } > > + > > + return -EINVAL; > > +} > > + > > +static int unicam_g_fmt_meta(struct file *file, void *priv, > > + struct v4l2_format *f) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + > > + f->fmt.meta = node->fmt.fmt.meta; > > + > > + return 0; > > +} > > + > > +static const struct unicam_format_info * > > +__unicam_try_fmt_meta(struct unicam_node *node, struct v4l2_meta_format *meta) > > +{ > > + const struct unicam_format_info *fmtinfo; > > + > > + /* > > + * Default to the first format if the requested pixel format code isn't > > + * supported. > > + */ > > + fmtinfo = unicam_find_format_by_fourcc(meta->dataformat, > > + UNICAM_SD_PAD_SOURCE_METADATA); > > + if (!fmtinfo) { > > + fmtinfo = &unicam_meta_formats[0]; > > + meta->dataformat = fmtinfo->fourcc; > > + } > > + > > + unicam_calc_meta_size_bpl(node->dev, fmtinfo, meta); > > + > > + return fmtinfo; > > +} > > + > > +static int unicam_try_fmt_meta(struct file *file, void *priv, > > + struct v4l2_format *f) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + > > + __unicam_try_fmt_vid(node, &f->fmt.pix); > > + return 0; > > +} > > + > > +static int unicam_s_fmt_meta(struct file *file, void *priv, > > + struct v4l2_format *f) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + > > + if (vb2_is_busy(&node->buffer_queue)) > > + return -EBUSY; > > + > > + node->fmtinfo = __unicam_try_fmt_meta(node, &f->fmt.meta); > > + node->fmt = *f; > > + > > + return 0; > > +} > > + > > +static int unicam_enum_framesizes(struct file *file, void *fh, > > + struct v4l2_frmsizeenum *fsize) > > +{ > > + struct unicam_node *node = video_drvdata(file); > > + int ret = -EINVAL; > > + > > + if (fsize->index > 0) > > + return ret; > > + > > + if (is_image_node(node)) { > > + if (!unicam_find_format_by_fourcc(fsize->pixel_format, > > + UNICAM_SD_PAD_SOURCE_IMAGE)) > > + return ret; > > + > > + fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE; > > + fsize->stepwise.min_width = UNICAM_IMAGE_MIN_WIDTH; > > + fsize->stepwise.max_width = UNICAM_IMAGE_MAX_WIDTH; > > + fsize->stepwise.step_width = 1; > > + fsize->stepwise.min_height = UNICAM_IMAGE_MIN_HEIGHT; > > + fsize->stepwise.max_height = UNICAM_IMAGE_MAX_HEIGHT; > > + fsize->stepwise.step_height = 1; > > + } else { > > + if (!unicam_find_format_by_fourcc(fsize->pixel_format, > > + UNICAM_SD_PAD_SOURCE_METADATA)) > > + return ret; > > + > > Isn't this V4L2_FRMSIZE_TYPE_STEPWISE as well ? Indeed, I'll fix that. > > + fsize->stepwise.min_width = UNICAM_META_MIN_WIDTH; > > + fsize->stepwise.max_width = UNICAM_META_MAX_WIDTH; > > + fsize->stepwise.step_width = 1; > > + fsize->stepwise.min_height = UNICAM_META_MIN_HEIGHT; > > + fsize->stepwise.max_height = UNICAM_META_MAX_HEIGHT; > > + fsize->stepwise.step_height = 1; > > + } > > + > > + return 0; > > +} [snip]
Hi Florian, On Fri, Mar 15, 2024 at 07:02:38AM -0700, Florian Fainelli wrote: > On 3/1/2024 1:38 PM, Laurent Pinchart wrote: > > The subject line should obviously have read 'PATCH v6', and I should > > have updated Jean-Michel's e-mail address instead of blindly relying on > > get-maintainer.pl. Maybe sending patches on a Friday evening isn't he > > best idea after all. Sorry about that. > > > > On Fri, Mar 01, 2024 at 11:32:15PM +0200, Laurent Pinchart wrote: > >> Hello everybody, > >> > >> This patch series adds a new driver for the BCM2835 (and derivative) > >> CCP2/CSI2 camera interface named Unicam. This IP core is found in the > >> VC4-based Raspberry Pi, namely the Pi Zero, Pi 3 and Pi 4. > >> > >> Camera support for Raspberry Pi 4 currently relies on a downstream > >> Unicam driver that live in the Raspberry Pi kernel tree ([1]). The > >> driver uses the V4L2 API, but works around the lack of features in V4L2 > >> to properly support sensor embedded data. Since the Unicam driver > >> development by Raspberry Pi, some of those features have been merged in > >> the kernel (namely the V4L2 streams API) or are being developed (namely > >> generic metadata formats and subdev internal pads), with patches posted > >> for review on the linux-media mailing list ([2]). > >> > >> This new upstream driver is based on the downstream code, extensively > >> reworked to use the new V4L2 APIs. > >> > >> The series is based on top of a merge of > >> > >> - v7 of the generic metadata and internal pads, rebased on v6.8-rc5 ([3]) > >> - the downstream ISP driver ported to mainline ([4]) > >> > >> For convenience, it can be found in [5]. Note that the ISP driver is > >> getting upstreamed separately. > >> > >> The series starts with five patches that add support for streams and > >> embedded data to the imx219 driver (01/15 to 05/15). Patches 06/15 to > >> 09/15 then add the Unicam driver, with new V4L2 pixel formats (06/15 and > >> 07/15) and DT bindings (08/15) The remaining patches cover DT > >> integration (10/15 to 14/15) with a sample DT overlay for the IMX219 > >> camera module (15/15). > > I am really keen on taking the DTS patches now so you know those are > taken care of, make it to linux-next shortly and then we can focus on > the drivers/media aspects. Stefan, does that work for you? 10/15 and 11/15 should be fine to merge already, although 11/15 depends on the DT bindings in 08/15. Even if the unicam driver needs a respin, I think the bindings are safe to merge already. Do you plan to take them in your tree with the corresponding DTS patches ? For 12/15 (the pinctrl-based I2C multiplexing), I'd really like an ack from Dave. 13/15 is pretty straightforward but depends on 12/15. 14/15 is independent from all the other patches and can safely be merged I think.
Hi Dave, On Tue, Mar 05, 2024 at 02:56:29PM +0000, Dave Stevenson wrote: > On Mon, 4 Mar 2024 at 19:51, Laurent Pinchart wrote: > > On Mon, Mar 04, 2024 at 06:12:21PM +0100, Jacopo Mondi wrote: > > > On Fri, Mar 01, 2024 at 11:32:24PM +0200, Laurent Pinchart wrote: > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > > > > > Add a driver for the Unicam camera receiver block on BCM283x processors. > > > > It is represented as two video device nodes: unicam-image and > > > > unicam-embedded which are connected to an internal subdev (named > > > > unicam-subdev) in order to manage streams routing. > > > > > > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > > Co-developed-by: Naushir Patuck <naush@raspberrypi.com> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > > Co-developed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > Changes since v5: > > > > > > > > - Move to drivers/media/platform/broadcom/ > > > > - Port to the upstream V4L2 streams API > > > > - Rebase on latest metadata API proposal > > > > - Add missing error message > > > > - Drop unneeded documentation block for unicam_isr() > > > > - Drop unneeded dev_dbg() and dev_err() messages > > > > - Drop unneeded streams_mask and fmt checks > > > > - Drop unused unicam_sd_pad_is_sink() > > > > - Drop unneeded includes > > > > - Drop v4l2_ctrl_subscribe_event() call > > > > - Use pm_runtime_resume_and_get() > > > > - Indentation and line wrap fixes > > > > - Let the framework set bus_info > > > > - Use v4l2_fwnode_endpoint_parse() > > > > - Fix media device cleanup > > > > - Drop lane reordering checks > > > > - Fix subdev state locking > > > > - Drop extra debug messages > > > > - Move clock handling to runtime PM handlers > > > > - Reorder functions > > > > - Rename init functions for more clarity > > > > - Initialize runtime PM earlier > > > > - Clarify error messages > > > > - Simplify subdev init with local variable > > > > - Fix subdev cleanup > > > > - Fix typos and indentation > > > > - Don't initialize local variables needlessly > > > > - Simplify num lanes check > > > > - Fix metadata handling in subdev set_fmt > > > > - Drop manual fallback to .s_stream() > > > > - Pass v4l2_pix_format to unicam_calc_format_size_bpl() > > > > - Simplify unicam_set_default_format() > > > > - Fix default format settings > > > > - Add busy check in unicam_s_fmt_meta() > > > > - Add missing \n at end of format strings > > > > - Fix metadata handling in subdev set_fmt > > > > - Fix locking when starting streaming > > > > - Return buffers from start streaming fails > > > > - Fix format validation for metadata node > > > > - Use video_device_pipeline_{start,stop}() helpers > > > > - Simplify format enumeration > > > > - Drop unset variable > > > > - Update MAINTAINERS entry > > > > - Update to the upstream v4l2_async_nf API > > > > - Update to the latest subdev routing API > > > > - Update to the latest subdev state API > > > > - Move from subdev .init_cfg() to .init_state() > > > > - Update to the latest videobuf2 API > > > > - Fix v4l2_subdev_enable_streams() error check > > > > - Use correct pad for the connected subdev > > > > - Return buffers to vb2 when start streaming fails > > > > - Improve debugging in start streaming handler > > > > - Simplify DMA address management > > > > - Drop comment about bcm2835-camera driver > > > > - Clarify comments that explain min/max sizes > > > > - Pass v4l2_pix_format to unicam_try_fmt() > > > > - Drop unneeded local variables > > > > - Rename image-related constants and functions > > > > - Turn unicam_fmt.metadata_fmt into bool > > > > - Rename unicam_fmt to unicam_format_info > > > > - Rename unicam_format_info variables to fmtinfo > > > > - Rename unicam_node.v_fmt to fmt > > > > - Add metadata formats for RAW10, RAW12 and RAW14 > > > > - Make metadata formats line-based > > > > - Validate format on metadata video device > > > > - Add Co-devlopped-by tags > > > > > > > > Changes since v3: > > > > > > > > - Add the vendor prefix for DT name > > > > - Use the reg-names in DT parsing > > > > - Remove MAINTAINERS entry > > > > > > > > Changes since v2: > > > > > > > > - Change code organization > > > > - Remove unused variables > > > > - Correct the fmt_meta functions > > > > - Rewrite the start/stop streaming > > > > - You can now start the image node alone, but not the metadata one > > > > - The buffers are allocated per-node > > > > - only the required stream is started, if the route exists and is > > > > enabled > > > > - Prefix the macros with UNICAM_ to not have too generic names > > > > - Drop colorspace support > > > > > > > > Changes since v1: > > > > > > > > - Replace the unicam_{info,debug,error} macros with dev_*() > > > > --- > > > > MAINTAINERS | 1 + > > > > drivers/media/platform/Kconfig | 1 + > > > > drivers/media/platform/Makefile | 1 + > > > > drivers/media/platform/broadcom/Kconfig | 23 + > > > > drivers/media/platform/broadcom/Makefile | 3 + > > > > .../platform/broadcom/bcm2835-unicam-regs.h | 255 ++ > > > > .../media/platform/broadcom/bcm2835-unicam.c | 2607 +++++++++++++++++ > > > > 7 files changed, 2891 insertions(+) > > > > create mode 100644 drivers/media/platform/broadcom/Kconfig > > > > create mode 100644 drivers/media/platform/broadcom/Makefile > > > > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h > > > > create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c > > > > [snip] > > > > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c > > > > new file mode 100644 > > > > index 000000000000..716c89b8a217 > > > > --- /dev/null > > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c > > > > @@ -0,0 +1,2607 @@ [snip] > > > > +/* ----------------------------------------------------------------------------- > > > > + * Videobuf2 queue operations > > > > + */ > > > > + > > > > +static int unicam_queue_setup(struct vb2_queue *vq, > > > > + unsigned int *nbuffers, > > > > + unsigned int *nplanes, > > > > + unsigned int sizes[], > > > > + struct device *alloc_devs[]) > > > > +{ > > > > + struct unicam_node *node = vb2_get_drv_priv(vq); > > > > + u32 size = is_image_node(node) ? node->fmt.fmt.pix.sizeimage > > > > + : node->fmt.fmt.meta.buffersize; > > > > + > > > > + if (vq->num_buffers + *nbuffers < 3) > > > > > > Why 3 ? Are these dummies ? > > > > This may be a remnant of old code. Dave, Naush, any comment ? > > I suspect this is legacy. > Originally the driver would only release the buffer at the frame end > when it had a new one to switch to. Naush updated with the dummy > buffer so in theory you can run with 1 buffer, but this min number of > buffers probably didn't get reduced. > Then again it may have been a misunderstanding of the framework, as > struct vb2_queue min_buffers_needed should set the minimum. I'll drop this. > > > > + *nbuffers = 3 - vq->num_buffers; > > > > + > > > > + if (*nplanes) { > > > > + if (sizes[0] < size) { > > > > + dev_dbg(node->dev->dev, "sizes[0] %i < size %u\n", > > > > + sizes[0], size); > > > > + return -EINVAL; > > > > + } > > > > + size = sizes[0]; > > > > + } > > > > + > > > > + *nplanes = 1; > > > > + sizes[0] = size; > > > > + > > > > + return 0; > > > > +} [snip] > > > > +static int unicam_start_streaming(struct vb2_queue *vq, unsigned int count) > > > > +{ > > > > + struct unicam_node *node = vb2_get_drv_priv(vq); > > > > + struct unicam_device *unicam = node->dev; > > > > + struct v4l2_subdev_state *state; > > > > + struct unicam_buffer *buf; > > > > + unsigned long flags; > > > > + int ret; > > > > + u32 pad, stream; > > > > + u32 remote_pad = is_image_node(node) ? UNICAM_SD_PAD_SOURCE_IMAGE > > > > + : UNICAM_SD_PAD_SOURCE_METADATA; > > > > + > > > > + /* Look for the route for the given pad and stream. */ > > > > + state = v4l2_subdev_lock_and_get_active_state(&unicam->subdev.sd); > > > > + ret = v4l2_subdev_routing_find_opposite_end(&state->routing, > > > > + remote_pad, 0, > > > > + &pad, &stream); > > > > + v4l2_subdev_unlock_state(state); > > > > + > > > > + if (ret) > > > > + goto err_return_buffers; > > > > + > > > > + dev_dbg(unicam->dev, "Starting stream on %s: %u/%u -> %u/%u (%s)\n", > > > > + unicam->subdev.sd.name, pad, stream, remote_pad, 0, > > > > + is_metadata_node(node) ? "metadata" : "image"); > > > > + > > > > + /* The metadata node can't be started alone. */ > > > > + if (is_metadata_node(node)) { > > > > + if (!unicam->node[UNICAM_IMAGE_NODE].streaming) { > > > > > > Does it mean the metadata node has to be started second, or should > > > this be made a nop and the metadata node gets started once the image > > > node is started too ? I'm fine with the constraint of having the > > > metadata node being started second fwiw > > > > I think it would be nice to change this indeed. Dave, Naush, any > > objection ? > > See previous email. > > > > > + dev_err(unicam->dev, > > > > + "Can't start metadata without image\n"); > > > > + ret = -EINVAL; > > > > + goto err_return_buffers; > > > > + } > > > > + > > > > + spin_lock_irqsave(&node->dma_queue_lock, flags); > > > > + buf = list_first_entry(&node->dma_queue, > > > > + struct unicam_buffer, list); > > > > + dev_dbg(unicam->dev, "buffer %p\n", buf); > > > > > > Is this useful ? > > > > Probably not much. I'll drop it. > > > > > > + node->cur_frm = buf; > > > > + node->next_frm = buf; > > > > + list_del(&buf->list); > > > > + spin_unlock_irqrestore(&node->dma_queue_lock, flags); > > > > + > > > > + unicam_start_metadata(unicam, buf); > > > > + node->streaming = true; > > > > + return 0; > > > > + } > > > > + > > > > + ret = pm_runtime_resume_and_get(unicam->dev); > > > > + if (ret < 0) { > > > > + dev_err(unicam->dev, "PM runtime resume failed: %d\n", ret); > > > > + goto err_return_buffers; > > > > + } > > > > + > > > > + ret = video_device_pipeline_start(&node->video_dev, &unicam->pipe); > > > > + if (ret < 0) { > > > > + dev_dbg(unicam->dev, "Failed to start media pipeline: %d\n", ret); > > > > > > Isn't this an err ? > > > > The main cause of failure is a pipeline validation error, triggered by > > userspace, hence the debug level. > > > > > > + goto err_pm_put; > > > > + } > > > > + > > > > + spin_lock_irqsave(&node->dma_queue_lock, flags); > > > > + buf = list_first_entry(&node->dma_queue, > > > > + struct unicam_buffer, list); > > > > + dev_dbg(unicam->dev, "buffer %p\n", buf); > > > > + node->cur_frm = buf; > > > > + node->next_frm = buf; > > > > + list_del(&buf->list); > > > > + spin_unlock_irqrestore(&node->dma_queue_lock, flags); > > > > + > > > > + unicam_start_rx(unicam, buf); > > > > + > > > > + ret = v4l2_subdev_enable_streams(&unicam->subdev.sd, remote_pad, BIT(0)); > > > > > > A bit confused by the API here, shouldn't we also do this for embedded > > > data ? > > > > Not here, as the two streams go over different pads, but likely above, > > as part of the change you proposed regarding stream start on the > > metadata device. I'll wait for Dave and Naush to reply, and I'll rework > > this function. > > I haven't read enough on the streams API, or what this implementation > looks like. > > There's no sensible way for a sensor to start embedded or other > metadata without image data, so starting the image node would seem to > be the main trigger for anything. I'm also assuming we don't support > enabling additional multiplexed streams once the pipeline is already > running, so that would seem to determine some of the sequencing. The API allows enabling and disabling streams independently, which is a useful feature for instance when multiple cameras are multiplexed over a single CSI-2 link with virtual channels. For image + embedded data, not only is the sequencing fixed (as you mentioned, the sensor can't send embedded data without image data), but synchronization is also important. > > > > + if (ret < 0) { > > > > + dev_err(unicam->dev, "stream on failed in subdev\n"); > > > > + goto error_pipeline; > > > > + } > > > > + > > > > + node->streaming = true; > > > > + > > > > + return 0; > > > > + > > > > +error_pipeline: > > > > + video_device_pipeline_stop(&node->video_dev); > > > > +err_pm_put: > > > > + pm_runtime_put_sync(unicam->dev); > > > > +err_return_buffers: > > > > + unicam_return_buffers(node, VB2_BUF_STATE_QUEUED); > > > > + return ret; > > > > +} [snip]
Hello everybody, This patch series adds a new driver for the BCM2835 (and derivative) CCP2/CSI2 camera interface named Unicam. This IP core is found in the VC4-based Raspberry Pi, namely the Pi Zero, Pi 3 and Pi 4. Camera support for Raspberry Pi 4 currently relies on a downstream Unicam driver that live in the Raspberry Pi kernel tree ([1]). The driver uses the V4L2 API, but works around the lack of features in V4L2 to properly support sensor embedded data. Since the Unicam driver development by Raspberry Pi, some of those features have been merged in the kernel (namely the V4L2 streams API) or are being developed (namely generic metadata formats and subdev internal pads), with patches posted for review on the linux-media mailing list ([2]). This new upstream driver is based on the downstream code, extensively reworked to use the new V4L2 APIs. The series is based on top of a merge of - v7 of the generic metadata and internal pads, rebased on v6.8-rc5 ([3]) - the downstream ISP driver ported to mainline ([4]) For convenience, it can be found in [5]. Note that the ISP driver is getting upstreamed separately. The series starts with five patches that add support for streams and embedded data to the imx219 driver (01/15 to 05/15). Patches 06/15 to 09/15 then add the Unicam driver, with new V4L2 pixel formats (06/15 and 07/15) and DT bindings (08/15) The remaining patches cover DT integration (10/15 to 14/15) with a sample DT overlay for the IMX219 camera module (15/15). The patches have been tested on a Raspberry Pi 4 using an IMX219 camera module (the Raspberry Pi camera v2), with libcamera. Updates are needed to libcamera to use the new V4L2 APIs, patches have been posted to [6]. For manual testing with media-ctl, corresponding API updates to v4l-utils are available at [7]. The changes to the imx219 driver effectively define the interface that raw sensors should expose to userspace. This needs to be documented explicitly. I would like this series to first get a review round, which will likely raise API questions. I will then work on the documentation. As a summary, more work is needed, but I believe we're nearing completion. This series, along with the libcamera support, help validating the new kernel APIs. We have in my opinion reached a point where we can start converting other sensor drivers from the downstream Raspberry Pi kernel to the standard APIs for embedded data, as well as integrating the APIs in the Raspberry Pi 5 CFE driver. [1] https://github.com/raspberrypi/linux/tree/rpi-6.1.y/drivers/media/platform/bcm2835 [2] https://lore.kernel.org/linux-media/20231106122539.1268265-1-sakari.ailus@linux.intel.com [3] https://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git/log/?h=rpi/v6.8/metadata/v7 [4] https://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git/log/?h=rpi/v6.8/isp/v2 [5] https://git.kernel.org/pub/scm/linux/kernel/git/pinchartl/linux.git/log/?h=rpi/v6.8/unicam/next [6] https://lists.libcamera.org/pipermail/libcamera-devel/2024-March/040711.html [7] https://git.linuxtv.org/pinchartl/v4l-utils.git/log/?h=metadata Here's the mandatory v4l2-compliance report. There are 5 errors, which are caused by v4l2-compliance not supporting the new embedded data APIs yet. This will be fixed and patches for v4l2-compliance will be submitted (alongside patches to media-ctl). v4l2-compliance 1.27.0-5180, 64 bits, 64-bit time_t v4l2-compliance SHA: c14579f7e5f5 2024-03-01 20:29:24 Compliance test for unicam device /dev/media0: Media Driver Info: Driver name : unicam Model : unicam Serial : Bus info : platform:fe801000.csi Media version : 6.8.0 Hardware revision: 0x00000000 (0) Driver version : 6.8.0 Required ioctls: test MEDIA_IOC_DEVICE_INFO: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/media0 open: OK test MEDIA_IOC_DEVICE_INFO: OK test for unlimited opens: OK Media Controller ioctls: test MEDIA_IOC_G_TOPOLOGY: OK Entities: 4 Interfaces: 4 Pads: 8 Links: 7 test MEDIA_IOC_ENUM_ENTITIES/LINKS: OK test MEDIA_IOC_SETUP_LINK: OK Total for unicam device /dev/media0: 8, Succeeded: 8, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for unicam device /dev/video0: Driver Info: Driver name : unicam Card type : unicam Bus info : platform:fe801000.csi Driver version : 6.8.0 Capabilities : 0xa4a00001 Video Capture Metadata Capture I/O MC Streaming Extended Pix Format Device Capabilities Device Caps : 0x24200001 Video Capture I/O MC Streaming Extended Pix Format Media Driver Info: Driver name : unicam Model : unicam Serial : Bus info : platform:fe801000.csi Media version : 6.8.0 Hardware revision: 0x00000000 (0) Driver version : 6.8.0 Interface Info: ID : 0x0300000d Type : V4L Video Entity Info: ID : 0x0000000b (11) Name : unicam-image Function : V4L2 I/O Flags : default Pad 0x0100000c : 0: Sink Link 0x0200000f: from remote pad 0x1000003 of entity 'unicam' (Video Interface Bridge): Data, Enabled, Immutable Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/video0 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK test VIDIOC_TRY_FMT: OK test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test CREATE_BUFS maximum buffers: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Total for unicam device /dev/video0: 47, Succeeded: 47, Failed: 0, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for unicam device /dev/video1: Driver Info: Driver name : unicam Card type : unicam Bus info : platform:fe801000.csi Driver version : 6.8.0 Capabilities : 0xa4a00001 Video Capture Metadata Capture I/O MC Streaming Extended Pix Format Device Capabilities Device Caps : 0x24a00000 Metadata Capture I/O MC Streaming Extended Pix Format Media Driver Info: Driver name : unicam Model : unicam Serial : Bus info : platform:fe801000.csi Media version : 6.8.0 Hardware revision: 0x00000000 (0) Driver version : 6.8.0 Interface Info: ID : 0x03000013 Type : V4L Video Entity Info: ID : 0x00000011 (17) Name : unicam-embedded Function : V4L2 I/O Pad 0x01000012 : 0: Sink Link 0x02000015: from remote pad 0x1000004 of entity 'unicam' (Video Interface Bridge): Data, Enabled, Immutable Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/video1 open: OK test VIDIOC_QUERYCAP: OK test VIDIOC_G/S_PRIORITY: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported) test VIDIOC_LOG_STATUS: OK Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 1 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Control ioctls (Input 0): test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls (Input 0): test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK fail: v4l2-test-formats.cpp(590): dataformat 00003ff8 (x?) for buftype 13 not reported by ENUM_FMT test VIDIOC_TRY_FMT: FAIL test VIDIOC_S_FMT: OK test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls (Input 0): test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls (Input 0): test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK test CREATE_BUFS maximum buffers: OK test VIDIOC_EXPBUF: OK test Requests: OK (Not Supported) Total for unicam device /dev/video1: 47, Succeeded: 46, Failed: 1, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for unicam device /dev/v4l-subdev0: Driver Info: Driver version : 6.8.0 Capabilities : 0x00000002 Streams Support Client Capabilities: 0x0000000000000003 streams interval-uses-which Media Driver Info: Driver name : unicam Model : unicam Serial : Bus info : platform:fe801000.csi Media version : 6.8.0 Hardware revision: 0x00000000 (0) Driver version : 6.8.0 Interface Info: ID : 0x03000017 Type : V4L Sub-Device Entity Info: ID : 0x00000001 (1) Name : unicam Function : Video Interface Bridge Pad 0x01000002 : 0: Sink Link 0x02000009: from remote pad 0x1000006 of entity 'imx219 5-0010' (Camera Sensor): Data, Enabled, Immutable Pad 0x01000003 : 1: Source Link 0x0200000f: to remote pad 0x100000c of entity 'unicam-image' (V4L2 I/O): Data, Enabled, Immutable Pad 0x01000004 : 2: Source Link 0x02000015: to remote pad 0x1000012 of entity 'unicam-embedded' (V4L2 I/O): Data, Enabled, Immutable Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev0 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Sub-Device routing ioctls: fail: v4l2-test-subdevs.cpp(631): !(source->flags & MEDIA_PAD_FL_SOURCE) test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL fail: v4l2-test-subdevs.cpp(631): !(source->flags & MEDIA_PAD_FL_SOURCE) test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL Sub-Device ioctls (Sink Pad 0): Sub-Device ioctls (Source Pad 1): Sub-Device ioctls (Source Pad 2): Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported) test VIDIOC_QUERYCTRL: OK (Not Supported) test VIDIOC_G/S_CTRL: OK (Not Supported) test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported) test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported) test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 0 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test CREATE_BUFS maximum buffers: OK test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) Total for unicam device /dev/v4l-subdev0: 47, Succeeded: 45, Failed: 2, Warnings: 0 -------------------------------------------------------------------------------- Compliance test for unicam device /dev/v4l-subdev1: Driver Info: Driver version : 6.8.0 Capabilities : 0x00000002 Streams Support Client Capabilities: 0x0000000000000003 streams interval-uses-which Media Driver Info: Driver name : unicam Model : unicam Serial : Bus info : platform:fe801000.csi Media version : 6.8.0 Hardware revision: 0x00000000 (0) Driver version : 6.8.0 Interface Info: ID : 0x03000019 Type : V4L Sub-Device Entity Info: ID : 0x00000005 (5) Name : imx219 5-0010 Function : Camera Sensor Pad 0x01000006 : 0: Source Link 0x02000009: to remote pad 0x1000002 of entity 'unicam' (Video Interface Bridge): Data, Enabled, Immutable Pad 0x01000007 : 1: Sink, 00000008 Pad 0x01000008 : 2: Sink, 00000008 Required ioctls: test MC information (see 'Media Driver Info' above): OK test VIDIOC_SUDBEV_QUERYCAP: OK test invalid ioctls: OK Allow for multiple opens: test second /dev/v4l-subdev1 open: OK test VIDIOC_SUBDEV_QUERYCAP: OK test for unlimited opens: OK Debug ioctls: test VIDIOC_LOG_STATUS: OK (Not Supported) Input ioctls: test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported) test VIDIOC_ENUMAUDIO: OK (Not Supported) test VIDIOC_G/S/ENUMINPUT: OK (Not Supported) test VIDIOC_G/S_AUDIO: OK (Not Supported) Inputs: 0 Audio Inputs: 0 Tuners: 0 Output ioctls: test VIDIOC_G/S_MODULATOR: OK (Not Supported) test VIDIOC_G/S_FREQUENCY: OK (Not Supported) test VIDIOC_ENUMAUDOUT: OK (Not Supported) test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported) test VIDIOC_G/S_AUDOUT: OK (Not Supported) Outputs: 0 Audio Outputs: 0 Modulators: 0 Input/Output configuration ioctls: test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported) test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported) test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported) test VIDIOC_G/S_EDID: OK (Not Supported) Sub-Device routing ioctls: fail: v4l2-test-subdevs.cpp(630): !(sink->flags & MEDIA_PAD_FL_SINK) test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL fail: v4l2-test-subdevs.cpp(630): !(sink->flags & MEDIA_PAD_FL_SINK) test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: FAIL Sub-Device ioctls (Source Pad 0): Sub-Device ioctls (Sink Pad 1): Sub-Device ioctls (Sink Pad 2): Control ioctls: test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK test VIDIOC_QUERYCTRL: OK test VIDIOC_G/S_CTRL: OK test VIDIOC_G/S/TRY_EXT_CTRLS: OK test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK test VIDIOC_G/S_JPEGCOMP: OK (Not Supported) Standard Controls: 20 Private Controls: 0 Format ioctls: test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported) test VIDIOC_G/S_PARM: OK (Not Supported) test VIDIOC_G_FBUF: OK (Not Supported) test VIDIOC_G_FMT: OK (Not Supported) test VIDIOC_TRY_FMT: OK (Not Supported) test VIDIOC_S_FMT: OK (Not Supported) test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported) test Cropping: OK (Not Supported) test Composing: OK (Not Supported) test Scaling: OK (Not Supported) Codec ioctls: test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported) test VIDIOC_G_ENC_INDEX: OK (Not Supported) test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported) Buffer ioctls: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported) test CREATE_BUFS maximum buffers: OK test VIDIOC_EXPBUF: OK (Not Supported) test Requests: OK (Not Supported) Total for unicam device /dev/v4l-subdev1: 47, Succeeded: 45, Failed: 2, Warnings: 0 Grand Total for unicam device /dev/media0: 196, Succeeded: 191, Failed: 5, Warnings: 0 Dave Stevenson (2): dt-bindings: media: Add bindings for bcm2835-unicam media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface Jean-Michel Hautbois (3): media: v4l: Add V4L2-PIX-FMT-Y12P format media: v4l: Add V4L2-PIX-FMT-Y14P format ARM: dts: bcm2835: Add Unicam CSI nodes Laurent Pinchart (8): media: i2c: imx219: Inline imx219_update_pad_format() in its caller media: i2c: imx219: Add internal image sink pad media: i2c: imx219: Report internal routes to userspace media: i2c: imx219: Report streams using frame descriptors media: i2c: imx219: Add embedded data support ARM: dts: bcm2835-rpi: Move firmware-clocks from bcm2711 to bcm2835 ARM: dts: bcm2711-rpi-4-b: Add CAM1 regulator [DNI] arm64: dts: broadcom: Add overlay for Raspberry Pi 4B IMX219 camera Uwe Kleine-König (2): ARM: dts: bcm2711-rpi: Add pinctrl-based multiplexing for I2C0 ARM: dts: bcm2711-rpi-cm4-io: Add RTC on I2C0 .../bindings/media/brcm,bcm2835-unicam.yaml | 117 + .../media/v4l/pixfmt-yuv-luma.rst | 48 + MAINTAINERS | 7 + .../arm/boot/dts/broadcom/bcm2711-rpi-4-b.dts | 7 + .../boot/dts/broadcom/bcm2711-rpi-cm4-io.dts | 17 + arch/arm/boot/dts/broadcom/bcm2711-rpi.dtsi | 36 +- arch/arm/boot/dts/broadcom/bcm2711.dtsi | 8 + arch/arm/boot/dts/broadcom/bcm2835-rpi.dtsi | 19 + arch/arm/boot/dts/broadcom/bcm283x.dtsi | 24 + arch/arm64/boot/dts/broadcom/Makefile | 2 + .../dts/broadcom/bcm2711-rpi-4-b-imx219.dtso | 65 + drivers/media/i2c/imx219.c | 419 ++- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/broadcom/Kconfig | 23 + drivers/media/platform/broadcom/Makefile | 3 + .../platform/broadcom/bcm2835-unicam-regs.h | 255 ++ .../media/platform/broadcom/bcm2835-unicam.c | 2607 +++++++++++++++++ drivers/media/v4l2-core/v4l2-ioctl.c | 2 + include/uapi/linux/videodev2.h | 2 + 20 files changed, 3588 insertions(+), 75 deletions(-) create mode 100644 Documentation/devicetree/bindings/media/brcm,bcm2835-unicam.yaml create mode 100644 arch/arm64/boot/dts/broadcom/bcm2711-rpi-4-b-imx219.dtso create mode 100644 drivers/media/platform/broadcom/Kconfig create mode 100644 drivers/media/platform/broadcom/Makefile create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam-regs.h create mode 100644 drivers/media/platform/broadcom/bcm2835-unicam.c base-commit: 384a3aa6b48b29b4c71130c028db78484a0f0ab4