Received-SPF: pass (google.com: domain of linux-kernel+bounces-182176-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 17 May 2024 16:16:04 +0200
From: Tommaso Merciai <tomm.merciai@gmail.com>
To: Julien Massot <julien.massot@collabora.com>
Cc: linux-media@vger.kernel.org, sakari.ailus@iki.fi,
	devicetree@vger.kernel.org, kernel@collabora.com,
	linux-kernel@vger.kernel.org, mchehab@kernel.org,
	robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	conor+dt@kernel.org, mhecht73@gmail.com, martin.hecht@avnet.eu
Subject: Re: [PATCH v7 0/5] Add support for MAX96714/F and MAX96717/F GMSL2
 ser/des
Message-ID: <ZkdmpED/nXUQ4J3r@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
References: <20240430131931.166012-1-julien.massot@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240430131931.166012-1-julien.massot@collabora.com>

Hi Julien,

On Tue, Apr 30, 2024 at 03:19:26PM +0200, Julien Massot wrote:
> Change since v6:
>   - Remove mention of C-PHY for MAX96717, this serializer is D-PHY only
>   - Remove bus-type requirement for MAX96717
>   - Minor changes requested by Sakari
>   - Workaround a MAX96717 issue, which occurs when stopping
>     the CSI source before stopping the MAX96717 CSI receiver.
> 
> Power management is not included in this patchset. The GMSL link is
> not always resuming when the deserializer is suspended without
> suspending the serializer.
> 
> Change since v5:
>  - Reverse fallback logic: max9671{4,7} can fallback to max9671{4,7}F
>  - use const instead of enum for max9671{4,7}f compatible as suggested
> 
> Change since v4:
>  - Add support for MAX96717 and MAX96714 and use them as a fallback for
>    MAX96717F and MAX96714F respectively
>  - The drivers are now compatible with MAX96717 and MAX96714 since no change in
>    the logic is needed
>  - Reference 'i2c-gate' instead of 'i2c-controller' in the bindings
> 
> Change since v3:
> - bindings
>   - Renamed bindings to drop the 'f' suffix
>   - Add bus type to MAX96717 and remove from MAX9674
>   - Add lane-polarities to both bindings
> 
> - drivers
>   - Address changes requested by Sakari in v3
>   - use v4l2_subdev_s_stream_helper for MAX96714
>   - do not init regmap twice in the MAX96714 driver
>   - Fix compilations on 32 bits platforms
> 
> Change since v2:
> - Convert drivers to use CCI helpers
> - Use generic node name
> - Use 'powerdown' as gpio name instead of 'enable'
> - Add pattern generator support for MAX96714
> 
> These patches add support for Maxim MAX96714F deserializer and
> MAX96717F serializer.
> 
> MAX96714F has one GMSL2 input port and one CSI2 4 lanes output port,
> MAX96717F has one CSI2 input port and one GMSL2 output port.
> 
> The drivers support the tunnel mode where all the
> CSI2 traffic coming from an imager is replicated through the deserializer
> output port.
> 
> Both MAX96714F and MAX96717F are limited to a 3Gbps forward link rate
> leaving a maximum of 2.6Gbps for the video payload.

Thanks for your great work! :)
I test your series with the following hw:

 alvium camera (GM2-319c) -> max96717 (non f variant) -> max96716a -> imx8mp-msc-sm2s-ep2 board

Note:
max96716a is pretty similar to max96714. I change only:

#define MAX96714_DEVICE_ID  0xb6
#define MAX96714F_DEVICE_ID 0xbe

And swapping lanes as you suggest:

	max96714_csi0_out: endpoint {
				data-lanes = <3 4 1 2>;
				link-frequencies = /bits/ 64 <750000000>;
				remote-endpoint = <&mipi_csi_0_in>;
	};

On my current setup csi2 to gmsl2 board is always powered on then, I have
ERRB pin that is triggered at uboot lvl because is not handled.
I think we can andle later this case I can suggest to clear/unclear the ERRB_EN bit of REG5(0x5)
in the very beginning of the probe.

