Re: [PATCH] imx7-media-csi: csi2 only

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Martin,

On Wed, Mar 17, 2021 at 11:08:57AM +0100, Martin Kepplinger wrote:
> Am Dienstag, dem 16.03.2021 um 20:05 +0200 schrieb Laurent Pinchart:
> > On Tue, Mar 16, 2021 at 12:56:35PM +0100, Martin Kepplinger wrote:
> > > ---
> > > 
> > > hi Laurent,
> > > 
> > > thanks a lot for posting this series!
> > > 
> > > First: I only test imx7-media-csi (csi bridge) because I run it on imx8mq.
> > > overall, I'm very happy with all of this and I get the same image out
> > > of it as I get with the mx6s_capture nxp driver.
> > 
> > That's good news :-)
> > 
> > > one issue I have is with is_csi2, so I post this patch that I need in
> > > order to test. It's obviously no solution, just to describe the issue:
> > > 
> > > I'm not sure why but imx7_csi_pad_link_validate() isn't called in my case
> > > and is_csi2 doesn't get set, so I force it. Would it make sense to> make
> > > a dts property for this?
> > 
> > Some platforms support both parallel and CSI-2 inputs, so we can't
> > hardcode which one is used in DT. I'd advise trying to debug why the
> > function is never called in your case, it's meant to be called with the
> > following call stack
> > 
> > - imx7_csi_pad_link_validate() (through
> > v4l2_subdev_pad_ops.link_validate)
> > - v4l2_subdev_link_validate() (through
> > media_entity_operations.link_validate)
> > - __media_pipeline_start()
> > - imx_media_pipeline_set_stream()
> > - capture_start_streaming()
> > - ...
> 
> possible the 2 issues are related. I have to say that media-ctl (and
> the api) is kind of new to me and this is not strictly related to the
> patchset anymore. The patchset is certainly required for us to have.
> this is just me trying to test it properly...
> 
> 
> I tried to set what I know that my sensor driver sends to mipi:
> 
> media-ctl -V "'csi':0 [fmt:SBGGR10/640x480 colorspace:raw]"
> 
> 
> Device topology
> - entity 1: csi (2 pads, 1 link)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
> 	pad0: Sink
> 		[fmt:SBGGR10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> 	pad1: Source
> 		[fmt:SBGGR10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> 		-> "csi capture":0 [ENABLED,IMMUTABLE]
> 
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
> 	pad0: Sink
> 		<- "csi":1 [ENABLED,IMMUTABLE]
> 
> - entity 10: mxc-mipi-csi2.0 (0 pad, 0 link)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1

The sensor is missing from the media graph, so that's not very likely to
work :-) Has the sensor driver probed correctly ? Also, the
mxc-mipi-csi2.0 entity doesn't have pads, that's not right.

> now the mipi and camera drivers don't seem to be a "media" drivers, but
> is that strictly necessary?
>
> anyways, capture_start_streaming() calls capture_validate_fmt() which
> fails due to the colorspace 0 it finds:
> 
> [   77.754721] capture_start_streaming: starting
> [   77.759758] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format err
> [   77.768150] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format found colorspace 0x0 != 0x1
> [   77.778372] imx7-csi 30a90000.csi1_bridge: capture format not valid
> 
> > > The other thing is that
> > > v4l2-ctl --set-fmt-video=width=1280,height=720,pixelformat=0
> > > doesn't call the sensor drivers' set_fmt() pad function. That means that
> > > only the one mode I hard-code as default will work. instead I just
> > > see this:
> > 
> > That's expected. With a driver based on the media controller API, you
> > have to configure the format on each subdev manually. You can use the
> > media-ctl utility for that. The video node is only used to control the
> > DMA engine, the kernel driver doesn't propagate the configuration to
> > subdevices.
> > 
> > > [  742.977677] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture'
> > > [  742.977702] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack
> > > [  742.977709] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen)
> > > [  742.977714] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi'
> > > [  742.977720] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture'
> > > [  742.977985] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture'
> > > [  742.977992] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack
> > > [  742.977997] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen)
> > > [  742.978002] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi'
> > > [  742.978008] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture'
> > > [  742.978025] mc: media_release: Media Release
> > > 
> > > Does anything come to your mind that would prevent the set_fmt call here?
> > > That's what the (nxp) mipi driver looks like I'm running here:
> > > https://source.puri.sm/martin.kepplinger/linux-next/-/blob/5.12-rc3/librem5__integration_byzantium/drivers/media/platform/imx8/mxc-mipi-csi2_yav.c

-- 
Regards,

Laurent Pinchart