Re: [PATCH v1 1/3] media: ti: cal: Add support for 1X16 mbus formats

Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> · Fri, 24 Feb 2023 11:02:44 +0200

On 23/02/2023 20:03, Jacopo Mondi wrote:
Hi Tomi

On Thu, Feb 23, 2023 at 07:52:48PM +0200, Tomi Valkeinen wrote:
On 23/02/2023 19:10, Jacopo Mondi wrote:
Hi Tomi

On Wed, Feb 22, 2023 at 02:56:28PM +0200, Tomi Valkeinen wrote:
For legacy reasons the CAL driver uses 2X8 mbus formats for the CSI-2
link (e.g. MEDIA_BUS_FMT_YUYV8_2X8). 1X16 formats are more correct and
used by many drivers, so add 1X16 support to CAL.

We keep the 2X8 formats for backward compatibility.

Eh, this is the usual question about what we should consider a
to be a userspace breakage or not.

I presume the reason to maintain 2X8 formats is (assuming the CAL
interface is CSI-2 only, right ?) because sensor drivers that only
support 2X8 formats will then fail to link_validate() if you remove

Yes.

all 2X8 formats here. I'm of the opinion that we should bite the
bullet and move to 1X16. If any driver that used to work with CAL now
breaks, the sensor driver will have to be fixed.

In my humble experience, that's what we did with the ov5640 sensor. We

Yes, you did.

removed the 2X8 formats in CSI-2 mode and some platform driver broke
and then had been fixed (it happened in the same release cycle), win win.

No, CAL is still broken =). OV5640 is the only sensor I have. I just haven't
had time to look at this and fix it (and no one has complained).

Ups, I was thinking at the ST and microchip receivers, I thought CAL
was fixed already :)

See ? win win (almost)

I know it's controversial, let's see what others think.

I'm all for dropping the 2X8 formats, if that's the consensus. It would keep
the driver simpler, as we could keep the 1:1 relationship between pixel
formats and mbus codes.

I'll look at your other comments tomorrow.

And I'll look at your last patch tomorrow if I can get a media graph
dump!

I have attached the txt and dot versions of the graph of my FPDLink 
setup with three cameras (MC mode with streams).

Some clarifications, which may not be obvious:

The current upstream driver supports two distinct modes, non-MC (legacy) 
and MC modes, selectable with the cal_mc_api module parameter. 
Supporting the legacy mode does make the driver rather confusing in some 
areas...

With this series, the non-MC mode is unchanged, but the MC mode will be 
extended to support streams.

The non-MC mode (afaics, I have never much used it) only supports a 
simple setup with a single sensor subdevice connected directly to CAL. 
As we don't have MC, the subdevice is not visible to the userspace, and 
thus the CAL driver does tricks like collects the controls from the 
sensor, and exposes them from CAL's video node.

And this is why we have two sets of ioctl ops in cal-video.c, 
cal_ioctl_legacy_ops and cal_ioctl_mc_ops, as the behavior is quite 
different between the two modes.

Describing this makes me wonder if the non-MC mode could be dropped... 
MC mode has been supported for some years now.

 Tomi
Attachment:
graph.dot

Description: application/msword-template
Media controller API version 6.2.0

Media device information
------------------------
driver          cal
model           CAL
serial          
bus info        platform:489b0000.cal
hw revision     0x40000300
driver version  6.2.0