Apart of this
All is working properly on my side and also on Martin side (that is in CC) :)
I'm able to stream properly using:

# SETUP TOPOLOGY LINKS
media-ctl --links "'alvium-csi2 3-003c':0->'max96717 6-0040':0[1]"
media-ctl --links "'max96717 6-0040':1->'max96714 3-0028':0[1]"
media-ctl --links "'max96714 3-0028':1->'csis-32e40000.csi':0[1]"
media-ctl --links "'csis-32e40000.csi':1->'crossbar':0[1]"
media-ctl --links "'crossbar':3->'mxc_isi.0':0[1]"
media-ctl --links "'mxc_isi.0':1->'mxc_isi.0.capture':0[1]"

# SETUP TOPOLOGY ENTITIES
media-ctl -d /dev/media0 --set-v4l2 '"alvium-csi2 3-003c":0[fmt:UYVY8_1X16/1280x800@1/60 field:none]'
media-ctl -d /dev/media0 --set-v4l2 '"max96717 6-0040":0[fmt:UYVY8_1X16/1280x800 field:none]'
media-ctl -d /dev/media0 --set-v4l2 '"max96714 3-0028":0[fmt:UYVY8_1X16/1280x800 field:none]'
media-ctl -d /dev/media0 --set-v4l2 '"csis-32e40000.csi":0[fmt:UYVY8_1X16/1280x800 field:none]'
media-ctl -d /dev/media0 --set-v4l2 '"crossbar":0[fmt:UYVY8_1X16/1280x800 field:none]'
media-ctl -d /dev/media0 --set-v4l2 '"mxc_isi.0":0[fmt:UYVY8_1X16/1280x800 field:none]'


gst-launch-1.0 v4l2src  io-mode=dmabuf blocksize=76800 ! video/x-raw,format=YUY2,width=1280,height=800 ! videoconvert ! waylandsink sync=false


# TOPOLOGY

Media controller API version 6.9.0

Media device information
------------------------
driver          mxc-isi
model           FSL Capture Media Device
serial
bus info        platform:32e00000.isi
hw revision     0x0
driver version  6.9.0

Device topology
- entity 1: crossbar (5 pads, 4 links, 0 routes)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
        pad0: Sink
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                <- "csis-32e40000.csi":1 [ENABLED,IMMUTABLE]
        pad1: Sink
                [stream:0 fmt:UYVY8_1X16/1920x1080 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
        pad2: Sink
                <- "mxc_isi.output":0 [ENABLED,IMMUTABLE]
        pad3: Source
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                -> "mxc_isi.0":0 [ENABLED,IMMUTABLE]
        pad4: Source
                [stream:0 fmt:UYVY8_1X16/1920x1080 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
                -> "mxc_isi.1":0 [ENABLED,IMMUTABLE]

- entity 7: mxc_isi.0 (2 pads, 2 links, 0 routes)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev1
        pad0: Sink
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none
                 compose.bounds:(0,0)/1280x800
                 compose:(0,0)/1280x800]
                <- "crossbar":3 [ENABLED,IMMUTABLE]
        pad1: Source
                [stream:0 fmt:YUV8_1X24/1280x800 field:none colorspace:jpeg xfer:srgb ycbcr:601 quantization:full-range
                 crop.bounds:(0,0)/1280x800
                 crop:(0,0)/1280x800]
                -> "mxc_isi.0.capture":0 [ENABLED,IMMUTABLE]

- entity 10: mxc_isi.0.capture (1 pad, 1 link)
             type Node subtype V4L flags 0
             device node name /dev/video0
        pad0: Sink
                <- "mxc_isi.0":1 [ENABLED,IMMUTABLE]

- entity 18: mxc_isi.1 (2 pads, 2 links, 0 routes)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev2
        pad0: Sink
                [stream:0 fmt:UYVY8_1X16/1920x1080 field:none colorspace:jpeg xfer:srgb ycbcr:601 quantization:full-range
                 compose.bounds:(0,0)/1920x1080
                 compose:(0,0)/1920x1080]
                <- "crossbar":4 [ENABLED,IMMUTABLE]
        pad1: Source
                [stream:0 fmt:YUV8_1X24/1920x1080 field:none colorspace:jpeg xfer:srgb ycbcr:601 quantization:full-range
                 crop.bounds:(0,0)/1920x1080
                 crop:(0,0)/1920x1080]
                -> "mxc_isi.1.capture":0 [ENABLED,IMMUTABLE]

- entity 21: mxc_isi.1.capture (1 pad, 1 link)
             type Node subtype V4L flags 0
             device node name /dev/video1
        pad0: Sink
                <- "mxc_isi.1":1 [ENABLED,IMMUTABLE]

- entity 29: mxc_isi.output (1 pad, 1 link)
             type Node subtype V4L flags 0
        pad0: Source
                -> "crossbar":2 [ENABLED,IMMUTABLE]

- entity 36: csis-32e40000.csi (2 pads, 2 links, 0 routes)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev3
        pad0: Sink
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                <- "max96714 3-0028":1 [ENABLED]
        pad1: Source
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                -> "crossbar":0 [ENABLED,IMMUTABLE]

- entity 41: max96714 3-0028 (2 pads, 2 links, 0 routes)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev4
        pad0: Sink
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                <- "max96717 6-0040":1 [ENABLED,IMMUTABLE]
        pad1: Source
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                -> "csis-32e40000.csi":0 [ENABLED]

- entity 46: max96717 6-0040 (2 pads, 2 links, 0 routes)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev5
        pad0: Sink
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                <- "alvium-csi2 3-003c":0 [ENABLED,IMMUTABLE]
        pad1: Source
                [stream:0 fmt:UYVY8_1X16/1280x800 field:none]
                -> "max96714 3-0028":0 [ENABLED,IMMUTABLE]

- entity 51: alvium-csi2 3-003c (1 pad, 1 link, 0 routes)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev6
        pad0: Source
                [stream:0 fmt:UYVY8_1X16/1280x800@1/60 field:none
                 crop.bounds:(0,0)/2064x1544
                 crop:(0,0)/640x480]
                -> "max96717 6-0040":0 [ENABLED,IMMUTABLE]

All this test was done on top of linux-media tree 6.9.0-rc2.
Here my tags for all the series:

Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
Tested-by: Tommaso Merciai <tomm.merciai@gmail.com>

Thanks again for your great work.
Hope this series will be merged soon :)

Regards,
Tommaso

> 
> Julien Massot (9):
>   dt-bindings: media: add Maxim MAX96717 GMSL2 Serializer
>   dt-bindings: media: add Maxim MAX96714 GMSL2 Deserializer
>   media: i2c: add MAX96717 driver
>   media: i2c: add MAX96714 driver
>   drivers: media: max96717: stop the csi receiver before the source
> 
>  .../bindings/media/i2c/maxim,max96714.yaml    |  174 +++
>  .../bindings/media/i2c/maxim,max96717.yaml    |  157 +++
>  MAINTAINERS                                   |   14 +
>  drivers/media/i2c/Kconfig                     |   34 +
>  drivers/media/i2c/Makefile                    |    2 +
>  drivers/media/i2c/max96714.c                  | 1024 +++++++++++++++++
>  drivers/media/i2c/max96717.c                  |  927 +++++++++++++++
>  7 files changed, 2332 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max96714.yaml
>  create mode 100644 Documentation/devicetree/bindings/media/i2c/maxim,max96717.yaml
>  create mode 100644 drivers/media/i2c/max96714.c
>  create mode 100644 drivers/media/i2c/max96717.c
> 
> -- 
> 2.44.0
> 
>