Device topology
- entity 1: CAMERARX0 (9 pads, 65 links, 0 route)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
	pad0: Sink
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "ds90ub960 4-003d":4 [ENABLED,IMMUTABLE]
	pad1: Source
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAL output 0":0 [ENABLED]
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad2: Source
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAL output 0":0 []
		-> "CAL output 1":0 [ENABLED]
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad3: Source
		[stream:0 fmt:UYVY8_1X16/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 [ENABLED]
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad4: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad5: Source
		[stream:0 fmt:unknown/1936x1 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 [ENABLED]
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad6: Source
		[stream:0 fmt:unknown/1936x1 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 [ENABLED]
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad7: Source
		[stream:0 fmt:unknown/1280x1 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 [ENABLED]
		-> "CAL output 7":0 []
	pad8: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []

- entity 11: CAMERARX1 (9 pads, 64 links, 0 route)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
	pad0: Sink
		[stream:0 fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
	pad1: Source
		[stream:0 fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad2: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad3: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad4: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad5: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad6: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad7: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []
	pad8: Source
		-> "CAL output 0":0 []
		-> "CAL output 1":0 []
		-> "CAL output 2":0 []
		-> "CAL output 3":0 []
		-> "CAL output 4":0 []
		-> "CAL output 5":0 []
		-> "CAL output 6":0 []
		-> "CAL output 7":0 []

- entity 21: ds90ub960 4-003d (6 pads, 4 links, 0 route)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev2
	pad0: Sink
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "ds90ub953 4-0044":1 [ENABLED,IMMUTABLE]
	pad1: Sink
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "ds90ub953 4-0045":1 [ENABLED,IMMUTABLE]
	pad2: Sink
		[stream:0 fmt:UYVY8_1X16/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "ds90ub913a 4-0046":1 [ENABLED,IMMUTABLE]
	pad3: Sink
	pad4: Source
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "CAMERARX0":0 [ENABLED,IMMUTABLE]
	pad5: Source

- entity 30: ds90ub913a 4-0046 (2 pads, 2 links, 0 route)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev3
	pad0: Sink
		[stream:0 fmt:UYVY8_2X8/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "ov10635 7-0030":0 [ENABLED,IMMUTABLE]
	pad1: Source
		[stream:0 fmt:UYVY8_1X16/1280x720 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "ds90ub960 4-003d":2 [ENABLED,IMMUTABLE]

- entity 35: ds90ub953 4-0045 (2 pads, 2 links, 0 route)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev4
	pad0: Sink
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "imx390 6-0021":0 [ENABLED,IMMUTABLE]
	pad1: Source
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "ds90ub960 4-003d":1 [ENABLED,IMMUTABLE]

- entity 40: ds90ub953 4-0044 (2 pads, 2 links, 0 route)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev5
	pad0: Sink
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		<- "imx390 5-0021":0 [ENABLED,IMMUTABLE]
	pad1: Source
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "ds90ub960 4-003d":0 [ENABLED,IMMUTABLE]

- entity 45: imx390 5-0021 (1 pad, 1 link, 0 route)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev6
	pad0: Source
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:smpte170m]
		-> "ds90ub953 4-0044":0 [ENABLED,IMMUTABLE]

- entity 49: imx390 6-0021 (1 pad, 1 link, 0 route)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev7
	pad0: Source
		[stream:0 fmt:SRGGB12_1X12/1936x1100 field:none colorspace:smpte170m]
		-> "ds90ub953 4-0045":0 [ENABLED,IMMUTABLE]

- entity 53: ov10635 7-0030 (1 pad, 1 link, 0 route)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev8
	pad0: Source
		[stream:0 fmt:UYVY8_2X8/1280x720 field:none colorspace:smpte170m]
		-> "ds90ub913a 4-0046":0 [ENABLED,IMMUTABLE]

- entity 57: CAL output 0 (1 pad, 16 links, 0 route)
             type Node subtype V4L flags 0
             device node name /dev/video0
	pad0: Sink
		<- "CAMERARX0":1 [ENABLED]
		<- "CAMERARX0":2 []
		<- "CAMERARX0":3 []
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 []
		<- "CAMERARX0":6 []
		<- "CAMERARX0":7 []
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []

- entity 93: CAL output 1 (1 pad, 16 links, 0 route)
             type Node subtype V4L flags 0
             device node name /dev/video1
	pad0: Sink
		<- "CAMERARX0":1 []
		<- "CAMERARX0":2 [ENABLED]
		<- "CAMERARX0":3 []
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 []
		<- "CAMERARX0":6 []
		<- "CAMERARX0":7 []
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []

- entity 129: CAL output 2 (1 pad, 16 links, 0 route)
              type Node subtype V4L flags 0
              device node name /dev/video2
	pad0: Sink
		<- "CAMERARX0":1 []
		<- "CAMERARX0":2 []
		<- "CAMERARX0":3 [ENABLED]
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 []
		<- "CAMERARX0":6 []
		<- "CAMERARX0":7 []
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []

- entity 165: CAL output 3 (1 pad, 16 links, 0 route)
              type Node subtype V4L flags 0
              device node name /dev/video3
	pad0: Sink
		<- "CAMERARX0":1 []
		<- "CAMERARX0":2 []
		<- "CAMERARX0":3 []
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 []
		<- "CAMERARX0":6 []
		<- "CAMERARX0":7 []
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []

- entity 201: CAL output 4 (1 pad, 16 links, 0 route)
              type Node subtype V4L flags 0
              device node name /dev/video4
	pad0: Sink
		<- "CAMERARX0":1 []
		<- "CAMERARX0":2 []
		<- "CAMERARX0":3 []
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 [ENABLED]
		<- "CAMERARX0":6 []
		<- "CAMERARX0":7 []
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []

- entity 237: CAL output 5 (1 pad, 16 links, 0 route)
              type Node subtype V4L flags 0
              device node name /dev/video5
	pad0: Sink
		<- "CAMERARX0":1 []
		<- "CAMERARX0":2 []
		<- "CAMERARX0":3 []
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 []
		<- "CAMERARX0":6 [ENABLED]
		<- "CAMERARX0":7 []
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []

- entity 273: CAL output 6 (1 pad, 16 links, 0 route)
              type Node subtype V4L flags 0
              device node name /dev/video6
	pad0: Sink
		<- "CAMERARX0":1 []
		<- "CAMERARX0":2 []
		<- "CAMERARX0":3 []
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 []
		<- "CAMERARX0":6 []
		<- "CAMERARX0":7 [ENABLED]
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []

- entity 309: CAL output 7 (1 pad, 16 links, 0 route)
              type Node subtype V4L flags 0
              device node name /dev/video7
	pad0: Sink
		<- "CAMERARX0":1 []
		<- "CAMERARX0":2 []
		<- "CAMERARX0":3 []
		<- "CAMERARX0":4 []
		<- "CAMERARX0":5 []
		<- "CAMERARX0":6 []
		<- "CAMERARX0":7 []
		<- "CAMERARX0":8 []
		<- "CAMERARX1":1 []
		<- "CAMERARX1":2 []
		<- "CAMERARX1":3 []
		<- "CAMERARX1":4 []
		<- "CAMERARX1":5 []
		<- "CAMERARX1":6 []
		<- "CAMERARX1":7 []
		<- "CAMERARX1":8 []