[PATCH v1 0/3] media: ti: cal: Streams support

[PATCH v1 0/3] media: ti: cal: Streams support

[Tomi Valkeinen]

Hi,

This series adds multiplexed streams support to TI's CAL driver. The
first two patches are somewhat unrelated, and could be merged separately
if the streams support needs lots of more work.

 Tomi

Tomi Valkeinen (3):
  media: ti: cal: Add support for 1X16 mbus formats
  media: ti: cal: Use subdev state
  media: ti: cal: add multiplexed streams support

 drivers/media/platform/ti/cal/cal-camerarx.c | 327 ++++++++++++-------
 drivers/media/platform/ti/cal/cal-video.c    | 181 ++++++++--
 drivers/media/platform/ti/cal/cal.c          |  90 +++--
 drivers/media/platform/ti/cal/cal.h          |  13 +-
 4 files changed, 419 insertions(+), 192 deletions(-)

-- 
2.34.1

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

[Tomi Valkeinen]

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.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal-video.c | 16 ++++++++++++++-
 drivers/media/platform/ti/cal/cal.c       | 25 +++++++++++++++++++++++
 drivers/media/platform/ti/cal/cal.h       |  1 +
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
index 4eade409d5d3..87db8761d94d 100644
--- a/drivers/media/platform/ti/cal/cal-video.c
+++ b/drivers/media/platform/ti/cal/cal-video.c
@@ -446,6 +446,15 @@ static int cal_mc_enum_fmt_vid_cap(struct file *file, void  *priv,
 		if (f->mbus_code && cal_formats[i].code != f->mbus_code)
 			continue;
 
+		/*
+		 * Skip legacy formats so that we don't return duplicate fourccs,
+		 * except in the case that the user explicitly asked for an
+		 * mbus_code which matches this legacy format.
+		 */
+		if (cal_formats[i].legacy &&
+		    (!f->mbus_code || cal_formats[i].code != f->mbus_code))
+			continue;
+
 		if (idx == f->index) {
 			f->pixelformat = cal_formats[i].fourcc;
 			f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
@@ -683,6 +692,7 @@ static void cal_release_buffers(struct cal_ctx *ctx,
 
 static int cal_video_check_format(struct cal_ctx *ctx)
 {
+	const struct cal_format_info *rx_fmtinfo;
 	const struct v4l2_mbus_framefmt *format;
 	struct media_pad *remote_pad;
 
@@ -692,7 +702,11 @@ static int cal_video_check_format(struct cal_ctx *ctx)
 
 	format = &ctx->phy->formats[remote_pad->index];
 
-	if (ctx->fmtinfo->code != format->code ||
+	rx_fmtinfo = cal_format_by_code(format->code);
+	if (!rx_fmtinfo)
+		return -EINVAL;
+
+	if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc ||
 	    ctx->v_fmt.fmt.pix.height != format->height ||
 	    ctx->v_fmt.fmt.pix.width != format->width ||
 	    ctx->v_fmt.fmt.pix.field != format->field)
diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
index 1236215ec70e..053bf1030af0 100644
--- a/drivers/media/platform/ti/cal/cal.c
+++ b/drivers/media/platform/ti/cal/cal.c
@@ -59,22 +59,47 @@ MODULE_PARM_DESC(mc_api, "activates the MC API");
  */
 
 const struct cal_format_info cal_formats[] = {
+	/*
+	 * 2X8 entries are legacy codes. All new drivers should use 1X16 modes.
+	 * The 2X8 modes must be before 1X16 in this list so that the driver
+	 * behavior for non-MC mode doesn't change.
+	 */
 	{
 		.fourcc		= V4L2_PIX_FMT_YUYV,
 		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
 		.bpp		= 16,
+		.legacy		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_UYVY,
 		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
 		.bpp		= 16,
+		.legacy		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_YVYU,
 		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
 		.bpp		= 16,
+		.legacy		= true,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_VYUY,
 		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
 		.bpp		= 16,
+		.legacy		= true,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_YUYV,
+		.code		= MEDIA_BUS_FMT_YUYV8_1X16,
+		.bpp		= 16,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_UYVY,
+		.code		= MEDIA_BUS_FMT_UYVY8_1X16,
+		.bpp		= 16,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_YVYU,
+		.code		= MEDIA_BUS_FMT_YVYU8_1X16,
+		.bpp		= 16,
+	}, {
+		.fourcc		= V4L2_PIX_FMT_VYUY,
+		.code		= MEDIA_BUS_FMT_VYUY8_1X16,
+		.bpp		= 16,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
 		.code		= MEDIA_BUS_FMT_RGB565_2X8_LE,
diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
index de73d6d21b6f..44ecae8a273a 100644
--- a/drivers/media/platform/ti/cal/cal.h
+++ b/drivers/media/platform/ti/cal/cal.h
@@ -89,6 +89,7 @@ struct cal_format_info {
 	/* Bits per pixel */
 	u8	bpp;
 	bool	meta;
+	bool	legacy;
 };
 
 /* buffer for one video frame */
-- 
2.34.1

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

[Jacopo Mondi]

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

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

>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/ti/cal/cal-video.c | 16 ++++++++++++++-
>  drivers/media/platform/ti/cal/cal.c       | 25 +++++++++++++++++++++++
>  drivers/media/platform/ti/cal/cal.h       |  1 +
>  3 files changed, 41 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
> index 4eade409d5d3..87db8761d94d 100644
> --- a/drivers/media/platform/ti/cal/cal-video.c
> +++ b/drivers/media/platform/ti/cal/cal-video.c
> @@ -446,6 +446,15 @@ static int cal_mc_enum_fmt_vid_cap(struct file *file, void  *priv,
>  		if (f->mbus_code && cal_formats[i].code != f->mbus_code)
>  			continue;
>
> +		/*
> +		 * Skip legacy formats so that we don't return duplicate fourccs,
> +		 * except in the case that the user explicitly asked for an
> +		 * mbus_code which matches this legacy format.
> +		 */
> +		if (cal_formats[i].legacy &&
> +		    (!f->mbus_code || cal_formats[i].code != f->mbus_code))

This only works as there are no duplicate codes in cal_formats[] (iow
a pixel format is assigned to a single media bus code). If that's how
the CAL interface works (no colorspace conversion, no components
re-ordering) I guess it's fine

> +			continue;
> +
>  		if (idx == f->index) {
>  			f->pixelformat = cal_formats[i].fourcc;
>  			f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> @@ -683,6 +692,7 @@ static void cal_release_buffers(struct cal_ctx *ctx,
>
>  static int cal_video_check_format(struct cal_ctx *ctx)
>  {
> +	const struct cal_format_info *rx_fmtinfo;
>  	const struct v4l2_mbus_framefmt *format;
>  	struct media_pad *remote_pad;
>
> @@ -692,7 +702,11 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>
>  	format = &ctx->phy->formats[remote_pad->index];
>
> -	if (ctx->fmtinfo->code != format->code ||
> +	rx_fmtinfo = cal_format_by_code(format->code);
> +	if (!rx_fmtinfo)
> +		return -EINVAL;
> +
> +	if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc ||

Is this meant to make 2X8 and 1X16 formats compatible during link
validation ?

>  	    ctx->v_fmt.fmt.pix.height != format->height ||
>  	    ctx->v_fmt.fmt.pix.width != format->width ||
>  	    ctx->v_fmt.fmt.pix.field != format->field)
> diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
> index 1236215ec70e..053bf1030af0 100644
> --- a/drivers/media/platform/ti/cal/cal.c
> +++ b/drivers/media/platform/ti/cal/cal.c
> @@ -59,22 +59,47 @@ MODULE_PARM_DESC(mc_api, "activates the MC API");
>   */
>
>  const struct cal_format_info cal_formats[] = {
> +	/*
> +	 * 2X8 entries are legacy codes. All new drivers should use 1X16 modes.
> +	 * The 2X8 modes must be before 1X16 in this list so that the driver
> +	 * behavior for non-MC mode doesn't change.
> +	 */
>  	{
>  		.fourcc		= V4L2_PIX_FMT_YUYV,
>  		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
>  		.bpp		= 16,
> +		.legacy		= true,

I then wonder if it isn't better to associate multiple mbus codes to
a fourcc instead of duplicating the entries with the same fourcc


>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_UYVY,
>  		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
>  		.bpp		= 16,
> +		.legacy		= true,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_YVYU,
>  		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
>  		.bpp		= 16,
> +		.legacy		= true,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_VYUY,
>  		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
>  		.bpp		= 16,
> +		.legacy		= true,
> +	}, {
> +		.fourcc		= V4L2_PIX_FMT_YUYV,
> +		.code		= MEDIA_BUS_FMT_YUYV8_1X16,
> +		.bpp		= 16,
> +	}, {
> +		.fourcc		= V4L2_PIX_FMT_UYVY,
> +		.code		= MEDIA_BUS_FMT_UYVY8_1X16,
> +		.bpp		= 16,
> +	}, {
> +		.fourcc		= V4L2_PIX_FMT_YVYU,
> +		.code		= MEDIA_BUS_FMT_YVYU8_1X16,
> +		.bpp		= 16,
> +	}, {
> +		.fourcc		= V4L2_PIX_FMT_VYUY,
> +		.code		= MEDIA_BUS_FMT_VYUY8_1X16,
> +		.bpp		= 16,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
>  		.code		= MEDIA_BUS_FMT_RGB565_2X8_LE,

Also RGB formats with 2X8 and 2X12 should probably be changed to 1X16
and 1X24 versions..

Thanks
   j

> diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
> index de73d6d21b6f..44ecae8a273a 100644
> --- a/drivers/media/platform/ti/cal/cal.h
> +++ b/drivers/media/platform/ti/cal/cal.h
> @@ -89,6 +89,7 @@ struct cal_format_info {
>  	/* Bits per pixel */
>  	u8	bpp;
>  	bool	meta;
> +	bool	legacy;
>  };
>
>  /* buffer for one video frame */
> --
> 2.34.1
>

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

[Tomi Valkeinen]

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

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

  Tomi

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

[Jacopo Mondi]

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!

>  Tomi
>

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

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3085 bytes --]

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 #2: graph.dot --]
[-- Type: application/msword-template, Size: 8119 bytes --]

[-- Attachment #3: graph.txt --]
[-- Type: text/plain, Size: 12303 bytes --]

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 []

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

[Tomi Valkeinen]

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
> 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
> 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.
> 
> I know it's controversial, let's see what others think.

After thinking about this, I feel we can just drop the 2X8 support from 
CAL. So I'll refresh this patch and just change the 2X8 formats to 1X16.

>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/ti/cal/cal-video.c | 16 ++++++++++++++-
>>   drivers/media/platform/ti/cal/cal.c       | 25 +++++++++++++++++++++++
>>   drivers/media/platform/ti/cal/cal.h       |  1 +
>>   3 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
>> index 4eade409d5d3..87db8761d94d 100644
>> --- a/drivers/media/platform/ti/cal/cal-video.c
>> +++ b/drivers/media/platform/ti/cal/cal-video.c
>> @@ -446,6 +446,15 @@ static int cal_mc_enum_fmt_vid_cap(struct file *file, void  *priv,
>>   		if (f->mbus_code && cal_formats[i].code != f->mbus_code)
>>   			continue;
>>
>> +		/*
>> +		 * Skip legacy formats so that we don't return duplicate fourccs,
>> +		 * except in the case that the user explicitly asked for an
>> +		 * mbus_code which matches this legacy format.
>> +		 */
>> +		if (cal_formats[i].legacy &&
>> +		    (!f->mbus_code || cal_formats[i].code != f->mbus_code))
> 
> This only works as there are no duplicate codes in cal_formats[] (iow
> a pixel format is assigned to a single media bus code). If that's how
> the CAL interface works (no colorspace conversion, no components
> re-ordering) I guess it's fine

No duplicate codes.

>> +			continue;
>> +
>>   		if (idx == f->index) {
>>   			f->pixelformat = cal_formats[i].fourcc;
>>   			f->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> @@ -683,6 +692,7 @@ static void cal_release_buffers(struct cal_ctx *ctx,
>>
>>   static int cal_video_check_format(struct cal_ctx *ctx)
>>   {
>> +	const struct cal_format_info *rx_fmtinfo;
>>   	const struct v4l2_mbus_framefmt *format;
>>   	struct media_pad *remote_pad;
>>
>> @@ -692,7 +702,11 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>>
>>   	format = &ctx->phy->formats[remote_pad->index];
>>
>> -	if (ctx->fmtinfo->code != format->code ||
>> +	rx_fmtinfo = cal_format_by_code(format->code);
>> +	if (!rx_fmtinfo)
>> +		return -EINVAL;
>> +
>> +	if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc ||
> 
> Is this meant to make 2X8 and 1X16 formats compatible during link
> validation ?

There's no link-validation as such here, but CAL driver checks 
internally (the code here) that the video node's and the camerarx 
subdevices formats match.

Here we check that if the video node uses specifies, say, YUYV, then 
both 2X8 and 1X16 codes from the camerarx are accepted.

>>   	    ctx->v_fmt.fmt.pix.height != format->height ||
>>   	    ctx->v_fmt.fmt.pix.width != format->width ||
>>   	    ctx->v_fmt.fmt.pix.field != format->field)
>> diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
>> index 1236215ec70e..053bf1030af0 100644
>> --- a/drivers/media/platform/ti/cal/cal.c
>> +++ b/drivers/media/platform/ti/cal/cal.c
>> @@ -59,22 +59,47 @@ MODULE_PARM_DESC(mc_api, "activates the MC API");
>>    */
>>
>>   const struct cal_format_info cal_formats[] = {
>> +	/*
>> +	 * 2X8 entries are legacy codes. All new drivers should use 1X16 modes.
>> +	 * The 2X8 modes must be before 1X16 in this list so that the driver
>> +	 * behavior for non-MC mode doesn't change.
>> +	 */
>>   	{
>>   		.fourcc		= V4L2_PIX_FMT_YUYV,
>>   		.code		= MEDIA_BUS_FMT_YUYV8_2X8,
>>   		.bpp		= 16,
>> +		.legacy		= true,
> 
> I then wonder if it isn't better to associate multiple mbus codes to
> a fourcc instead of duplicating the entries with the same fourcc

Maybe, but this way the legacy ones are kept separate, and we don't 
"mess up" the other modes with arrays of mbus codes, when no arrays are 
needed.

> 
>>   	}, {
>>   		.fourcc		= V4L2_PIX_FMT_UYVY,
>>   		.code		= MEDIA_BUS_FMT_UYVY8_2X8,
>>   		.bpp		= 16,
>> +		.legacy		= true,
>>   	}, {
>>   		.fourcc		= V4L2_PIX_FMT_YVYU,
>>   		.code		= MEDIA_BUS_FMT_YVYU8_2X8,
>>   		.bpp		= 16,
>> +		.legacy		= true,
>>   	}, {
>>   		.fourcc		= V4L2_PIX_FMT_VYUY,
>>   		.code		= MEDIA_BUS_FMT_VYUY8_2X8,
>>   		.bpp		= 16,
>> +		.legacy		= true,
>> +	}, {
>> +		.fourcc		= V4L2_PIX_FMT_YUYV,
>> +		.code		= MEDIA_BUS_FMT_YUYV8_1X16,
>> +		.bpp		= 16,
>> +	}, {
>> +		.fourcc		= V4L2_PIX_FMT_UYVY,
>> +		.code		= MEDIA_BUS_FMT_UYVY8_1X16,
>> +		.bpp		= 16,
>> +	}, {
>> +		.fourcc		= V4L2_PIX_FMT_YVYU,
>> +		.code		= MEDIA_BUS_FMT_YVYU8_1X16,
>> +		.bpp		= 16,
>> +	}, {
>> +		.fourcc		= V4L2_PIX_FMT_VYUY,
>> +		.code		= MEDIA_BUS_FMT_VYUY8_1X16,
>> +		.bpp		= 16,
>>   	}, {
>>   		.fourcc		= V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
>>   		.code		= MEDIA_BUS_FMT_RGB565_2X8_LE,
> 
> Also RGB formats with 2X8 and 2X12 should probably be changed to 1X16
> and 1X24 versions..

Good point, I'll change those too.

  Tomi

[PATCH v1 2/3] media: ti: cal: Use subdev state

[Tomi Valkeinen]

Change TI CAL driver to use subdev state. No functional changes
(intended).

This allows us to get rid of the 'formats' field, as the formats are
kept in the state, and also the 'mutex' as we already have state locking.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal-camerarx.c | 134 ++++++-------------
 drivers/media/platform/ti/cal/cal-video.c    |  27 +++-
 drivers/media/platform/ti/cal/cal.h          |   8 --
 3 files changed, 60 insertions(+), 109 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
index 16ae52879a79..faafbd0e9240 100644
--- a/drivers/media/platform/ti/cal/cal-camerarx.c
+++ b/drivers/media/platform/ti/cal/cal-camerarx.c
@@ -50,10 +50,16 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
 	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
 	u32 num_lanes = mipi_csi2->num_data_lanes;
 	const struct cal_format_info *fmtinfo;
+	struct v4l2_subdev_state *state;
+	struct v4l2_mbus_framefmt *fmt;
 	u32 bpp;
 	s64 freq;
 
-	fmtinfo = cal_format_by_code(phy->formats[CAL_CAMERARX_PAD_SINK].code);
+	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
+
+	fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
+
+	fmtinfo = cal_format_by_code(fmt->code);
 	if (!fmtinfo)
 		return -EINVAL;
 
@@ -620,34 +626,20 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
 	return container_of(sd, struct cal_camerarx, subdev);
 }
 
-static struct v4l2_mbus_framefmt *
-cal_camerarx_get_pad_format(struct cal_camerarx *phy,
-			    struct v4l2_subdev_state *state,
-			    unsigned int pad, u32 which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_get_try_format(&phy->subdev, state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &phy->formats[pad];
-	default:
-		return NULL;
-	}
-}
-
 static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct cal_camerarx *phy = to_cal_camerarx(sd);
+	struct v4l2_subdev_state *state;
 	int ret = 0;
 
-	mutex_lock(&phy->mutex);
+	state = v4l2_subdev_lock_and_get_active_state(sd);
 
 	if (enable)
 		ret = cal_camerarx_start(phy);
 	else
 		cal_camerarx_stop(phy);
 
-	mutex_unlock(&phy->mutex);
+	v4l2_subdev_unlock_state(state);
 
 	return ret;
 }
@@ -657,62 +649,44 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
 					  struct v4l2_subdev_mbus_code_enum *code)
 {
 	struct cal_camerarx *phy = to_cal_camerarx(sd);
-	int ret = 0;
-
-	mutex_lock(&phy->mutex);
 
 	/* No transcoding, source and sink codes must match. */
 	if (cal_rx_pad_is_source(code->pad)) {
 		struct v4l2_mbus_framefmt *fmt;
 
-		if (code->index > 0) {
-			ret = -EINVAL;
-			goto out;
-		}
+		if (code->index > 0)
+			return -EINVAL;
 
-		fmt = cal_camerarx_get_pad_format(phy, state,
-						  CAL_CAMERARX_PAD_SINK,
-						  code->which);
+		fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
+						 CAL_CAMERARX_PAD_SINK);
 		code->code = fmt->code;
 	} else {
-		if (code->index >= cal_num_formats) {
-			ret = -EINVAL;
-			goto out;
-		}
+		if (code->index >= cal_num_formats)
+			return -EINVAL;
 
 		code->code = cal_formats[code->index].code;
 	}
 
-out:
-	mutex_unlock(&phy->mutex);
-
-	return ret;
+	return 0;
 }
 
 static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
 					   struct v4l2_subdev_state *state,
 					   struct v4l2_subdev_frame_size_enum *fse)
 {
-	struct cal_camerarx *phy = to_cal_camerarx(sd);
 	const struct cal_format_info *fmtinfo;
-	int ret = 0;
 
 	if (fse->index > 0)
 		return -EINVAL;
 
-	mutex_lock(&phy->mutex);
-
 	/* No transcoding, source and sink formats must match. */
 	if (cal_rx_pad_is_source(fse->pad)) {
 		struct v4l2_mbus_framefmt *fmt;
 
-		fmt = cal_camerarx_get_pad_format(phy, state,
-						  CAL_CAMERARX_PAD_SINK,
-						  fse->which);
-		if (fse->code != fmt->code) {
-			ret = -EINVAL;
-			goto out;
-		}
+		fmt = v4l2_subdev_get_pad_format(sd, state,
+						 CAL_CAMERARX_PAD_SINK);
+		if (fse->code != fmt->code)
+			return -EINVAL;
 
 		fse->min_width = fmt->width;
 		fse->max_width = fmt->width;
@@ -720,10 +694,8 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
 		fse->max_height = fmt->height;
 	} else {
 		fmtinfo = cal_format_by_code(fse->code);
-		if (!fmtinfo) {
-			ret = -EINVAL;
-			goto out;
-		}
+		if (!fmtinfo)
+			return -EINVAL;
 
 		fse->min_width = CAL_MIN_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
 		fse->max_width = CAL_MAX_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
@@ -731,27 +703,6 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
 		fse->max_height = CAL_MAX_HEIGHT_LINES;
 	}
 
-out:
-	mutex_unlock(&phy->mutex);
-
-	return ret;
-}
-
-static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
-				   struct v4l2_subdev_state *state,
-				   struct v4l2_subdev_format *format)
-{
-	struct cal_camerarx *phy = to_cal_camerarx(sd);
-	struct v4l2_mbus_framefmt *fmt;
-
-	mutex_lock(&phy->mutex);
-
-	fmt = cal_camerarx_get_pad_format(phy, state, format->pad,
-					  format->which);
-	format->format = *fmt;
-
-	mutex_unlock(&phy->mutex);
-
 	return 0;
 }
 
@@ -759,14 +710,13 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *state,
 				   struct v4l2_subdev_format *format)
 {
-	struct cal_camerarx *phy = to_cal_camerarx(sd);
 	const struct cal_format_info *fmtinfo;
 	struct v4l2_mbus_framefmt *fmt;
 	unsigned int bpp;
 
 	/* No transcoding, source and sink formats must match. */
 	if (cal_rx_pad_is_source(format->pad))
-		return cal_camerarx_sd_get_fmt(sd, state, format);
+		return v4l2_subdev_get_fmt(sd, state, format);
 
 	/*
 	 * Default to the first format if the requested media bus code isn't
@@ -790,20 +740,13 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
 
 	/* Store the format and propagate it to the source pad. */
 
-	mutex_lock(&phy->mutex);
-
-	fmt = cal_camerarx_get_pad_format(phy, state,
-					  CAL_CAMERARX_PAD_SINK,
-					  format->which);
+	fmt = v4l2_subdev_get_pad_format(sd, state, CAL_CAMERARX_PAD_SINK);
 	*fmt = format->format;
 
-	fmt = cal_camerarx_get_pad_format(phy, state,
-					  CAL_CAMERARX_PAD_FIRST_SOURCE,
-					  format->which);
+	fmt = v4l2_subdev_get_pad_format(sd, state,
+					 CAL_CAMERARX_PAD_FIRST_SOURCE);
 	*fmt = format->format;
 
-	mutex_unlock(&phy->mutex);
-
 	return 0;
 }
 
@@ -837,7 +780,7 @@ static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = {
 	.init_cfg = cal_camerarx_sd_init_cfg,
 	.enum_mbus_code = cal_camerarx_sd_enum_mbus_code,
 	.enum_frame_size = cal_camerarx_sd_enum_frame_size,
-	.get_fmt = cal_camerarx_sd_get_fmt,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = cal_camerarx_sd_set_fmt,
 };
 
@@ -872,7 +815,6 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 	phy->instance = instance;
 
 	spin_lock_init(&phy->vc_lock);
-	mutex_init(&phy->mutex);
 
 	phy->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 						(instance == 0) ?
@@ -882,7 +824,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 	if (IS_ERR(phy->base)) {
 		cal_err(cal, "failed to ioremap\n");
 		ret = PTR_ERR(phy->base);
-		goto error;
+		goto err_entity_cleanup;
 	}
 
 	cal_dbg(1, cal, "ioresource %s at %pa - %pa\n",
@@ -890,11 +832,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 
 	ret = cal_camerarx_regmap_init(cal, phy);
 	if (ret)
-		goto error;
+		goto err_entity_cleanup;
 
 	ret = cal_camerarx_parse_dt(phy);
 	if (ret)
-		goto error;
+		goto err_entity_cleanup;
 
 	/* Initialize the V4L2 subdev and media entity. */
 	sd = &phy->subdev;
@@ -911,19 +853,21 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 	ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(phy->pads),
 				     phy->pads);
 	if (ret)
-		goto error;
+		goto err_entity_cleanup;
 
-	ret = cal_camerarx_sd_init_cfg(sd, NULL);
+	ret = v4l2_subdev_init_finalize(sd);
 	if (ret)
-		goto error;
+		goto err_entity_cleanup;
 
 	ret = v4l2_device_register_subdev(&cal->v4l2_dev, sd);
 	if (ret)
-		goto error;
+		goto err_free_state;
 
 	return phy;
 
-error:
+err_free_state:
+	v4l2_subdev_cleanup(sd);
+err_entity_cleanup:
 	media_entity_cleanup(&phy->subdev.entity);
 	kfree(phy);
 	return ERR_PTR(ret);
@@ -935,9 +879,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
 		return;
 
 	v4l2_device_unregister_subdev(&phy->subdev);
+	v4l2_subdev_cleanup(&phy->subdev);
 	media_entity_cleanup(&phy->subdev.entity);
 	of_node_put(phy->source_ep_node);
 	of_node_put(phy->source_node);
-	mutex_destroy(&phy->mutex);
 	kfree(phy);
 }
diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
index 87db8761d94d..d363e123d4bb 100644
--- a/drivers/media/platform/ti/cal/cal-video.c
+++ b/drivers/media/platform/ti/cal/cal-video.c
@@ -694,25 +694,40 @@ static int cal_video_check_format(struct cal_ctx *ctx)
 {
 	const struct cal_format_info *rx_fmtinfo;
 	const struct v4l2_mbus_framefmt *format;
+	struct v4l2_subdev_state *state;
 	struct media_pad *remote_pad;
+	int ret = 0;
 
 	remote_pad = media_pad_remote_pad_first(&ctx->pad);
 	if (!remote_pad)
 		return -ENODEV;
 
-	format = &ctx->phy->formats[remote_pad->index];
+	state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
+
+	format = v4l2_subdev_get_pad_format(&ctx->phy->subdev, state, remote_pad->index);
+	if (!format) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	rx_fmtinfo = cal_format_by_code(format->code);
-	if (!rx_fmtinfo)
-		return -EINVAL;
+	if (!rx_fmtinfo) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc ||
 	    ctx->v_fmt.fmt.pix.height != format->height ||
 	    ctx->v_fmt.fmt.pix.width != format->width ||
-	    ctx->v_fmt.fmt.pix.field != format->field)
-		return -EPIPE;
+	    ctx->v_fmt.fmt.pix.field != format->field) {
+		ret = -EPIPE;
+		goto out;
+	}
 
-	return 0;
+out:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
 }
 
 static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
index 44ecae8a273a..79cd0171e701 100644
--- a/drivers/media/platform/ti/cal/cal.h
+++ b/drivers/media/platform/ti/cal/cal.h
@@ -178,7 +178,6 @@ struct cal_camerarx {
 
 	struct v4l2_subdev	subdev;
 	struct media_pad	pads[CAL_CAMERARX_NUM_PADS];
-	struct v4l2_mbus_framefmt	formats[CAL_CAMERARX_NUM_PADS];
 
 	/* protects the vc_* fields below */
 	spinlock_t		vc_lock;
@@ -186,13 +185,6 @@ struct cal_camerarx {
 	u16			vc_frame_number[4];
 	u32			vc_sequence[4];
 
-	/*
-	 * Lock for camerarx ops. Protects:
-	 * - formats
-	 * - enable_count
-	 */
-	struct mutex		mutex;
-
 	unsigned int		enable_count;
 };
 
-- 
2.34.1

Re: [PATCH v1 2/3] media: ti: cal: Use subdev state

[Jacopo Mondi]

Hi Tomi

On Wed, Feb 22, 2023 at 02:56:29PM +0200, Tomi Valkeinen wrote:
> Change TI CAL driver to use subdev state. No functional changes
> (intended).
>
> This allows us to get rid of the 'formats' field, as the formats are
> kept in the state, and also the 'mutex' as we already have state locking.

I'm not sure I fully get how the ctrl_handler is used here, but
shouldn't the state->lock be set to the ctrl_handler.lock so that
controls and state interaction are guaranteed to be protected ?

>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/ti/cal/cal-camerarx.c | 134 ++++++-------------
>  drivers/media/platform/ti/cal/cal-video.c    |  27 +++-
>  drivers/media/platform/ti/cal/cal.h          |   8 --
>  3 files changed, 60 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> index 16ae52879a79..faafbd0e9240 100644
> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> @@ -50,10 +50,16 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
>  	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
>  	u32 num_lanes = mipi_csi2->num_data_lanes;
>  	const struct cal_format_info *fmtinfo;
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_mbus_framefmt *fmt;
>  	u32 bpp;
>  	s64 freq;
>
> -	fmtinfo = cal_format_by_code(phy->formats[CAL_CAMERARX_PAD_SINK].code);
> +	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> +
> +	fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
> +
> +	fmtinfo = cal_format_by_code(fmt->code);
>  	if (!fmtinfo)
>  		return -EINVAL;
>
> @@ -620,34 +626,20 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
>  	return container_of(sd, struct cal_camerarx, subdev);
>  }
>
> -static struct v4l2_mbus_framefmt *
> -cal_camerarx_get_pad_format(struct cal_camerarx *phy,
> -			    struct v4l2_subdev_state *state,
> -			    unsigned int pad, u32 which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_get_try_format(&phy->subdev, state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &phy->formats[pad];
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct cal_camerarx *phy = to_cal_camerarx(sd);
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
>
> -	mutex_lock(&phy->mutex);
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
>
>  	if (enable)
>  		ret = cal_camerarx_start(phy);
>  	else
>  		cal_camerarx_stop(phy);
>
> -	mutex_unlock(&phy->mutex);
> +	v4l2_subdev_unlock_state(state);
>
>  	return ret;
>  }
> @@ -657,62 +649,44 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>  					  struct v4l2_subdev_mbus_code_enum *code)
>  {
>  	struct cal_camerarx *phy = to_cal_camerarx(sd);
> -	int ret = 0;
> -
> -	mutex_lock(&phy->mutex);
>
>  	/* No transcoding, source and sink codes must match. */
>  	if (cal_rx_pad_is_source(code->pad)) {
>  		struct v4l2_mbus_framefmt *fmt;
>
> -		if (code->index > 0) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> +		if (code->index > 0)
> +			return -EINVAL;
>
> -		fmt = cal_camerarx_get_pad_format(phy, state,
> -						  CAL_CAMERARX_PAD_SINK,
> -						  code->which);
> +		fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
> +						 CAL_CAMERARX_PAD_SINK);

By removing the lock here and in cal_camerarx_sd_set_fmt() don't we
risk that the format on the sink pad is updated while we fetch it
here? I know it's a very tiny window, but should we lock the state
just to be safe ?

>  		code->code = fmt->code;
>  	} else {
> -		if (code->index >= cal_num_formats) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> +		if (code->index >= cal_num_formats)
> +			return -EINVAL;
>
>  		code->code = cal_formats[code->index].code;
>  	}
>
> -out:
> -	mutex_unlock(&phy->mutex);
> -
> -	return ret;
> +	return 0;
>  }
>
>  static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>  					   struct v4l2_subdev_state *state,
>  					   struct v4l2_subdev_frame_size_enum *fse)
>  {
> -	struct cal_camerarx *phy = to_cal_camerarx(sd);
>  	const struct cal_format_info *fmtinfo;
> -	int ret = 0;
>
>  	if (fse->index > 0)
>  		return -EINVAL;
>
> -	mutex_lock(&phy->mutex);

Same question here

> -
>  	/* No transcoding, source and sink formats must match. */
>  	if (cal_rx_pad_is_source(fse->pad)) {
>  		struct v4l2_mbus_framefmt *fmt;
>
> -		fmt = cal_camerarx_get_pad_format(phy, state,
> -						  CAL_CAMERARX_PAD_SINK,
> -						  fse->which);
> -		if (fse->code != fmt->code) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> +		fmt = v4l2_subdev_get_pad_format(sd, state,
> +						 CAL_CAMERARX_PAD_SINK);
> +		if (fse->code != fmt->code)
> +			return -EINVAL;
>
>  		fse->min_width = fmt->width;
>  		fse->max_width = fmt->width;
> @@ -720,10 +694,8 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>  		fse->max_height = fmt->height;
>  	} else {
>  		fmtinfo = cal_format_by_code(fse->code);
> -		if (!fmtinfo) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> +		if (!fmtinfo)
> +			return -EINVAL;
>
>  		fse->min_width = CAL_MIN_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
>  		fse->max_width = CAL_MAX_WIDTH_BYTES * 8 / ALIGN(fmtinfo->bpp, 8);
> @@ -731,27 +703,6 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>  		fse->max_height = CAL_MAX_HEIGHT_LINES;
>  	}
>
> -out:
> -	mutex_unlock(&phy->mutex);
> -
> -	return ret;
> -}
> -
> -static int cal_camerarx_sd_get_fmt(struct v4l2_subdev *sd,
> -				   struct v4l2_subdev_state *state,
> -				   struct v4l2_subdev_format *format)
> -{
> -	struct cal_camerarx *phy = to_cal_camerarx(sd);
> -	struct v4l2_mbus_framefmt *fmt;
> -
> -	mutex_lock(&phy->mutex);
> -
> -	fmt = cal_camerarx_get_pad_format(phy, state, format->pad,
> -					  format->which);
> -	format->format = *fmt;
> -
> -	mutex_unlock(&phy->mutex);
> -
>  	return 0;
>  }
>
> @@ -759,14 +710,13 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_state *state,
>  				   struct v4l2_subdev_format *format)
>  {
> -	struct cal_camerarx *phy = to_cal_camerarx(sd);
>  	const struct cal_format_info *fmtinfo;
>  	struct v4l2_mbus_framefmt *fmt;
>  	unsigned int bpp;
>
>  	/* No transcoding, source and sink formats must match. */
>  	if (cal_rx_pad_is_source(format->pad))
> -		return cal_camerarx_sd_get_fmt(sd, state, format);
> +		return v4l2_subdev_get_fmt(sd, state, format);
>
>  	/*
>  	 * Default to the first format if the requested media bus code isn't
> @@ -790,20 +740,13 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>
>  	/* Store the format and propagate it to the source pad. */
>
> -	mutex_lock(&phy->mutex);
> -
> -	fmt = cal_camerarx_get_pad_format(phy, state,
> -					  CAL_CAMERARX_PAD_SINK,
> -					  format->which);
> +	fmt = v4l2_subdev_get_pad_format(sd, state, CAL_CAMERARX_PAD_SINK);
>  	*fmt = format->format;
>
> -	fmt = cal_camerarx_get_pad_format(phy, state,
> -					  CAL_CAMERARX_PAD_FIRST_SOURCE,
> -					  format->which);
> +	fmt = v4l2_subdev_get_pad_format(sd, state,
> +					 CAL_CAMERARX_PAD_FIRST_SOURCE);
>  	*fmt = format->format;
>
> -	mutex_unlock(&phy->mutex);
> -
>  	return 0;
>  }
>
> @@ -837,7 +780,7 @@ static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = {
>  	.init_cfg = cal_camerarx_sd_init_cfg,
>  	.enum_mbus_code = cal_camerarx_sd_enum_mbus_code,
>  	.enum_frame_size = cal_camerarx_sd_enum_frame_size,
> -	.get_fmt = cal_camerarx_sd_get_fmt,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = cal_camerarx_sd_set_fmt,
>  };
>
> @@ -872,7 +815,6 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  	phy->instance = instance;
>
>  	spin_lock_init(&phy->vc_lock);
> -	mutex_init(&phy->mutex);
>
>  	phy->res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>  						(instance == 0) ?
> @@ -882,7 +824,7 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  	if (IS_ERR(phy->base)) {
>  		cal_err(cal, "failed to ioremap\n");
>  		ret = PTR_ERR(phy->base);
> -		goto error;
> +		goto err_entity_cleanup;
>  	}
>
>  	cal_dbg(1, cal, "ioresource %s at %pa - %pa\n",
> @@ -890,11 +832,11 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>
>  	ret = cal_camerarx_regmap_init(cal, phy);
>  	if (ret)
> -		goto error;
> +		goto err_entity_cleanup;
>
>  	ret = cal_camerarx_parse_dt(phy);
>  	if (ret)
> -		goto error;
> +		goto err_entity_cleanup;
>
>  	/* Initialize the V4L2 subdev and media entity. */
>  	sd = &phy->subdev;
> @@ -911,19 +853,21 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  	ret = media_entity_pads_init(&sd->entity, ARRAY_SIZE(phy->pads),
>  				     phy->pads);
>  	if (ret)
> -		goto error;
> +		goto err_entity_cleanup;
>
> -	ret = cal_camerarx_sd_init_cfg(sd, NULL);
> +	ret = v4l2_subdev_init_finalize(sd);
>  	if (ret)
> -		goto error;
> +		goto err_entity_cleanup;
>
>  	ret = v4l2_device_register_subdev(&cal->v4l2_dev, sd);
>  	if (ret)
> -		goto error;
> +		goto err_free_state;
>
>  	return phy;
>
> -error:
> +err_free_state:
> +	v4l2_subdev_cleanup(sd);
> +err_entity_cleanup:
>  	media_entity_cleanup(&phy->subdev.entity);
>  	kfree(phy);
>  	return ERR_PTR(ret);
> @@ -935,9 +879,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
>  		return;
>
>  	v4l2_device_unregister_subdev(&phy->subdev);
> +	v4l2_subdev_cleanup(&phy->subdev);
>  	media_entity_cleanup(&phy->subdev.entity);
>  	of_node_put(phy->source_ep_node);
>  	of_node_put(phy->source_node);
> -	mutex_destroy(&phy->mutex);
>  	kfree(phy);
>  }
> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
> index 87db8761d94d..d363e123d4bb 100644
> --- a/drivers/media/platform/ti/cal/cal-video.c
> +++ b/drivers/media/platform/ti/cal/cal-video.c
> @@ -694,25 +694,40 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>  {
>  	const struct cal_format_info *rx_fmtinfo;
>  	const struct v4l2_mbus_framefmt *format;
> +	struct v4l2_subdev_state *state;
>  	struct media_pad *remote_pad;
> +	int ret = 0;
>
>  	remote_pad = media_pad_remote_pad_first(&ctx->pad);
>  	if (!remote_pad)
>  		return -ENODEV;
>
> -	format = &ctx->phy->formats[remote_pad->index];
> +	state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
> +
> +	format = v4l2_subdev_get_pad_format(&ctx->phy->subdev, state, remote_pad->index);
> +	if (!format) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>
>  	rx_fmtinfo = cal_format_by_code(format->code);
> -	if (!rx_fmtinfo)
> -		return -EINVAL;
> +	if (!rx_fmtinfo) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>
>  	if (ctx->fmtinfo->fourcc != rx_fmtinfo->fourcc ||
>  	    ctx->v_fmt.fmt.pix.height != format->height ||
>  	    ctx->v_fmt.fmt.pix.width != format->width ||
> -	    ctx->v_fmt.fmt.pix.field != format->field)
> -		return -EPIPE;
> +	    ctx->v_fmt.fmt.pix.field != format->field) {
> +		ret = -EPIPE;
> +		goto out;

No need for the goto, but it doesn't hurt

> +	}
>
> -	return 0;
> +out:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
>  }
>
>  static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
> index 44ecae8a273a..79cd0171e701 100644
> --- a/drivers/media/platform/ti/cal/cal.h
> +++ b/drivers/media/platform/ti/cal/cal.h
> @@ -178,7 +178,6 @@ struct cal_camerarx {
>
>  	struct v4l2_subdev	subdev;
>  	struct media_pad	pads[CAL_CAMERARX_NUM_PADS];
> -	struct v4l2_mbus_framefmt	formats[CAL_CAMERARX_NUM_PADS];
>
>  	/* protects the vc_* fields below */
>  	spinlock_t		vc_lock;
> @@ -186,13 +185,6 @@ struct cal_camerarx {
>  	u16			vc_frame_number[4];
>  	u32			vc_sequence[4];
>
> -	/*
> -	 * Lock for camerarx ops. Protects:
> -	 * - formats
> -	 * - enable_count
> -	 */
> -	struct mutex		mutex;
> -

Nice!

Thanks
  j

>  	unsigned int		enable_count;
>  };
>
> --
> 2.34.1
>

Re: [PATCH v1 2/3] media: ti: cal: Use subdev state

[Tomi Valkeinen]

On 23/02/2023 19:32, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Wed, Feb 22, 2023 at 02:56:29PM +0200, Tomi Valkeinen wrote:
>> Change TI CAL driver to use subdev state. No functional changes
>> (intended).
>>
>> This allows us to get rid of the 'formats' field, as the formats are
>> kept in the state, and also the 'mutex' as we already have state locking.
> 
> I'm not sure I fully get how the ctrl_handler is used here, but
> shouldn't the state->lock be set to the ctrl_handler.lock so that
> controls and state interaction are guaranteed to be protected ?

Hmm, interesting point... As I mentioned in the other email, the ctrls 
are used in the non-MC mode, and they're the sensor subdev's controls. 
But the sensor is not visible to the userspace... Still, some locking is 
needed. But doesn't the sensor subdevice already add locking for its 
controls. Does it apply here, when the controls are exposed by the CAL. 
It's rather confusing, I need to look at this in more detail.

>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/ti/cal/cal-camerarx.c | 134 ++++++-------------
>>   drivers/media/platform/ti/cal/cal-video.c    |  27 +++-
>>   drivers/media/platform/ti/cal/cal.h          |   8 --
>>   3 files changed, 60 insertions(+), 109 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
>> index 16ae52879a79..faafbd0e9240 100644
>> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
>> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
>> @@ -50,10 +50,16 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
>>   	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
>>   	u32 num_lanes = mipi_csi2->num_data_lanes;
>>   	const struct cal_format_info *fmtinfo;
>> +	struct v4l2_subdev_state *state;
>> +	struct v4l2_mbus_framefmt *fmt;
>>   	u32 bpp;
>>   	s64 freq;
>>
>> -	fmtinfo = cal_format_by_code(phy->formats[CAL_CAMERARX_PAD_SINK].code);
>> +	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
>> +
>> +	fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
>> +
>> +	fmtinfo = cal_format_by_code(fmt->code);
>>   	if (!fmtinfo)
>>   		return -EINVAL;
>>
>> @@ -620,34 +626,20 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
>>   	return container_of(sd, struct cal_camerarx, subdev);
>>   }
>>
>> -static struct v4l2_mbus_framefmt *
>> -cal_camerarx_get_pad_format(struct cal_camerarx *phy,
>> -			    struct v4l2_subdev_state *state,
>> -			    unsigned int pad, u32 which)
>> -{
>> -	switch (which) {
>> -	case V4L2_SUBDEV_FORMAT_TRY:
>> -		return v4l2_subdev_get_try_format(&phy->subdev, state, pad);
>> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
>> -		return &phy->formats[pad];
>> -	default:
>> -		return NULL;
>> -	}
>> -}
>> -
>>   static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
>>   {
>>   	struct cal_camerarx *phy = to_cal_camerarx(sd);
>> +	struct v4l2_subdev_state *state;
>>   	int ret = 0;
>>
>> -	mutex_lock(&phy->mutex);
>> +	state = v4l2_subdev_lock_and_get_active_state(sd);
>>
>>   	if (enable)
>>   		ret = cal_camerarx_start(phy);
>>   	else
>>   		cal_camerarx_stop(phy);
>>
>> -	mutex_unlock(&phy->mutex);
>> +	v4l2_subdev_unlock_state(state);
>>
>>   	return ret;
>>   }
>> @@ -657,62 +649,44 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>>   					  struct v4l2_subdev_mbus_code_enum *code)
>>   {
>>   	struct cal_camerarx *phy = to_cal_camerarx(sd);
>> -	int ret = 0;
>> -
>> -	mutex_lock(&phy->mutex);
>>
>>   	/* No transcoding, source and sink codes must match. */
>>   	if (cal_rx_pad_is_source(code->pad)) {
>>   		struct v4l2_mbus_framefmt *fmt;
>>
>> -		if (code->index > 0) {
>> -			ret = -EINVAL;
>> -			goto out;
>> -		}
>> +		if (code->index > 0)
>> +			return -EINVAL;
>>
>> -		fmt = cal_camerarx_get_pad_format(phy, state,
>> -						  CAL_CAMERARX_PAD_SINK,
>> -						  code->which);
>> +		fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
>> +						 CAL_CAMERARX_PAD_SINK);
> 
> By removing the lock here and in cal_camerarx_sd_set_fmt() don't we
> risk that the format on the sink pad is updated while we fetch it
> here? I know it's a very tiny window, but should we lock the state
> just to be safe ?

For a state aware subdev, any subdev ops that get the state as a 
parameter, the state is already locked. So we don't need any locking 
here anymore.

  Tomi

[PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[Tomi Valkeinen]

Add routing and stream_config support to CAL driver.

Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
separate streams at the same time.

Add 8 video device nodes, each representing a single dma-engine, and set
the number of source pads on camerarx to 8. Each video node can be
connected to any of the source pads on either of the camerarx instances
using media links. Camerarx internal routing is used to route the
incoming CSI-2 streams to one of the 8 source pads.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
 drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
 drivers/media/platform/ti/cal/cal.c          |  65 ++++--
 drivers/media/platform/ti/cal/cal.h          |   4 +-
 4 files changed, 342 insertions(+), 106 deletions(-)

diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
index faafbd0e9240..49ae29065cd1 100644
--- a/drivers/media/platform/ti/cal/cal-camerarx.c
+++ b/drivers/media/platform/ti/cal/cal-camerarx.c
@@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
 {
 	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
 	u32 num_lanes = mipi_csi2->num_data_lanes;
-	const struct cal_format_info *fmtinfo;
 	struct v4l2_subdev_state *state;
-	struct v4l2_mbus_framefmt *fmt;
 	u32 bpp;
 	s64 freq;
 
-	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
+	/*
+	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
+	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
+	 *
+	 * With multistream input there is no single pixel rate, and thus we
+	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
+	 * causes v4l2_get_link_freq() to return an error if it falls back to
+	 * V4L2_CID_PIXEL_RATE.
+	 */
 
-	fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
+	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
 
-	fmtinfo = cal_format_by_code(fmt->code);
-	if (!fmtinfo)
+	if (state->routing.num_routes == 0)
 		return -EINVAL;
 
-	bpp = fmtinfo->bpp;
+	if (state->routing.num_routes > 1) {
+		bpp = 0;
+	} else {
+		const struct cal_format_info *fmtinfo;
+		struct v4l2_subdev_route *route = &state->routing.routes[0];
+		struct v4l2_mbus_framefmt *fmt;
+
+		fmt = v4l2_subdev_state_get_stream_format(
+			state, route->sink_pad, route->sink_stream);
+
+		fmtinfo = cal_format_by_code(fmt->code);
+		if (!fmtinfo)
+			return -EINVAL;
+
+		bpp = fmtinfo->bpp;
+	}
 
 	freq = v4l2_get_link_freq(phy->source->ctrl_handler, bpp, 2 * num_lanes);
 	if (freq < 0) {
@@ -284,15 +304,28 @@ static void cal_camerarx_ppi_disable(struct cal_camerarx *phy)
 			0, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
 }
 
-static int cal_camerarx_start(struct cal_camerarx *phy)
+static int cal_camerarx_start(struct cal_camerarx *phy, u32 pad, u32 stream)
 {
+	struct media_pad *remote_pad;
 	s64 link_freq;
 	u32 sscounter;
 	u32 val;
 	int ret;
 
+	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
+
 	if (phy->enable_count > 0) {
 		phy->enable_count++;
+
+		ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
+						 BIT(stream));
+		if (ret) {
+			phy->enable_count--;
+
+			phy_err(phy, "enable streams failed in source: %d\n", ret);
+			return ret;
+		}
+
 		return 0;
 	}
 
@@ -394,7 +427,9 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
 	 * Start the source to enable the CSI-2 HS clock. We can now wait for
 	 * CSI-2 PHY reset to complete.
 	 */
-	ret = v4l2_subdev_call(phy->source, video, s_stream, 1);
+
+	ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
+					 BIT(stream));
 	if (ret) {
 		v4l2_subdev_call(phy->source, core, s_power, 0);
 		cal_camerarx_disable_irqs(phy);
@@ -425,12 +460,22 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
 	return 0;
 }
 
-static void cal_camerarx_stop(struct cal_camerarx *phy)
+static void cal_camerarx_stop(struct cal_camerarx *phy, u32 pad, u32 stream)
 {
+	struct media_pad *remote_pad;
 	int ret;
 
-	if (--phy->enable_count > 0)
+	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
+
+	if (--phy->enable_count > 0) {
+		ret = v4l2_subdev_disable_streams(phy->source,
+						  remote_pad->index,
+						  BIT(stream));
+		if (ret)
+			phy_err(phy, "stream off failed in subdev\n");
+
 		return;
+	}
 
 	cal_camerarx_ppi_disable(phy);
 
@@ -450,7 +495,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy)
 	/* Disable the phy */
 	cal_camerarx_disable(phy);
 
-	if (v4l2_subdev_call(phy->source, video, s_stream, 0))
+	ret = v4l2_subdev_disable_streams(phy->source, remote_pad->index,
+					  BIT(stream));
+	if (ret)
 		phy_err(phy, "stream off failed in subdev\n");
 
 	ret = v4l2_subdev_call(phy->source, core, s_power, 0);
@@ -626,30 +673,62 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
 	return container_of(sd, struct cal_camerarx, subdev);
 }
 
-static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
+struct cal_camerarx *
+cal_camerarx_get_phy_from_entity(struct media_entity *entity)
+{
+	struct v4l2_subdev *sd;
+
+	sd = media_entity_to_v4l2_subdev(entity);
+	if (!sd)
+		return NULL;
+
+	return to_cal_camerarx(sd);
+}
+
+static int cal_camerarx_sd_enable_streams(struct v4l2_subdev *sd,
+					  struct v4l2_subdev_state *state,
+					  u32 pad, u64 streams_mask)
 {
 	struct cal_camerarx *phy = to_cal_camerarx(sd);
-	struct v4l2_subdev_state *state;
-	int ret = 0;
+	u32 other_pad, other_stream;
+	int ret;
 
-	state = v4l2_subdev_lock_and_get_active_state(sd);
+	if (WARN_ON(streams_mask != 1))
+		return -EINVAL;
 
-	if (enable)
-		ret = cal_camerarx_start(phy);
-	else
-		cal_camerarx_stop(phy);
+	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
+						    &other_pad, &other_stream);
+	if (ret)
+		return ret;
 
-	v4l2_subdev_unlock_state(state);
+	return cal_camerarx_start(phy, other_pad, other_stream);
+}
 
-	return ret;
+static int cal_camerarx_sd_disable_streams(struct v4l2_subdev *sd,
+					   struct v4l2_subdev_state *state,
+					   u32 pad, u64 streams_mask)
+{
+	struct cal_camerarx *phy = to_cal_camerarx(sd);
+	u32 other_pad, other_stream;
+	int ret;
+
+	if (WARN_ON(streams_mask != 1))
+		return -EINVAL;
+
+	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
+						    &other_pad, &other_stream);
+	if (ret)
+		return ret;
+
+	cal_camerarx_stop(phy, other_pad, other_stream);
+
+	return 0;
 }
 
 static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
 					  struct v4l2_subdev_state *state,
 					  struct v4l2_subdev_mbus_code_enum *code)
 {
-	struct cal_camerarx *phy = to_cal_camerarx(sd);
-
 	/* No transcoding, source and sink codes must match. */
 	if (cal_rx_pad_is_source(code->pad)) {
 		struct v4l2_mbus_framefmt *fmt;
@@ -657,8 +736,12 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
 		if (code->index > 0)
 			return -EINVAL;
 
-		fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
-						 CAL_CAMERARX_PAD_SINK);
+		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
+								   code->pad,
+								   code->stream);
+		if (!fmt)
+			return -EINVAL;
+
 		code->code = fmt->code;
 	} else {
 		if (code->index >= cal_num_formats)
@@ -683,8 +766,12 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
 	if (cal_rx_pad_is_source(fse->pad)) {
 		struct v4l2_mbus_framefmt *fmt;
 
-		fmt = v4l2_subdev_get_pad_format(sd, state,
-						 CAL_CAMERARX_PAD_SINK);
+		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
+								   fse->pad,
+								   fse->stream);
+		if (!fmt)
+			return -EINVAL;
+
 		if (fse->code != fmt->code)
 			return -EINVAL;
 
@@ -740,57 +827,96 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
 
 	/* Store the format and propagate it to the source pad. */
 
-	fmt = v4l2_subdev_get_pad_format(sd, state, CAL_CAMERARX_PAD_SINK);
+	fmt = v4l2_subdev_state_get_stream_format(state, format->pad,
+						  format->stream);
+	if (!fmt)
+		return -EINVAL;
+
 	*fmt = format->format;
 
-	fmt = v4l2_subdev_get_pad_format(sd, state,
-					 CAL_CAMERARX_PAD_FIRST_SOURCE);
+	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
+							   format->stream);
+	if (!fmt)
+		return -EINVAL;
+
 	*fmt = format->format;
 
 	return 0;
 }
 
+static int _cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
+					struct v4l2_subdev_state *state,
+					struct v4l2_subdev_krouting *routing)
+{
+	static const struct v4l2_mbus_framefmt format = {
+		.width = 640,
+		.height = 480,
+		.code = MEDIA_BUS_FMT_UYVY8_2X8,
+		.field = V4L2_FIELD_NONE,
+		.colorspace = V4L2_COLORSPACE_SRGB,
+		.ycbcr_enc = V4L2_YCBCR_ENC_601,
+		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
+		.xfer_func = V4L2_XFER_FUNC_SRGB,
+	};
+	int ret;
+
+	ret = v4l2_subdev_routing_validate(sd, routing, V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
+	if (ret)
+		return ret;
+
+	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
+				       struct v4l2_subdev_state *state,
+				       enum v4l2_subdev_format_whence which,
+				       struct v4l2_subdev_krouting *routing)
+{
+	return _cal_camerarx_sd_set_routing(sd, state, routing);
+}
+
 static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_state *state)
 {
-	struct v4l2_subdev_format format = {
-		.which = state ? V4L2_SUBDEV_FORMAT_TRY
-		: V4L2_SUBDEV_FORMAT_ACTIVE,
-		.pad = CAL_CAMERARX_PAD_SINK,
-		.format = {
-			.width = 640,
-			.height = 480,
-			.code = MEDIA_BUS_FMT_UYVY8_2X8,
-			.field = V4L2_FIELD_NONE,
-			.colorspace = V4L2_COLORSPACE_SRGB,
-			.ycbcr_enc = V4L2_YCBCR_ENC_601,
-			.quantization = V4L2_QUANTIZATION_LIM_RANGE,
-			.xfer_func = V4L2_XFER_FUNC_SRGB,
-		},
+	struct v4l2_subdev_route routes[] = { {
+		.sink_pad = 0,
+		.sink_stream = 0,
+		.source_pad = 1,
+		.source_stream = 0,
+		.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+	} };
+
+	struct v4l2_subdev_krouting routing = {
+		.num_routes = 1,
+		.routes = routes,
 	};
 
-	return cal_camerarx_sd_set_fmt(sd, state, &format);
+	/* Initialize routing to single route to the fist source pad */
+	return _cal_camerarx_sd_set_routing(sd, state, &routing);
 }
 
-static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = {
-	.s_stream = cal_camerarx_sd_s_stream,
-};
-
 static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = {
+	.enable_streams = cal_camerarx_sd_enable_streams,
+	.disable_streams = cal_camerarx_sd_disable_streams,
 	.init_cfg = cal_camerarx_sd_init_cfg,
 	.enum_mbus_code = cal_camerarx_sd_enum_mbus_code,
 	.enum_frame_size = cal_camerarx_sd_enum_frame_size,
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = cal_camerarx_sd_set_fmt,
+	.set_routing = cal_camerarx_sd_set_routing,
 };
 
 static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = {
-	.video = &cal_camerarx_video_ops,
 	.pad = &cal_camerarx_pad_ops,
 };
 
 static struct media_entity_operations cal_camerarx_media_ops = {
 	.link_validate = v4l2_subdev_link_validate,
+	.has_pad_interdep = v4l2_subdev_has_pad_interdep,
 };
 
 /* ------------------------------------------------------------------
@@ -842,11 +968,12 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
 	sd = &phy->subdev;
 	v4l2_subdev_init(sd, &cal_camerarx_subdev_ops);
 	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
-	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
 	snprintf(sd->name, sizeof(sd->name), "CAMERARX%u", instance);
 	sd->dev = cal->dev;
 
 	phy->pads[CAL_CAMERARX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+
 	for (i = CAL_CAMERARX_PAD_FIRST_SOURCE; i < CAL_CAMERARX_NUM_PADS; ++i)
 		phy->pads[i].flags = MEDIA_PAD_FL_SOURCE;
 	sd->entity.ops = &cal_camerarx_media_ops;
@@ -879,7 +1006,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
 		return;
 
 	v4l2_device_unregister_subdev(&phy->subdev);
+
 	v4l2_subdev_cleanup(&phy->subdev);
+
 	media_entity_cleanup(&phy->subdev.entity);
 	of_node_put(phy->source_ep_node);
 	of_node_put(phy->source_node);
diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
index d363e123d4bb..71578bfc97ba 100644
--- a/drivers/media/platform/ti/cal/cal-video.c
+++ b/drivers/media/platform/ti/cal/cal-video.c
@@ -119,12 +119,13 @@ static int __subdev_get_format(struct cal_ctx *ctx,
 {
 	struct v4l2_subdev_format sd_fmt;
 	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
+	struct v4l2_subdev *sd = ctx->phy->source;
 	int ret;
 
 	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	sd_fmt.pad = 0;
 
-	ret = v4l2_subdev_call(ctx->phy->source, pad, get_fmt, NULL, &sd_fmt);
+	ret = v4l2_subdev_call_state_active(sd, pad, get_fmt, &sd_fmt);
 	if (ret)
 		return ret;
 
@@ -141,13 +142,14 @@ static int __subdev_set_format(struct cal_ctx *ctx,
 {
 	struct v4l2_subdev_format sd_fmt;
 	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
+	struct v4l2_subdev *sd = ctx->phy->source;
 	int ret;
 
 	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	sd_fmt.pad = 0;
 	*mbus_fmt = *fmt;
 
-	ret = v4l2_subdev_call(ctx->phy->source, pad, set_fmt, NULL, &sd_fmt);
+	ret = v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
 	if (ret)
 		return ret;
 
@@ -189,6 +191,7 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
 				      struct v4l2_format *f)
 {
 	struct cal_ctx *ctx = video_drvdata(file);
+	struct v4l2_subdev *sd = ctx->phy->source;
 	const struct cal_format_info *fmtinfo;
 	struct v4l2_subdev_frame_size_enum fse;
 	int found;
@@ -213,8 +216,8 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
 	for (fse.index = 0; ; fse.index++) {
 		int ret;
 
-		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size,
-				       NULL, &fse);
+		ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size,
+						    &fse);
 		if (ret)
 			break;
 
@@ -250,6 +253,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
 				    struct v4l2_format *f)
 {
 	struct cal_ctx *ctx = video_drvdata(file);
+	struct v4l2_subdev *sd = &ctx->phy->subdev;
 	struct vb2_queue *q = &ctx->vb_vidq;
 	struct v4l2_subdev_format sd_fmt = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
@@ -289,7 +293,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
 	ctx->v_fmt.fmt.pix.field = sd_fmt.format.field;
 	cal_calc_format_size(ctx, fmtinfo, &ctx->v_fmt);
 
-	v4l2_subdev_call(&ctx->phy->subdev, pad, set_fmt, NULL, &sd_fmt);
+	v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
 
 	ctx->fmtinfo = fmtinfo;
 	*f = ctx->v_fmt;
@@ -301,6 +305,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
 				      struct v4l2_frmsizeenum *fsize)
 {
 	struct cal_ctx *ctx = video_drvdata(file);
+	struct v4l2_subdev *sd = ctx->phy->source;
 	const struct cal_format_info *fmtinfo;
 	struct v4l2_subdev_frame_size_enum fse;
 	int ret;
@@ -318,8 +323,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
 	fse.code = fmtinfo->code;
 	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 
-	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size, NULL,
-			       &fse);
+	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size, &fse);
 	if (ret)
 		return ret;
 
@@ -361,6 +365,7 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
 					  struct v4l2_frmivalenum *fival)
 {
 	struct cal_ctx *ctx = video_drvdata(file);
+	struct v4l2_subdev *sd = ctx->phy->source;
 	const struct cal_format_info *fmtinfo;
 	struct v4l2_subdev_frame_interval_enum fie = {
 		.index = fival->index,
@@ -375,8 +380,8 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
 		return -EINVAL;
 
 	fie.code = fmtinfo->code;
-	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_interval,
-			       NULL, &fie);
+
+	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_interval, &fie);
 	if (ret)
 		return ret;
 	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
@@ -694,8 +699,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
 {
 	const struct cal_format_info *rx_fmtinfo;
 	const struct v4l2_mbus_framefmt *format;
-	struct v4l2_subdev_state *state;
 	struct media_pad *remote_pad;
+	struct v4l2_subdev_state *state;
 	int ret = 0;
 
 	remote_pad = media_pad_remote_pad_first(&ctx->pad);
@@ -704,7 +709,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
 
 	state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
 
-	format = v4l2_subdev_get_pad_format(&ctx->phy->subdev, state, remote_pad->index);
+	format = v4l2_subdev_state_get_stream_format(state, remote_pad->index,
+						     0);
 	if (!format) {
 		ret = -EINVAL;
 		goto out;
@@ -733,10 +739,52 @@ static int cal_video_check_format(struct cal_ctx *ctx)
 static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
+	struct media_pad *remote_pad;
 	struct cal_buffer *buf;
 	dma_addr_t addr;
 	int ret;
 
+	remote_pad = media_pad_remote_pad_first(&ctx->pad);
+	if (!remote_pad) {
+		ctx_err(ctx, "Context not connected\n");
+		ret = -ENODEV;
+		goto error_release_buffers;
+	}
+
+	if (cal_mc_api) {
+		struct v4l2_subdev_route *route = NULL;
+		struct v4l2_subdev_route *r;
+		struct v4l2_subdev_state *state;
+
+		/* Find the PHY connected to this video device */
+
+		ctx->phy = cal_camerarx_get_phy_from_entity(remote_pad->entity);
+
+		state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
+
+		/* Find the stream */
+
+		for_each_active_route(&state->routing, r) {
+			if (r->source_pad != remote_pad->index)
+				continue;
+
+			route = r;
+
+			break;
+		}
+
+		if (!route) {
+			v4l2_subdev_unlock_state(state);
+			ctx_err(ctx, "Failed to find route\n");
+			ret = -ENODEV;
+			goto error_release_buffers;
+		}
+
+		ctx->stream = route->sink_stream;
+
+		v4l2_subdev_unlock_state(state);
+	}
+
 	ret = video_device_pipeline_alloc_start(&ctx->vdev);
 	if (ret < 0) {
 		ctx_err(ctx, "Failed to start media pipeline: %d\n", ret);
@@ -775,7 +823,8 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
 	cal_ctx_set_dma_addr(ctx, addr);
 	cal_ctx_start(ctx);
 
-	ret = v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 1);
+	ret = v4l2_subdev_enable_streams(&ctx->phy->subdev, remote_pad->index,
+					 BIT(0));
 	if (ret)
 		goto error_stop;
 
@@ -800,10 +849,14 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
 static void cal_stop_streaming(struct vb2_queue *vq)
 {
 	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
+	struct media_pad *remote_pad;
 
 	cal_ctx_stop(ctx);
 
-	v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 0);
+	remote_pad = media_pad_remote_pad_first(&ctx->pad);
+
+	v4l2_subdev_disable_streams(&ctx->phy->subdev, remote_pad->index,
+				    BIT(0));
 
 	pm_runtime_put_sync(ctx->cal->dev);
 
@@ -812,6 +865,9 @@ static void cal_stop_streaming(struct vb2_queue *vq)
 	cal_release_buffers(ctx, VB2_BUF_STATE_ERROR);
 
 	video_device_pipeline_stop(&ctx->vdev);
+
+	if (cal_mc_api)
+		ctx->phy = NULL;
 }
 
 static const struct vb2_ops cal_video_qops = {
@@ -845,6 +901,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
 	const struct cal_format_info *fmtinfo;
 	unsigned int i, j, k;
 	int ret = 0;
+	struct v4l2_subdev *sd = ctx->phy->source;
 
 	/* Enumerate sub device formats and enable all matching local formats */
 	ctx->active_fmt = devm_kcalloc(ctx->cal->dev, cal_num_formats,
@@ -859,20 +916,20 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
 		memset(&mbus_code, 0, sizeof(mbus_code));
 		mbus_code.index = j;
 		mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
-		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_mbus_code,
-				       NULL, &mbus_code);
+		ret = v4l2_subdev_call_state_active(sd, pad, enum_mbus_code,
+						    &mbus_code);
 		if (ret == -EINVAL)
 			break;
 
 		if (ret) {
 			ctx_err(ctx, "Error enumerating mbus codes in subdev %s: %d\n",
-				ctx->phy->source->name, ret);
+				sd->name, ret);
 			return ret;
 		}
 
 		ctx_dbg(2, ctx,
 			"subdev %s: code: %04x idx: %u\n",
-			ctx->phy->source->name, mbus_code.code, j);
+			sd->name, mbus_code.code, j);
 
 		for (k = 0; k < cal_num_formats; k++) {
 			fmtinfo = &cal_formats[k];
@@ -890,7 +947,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
 
 	if (i == 0) {
 		ctx_err(ctx, "No suitable format reported by subdev %s\n",
-			ctx->phy->source->name);
+			sd->name);
 		return -EINVAL;
 	}
 
@@ -976,16 +1033,49 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx)
 		return ret;
 	}
 
-	ret = media_create_pad_link(&ctx->phy->subdev.entity,
-				    CAL_CAMERARX_PAD_FIRST_SOURCE,
-				    &vfd->entity, 0,
-				    MEDIA_LNK_FL_IMMUTABLE |
-				    MEDIA_LNK_FL_ENABLED);
-	if (ret) {
-		ctx_err(ctx, "Failed to create media link for context %u\n",
-			ctx->dma_ctx);
-		video_unregister_device(vfd);
-		return ret;
+	if (cal_mc_api) {
+		u16 phy_idx;
+		u16 pad_idx;
+
+		/* Create links from all video nodes to all PHYs */
+
+		for (phy_idx = 0; phy_idx < ctx->cal->data->num_csi2_phy;
+		     ++phy_idx) {
+			for (pad_idx = 1; pad_idx < CAL_CAMERARX_NUM_PADS;
+			     ++pad_idx) {
+				/*
+				 * Enable only links from video0 to PHY0 pad 1,
+				 * and video1 to PHY1 pad 1.
+				 */
+				bool enable = (ctx->dma_ctx == 0 &&
+					       phy_idx == 0 && pad_idx == 1) ||
+					      (ctx->dma_ctx == 1 &&
+					       phy_idx == 1 && pad_idx == 1);
+
+				ret = media_create_pad_link(
+					&ctx->cal->phy[phy_idx]->subdev.entity,
+					pad_idx, &vfd->entity, 0,
+					enable ? MEDIA_LNK_FL_ENABLED : 0);
+				if (ret) {
+					ctx_err(ctx,
+						"Failed to create media link for context %u\n",
+						ctx->dma_ctx);
+					video_unregister_device(vfd);
+					return ret;
+				}
+			}
+		}
+	} else {
+		ret = media_create_pad_link(&ctx->phy->subdev.entity,
+			CAL_CAMERARX_PAD_FIRST_SOURCE, &vfd->entity, 0,
+			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+		if (ret) {
+			ctx_err(ctx,
+				"Failed to create media link for context %u\n",
+				ctx->dma_ctx);
+			video_unregister_device(vfd);
+			return ret;
+		}
 	}
 
 	ctx_info(ctx, "V4L2 device registered as %s\n",
diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
index 053bf1030af0..074bf33c3697 100644
--- a/drivers/media/platform/ti/cal/cal.c
+++ b/drivers/media/platform/ti/cal/cal.c
@@ -495,10 +495,11 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx)
 }
 
 static int
-cal_get_remote_frame_desc_entry(struct cal_camerarx *phy,
+cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, u32 stream,
 				struct v4l2_mbus_frame_desc_entry *entry)
 {
 	struct v4l2_mbus_frame_desc fd;
+	unsigned int i;
 	int ret;
 
 	ret = cal_camerarx_get_remote_frame_desc(phy, &fd);
@@ -509,20 +510,18 @@ cal_get_remote_frame_desc_entry(struct cal_camerarx *phy,
 		return ret;
 	}
 
-	if (fd.num_entries == 0) {
-		dev_err(phy->cal->dev,
-			"No streams found in the remote frame descriptor\n");
-
-		return -ENODEV;
+	for (i = 0; i < fd.num_entries; i++) {
+		if (stream == fd.entry[i].stream) {
+			*entry = fd.entry[i];
+			return 0;
+		}
 	}
 
-	if (fd.num_entries > 1)
-		dev_dbg(phy->cal->dev,
-			"Multiple streams not supported in remote frame descriptor, using the first one\n");
+	dev_err(phy->cal->dev,
+		"Failed to find stream %u from remote frame descriptor\n",
+		stream);
 
-	*entry = fd.entry[0];
-
-	return 0;
+	return -ENODEV;
 }
 
 int cal_ctx_prepare(struct cal_ctx *ctx)
@@ -530,14 +529,15 @@ int cal_ctx_prepare(struct cal_ctx *ctx)
 	struct v4l2_mbus_frame_desc_entry entry;
 	int ret;
 
-	ret = cal_get_remote_frame_desc_entry(ctx->phy, &entry);
+	ret = cal_get_remote_frame_desc_entry(ctx->phy, ctx->stream, &entry);
 
 	if (ret == -ENOIOCTLCMD) {
 		ctx->vc = 0;
 		ctx->datatype = CAL_CSI2_CTX_DT_ANY;
 	} else if (!ret) {
-		ctx_dbg(2, ctx, "Framedesc: len %u, vc %u, dt %#x\n",
-			entry.length, entry.bus.csi2.vc, entry.bus.csi2.dt);
+		ctx_dbg(2, ctx, "Framedesc: stream %u, len %u, vc %u, dt %#x\n",
+			entry.stream, entry.length, entry.bus.csi2.vc,
+			entry.bus.csi2.dt);
 
 		ctx->vc = entry.bus.csi2.vc;
 		ctx->datatype = entry.bus.csi2.dt;
@@ -1069,10 +1069,10 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
 		return NULL;
 
 	ctx->cal = cal;
-	ctx->phy = cal->phy[inst];
 	ctx->dma_ctx = inst;
 	ctx->csi2_ctx = inst;
 	ctx->cport = inst;
+	ctx->stream = 0;
 
 	ret = cal_ctx_v4l2_init(ctx);
 	if (ret) {
@@ -1281,18 +1281,33 @@ static int cal_probe(struct platform_device *pdev)
 	}
 
 	/* Create contexts. */
-	for (i = 0; i < cal->data->num_csi2_phy; ++i) {
-		if (!cal->phy[i]->source_node)
-			continue;
+	if (!cal_mc_api) {
+		for (i = 0; i < cal->data->num_csi2_phy; ++i) {
+			if (!cal->phy[i]->source_node)
+				continue;
+
+			cal->ctx[cal->num_contexts] = cal_ctx_create(cal, i);
+			if (!cal->ctx[cal->num_contexts]) {
+				cal_err(cal, "Failed to create context %u\n", cal->num_contexts);
+				ret = -ENODEV;
+				goto error_context;
+			}
+
+			cal->ctx[cal->num_contexts]->phy = cal->phy[i];
 
-		cal->ctx[cal->num_contexts] = cal_ctx_create(cal, i);
-		if (!cal->ctx[cal->num_contexts]) {
-			cal_err(cal, "Failed to create context %u\n", cal->num_contexts);
-			ret = -ENODEV;
-			goto error_context;
+			cal->num_contexts++;
 		}
+	} else {
+		for (i = 0; i < ARRAY_SIZE(cal->ctx); ++i) {
+			cal->ctx[i] = cal_ctx_create(cal, i);
+			if (!cal->ctx[i]) {
+				cal_err(cal, "Failed to create context %u\n", i);
+				ret = -ENODEV;
+				goto error_context;
+			}
 
-		cal->num_contexts++;
+			cal->num_contexts++;
+		}
 	}
 
 	/* Register the media device. */
diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
index 79cd0171e701..e1f693bbeb07 100644
--- a/drivers/media/platform/ti/cal/cal.h
+++ b/drivers/media/platform/ti/cal/cal.h
@@ -45,7 +45,7 @@
 
 #define CAL_CAMERARX_PAD_SINK		0
 #define CAL_CAMERARX_PAD_FIRST_SOURCE	1
-#define CAL_CAMERARX_NUM_SOURCE_PADS	1
+#define CAL_CAMERARX_NUM_SOURCE_PADS	8
 #define CAL_CAMERARX_NUM_PADS		(1 + CAL_CAMERARX_NUM_SOURCE_PADS)
 
 static inline bool cal_rx_pad_is_sink(u32 pad)
@@ -247,6 +247,7 @@ struct cal_ctx {
 	u8			pix_proc;
 	u8			vc;
 	u8			datatype;
+	u32			stream;
 
 	bool			use_pix_proc;
 };
@@ -322,6 +323,7 @@ void cal_quickdump_regs(struct cal_dev *cal);
 
 int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy,
 				       struct v4l2_mbus_frame_desc *desc);
+struct cal_camerarx *cal_camerarx_get_phy_from_entity(struct media_entity *entity);
 void cal_camerarx_disable(struct cal_camerarx *phy);
 void cal_camerarx_i913_errata(struct cal_camerarx *phy);
 struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
-- 
2.34.1

Re: [PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[Jacopo Mondi]

Hi Tomi

On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
> Add routing and stream_config support to CAL driver.
>
> Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
> separate streams at the same time.
>
> Add 8 video device nodes, each representing a single dma-engine, and set
> the number of source pads on camerarx to 8. Each video node can be
> connected to any of the source pads on either of the camerarx instances
> using media links. Camerarx internal routing is used to route the
> incoming CSI-2 streams to one of the 8 source pads.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
>  drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
>  drivers/media/platform/ti/cal/cal.c          |  65 ++++--
>  drivers/media/platform/ti/cal/cal.h          |   4 +-
>  4 files changed, 342 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> index faafbd0e9240..49ae29065cd1 100644
> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
>  {
>  	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
>  	u32 num_lanes = mipi_csi2->num_data_lanes;
> -	const struct cal_format_info *fmtinfo;
>  	struct v4l2_subdev_state *state;
> -	struct v4l2_mbus_framefmt *fmt;
>  	u32 bpp;
>  	s64 freq;
>
> -	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> +	/*
> +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> +	 *
> +	 * With multistream input there is no single pixel rate, and thus we
> +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> +	 * causes v4l2_get_link_freq() to return an error if it falls back to
> +	 * V4L2_CID_PIXEL_RATE.
> +	 */

To recap a bit of our offline discussion:
- max9286 GMSL deserializer (as a comparison for a multiplexed
  transmitter) use PIXEL_RATE to report the cumulative pixel rate of
  enabled transmitters. This is because the R-Car CSI-2 receiver on
  which use PIXEL_RATE to compute the link freq [1]

- according to [2]
  pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)

  from which:
  link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)

  This works as long the reported pixel rate includes visible and
  blankings, something I'm not sure how many transmitters handle
  correctly as PIXEL_RATE control is meant to report the visible pixel
  sampling rate on the pixel array.

I guess we should go towards mandating LINK_FREQ for transmitters.

cc-Niklas for opinions on R-Car CSI-2 rcsi2_calc_mbps()

[1] https://elixir.bootlin.com/linux/v6.2/source/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c#L608
[2] https://www.kernel.org/doc/html/latest/driver-api/media/tx-rx.html#csi-2-transmitter-drivers

>
> -	fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
> +	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
>
> -	fmtinfo = cal_format_by_code(fmt->code);
> -	if (!fmtinfo)
> +	if (state->routing.num_routes == 0)
>  		return -EINVAL;

This function is in the call path of .enable_streams which if I'm not
reading the code wrong is called with

int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
{

        ...

	for_each_active_route(&state->routing, route)
		source_mask |= BIT_ULL(route->source_stream);

	if (enable)
		return v4l2_subdev_enable_streams(sd, pad_index, source_mask);

        ...
}

int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
			       u64 streams_mask)
{
	struct device *dev = sd->entity.graph_obj.mdev->dev;
	struct v4l2_subdev_state *state;
	u64 found_streams = 0;
	unsigned int i;
	int ret;

	/* A few basic sanity checks first. */
	if (pad >= sd->entity.num_pads)
		return -EINVAL;

	if (!streams_mask)
		return 0;

        ...
}

So the question is: can we get to enable_streams without enabled
routes (sorry I should have tested before asking it but I don't have a
multiplexed setup easily accessible) ?

>
> -	bpp = fmtinfo->bpp;
> +	if (state->routing.num_routes > 1) {
> +		bpp = 0;
> +	} else {
> +		const struct cal_format_info *fmtinfo;
> +		struct v4l2_subdev_route *route = &state->routing.routes[0];

Nit: if I'm not mistaken along the driver reverse-xmas-tree (I
know...) is respected. Can you do it here as well ?

> +		struct v4l2_mbus_framefmt *fmt;
> +
> +		fmt = v4l2_subdev_state_get_stream_format(
> +			state, route->sink_pad, route->sink_stream);
> +
> +		fmtinfo = cal_format_by_code(fmt->code);
> +		if (!fmtinfo)
> +			return -EINVAL;
> +
> +		bpp = fmtinfo->bpp;
> +	}
>
>  	freq = v4l2_get_link_freq(phy->source->ctrl_handler, bpp, 2 * num_lanes);
>  	if (freq < 0) {
> @@ -284,15 +304,28 @@ static void cal_camerarx_ppi_disable(struct cal_camerarx *phy)
>  			0, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
>  }
>
> -static int cal_camerarx_start(struct cal_camerarx *phy)
> +static int cal_camerarx_start(struct cal_camerarx *phy, u32 pad, u32 stream)
>  {
> +	struct media_pad *remote_pad;
>  	s64 link_freq;
>  	u32 sscounter;
>  	u32 val;
>  	int ret;
>
> +	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
> +

Would it hurt a comment here to explain that in case it's the first to
be enabled you need to start the RX and power up the transmitter,
otherwise it's enough to enable the additional stream ?

>  	if (phy->enable_count > 0) {
>  		phy->enable_count++;
> +
> +		ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
> +						 BIT(stream));
> +		if (ret) {
> +			phy->enable_count--;

You can avoid this by enable_count++ after the error check ?

> +
> +			phy_err(phy, "enable streams failed in source: %d\n", ret);
> +			return ret;
> +		}
> +
>  		return 0;
>  	}
>
> @@ -394,7 +427,9 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
>  	 * Start the source to enable the CSI-2 HS clock. We can now wait for
>  	 * CSI-2 PHY reset to complete.
>  	 */
> -	ret = v4l2_subdev_call(phy->source, video, s_stream, 1);
> +

Intentional additional blank line ?

> +	ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
> +					 BIT(stream));
>  	if (ret) {
>  		v4l2_subdev_call(phy->source, core, s_power, 0);
>  		cal_camerarx_disable_irqs(phy);
> @@ -425,12 +460,22 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
>  	return 0;
>  }
>
> -static void cal_camerarx_stop(struct cal_camerarx *phy)
> +static void cal_camerarx_stop(struct cal_camerarx *phy, u32 pad, u32 stream)
>  {
> +	struct media_pad *remote_pad;
>  	int ret;
>
> -	if (--phy->enable_count > 0)
> +	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
> +
> +	if (--phy->enable_count > 0) {
> +		ret = v4l2_subdev_disable_streams(phy->source,
> +						  remote_pad->index,
> +						  BIT(stream));
> +		if (ret)
> +			phy_err(phy, "stream off failed in subdev\n");
> +
>  		return;
> +	}
>
>  	cal_camerarx_ppi_disable(phy);
>
> @@ -450,7 +495,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy)
>  	/* Disable the phy */
>  	cal_camerarx_disable(phy);
>
> -	if (v4l2_subdev_call(phy->source, video, s_stream, 0))
> +	ret = v4l2_subdev_disable_streams(phy->source, remote_pad->index,
> +					  BIT(stream));
> +	if (ret)
>  		phy_err(phy, "stream off failed in subdev\n");
>
>  	ret = v4l2_subdev_call(phy->source, core, s_power, 0);
> @@ -626,30 +673,62 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
>  	return container_of(sd, struct cal_camerarx, subdev);
>  }
>
> -static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
> +struct cal_camerarx *
> +cal_camerarx_get_phy_from_entity(struct media_entity *entity)
> +{
> +	struct v4l2_subdev *sd;
> +
> +	sd = media_entity_to_v4l2_subdev(entity);
> +	if (!sd)
> +		return NULL;
> +
> +	return to_cal_camerarx(sd);
> +}
> +
> +static int cal_camerarx_sd_enable_streams(struct v4l2_subdev *sd,
> +					  struct v4l2_subdev_state *state,
> +					  u32 pad, u64 streams_mask)
>  {
>  	struct cal_camerarx *phy = to_cal_camerarx(sd);
> -	struct v4l2_subdev_state *state;
> -	int ret = 0;
> +	u32 other_pad, other_stream;
> +	int ret;
>
> -	state = v4l2_subdev_lock_and_get_active_state(sd);
> +	if (WARN_ON(streams_mask != 1))

as streams_mask is unsigned and you can't get here if streams_mask ==
0, I wonder if checking for > 1 isn't more explicit. A detail though.

> +		return -EINVAL;
>
> -	if (enable)
> -		ret = cal_camerarx_start(phy);
> -	else
> -		cal_camerarx_stop(phy);
> +	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
> +						    &other_pad, &other_stream);
> +	if (ret)
> +		return ret;
>
> -	v4l2_subdev_unlock_state(state);
> +	return cal_camerarx_start(phy, other_pad, other_stream);
> +}
>
> -	return ret;
> +static int cal_camerarx_sd_disable_streams(struct v4l2_subdev *sd,
> +					   struct v4l2_subdev_state *state,
> +					   u32 pad, u64 streams_mask)
> +{
> +	struct cal_camerarx *phy = to_cal_camerarx(sd);
> +	u32 other_pad, other_stream;
> +	int ret;
> +
> +	if (WARN_ON(streams_mask != 1))
> +		return -EINVAL;
> +
> +	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
> +						    &other_pad, &other_stream);
> +	if (ret)
> +		return ret;
> +
> +	cal_camerarx_stop(phy, other_pad, other_stream);
> +
> +	return 0;
>  }
>
>  static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>  					  struct v4l2_subdev_state *state,
>  					  struct v4l2_subdev_mbus_code_enum *code)
>  {
> -	struct cal_camerarx *phy = to_cal_camerarx(sd);
> -
>  	/* No transcoding, source and sink codes must match. */
>  	if (cal_rx_pad_is_source(code->pad)) {
>  		struct v4l2_mbus_framefmt *fmt;
> @@ -657,8 +736,12 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>  		if (code->index > 0)
>  			return -EINVAL;
>
> -		fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
> -						 CAL_CAMERARX_PAD_SINK);
> +		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
> +								   code->pad,
> +								   code->stream);
> +		if (!fmt)
> +			return -EINVAL;
> +
>  		code->code = fmt->code;
>  	} else {
>  		if (code->index >= cal_num_formats)
> @@ -683,8 +766,12 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>  	if (cal_rx_pad_is_source(fse->pad)) {
>  		struct v4l2_mbus_framefmt *fmt;
>
> -		fmt = v4l2_subdev_get_pad_format(sd, state,
> -						 CAL_CAMERARX_PAD_SINK);
> +		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
> +								   fse->pad,
> +								   fse->stream);
> +		if (!fmt)
> +			return -EINVAL;
> +
>  		if (fse->code != fmt->code)
>  			return -EINVAL;
>
> @@ -740,57 +827,96 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>
>  	/* Store the format and propagate it to the source pad. */
>
> -	fmt = v4l2_subdev_get_pad_format(sd, state, CAL_CAMERARX_PAD_SINK);
> +	fmt = v4l2_subdev_state_get_stream_format(state, format->pad,
> +						  format->stream);
> +	if (!fmt)
> +		return -EINVAL;
> +
>  	*fmt = format->format;
>
> -	fmt = v4l2_subdev_get_pad_format(sd, state,
> -					 CAL_CAMERARX_PAD_FIRST_SOURCE);
> +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> +							   format->stream);
> +	if (!fmt)
> +		return -EINVAL;
> +
>  	*fmt = format->format;
>
>  	return 0;
>  }
>
> +static int _cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
> +					struct v4l2_subdev_state *state,
> +					struct v4l2_subdev_krouting *routing)

Functions starting with _ are a bit unusual :)

> +{
> +	static const struct v4l2_mbus_framefmt format = {
> +		.width = 640,
> +		.height = 480,
> +		.code = MEDIA_BUS_FMT_UYVY8_2X8,
> +		.field = V4L2_FIELD_NONE,
> +		.colorspace = V4L2_COLORSPACE_SRGB,
> +		.ycbcr_enc = V4L2_YCBCR_ENC_601,
> +		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
> +		.xfer_func = V4L2_XFER_FUNC_SRGB,
> +	};
> +	int ret;
> +
> +	ret = v4l2_subdev_routing_validate(sd, routing, V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
> +				       struct v4l2_subdev_state *state,
> +				       enum v4l2_subdev_format_whence which,
> +				       struct v4l2_subdev_krouting *routing)
> +{
> +	return _cal_camerarx_sd_set_routing(sd, state, routing);
> +}
> +
>  static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_state *state)
>  {
> -	struct v4l2_subdev_format format = {
> -		.which = state ? V4L2_SUBDEV_FORMAT_TRY
> -		: V4L2_SUBDEV_FORMAT_ACTIVE,
> -		.pad = CAL_CAMERARX_PAD_SINK,
> -		.format = {
> -			.width = 640,
> -			.height = 480,
> -			.code = MEDIA_BUS_FMT_UYVY8_2X8,
> -			.field = V4L2_FIELD_NONE,
> -			.colorspace = V4L2_COLORSPACE_SRGB,
> -			.ycbcr_enc = V4L2_YCBCR_ENC_601,
> -			.quantization = V4L2_QUANTIZATION_LIM_RANGE,
> -			.xfer_func = V4L2_XFER_FUNC_SRGB,
> -		},
> +	struct v4l2_subdev_route routes[] = { {
> +		.sink_pad = 0,
> +		.sink_stream = 0,
> +		.source_pad = 1,
> +		.source_stream = 0,
> +		.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> +	} };
> +
> +	struct v4l2_subdev_krouting routing = {
> +		.num_routes = 1,
> +		.routes = routes,
>  	};
>
> -	return cal_camerarx_sd_set_fmt(sd, state, &format);
> +	/* Initialize routing to single route to the fist source pad */
> +	return _cal_camerarx_sd_set_routing(sd, state, &routing);
>  }
>
> -static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = {
> -	.s_stream = cal_camerarx_sd_s_stream,
> -};
> -
>  static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = {
> +	.enable_streams = cal_camerarx_sd_enable_streams,
> +	.disable_streams = cal_camerarx_sd_disable_streams,
>  	.init_cfg = cal_camerarx_sd_init_cfg,
>  	.enum_mbus_code = cal_camerarx_sd_enum_mbus_code,
>  	.enum_frame_size = cal_camerarx_sd_enum_frame_size,
>  	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = cal_camerarx_sd_set_fmt,
> +	.set_routing = cal_camerarx_sd_set_routing,
>  };
>
>  static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = {
> -	.video = &cal_camerarx_video_ops,
>  	.pad = &cal_camerarx_pad_ops,
>  };
>
>  static struct media_entity_operations cal_camerarx_media_ops = {
>  	.link_validate = v4l2_subdev_link_validate,
> +	.has_pad_interdep = v4l2_subdev_has_pad_interdep,
>  };
>
>  /* ------------------------------------------------------------------
> @@ -842,11 +968,12 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>  	sd = &phy->subdev;
>  	v4l2_subdev_init(sd, &cal_camerarx_subdev_ops);
>  	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> -	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
>  	snprintf(sd->name, sizeof(sd->name), "CAMERARX%u", instance);
>  	sd->dev = cal->dev;
>
>  	phy->pads[CAL_CAMERARX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +

Intentional ?

>  	for (i = CAL_CAMERARX_PAD_FIRST_SOURCE; i < CAL_CAMERARX_NUM_PADS; ++i)
>  		phy->pads[i].flags = MEDIA_PAD_FL_SOURCE;
>  	sd->entity.ops = &cal_camerarx_media_ops;
> @@ -879,7 +1006,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
>  		return;
>
>  	v4l2_device_unregister_subdev(&phy->subdev);
> +
>  	v4l2_subdev_cleanup(&phy->subdev);
> +

ditto

>  	media_entity_cleanup(&phy->subdev.entity);
>  	of_node_put(phy->source_ep_node);
>  	of_node_put(phy->source_node);
> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
> index d363e123d4bb..71578bfc97ba 100644
> --- a/drivers/media/platform/ti/cal/cal-video.c
> +++ b/drivers/media/platform/ti/cal/cal-video.c
> @@ -119,12 +119,13 @@ static int __subdev_get_format(struct cal_ctx *ctx,
>  {
>  	struct v4l2_subdev_format sd_fmt;
>  	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
> +	struct v4l2_subdev *sd = ctx->phy->source;
>  	int ret;
>
>  	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	sd_fmt.pad = 0;
>
> -	ret = v4l2_subdev_call(ctx->phy->source, pad, get_fmt, NULL, &sd_fmt);
> +	ret = v4l2_subdev_call_state_active(sd, pad, get_fmt, &sd_fmt);
>  	if (ret)
>  		return ret;
>
> @@ -141,13 +142,14 @@ static int __subdev_set_format(struct cal_ctx *ctx,
>  {
>  	struct v4l2_subdev_format sd_fmt;
>  	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
> +	struct v4l2_subdev *sd = ctx->phy->source;
>  	int ret;
>
>  	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	sd_fmt.pad = 0;
>  	*mbus_fmt = *fmt;
>
> -	ret = v4l2_subdev_call(ctx->phy->source, pad, set_fmt, NULL, &sd_fmt);
> +	ret = v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
>  	if (ret)
>  		return ret;
>
> @@ -189,6 +191,7 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
>  				      struct v4l2_format *f)
>  {
>  	struct cal_ctx *ctx = video_drvdata(file);
> +	struct v4l2_subdev *sd = ctx->phy->source;
>  	const struct cal_format_info *fmtinfo;
>  	struct v4l2_subdev_frame_size_enum fse;
>  	int found;
> @@ -213,8 +216,8 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
>  	for (fse.index = 0; ; fse.index++) {
>  		int ret;
>
> -		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size,
> -				       NULL, &fse);
> +		ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size,
> +						    &fse);
>  		if (ret)
>  			break;
>
> @@ -250,6 +253,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
>  				    struct v4l2_format *f)
>  {
>  	struct cal_ctx *ctx = video_drvdata(file);
> +	struct v4l2_subdev *sd = &ctx->phy->subdev;
>  	struct vb2_queue *q = &ctx->vb_vidq;
>  	struct v4l2_subdev_format sd_fmt = {
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> @@ -289,7 +293,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
>  	ctx->v_fmt.fmt.pix.field = sd_fmt.format.field;
>  	cal_calc_format_size(ctx, fmtinfo, &ctx->v_fmt);
>
> -	v4l2_subdev_call(&ctx->phy->subdev, pad, set_fmt, NULL, &sd_fmt);
> +	v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
>
>  	ctx->fmtinfo = fmtinfo;
>  	*f = ctx->v_fmt;
> @@ -301,6 +305,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
>  				      struct v4l2_frmsizeenum *fsize)
>  {
>  	struct cal_ctx *ctx = video_drvdata(file);
> +	struct v4l2_subdev *sd = ctx->phy->source;
>  	const struct cal_format_info *fmtinfo;
>  	struct v4l2_subdev_frame_size_enum fse;
>  	int ret;
> @@ -318,8 +323,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
>  	fse.code = fmtinfo->code;
>  	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>
> -	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size, NULL,
> -			       &fse);
> +	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size, &fse);
>  	if (ret)
>  		return ret;
>
> @@ -361,6 +365,7 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
>  					  struct v4l2_frmivalenum *fival)
>  {
>  	struct cal_ctx *ctx = video_drvdata(file);
> +	struct v4l2_subdev *sd = ctx->phy->source;
>  	const struct cal_format_info *fmtinfo;
>  	struct v4l2_subdev_frame_interval_enum fie = {
>  		.index = fival->index,
> @@ -375,8 +380,8 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
>  		return -EINVAL;
>
>  	fie.code = fmtinfo->code;
> -	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_interval,
> -			       NULL, &fie);
> +
> +	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_interval, &fie);
>  	if (ret)
>  		return ret;
>  	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> @@ -694,8 +699,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>  {
>  	const struct cal_format_info *rx_fmtinfo;
>  	const struct v4l2_mbus_framefmt *format;
> -	struct v4l2_subdev_state *state;
>  	struct media_pad *remote_pad;
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
>
>  	remote_pad = media_pad_remote_pad_first(&ctx->pad);
> @@ -704,7 +709,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>
>  	state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
>
> -	format = v4l2_subdev_get_pad_format(&ctx->phy->subdev, state, remote_pad->index);
> +	format = v4l2_subdev_state_get_stream_format(state, remote_pad->index,
> +						     0);
>  	if (!format) {
>  		ret = -EINVAL;
>  		goto out;
> @@ -733,10 +739,52 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>  static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
> +	struct media_pad *remote_pad;
>  	struct cal_buffer *buf;
>  	dma_addr_t addr;
>  	int ret;
>
> +	remote_pad = media_pad_remote_pad_first(&ctx->pad);
> +	if (!remote_pad) {
> +		ctx_err(ctx, "Context not connected\n");
> +		ret = -ENODEV;
> +		goto error_release_buffers;
> +	}
> +
> +	if (cal_mc_api) {
> +		struct v4l2_subdev_route *route = NULL;
> +		struct v4l2_subdev_route *r;
> +		struct v4l2_subdev_state *state;
> +
> +		/* Find the PHY connected to this video device */
> +
> +		ctx->phy = cal_camerarx_get_phy_from_entity(remote_pad->entity);
> +
> +		state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
> +
> +		/* Find the stream */
> +
> +		for_each_active_route(&state->routing, r) {
> +			if (r->source_pad != remote_pad->index)
> +				continue;
> +
> +			route = r;
> +
> +			break;
> +		}
> +
> +		if (!route) {
> +			v4l2_subdev_unlock_state(state);
> +			ctx_err(ctx, "Failed to find route\n");
> +			ret = -ENODEV;
> +			goto error_release_buffers;
> +		}

If I got it right: we inspect the PHY routing table, pick the route that ends in
the pad connected to the this video device and store the sink-stream
id. The sink-stream id is used for... digging into the frame_desc
handling I have a slightly unrelated question if the PHY shouldn't
implement .get_frame_desc() instead of filtering what is returned from
the deser's implementation of .get_frame_desc() which is called by
cal_camerarx_get_remote_frame_desc(). If I'm not mitaken in that case
you could just call the PHY's .get_frame_desc() instead of inspecting its
routing table here. Does it make sense..

> +
> +		ctx->stream = route->sink_stream;
> +
> +		v4l2_subdev_unlock_state(state);
> +	}
> +
>  	ret = video_device_pipeline_alloc_start(&ctx->vdev);
>  	if (ret < 0) {
>  		ctx_err(ctx, "Failed to start media pipeline: %d\n", ret);
> @@ -775,7 +823,8 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	cal_ctx_set_dma_addr(ctx, addr);
>  	cal_ctx_start(ctx);
>
> -	ret = v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 1);
> +	ret = v4l2_subdev_enable_streams(&ctx->phy->subdev, remote_pad->index,
> +					 BIT(0));
>  	if (ret)
>  		goto error_stop;
>
> @@ -800,10 +849,14 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>  static void cal_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
> +	struct media_pad *remote_pad;
>
>  	cal_ctx_stop(ctx);
>
> -	v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 0);
> +	remote_pad = media_pad_remote_pad_first(&ctx->pad);
> +
> +	v4l2_subdev_disable_streams(&ctx->phy->subdev, remote_pad->index,
> +				    BIT(0));
>
>  	pm_runtime_put_sync(ctx->cal->dev);
>
> @@ -812,6 +865,9 @@ static void cal_stop_streaming(struct vb2_queue *vq)
>  	cal_release_buffers(ctx, VB2_BUF_STATE_ERROR);
>
>  	video_device_pipeline_stop(&ctx->vdev);
> +
> +	if (cal_mc_api)
> +		ctx->phy = NULL;
>  }
>
>  static const struct vb2_ops cal_video_qops = {
> @@ -845,6 +901,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
>  	const struct cal_format_info *fmtinfo;
>  	unsigned int i, j, k;
>  	int ret = 0;
> +	struct v4l2_subdev *sd = ctx->phy->source;

Move it up ? :P

The rest (for my undestanding) seems right. By dropping the !mc
support things would look much simpler :)


>
>  	/* Enumerate sub device formats and enable all matching local formats */
>  	ctx->active_fmt = devm_kcalloc(ctx->cal->dev, cal_num_formats,
> @@ -859,20 +916,20 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
>  		memset(&mbus_code, 0, sizeof(mbus_code));
>  		mbus_code.index = j;
>  		mbus_code.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> -		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_mbus_code,
> -				       NULL, &mbus_code);
> +		ret = v4l2_subdev_call_state_active(sd, pad, enum_mbus_code,
> +						    &mbus_code);
>  		if (ret == -EINVAL)
>  			break;
>
>  		if (ret) {
>  			ctx_err(ctx, "Error enumerating mbus codes in subdev %s: %d\n",
> -				ctx->phy->source->name, ret);
> +				sd->name, ret);
>  			return ret;
>  		}
>
>  		ctx_dbg(2, ctx,
>  			"subdev %s: code: %04x idx: %u\n",
> -			ctx->phy->source->name, mbus_code.code, j);
> +			sd->name, mbus_code.code, j);
>
>  		for (k = 0; k < cal_num_formats; k++) {
>  			fmtinfo = &cal_formats[k];
> @@ -890,7 +947,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
>
>  	if (i == 0) {
>  		ctx_err(ctx, "No suitable format reported by subdev %s\n",
> -			ctx->phy->source->name);
> +			sd->name);
>  		return -EINVAL;
>  	}
>
> @@ -976,16 +1033,49 @@ int cal_ctx_v4l2_register(struct cal_ctx *ctx)
>  		return ret;
>  	}
>
> -	ret = media_create_pad_link(&ctx->phy->subdev.entity,
> -				    CAL_CAMERARX_PAD_FIRST_SOURCE,
> -				    &vfd->entity, 0,
> -				    MEDIA_LNK_FL_IMMUTABLE |
> -				    MEDIA_LNK_FL_ENABLED);
> -	if (ret) {
> -		ctx_err(ctx, "Failed to create media link for context %u\n",
> -			ctx->dma_ctx);
> -		video_unregister_device(vfd);
> -		return ret;
> +	if (cal_mc_api) {
> +		u16 phy_idx;
> +		u16 pad_idx;
> +
> +		/* Create links from all video nodes to all PHYs */
> +
> +		for (phy_idx = 0; phy_idx < ctx->cal->data->num_csi2_phy;
> +		     ++phy_idx) {
> +			for (pad_idx = 1; pad_idx < CAL_CAMERARX_NUM_PADS;
> +			     ++pad_idx) {
> +				/*
> +				 * Enable only links from video0 to PHY0 pad 1,
> +				 * and video1 to PHY1 pad 1.
> +				 */
> +				bool enable = (ctx->dma_ctx == 0 &&
> +					       phy_idx == 0 && pad_idx == 1) ||
> +					      (ctx->dma_ctx == 1 &&
> +					       phy_idx == 1 && pad_idx == 1);
> +
> +				ret = media_create_pad_link(
> +					&ctx->cal->phy[phy_idx]->subdev.entity,
> +					pad_idx, &vfd->entity, 0,
> +					enable ? MEDIA_LNK_FL_ENABLED : 0);
> +				if (ret) {
> +					ctx_err(ctx,
> +						"Failed to create media link for context %u\n",
> +						ctx->dma_ctx);
> +					video_unregister_device(vfd);
> +					return ret;
> +				}
> +			}
> +		}
> +	} else {
> +		ret = media_create_pad_link(&ctx->phy->subdev.entity,
> +			CAL_CAMERARX_PAD_FIRST_SOURCE, &vfd->entity, 0,
> +			MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> +		if (ret) {
> +			ctx_err(ctx,
> +				"Failed to create media link for context %u\n",
> +				ctx->dma_ctx);
> +			video_unregister_device(vfd);
> +			return ret;
> +		}
>  	}
>
>  	ctx_info(ctx, "V4L2 device registered as %s\n",
> diff --git a/drivers/media/platform/ti/cal/cal.c b/drivers/media/platform/ti/cal/cal.c
> index 053bf1030af0..074bf33c3697 100644
> --- a/drivers/media/platform/ti/cal/cal.c
> +++ b/drivers/media/platform/ti/cal/cal.c
> @@ -495,10 +495,11 @@ static bool cal_ctx_wr_dma_stopped(struct cal_ctx *ctx)
>  }
>
>  static int
> -cal_get_remote_frame_desc_entry(struct cal_camerarx *phy,
> +cal_get_remote_frame_desc_entry(struct cal_camerarx *phy, u32 stream,
>  				struct v4l2_mbus_frame_desc_entry *entry)
>  {
>  	struct v4l2_mbus_frame_desc fd;
> +	unsigned int i;
>  	int ret;
>
>  	ret = cal_camerarx_get_remote_frame_desc(phy, &fd);
> @@ -509,20 +510,18 @@ cal_get_remote_frame_desc_entry(struct cal_camerarx *phy,
>  		return ret;
>  	}
>
> -	if (fd.num_entries == 0) {
> -		dev_err(phy->cal->dev,
> -			"No streams found in the remote frame descriptor\n");
> -
> -		return -ENODEV;
> +	for (i = 0; i < fd.num_entries; i++) {
> +		if (stream == fd.entry[i].stream) {
> +			*entry = fd.entry[i];
> +			return 0;
> +		}
>  	}
>
> -	if (fd.num_entries > 1)
> -		dev_dbg(phy->cal->dev,
> -			"Multiple streams not supported in remote frame descriptor, using the first one\n");
> +	dev_err(phy->cal->dev,
> +		"Failed to find stream %u from remote frame descriptor\n",
> +		stream);
>
> -	*entry = fd.entry[0];
> -
> -	return 0;
> +	return -ENODEV;
>  }
>
>  int cal_ctx_prepare(struct cal_ctx *ctx)
> @@ -530,14 +529,15 @@ int cal_ctx_prepare(struct cal_ctx *ctx)
>  	struct v4l2_mbus_frame_desc_entry entry;
>  	int ret;
>
> -	ret = cal_get_remote_frame_desc_entry(ctx->phy, &entry);
> +	ret = cal_get_remote_frame_desc_entry(ctx->phy, ctx->stream, &entry);
>
>  	if (ret == -ENOIOCTLCMD) {
>  		ctx->vc = 0;
>  		ctx->datatype = CAL_CSI2_CTX_DT_ANY;
>  	} else if (!ret) {
> -		ctx_dbg(2, ctx, "Framedesc: len %u, vc %u, dt %#x\n",
> -			entry.length, entry.bus.csi2.vc, entry.bus.csi2.dt);
> +		ctx_dbg(2, ctx, "Framedesc: stream %u, len %u, vc %u, dt %#x\n",
> +			entry.stream, entry.length, entry.bus.csi2.vc,
> +			entry.bus.csi2.dt);
>
>  		ctx->vc = entry.bus.csi2.vc;
>  		ctx->datatype = entry.bus.csi2.dt;
> @@ -1069,10 +1069,10 @@ static struct cal_ctx *cal_ctx_create(struct cal_dev *cal, int inst)
>  		return NULL;
>
>  	ctx->cal = cal;
> -	ctx->phy = cal->phy[inst];
>  	ctx->dma_ctx = inst;
>  	ctx->csi2_ctx = inst;
>  	ctx->cport = inst;
> +	ctx->stream = 0;
>
>  	ret = cal_ctx_v4l2_init(ctx);
>  	if (ret) {
> @@ -1281,18 +1281,33 @@ static int cal_probe(struct platform_device *pdev)
>  	}
>
>  	/* Create contexts. */
> -	for (i = 0; i < cal->data->num_csi2_phy; ++i) {
> -		if (!cal->phy[i]->source_node)
> -			continue;
> +	if (!cal_mc_api) {
> +		for (i = 0; i < cal->data->num_csi2_phy; ++i) {
> +			if (!cal->phy[i]->source_node)
> +				continue;
> +
> +			cal->ctx[cal->num_contexts] = cal_ctx_create(cal, i);
> +			if (!cal->ctx[cal->num_contexts]) {
> +				cal_err(cal, "Failed to create context %u\n", cal->num_contexts);
> +				ret = -ENODEV;
> +				goto error_context;
> +			}
> +
> +			cal->ctx[cal->num_contexts]->phy = cal->phy[i];
>
> -		cal->ctx[cal->num_contexts] = cal_ctx_create(cal, i);
> -		if (!cal->ctx[cal->num_contexts]) {
> -			cal_err(cal, "Failed to create context %u\n", cal->num_contexts);
> -			ret = -ENODEV;
> -			goto error_context;
> +			cal->num_contexts++;
>  		}
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(cal->ctx); ++i) {
> +			cal->ctx[i] = cal_ctx_create(cal, i);
> +			if (!cal->ctx[i]) {
> +				cal_err(cal, "Failed to create context %u\n", i);
> +				ret = -ENODEV;
> +				goto error_context;
> +			}
>
> -		cal->num_contexts++;
> +			cal->num_contexts++;
> +		}
>  	}
>
>  	/* Register the media device. */
> diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
> index 79cd0171e701..e1f693bbeb07 100644
> --- a/drivers/media/platform/ti/cal/cal.h
> +++ b/drivers/media/platform/ti/cal/cal.h
> @@ -45,7 +45,7 @@
>
>  #define CAL_CAMERARX_PAD_SINK		0
>  #define CAL_CAMERARX_PAD_FIRST_SOURCE	1
> -#define CAL_CAMERARX_NUM_SOURCE_PADS	1
> +#define CAL_CAMERARX_NUM_SOURCE_PADS	8
>  #define CAL_CAMERARX_NUM_PADS		(1 + CAL_CAMERARX_NUM_SOURCE_PADS)
>
>  static inline bool cal_rx_pad_is_sink(u32 pad)
> @@ -247,6 +247,7 @@ struct cal_ctx {
>  	u8			pix_proc;
>  	u8			vc;
>  	u8			datatype;
> +	u32			stream;
>
>  	bool			use_pix_proc;
>  };
> @@ -322,6 +323,7 @@ void cal_quickdump_regs(struct cal_dev *cal);
>
>  int cal_camerarx_get_remote_frame_desc(struct cal_camerarx *phy,
>  				       struct v4l2_mbus_frame_desc *desc);
> +struct cal_camerarx *cal_camerarx_get_phy_from_entity(struct media_entity *entity);
>  void cal_camerarx_disable(struct cal_camerarx *phy);
>  void cal_camerarx_i913_errata(struct cal_camerarx *phy);
>  struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
> --
> 2.34.1
>

Re: [PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[niklas soderlund]

Hello,

On 2023-02-24 16:48:55 +0100, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
> > Add routing and stream_config support to CAL driver.
> >
> > Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
> > separate streams at the same time.
> >
> > Add 8 video device nodes, each representing a single dma-engine, and set
> > the number of source pads on camerarx to 8. Each video node can be
> > connected to any of the source pads on either of the camerarx instances
> > using media links. Camerarx internal routing is used to route the
> > incoming CSI-2 streams to one of the 8 source pads.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
> >  drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
> >  drivers/media/platform/ti/cal/cal.c          |  65 ++++--
> >  drivers/media/platform/ti/cal/cal.h          |   4 +-
> >  4 files changed, 342 insertions(+), 106 deletions(-)
> >
> > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> > index faafbd0e9240..49ae29065cd1 100644
> > --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> > @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
> >  {
> >  	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
> >  	u32 num_lanes = mipi_csi2->num_data_lanes;
> > -	const struct cal_format_info *fmtinfo;
> >  	struct v4l2_subdev_state *state;
> > -	struct v4l2_mbus_framefmt *fmt;
> >  	u32 bpp;
> >  	s64 freq;
> >
> > -	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > +	/*
> > +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> > +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> > +	 *
> > +	 * With multistream input there is no single pixel rate, and thus we
> > +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> > +	 * causes v4l2_get_link_freq() to return an error if it falls back to
> > +	 * V4L2_CID_PIXEL_RATE.
> > +	 */
> 
> To recap a bit of our offline discussion:
> - max9286 GMSL deserializer (as a comparison for a multiplexed
>   transmitter) use PIXEL_RATE to report the cumulative pixel rate of
>   enabled transmitters. This is because the R-Car CSI-2 receiver on
>   which use PIXEL_RATE to compute the link freq [1]
> 
> - according to [2]
>   pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)
> 
>   from which:
>   link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)
> 
>   This works as long the reported pixel rate includes visible and
>   blankings, something I'm not sure how many transmitters handle
>   correctly as PIXEL_RATE control is meant to report the visible pixel
>   sampling rate on the pixel array.
> 
> I guess we should go towards mandating LINK_FREQ for transmitters.
> 
> cc-Niklas for opinions on R-Car CSI-2 rcsi2_calc_mbps()

Thanks for the ping.

The choice to use the PIXEL_RATE instead of the LINK_FREQ control for 
the R-Car CSI-2 was originally because the ADV748x which was the first 
CSI-2 transmitter used during development.

AFIK the ADV748x adjusts the CSI-2 TX link frequency to match the pixel 
clock. This results in quiet a big range of possible values that need to 
be communicated between the two sub devices. The V4L2_CID_LINK_FREQ 
control is a V4L2_CTRL_TYPE_INTEGER_MENU which do not render itself to 
report the large range of values needed.

When we added MAX9286 and friends to the mix, we built on-top of this by 
reporting the total pixel rate of all streams being transmitted on the 
CSI-2 link. IMHO the v4l2_get_link_freq() was an OK middle ground on how 
to align the two use-cases.

I agree that situation is not ideal. And in a perfect world a control 
other then PIXEL_RATE would be used for the R-Car CSI-2 driver, but no 
such control exists. And chancing the control type of LINK_FREQ is not a 
good idea as that is usually specified in as a list in DT.

Adding a new control DYNAMIC_LINK_FREQ and wire that into 
v4l2_get_link_freq() ?

> 
> [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c#L608
> [2] https://www.kernel.org/doc/html/latest/driver-api/media/tx-rx.html#csi-2-transmitter-drivers
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[Jacopo Mondi]

Hi Niklas

On Fri, Feb 24, 2023 at 06:20:49PM +0100, niklas soderlund wrote:
> Hello,
>
> On 2023-02-24 16:48:55 +0100, Jacopo Mondi wrote:
> > Hi Tomi
> >
> > On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
> > > Add routing and stream_config support to CAL driver.
> > >
> > > Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
> > > separate streams at the same time.
> > >
> > > Add 8 video device nodes, each representing a single dma-engine, and set
> > > the number of source pads on camerarx to 8. Each video node can be
> > > connected to any of the source pads on either of the camerarx instances
> > > using media links. Camerarx internal routing is used to route the
> > > incoming CSI-2 streams to one of the 8 source pads.
> > >
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
> > >  drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
> > >  drivers/media/platform/ti/cal/cal.c          |  65 ++++--
> > >  drivers/media/platform/ti/cal/cal.h          |   4 +-
> > >  4 files changed, 342 insertions(+), 106 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > index faafbd0e9240..49ae29065cd1 100644
> > > --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> > > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
> > >  {
> > >  	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
> > >  	u32 num_lanes = mipi_csi2->num_data_lanes;
> > > -	const struct cal_format_info *fmtinfo;
> > >  	struct v4l2_subdev_state *state;
> > > -	struct v4l2_mbus_framefmt *fmt;
> > >  	u32 bpp;
> > >  	s64 freq;
> > >
> > > -	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > > +	/*
> > > +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> > > +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> > > +	 *
> > > +	 * With multistream input there is no single pixel rate, and thus we
> > > +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> > > +	 * causes v4l2_get_link_freq() to return an error if it falls back to
> > > +	 * V4L2_CID_PIXEL_RATE.
> > > +	 */
> >
> > To recap a bit of our offline discussion:
> > - max9286 GMSL deserializer (as a comparison for a multiplexed
> >   transmitter) use PIXEL_RATE to report the cumulative pixel rate of
> >   enabled transmitters. This is because the R-Car CSI-2 receiver on
> >   which use PIXEL_RATE to compute the link freq [1]
> >
> > - according to [2]
> >   pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)
> >
> >   from which:
> >   link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)
> >
> >   This works as long the reported pixel rate includes visible and
> >   blankings, something I'm not sure how many transmitters handle
> >   correctly as PIXEL_RATE control is meant to report the visible pixel
> >   sampling rate on the pixel array.
> >
> > I guess we should go towards mandating LINK_FREQ for transmitters.
> >
> > cc-Niklas for opinions on R-Car CSI-2 rcsi2_calc_mbps()
>
> Thanks for the ping.
>
> The choice to use the PIXEL_RATE instead of the LINK_FREQ control for
> the R-Car CSI-2 was originally because the ADV748x which was the first
> CSI-2 transmitter used during development.
>
> AFIK the ADV748x adjusts the CSI-2 TX link frequency to match the pixel
> clock. This results in quiet a big range of possible values that need to
> be communicated between the two sub devices. The V4L2_CID_LINK_FREQ
> control is a V4L2_CTRL_TYPE_INTEGER_MENU which do not render itself to
> report the large range of values needed.

I see the HDMI and analog front-end adjusting the value of PIXEL_RATE
control through adv748x_csi2_set_pixelrate() which however only update
the value of the control. The ADV748x automatically adjusts the "MIPI
output frequency" (Chapter 9.6 of the chip manual) according to the
pixel rate, so I concur it is not possible/resonable to express all
the possible values as menu control items.

To be honest I always had troubles with LINK_FREQ being a menu
control, as it requires to pre-calculate all of the possible values it
can assume when the bus link frequency varies according to the pixel
rate or to the image format and output size.

>
> When we added MAX9286 and friends to the mix, we built on-top of this by
> reporting the total pixel rate of all streams being transmitted on the
> CSI-2 link. IMHO the v4l2_get_link_freq() was an OK middle ground on how
> to align the two use-cases.
>
> I agree that situation is not ideal. And in a perfect world a control
> other then PIXEL_RATE would be used for the R-Car CSI-2 driver, but no
> such control exists. And chancing the control type of LINK_FREQ is not a
> good idea as that is usually specified in as a list in DT.
>

I agree with most of this. I'm also concerned changing a control type
which userspace has access to might be considered a regression, so I
guess we have to live with LINK_FREQ as a menu control.

> Adding a new control DYNAMIC_LINK_FREQ and wire that into
> v4l2_get_link_freq() ?
>

I wonder if we shouldn't instead move away from controls and report
the CSI-2 link frequency between the transmitter and the receiver
drivers through the .get_mbus_config pad operation..

> >
> > [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c#L608
> > [2] https://www.kernel.org/doc/html/latest/driver-api/media/tx-rx.html#csi-2-transmitter-drivers
> >
>
> --
> Kind Regards,
> Niklas Söderlund

Re: [PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[Tomi Valkeinen]

Hi Jacopo,

On 24/02/2023 17:48, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
>> Add routing and stream_config support to CAL driver.
>>
>> Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
>> separate streams at the same time.
>>
>> Add 8 video device nodes, each representing a single dma-engine, and set
>> the number of source pads on camerarx to 8. Each video node can be
>> connected to any of the source pads on either of the camerarx instances
>> using media links. Camerarx internal routing is used to route the
>> incoming CSI-2 streams to one of the 8 source pads.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
>>   drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
>>   drivers/media/platform/ti/cal/cal.c          |  65 ++++--
>>   drivers/media/platform/ti/cal/cal.h          |   4 +-
>>   4 files changed, 342 insertions(+), 106 deletions(-)
>>
>> diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
>> index faafbd0e9240..49ae29065cd1 100644
>> --- a/drivers/media/platform/ti/cal/cal-camerarx.c
>> +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
>> @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
>>   {
>>   	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
>>   	u32 num_lanes = mipi_csi2->num_data_lanes;
>> -	const struct cal_format_info *fmtinfo;
>>   	struct v4l2_subdev_state *state;
>> -	struct v4l2_mbus_framefmt *fmt;
>>   	u32 bpp;
>>   	s64 freq;
>>
>> -	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
>> +	/*
>> +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
>> +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
>> +	 *
>> +	 * With multistream input there is no single pixel rate, and thus we
>> +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
>> +	 * causes v4l2_get_link_freq() to return an error if it falls back to
>> +	 * V4L2_CID_PIXEL_RATE.
>> +	 */
> 
> To recap a bit of our offline discussion:
> - max9286 GMSL deserializer (as a comparison for a multiplexed
>    transmitter) use PIXEL_RATE to report the cumulative pixel rate of
>    enabled transmitters. This is because the R-Car CSI-2 receiver on
>    which use PIXEL_RATE to compute the link freq [1]
> 
> - according to [2]
>    pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)
> 
>    from which:
>    link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)
> 
>    This works as long the reported pixel rate includes visible and
>    blankings, something I'm not sure how many transmitters handle
>    correctly as PIXEL_RATE control is meant to report the visible pixel
>    sampling rate on the pixel array.
> 
> I guess we should go towards mandating LINK_FREQ for transmitters.
> 
> cc-Niklas for opinions on R-Car CSI-2 rcsi2_calc_mbps()
> 
> [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c#L608
> [2] https://www.kernel.org/doc/html/latest/driver-api/media/tx-rx.html#csi-2-transmitter-drivers
> 
>>
>> -	fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
>> +	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
>>
>> -	fmtinfo = cal_format_by_code(fmt->code);
>> -	if (!fmtinfo)
>> +	if (state->routing.num_routes == 0)
>>   		return -EINVAL;
> 
> This function is in the call path of .enable_streams which if I'm not
> reading the code wrong is called with
> 
> int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
> {
> 
>          ...
> 
> 	for_each_active_route(&state->routing, route)
> 		source_mask |= BIT_ULL(route->source_stream);
> 
> 	if (enable)
> 		return v4l2_subdev_enable_streams(sd, pad_index, source_mask);
> 
>          ...
> }
> 
> int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> 			       u64 streams_mask)
> {
> 	struct device *dev = sd->entity.graph_obj.mdev->dev;
> 	struct v4l2_subdev_state *state;
> 	u64 found_streams = 0;
> 	unsigned int i;
> 	int ret;
> 
> 	/* A few basic sanity checks first. */
> 	if (pad >= sd->entity.num_pads)
> 		return -EINVAL;
> 
> 	if (!streams_mask)
> 		return 0;
> 
>          ...
> }
> 
> So the question is: can we get to enable_streams without enabled
> routes (sorry I should have tested before asking it but I don't have a
> multiplexed setup easily accessible) ?

No, I don't think we can. Why do you ask?

>>
>> -	bpp = fmtinfo->bpp;
>> +	if (state->routing.num_routes > 1) {
>> +		bpp = 0;
>> +	} else {
>> +		const struct cal_format_info *fmtinfo;
>> +		struct v4l2_subdev_route *route = &state->routing.routes[0];
> 
> Nit: if I'm not mistaken along the driver reverse-xmas-tree (I
> know...) is respected. Can you do it here as well ?

Sure.

>> +		struct v4l2_mbus_framefmt *fmt;
>> +
>> +		fmt = v4l2_subdev_state_get_stream_format(
>> +			state, route->sink_pad, route->sink_stream);
>> +
>> +		fmtinfo = cal_format_by_code(fmt->code);
>> +		if (!fmtinfo)
>> +			return -EINVAL;
>> +
>> +		bpp = fmtinfo->bpp;
>> +	}
>>
>>   	freq = v4l2_get_link_freq(phy->source->ctrl_handler, bpp, 2 * num_lanes);
>>   	if (freq < 0) {
>> @@ -284,15 +304,28 @@ static void cal_camerarx_ppi_disable(struct cal_camerarx *phy)
>>   			0, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
>>   }
>>
>> -static int cal_camerarx_start(struct cal_camerarx *phy)
>> +static int cal_camerarx_start(struct cal_camerarx *phy, u32 pad, u32 stream)
>>   {
>> +	struct media_pad *remote_pad;
>>   	s64 link_freq;
>>   	u32 sscounter;
>>   	u32 val;
>>   	int ret;
>>
>> +	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
>> +
> 
> Would it hurt a comment here to explain that in case it's the first to
> be enabled you need to start the RX and power up the transmitter,
> otherwise it's enough to enable the additional stream ?

I'll add a comment.

>>   	if (phy->enable_count > 0) {
>>   		phy->enable_count++;
>> +
>> +		ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
>> +						 BIT(stream));
>> +		if (ret) {
>> +			phy->enable_count--;
> 
> You can avoid this by enable_count++ after the error check ?

Yep.

>> +
>> +			phy_err(phy, "enable streams failed in source: %d\n", ret);
>> +			return ret;
>> +		}
>> +
>>   		return 0;
>>   	}
>>
>> @@ -394,7 +427,9 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
>>   	 * Start the source to enable the CSI-2 HS clock. We can now wait for
>>   	 * CSI-2 PHY reset to complete.
>>   	 */
>> -	ret = v4l2_subdev_call(phy->source, video, s_stream, 1);
>> +
> 
> Intentional additional blank line ?

No.

>> +	ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
>> +					 BIT(stream));
>>   	if (ret) {
>>   		v4l2_subdev_call(phy->source, core, s_power, 0);
>>   		cal_camerarx_disable_irqs(phy);
>> @@ -425,12 +460,22 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
>>   	return 0;
>>   }
>>
>> -static void cal_camerarx_stop(struct cal_camerarx *phy)
>> +static void cal_camerarx_stop(struct cal_camerarx *phy, u32 pad, u32 stream)
>>   {
>> +	struct media_pad *remote_pad;
>>   	int ret;
>>
>> -	if (--phy->enable_count > 0)
>> +	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
>> +
>> +	if (--phy->enable_count > 0) {
>> +		ret = v4l2_subdev_disable_streams(phy->source,
>> +						  remote_pad->index,
>> +						  BIT(stream));
>> +		if (ret)
>> +			phy_err(phy, "stream off failed in subdev\n");
>> +
>>   		return;
>> +	}
>>
>>   	cal_camerarx_ppi_disable(phy);
>>
>> @@ -450,7 +495,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy)
>>   	/* Disable the phy */
>>   	cal_camerarx_disable(phy);
>>
>> -	if (v4l2_subdev_call(phy->source, video, s_stream, 0))
>> +	ret = v4l2_subdev_disable_streams(phy->source, remote_pad->index,
>> +					  BIT(stream));
>> +	if (ret)
>>   		phy_err(phy, "stream off failed in subdev\n");
>>
>>   	ret = v4l2_subdev_call(phy->source, core, s_power, 0);
>> @@ -626,30 +673,62 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
>>   	return container_of(sd, struct cal_camerarx, subdev);
>>   }
>>
>> -static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
>> +struct cal_camerarx *
>> +cal_camerarx_get_phy_from_entity(struct media_entity *entity)
>> +{
>> +	struct v4l2_subdev *sd;
>> +
>> +	sd = media_entity_to_v4l2_subdev(entity);
>> +	if (!sd)
>> +		return NULL;
>> +
>> +	return to_cal_camerarx(sd);
>> +}
>> +
>> +static int cal_camerarx_sd_enable_streams(struct v4l2_subdev *sd,
>> +					  struct v4l2_subdev_state *state,
>> +					  u32 pad, u64 streams_mask)
>>   {
>>   	struct cal_camerarx *phy = to_cal_camerarx(sd);
>> -	struct v4l2_subdev_state *state;
>> -	int ret = 0;
>> +	u32 other_pad, other_stream;
>> +	int ret;
>>
>> -	state = v4l2_subdev_lock_and_get_active_state(sd);
>> +	if (WARN_ON(streams_mask != 1))
> 
> as streams_mask is unsigned and you can't get here if streams_mask ==
> 0, I wonder if checking for > 1 isn't more explicit. A detail though.

Well, we need streams_mask to be exactly 1. I think the current test 
says that. If we'd check for > 1, the reader's thoughts might be that 0 
and 1 are ok, which is not the case.

Then again, I think the whole check can be dropped. We're calling it 
from inside the CAL driver, always with 1, so there's little chance it 
could be != 1.

>> +		return -EINVAL;
>>
>> -	if (enable)
>> -		ret = cal_camerarx_start(phy);
>> -	else
>> -		cal_camerarx_stop(phy);
>> +	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
>> +						    &other_pad, &other_stream);
>> +	if (ret)
>> +		return ret;
>>
>> -	v4l2_subdev_unlock_state(state);
>> +	return cal_camerarx_start(phy, other_pad, other_stream);
>> +}
>>
>> -	return ret;
>> +static int cal_camerarx_sd_disable_streams(struct v4l2_subdev *sd,
>> +					   struct v4l2_subdev_state *state,
>> +					   u32 pad, u64 streams_mask)
>> +{
>> +	struct cal_camerarx *phy = to_cal_camerarx(sd);
>> +	u32 other_pad, other_stream;
>> +	int ret;
>> +
>> +	if (WARN_ON(streams_mask != 1))
>> +		return -EINVAL;
>> +
>> +	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
>> +						    &other_pad, &other_stream);
>> +	if (ret)
>> +		return ret;
>> +
>> +	cal_camerarx_stop(phy, other_pad, other_stream);
>> +
>> +	return 0;
>>   }
>>
>>   static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>>   					  struct v4l2_subdev_state *state,
>>   					  struct v4l2_subdev_mbus_code_enum *code)
>>   {
>> -	struct cal_camerarx *phy = to_cal_camerarx(sd);
>> -
>>   	/* No transcoding, source and sink codes must match. */
>>   	if (cal_rx_pad_is_source(code->pad)) {
>>   		struct v4l2_mbus_framefmt *fmt;
>> @@ -657,8 +736,12 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
>>   		if (code->index > 0)
>>   			return -EINVAL;
>>
>> -		fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
>> -						 CAL_CAMERARX_PAD_SINK);
>> +		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
>> +								   code->pad,
>> +								   code->stream);
>> +		if (!fmt)
>> +			return -EINVAL;
>> +
>>   		code->code = fmt->code;
>>   	} else {
>>   		if (code->index >= cal_num_formats)
>> @@ -683,8 +766,12 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
>>   	if (cal_rx_pad_is_source(fse->pad)) {
>>   		struct v4l2_mbus_framefmt *fmt;
>>
>> -		fmt = v4l2_subdev_get_pad_format(sd, state,
>> -						 CAL_CAMERARX_PAD_SINK);
>> +		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
>> +								   fse->pad,
>> +								   fse->stream);
>> +		if (!fmt)
>> +			return -EINVAL;
>> +
>>   		if (fse->code != fmt->code)
>>   			return -EINVAL;
>>
>> @@ -740,57 +827,96 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
>>
>>   	/* Store the format and propagate it to the source pad. */
>>
>> -	fmt = v4l2_subdev_get_pad_format(sd, state, CAL_CAMERARX_PAD_SINK);
>> +	fmt = v4l2_subdev_state_get_stream_format(state, format->pad,
>> +						  format->stream);
>> +	if (!fmt)
>> +		return -EINVAL;
>> +
>>   	*fmt = format->format;
>>
>> -	fmt = v4l2_subdev_get_pad_format(sd, state,
>> -					 CAL_CAMERARX_PAD_FIRST_SOURCE);
>> +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
>> +							   format->stream);
>> +	if (!fmt)
>> +		return -EINVAL;
>> +
>>   	*fmt = format->format;
>>
>>   	return 0;
>>   }
>>
>> +static int _cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
>> +					struct v4l2_subdev_state *state,
>> +					struct v4l2_subdev_krouting *routing)
> 
> Functions starting with _ are a bit unusual :)

Hmm, well... I don't have any ideas for a good name =). 
cal_camerarx_sd_set_routing_impl() ?

>> +{
>> +	static const struct v4l2_mbus_framefmt format = {
>> +		.width = 640,
>> +		.height = 480,
>> +		.code = MEDIA_BUS_FMT_UYVY8_2X8,
>> +		.field = V4L2_FIELD_NONE,
>> +		.colorspace = V4L2_COLORSPACE_SRGB,
>> +		.ycbcr_enc = V4L2_YCBCR_ENC_601,
>> +		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
>> +		.xfer_func = V4L2_XFER_FUNC_SRGB,
>> +	};
>> +	int ret;
>> +
>> +	ret = v4l2_subdev_routing_validate(sd, routing, V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
>> +				       struct v4l2_subdev_state *state,
>> +				       enum v4l2_subdev_format_whence which,
>> +				       struct v4l2_subdev_krouting *routing)
>> +{
>> +	return _cal_camerarx_sd_set_routing(sd, state, routing);
>> +}
>> +
>>   static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
>>   				    struct v4l2_subdev_state *state)
>>   {
>> -	struct v4l2_subdev_format format = {
>> -		.which = state ? V4L2_SUBDEV_FORMAT_TRY
>> -		: V4L2_SUBDEV_FORMAT_ACTIVE,
>> -		.pad = CAL_CAMERARX_PAD_SINK,
>> -		.format = {
>> -			.width = 640,
>> -			.height = 480,
>> -			.code = MEDIA_BUS_FMT_UYVY8_2X8,
>> -			.field = V4L2_FIELD_NONE,
>> -			.colorspace = V4L2_COLORSPACE_SRGB,
>> -			.ycbcr_enc = V4L2_YCBCR_ENC_601,
>> -			.quantization = V4L2_QUANTIZATION_LIM_RANGE,
>> -			.xfer_func = V4L2_XFER_FUNC_SRGB,
>> -		},
>> +	struct v4l2_subdev_route routes[] = { {
>> +		.sink_pad = 0,
>> +		.sink_stream = 0,
>> +		.source_pad = 1,
>> +		.source_stream = 0,
>> +		.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
>> +	} };
>> +
>> +	struct v4l2_subdev_krouting routing = {
>> +		.num_routes = 1,
>> +		.routes = routes,
>>   	};
>>
>> -	return cal_camerarx_sd_set_fmt(sd, state, &format);
>> +	/* Initialize routing to single route to the fist source pad */
>> +	return _cal_camerarx_sd_set_routing(sd, state, &routing);
>>   }
>>
>> -static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = {
>> -	.s_stream = cal_camerarx_sd_s_stream,
>> -};
>> -
>>   static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = {
>> +	.enable_streams = cal_camerarx_sd_enable_streams,
>> +	.disable_streams = cal_camerarx_sd_disable_streams,
>>   	.init_cfg = cal_camerarx_sd_init_cfg,
>>   	.enum_mbus_code = cal_camerarx_sd_enum_mbus_code,
>>   	.enum_frame_size = cal_camerarx_sd_enum_frame_size,
>>   	.get_fmt = v4l2_subdev_get_fmt,
>>   	.set_fmt = cal_camerarx_sd_set_fmt,
>> +	.set_routing = cal_camerarx_sd_set_routing,
>>   };
>>
>>   static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = {
>> -	.video = &cal_camerarx_video_ops,
>>   	.pad = &cal_camerarx_pad_ops,
>>   };
>>
>>   static struct media_entity_operations cal_camerarx_media_ops = {
>>   	.link_validate = v4l2_subdev_link_validate,
>> +	.has_pad_interdep = v4l2_subdev_has_pad_interdep,
>>   };
>>
>>   /* ------------------------------------------------------------------
>> @@ -842,11 +968,12 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
>>   	sd = &phy->subdev;
>>   	v4l2_subdev_init(sd, &cal_camerarx_subdev_ops);
>>   	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
>> -	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
>>   	snprintf(sd->name, sizeof(sd->name), "CAMERARX%u", instance);
>>   	sd->dev = cal->dev;
>>
>>   	phy->pads[CAL_CAMERARX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>> +
> 
> Intentional ?
> 
>>   	for (i = CAL_CAMERARX_PAD_FIRST_SOURCE; i < CAL_CAMERARX_NUM_PADS; ++i)
>>   		phy->pads[i].flags = MEDIA_PAD_FL_SOURCE;
>>   	sd->entity.ops = &cal_camerarx_media_ops;
>> @@ -879,7 +1006,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
>>   		return;
>>
>>   	v4l2_device_unregister_subdev(&phy->subdev);
>> +
>>   	v4l2_subdev_cleanup(&phy->subdev);
>> +
> 
> ditto

I can drop these.

>>   	media_entity_cleanup(&phy->subdev.entity);
>>   	of_node_put(phy->source_ep_node);
>>   	of_node_put(phy->source_node);
>> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
>> index d363e123d4bb..71578bfc97ba 100644
>> --- a/drivers/media/platform/ti/cal/cal-video.c
>> +++ b/drivers/media/platform/ti/cal/cal-video.c
>> @@ -119,12 +119,13 @@ static int __subdev_get_format(struct cal_ctx *ctx,
>>   {
>>   	struct v4l2_subdev_format sd_fmt;
>>   	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
>> +	struct v4l2_subdev *sd = ctx->phy->source;
>>   	int ret;
>>
>>   	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>   	sd_fmt.pad = 0;
>>
>> -	ret = v4l2_subdev_call(ctx->phy->source, pad, get_fmt, NULL, &sd_fmt);
>> +	ret = v4l2_subdev_call_state_active(sd, pad, get_fmt, &sd_fmt);
>>   	if (ret)
>>   		return ret;
>>
>> @@ -141,13 +142,14 @@ static int __subdev_set_format(struct cal_ctx *ctx,
>>   {
>>   	struct v4l2_subdev_format sd_fmt;
>>   	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
>> +	struct v4l2_subdev *sd = ctx->phy->source;
>>   	int ret;
>>
>>   	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>   	sd_fmt.pad = 0;
>>   	*mbus_fmt = *fmt;
>>
>> -	ret = v4l2_subdev_call(ctx->phy->source, pad, set_fmt, NULL, &sd_fmt);
>> +	ret = v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
>>   	if (ret)
>>   		return ret;
>>
>> @@ -189,6 +191,7 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
>>   				      struct v4l2_format *f)
>>   {
>>   	struct cal_ctx *ctx = video_drvdata(file);
>> +	struct v4l2_subdev *sd = ctx->phy->source;
>>   	const struct cal_format_info *fmtinfo;
>>   	struct v4l2_subdev_frame_size_enum fse;
>>   	int found;
>> @@ -213,8 +216,8 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
>>   	for (fse.index = 0; ; fse.index++) {
>>   		int ret;
>>
>> -		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size,
>> -				       NULL, &fse);
>> +		ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size,
>> +						    &fse);
>>   		if (ret)
>>   			break;
>>
>> @@ -250,6 +253,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
>>   				    struct v4l2_format *f)
>>   {
>>   	struct cal_ctx *ctx = video_drvdata(file);
>> +	struct v4l2_subdev *sd = &ctx->phy->subdev;
>>   	struct vb2_queue *q = &ctx->vb_vidq;
>>   	struct v4l2_subdev_format sd_fmt = {
>>   		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>> @@ -289,7 +293,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
>>   	ctx->v_fmt.fmt.pix.field = sd_fmt.format.field;
>>   	cal_calc_format_size(ctx, fmtinfo, &ctx->v_fmt);
>>
>> -	v4l2_subdev_call(&ctx->phy->subdev, pad, set_fmt, NULL, &sd_fmt);
>> +	v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
>>
>>   	ctx->fmtinfo = fmtinfo;
>>   	*f = ctx->v_fmt;
>> @@ -301,6 +305,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
>>   				      struct v4l2_frmsizeenum *fsize)
>>   {
>>   	struct cal_ctx *ctx = video_drvdata(file);
>> +	struct v4l2_subdev *sd = ctx->phy->source;
>>   	const struct cal_format_info *fmtinfo;
>>   	struct v4l2_subdev_frame_size_enum fse;
>>   	int ret;
>> @@ -318,8 +323,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
>>   	fse.code = fmtinfo->code;
>>   	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>
>> -	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size, NULL,
>> -			       &fse);
>> +	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size, &fse);
>>   	if (ret)
>>   		return ret;
>>
>> @@ -361,6 +365,7 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
>>   					  struct v4l2_frmivalenum *fival)
>>   {
>>   	struct cal_ctx *ctx = video_drvdata(file);
>> +	struct v4l2_subdev *sd = ctx->phy->source;
>>   	const struct cal_format_info *fmtinfo;
>>   	struct v4l2_subdev_frame_interval_enum fie = {
>>   		.index = fival->index,
>> @@ -375,8 +380,8 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
>>   		return -EINVAL;
>>
>>   	fie.code = fmtinfo->code;
>> -	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_interval,
>> -			       NULL, &fie);
>> +
>> +	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_interval, &fie);
>>   	if (ret)
>>   		return ret;
>>   	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
>> @@ -694,8 +699,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>>   {
>>   	const struct cal_format_info *rx_fmtinfo;
>>   	const struct v4l2_mbus_framefmt *format;
>> -	struct v4l2_subdev_state *state;
>>   	struct media_pad *remote_pad;
>> +	struct v4l2_subdev_state *state;
>>   	int ret = 0;
>>
>>   	remote_pad = media_pad_remote_pad_first(&ctx->pad);
>> @@ -704,7 +709,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>>
>>   	state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
>>
>> -	format = v4l2_subdev_get_pad_format(&ctx->phy->subdev, state, remote_pad->index);
>> +	format = v4l2_subdev_state_get_stream_format(state, remote_pad->index,
>> +						     0);
>>   	if (!format) {
>>   		ret = -EINVAL;
>>   		goto out;
>> @@ -733,10 +739,52 @@ static int cal_video_check_format(struct cal_ctx *ctx)
>>   static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   {
>>   	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
>> +	struct media_pad *remote_pad;
>>   	struct cal_buffer *buf;
>>   	dma_addr_t addr;
>>   	int ret;
>>
>> +	remote_pad = media_pad_remote_pad_first(&ctx->pad);
>> +	if (!remote_pad) {
>> +		ctx_err(ctx, "Context not connected\n");
>> +		ret = -ENODEV;
>> +		goto error_release_buffers;
>> +	}
>> +
>> +	if (cal_mc_api) {
>> +		struct v4l2_subdev_route *route = NULL;
>> +		struct v4l2_subdev_route *r;
>> +		struct v4l2_subdev_state *state;
>> +
>> +		/* Find the PHY connected to this video device */
>> +
>> +		ctx->phy = cal_camerarx_get_phy_from_entity(remote_pad->entity);
>> +
>> +		state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
>> +
>> +		/* Find the stream */
>> +
>> +		for_each_active_route(&state->routing, r) {
>> +			if (r->source_pad != remote_pad->index)
>> +				continue;
>> +
>> +			route = r;
>> +
>> +			break;
>> +		}
>> +
>> +		if (!route) {
>> +			v4l2_subdev_unlock_state(state);
>> +			ctx_err(ctx, "Failed to find route\n");
>> +			ret = -ENODEV;
>> +			goto error_release_buffers;
>> +		}
> 
> If I got it right: we inspect the PHY routing table, pick the route that ends in
> the pad connected to the this video device and store the sink-stream
> id. The sink-stream id is used for... digging into the frame_desc
> handling I have a slightly unrelated question if the PHY shouldn't
> implement .get_frame_desc() instead of filtering what is returned from
> the deser's implementation of .get_frame_desc() which is called by
> cal_camerarx_get_remote_frame_desc(). If I'm not mitaken in that case
> you could just call the PHY's .get_frame_desc() instead of inspecting its
> routing table here. Does it make sense..

Indeed, this looks a bit odd. This patch has evolved along the stream 
series, so it has gone though almost total rewrites a few times. And 
there seem to be some odd designs like this.

I think adding .get_frame_desc() to the camerarx makes sense.

>> +
>> +		ctx->stream = route->sink_stream;
>> +
>> +		v4l2_subdev_unlock_state(state);
>> +	}
>> +
>>   	ret = video_device_pipeline_alloc_start(&ctx->vdev);
>>   	if (ret < 0) {
>>   		ctx_err(ctx, "Failed to start media pipeline: %d\n", ret);
>> @@ -775,7 +823,8 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	cal_ctx_set_dma_addr(ctx, addr);
>>   	cal_ctx_start(ctx);
>>
>> -	ret = v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 1);
>> +	ret = v4l2_subdev_enable_streams(&ctx->phy->subdev, remote_pad->index,
>> +					 BIT(0));
>>   	if (ret)
>>   		goto error_stop;
>>
>> @@ -800,10 +849,14 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   static void cal_stop_streaming(struct vb2_queue *vq)
>>   {
>>   	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
>> +	struct media_pad *remote_pad;
>>
>>   	cal_ctx_stop(ctx);
>>
>> -	v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 0);
>> +	remote_pad = media_pad_remote_pad_first(&ctx->pad);
>> +
>> +	v4l2_subdev_disable_streams(&ctx->phy->subdev, remote_pad->index,
>> +				    BIT(0));
>>
>>   	pm_runtime_put_sync(ctx->cal->dev);
>>
>> @@ -812,6 +865,9 @@ static void cal_stop_streaming(struct vb2_queue *vq)
>>   	cal_release_buffers(ctx, VB2_BUF_STATE_ERROR);
>>
>>   	video_device_pipeline_stop(&ctx->vdev);
>> +
>> +	if (cal_mc_api)
>> +		ctx->phy = NULL;
>>   }
>>
>>   static const struct vb2_ops cal_video_qops = {
>> @@ -845,6 +901,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
>>   	const struct cal_format_info *fmtinfo;
>>   	unsigned int i, j, k;
>>   	int ret = 0;
>> +	struct v4l2_subdev *sd = ctx->phy->source;
> 
> Move it up ? :P

Sure.

> The rest (for my undestanding) seems right. By dropping the !mc
> support things would look much simpler :)

Yes, I'd love to drop !mc =).

Thanks!

  Tomi

Re: [PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[Jacopo Mondi]

Hi Tomi

On Mon, Feb 27, 2023 at 10:35:23AM +0200, Tomi Valkeinen wrote:
> Hi Jacopo,
>
> On 24/02/2023 17:48, Jacopo Mondi wrote:
> > Hi Tomi
> >
> > On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
> > > Add routing and stream_config support to CAL driver.
> > >
> > > Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
> > > separate streams at the same time.
> > >
> > > Add 8 video device nodes, each representing a single dma-engine, and set
> > > the number of source pads on camerarx to 8. Each video node can be
> > > connected to any of the source pads on either of the camerarx instances
> > > using media links. Camerarx internal routing is used to route the
> > > incoming CSI-2 streams to one of the 8 source pads.
> > >
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > ---
> > >   drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
> > >   drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
> > >   drivers/media/platform/ti/cal/cal.c          |  65 ++++--
> > >   drivers/media/platform/ti/cal/cal.h          |   4 +-
> > >   4 files changed, 342 insertions(+), 106 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > index faafbd0e9240..49ae29065cd1 100644
> > > --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> > > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> > > @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
> > >   {
> > >   	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
> > >   	u32 num_lanes = mipi_csi2->num_data_lanes;
> > > -	const struct cal_format_info *fmtinfo;
> > >   	struct v4l2_subdev_state *state;
> > > -	struct v4l2_mbus_framefmt *fmt;
> > >   	u32 bpp;
> > >   	s64 freq;
> > >
> > > -	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > > +	/*
> > > +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> > > +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> > > +	 *
> > > +	 * With multistream input there is no single pixel rate, and thus we
> > > +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> > > +	 * causes v4l2_get_link_freq() to return an error if it falls back to
> > > +	 * V4L2_CID_PIXEL_RATE.
> > > +	 */
> >
> > To recap a bit of our offline discussion:
> > - max9286 GMSL deserializer (as a comparison for a multiplexed
> >    transmitter) use PIXEL_RATE to report the cumulative pixel rate of
> >    enabled transmitters. This is because the R-Car CSI-2 receiver on
> >    which use PIXEL_RATE to compute the link freq [1]
> >
> > - according to [2]
> >    pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)
> >
> >    from which:
> >    link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)
> >
> >    This works as long the reported pixel rate includes visible and
> >    blankings, something I'm not sure how many transmitters handle
> >    correctly as PIXEL_RATE control is meant to report the visible pixel
> >    sampling rate on the pixel array.
> >
> > I guess we should go towards mandating LINK_FREQ for transmitters.
> >
> > cc-Niklas for opinions on R-Car CSI-2 rcsi2_calc_mbps()
> >
> > [1] https://elixir.bootlin.com/linux/v6.2/source/drivers/media/platform/renesas/rcar-vin/rcar-csi2.c#L608
> > [2] https://www.kernel.org/doc/html/latest/driver-api/media/tx-rx.html#csi-2-transmitter-drivers
> >
> > >
> > > -	fmt = v4l2_subdev_get_pad_format(&phy->subdev, state, CAL_CAMERARX_PAD_SINK);
> > > +	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > >
> > > -	fmtinfo = cal_format_by_code(fmt->code);
> > > -	if (!fmtinfo)
> > > +	if (state->routing.num_routes == 0)
> > >   		return -EINVAL;
> >
> > This function is in the call path of .enable_streams which if I'm not
> > reading the code wrong is called with
> >
> > int v4l2_subdev_s_stream_helper(struct v4l2_subdev *sd, int enable)
> > {
> >
> >          ...
> >
> > 	for_each_active_route(&state->routing, route)
> > 		source_mask |= BIT_ULL(route->source_stream);
> >
> > 	if (enable)
> > 		return v4l2_subdev_enable_streams(sd, pad_index, source_mask);
> >
> >          ...
> > }
> >
> > int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> > 			       u64 streams_mask)
> > {
> > 	struct device *dev = sd->entity.graph_obj.mdev->dev;
> > 	struct v4l2_subdev_state *state;
> > 	u64 found_streams = 0;
> > 	unsigned int i;
> > 	int ret;
> >
> > 	/* A few basic sanity checks first. */
> > 	if (pad >= sd->entity.num_pads)
> > 		return -EINVAL;
> >
> > 	if (!streams_mask)
> > 		return 0;
> >
> >          ...
> > }
> >
> > So the question is: can we get to enable_streams without enabled
> > routes (sorry I should have tested before asking it but I don't have a
> > multiplexed setup easily accessible) ?
>
> No, I don't think we can. Why do you ask?

Because if that's the case, you can drop

 if (state->routing.num_routes == 0)
        return -EINVAL;

>
> > >
> > > -	bpp = fmtinfo->bpp;
> > > +	if (state->routing.num_routes > 1) {
> > > +		bpp = 0;
> > > +	} else {
> > > +		const struct cal_format_info *fmtinfo;
> > > +		struct v4l2_subdev_route *route = &state->routing.routes[0];
> >
> > Nit: if I'm not mistaken along the driver reverse-xmas-tree (I
> > know...) is respected. Can you do it here as well ?
>
> Sure.
>
> > > +		struct v4l2_mbus_framefmt *fmt;
> > > +
> > > +		fmt = v4l2_subdev_state_get_stream_format(
> > > +			state, route->sink_pad, route->sink_stream);
> > > +
> > > +		fmtinfo = cal_format_by_code(fmt->code);
> > > +		if (!fmtinfo)
> > > +			return -EINVAL;
> > > +
> > > +		bpp = fmtinfo->bpp;
> > > +	}
> > >
> > >   	freq = v4l2_get_link_freq(phy->source->ctrl_handler, bpp, 2 * num_lanes);
> > >   	if (freq < 0) {
> > > @@ -284,15 +304,28 @@ static void cal_camerarx_ppi_disable(struct cal_camerarx *phy)
> > >   			0, CAL_CSI2_PPI_CTRL_IF_EN_MASK);
> > >   }
> > >
> > > -static int cal_camerarx_start(struct cal_camerarx *phy)
> > > +static int cal_camerarx_start(struct cal_camerarx *phy, u32 pad, u32 stream)
> > >   {
> > > +	struct media_pad *remote_pad;
> > >   	s64 link_freq;
> > >   	u32 sscounter;
> > >   	u32 val;
> > >   	int ret;
> > >
> > > +	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
> > > +
> >
> > Would it hurt a comment here to explain that in case it's the first to
> > be enabled you need to start the RX and power up the transmitter,
> > otherwise it's enough to enable the additional stream ?
>
> I'll add a comment.
>
> > >   	if (phy->enable_count > 0) {
> > >   		phy->enable_count++;
> > > +
> > > +		ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
> > > +						 BIT(stream));
> > > +		if (ret) {
> > > +			phy->enable_count--;
> >
> > You can avoid this by enable_count++ after the error check ?
>
> Yep.
>
> > > +
> > > +			phy_err(phy, "enable streams failed in source: %d\n", ret);
> > > +			return ret;
> > > +		}
> > > +
> > >   		return 0;
> > >   	}
> > >
> > > @@ -394,7 +427,9 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
> > >   	 * Start the source to enable the CSI-2 HS clock. We can now wait for
> > >   	 * CSI-2 PHY reset to complete.
> > >   	 */
> > > -	ret = v4l2_subdev_call(phy->source, video, s_stream, 1);
> > > +
> >
> > Intentional additional blank line ?
>
> No.
>
> > > +	ret = v4l2_subdev_enable_streams(phy->source, remote_pad->index,
> > > +					 BIT(stream));
> > >   	if (ret) {
> > >   		v4l2_subdev_call(phy->source, core, s_power, 0);
> > >   		cal_camerarx_disable_irqs(phy);
> > > @@ -425,12 +460,22 @@ static int cal_camerarx_start(struct cal_camerarx *phy)
> > >   	return 0;
> > >   }
> > >
> > > -static void cal_camerarx_stop(struct cal_camerarx *phy)
> > > +static void cal_camerarx_stop(struct cal_camerarx *phy, u32 pad, u32 stream)
> > >   {
> > > +	struct media_pad *remote_pad;
> > >   	int ret;
> > >
> > > -	if (--phy->enable_count > 0)
> > > +	remote_pad = media_pad_remote_pad_first(&phy->pads[pad]);
> > > +
> > > +	if (--phy->enable_count > 0) {
> > > +		ret = v4l2_subdev_disable_streams(phy->source,
> > > +						  remote_pad->index,
> > > +						  BIT(stream));
> > > +		if (ret)
> > > +			phy_err(phy, "stream off failed in subdev\n");
> > > +
> > >   		return;
> > > +	}
> > >
> > >   	cal_camerarx_ppi_disable(phy);
> > >
> > > @@ -450,7 +495,9 @@ static void cal_camerarx_stop(struct cal_camerarx *phy)
> > >   	/* Disable the phy */
> > >   	cal_camerarx_disable(phy);
> > >
> > > -	if (v4l2_subdev_call(phy->source, video, s_stream, 0))
> > > +	ret = v4l2_subdev_disable_streams(phy->source, remote_pad->index,
> > > +					  BIT(stream));
> > > +	if (ret)
> > >   		phy_err(phy, "stream off failed in subdev\n");
> > >
> > >   	ret = v4l2_subdev_call(phy->source, core, s_power, 0);
> > > @@ -626,30 +673,62 @@ static inline struct cal_camerarx *to_cal_camerarx(struct v4l2_subdev *sd)
> > >   	return container_of(sd, struct cal_camerarx, subdev);
> > >   }
> > >
> > > -static int cal_camerarx_sd_s_stream(struct v4l2_subdev *sd, int enable)
> > > +struct cal_camerarx *
> > > +cal_camerarx_get_phy_from_entity(struct media_entity *entity)
> > > +{
> > > +	struct v4l2_subdev *sd;
> > > +
> > > +	sd = media_entity_to_v4l2_subdev(entity);
> > > +	if (!sd)
> > > +		return NULL;
> > > +
> > > +	return to_cal_camerarx(sd);
> > > +}
> > > +
> > > +static int cal_camerarx_sd_enable_streams(struct v4l2_subdev *sd,
> > > +					  struct v4l2_subdev_state *state,
> > > +					  u32 pad, u64 streams_mask)
> > >   {
> > >   	struct cal_camerarx *phy = to_cal_camerarx(sd);
> > > -	struct v4l2_subdev_state *state;
> > > -	int ret = 0;
> > > +	u32 other_pad, other_stream;
> > > +	int ret;
> > >
> > > -	state = v4l2_subdev_lock_and_get_active_state(sd);
> > > +	if (WARN_ON(streams_mask != 1))
> >
> > as streams_mask is unsigned and you can't get here if streams_mask ==
> > 0, I wonder if checking for > 1 isn't more explicit. A detail though.
>
> Well, we need streams_mask to be exactly 1. I think the current test says
> that. If we'd check for > 1, the reader's thoughts might be that 0 and 1 are
> ok, which is not the case.

Yeah, you're probably right, it's easier to read it in the way you
have it

>
> Then again, I think the whole check can be dropped. We're calling it from
> inside the CAL driver, always with 1, so there's little chance it could be
> != 1.
>

That would be even better

> > > +		return -EINVAL;
> > >
> > > -	if (enable)
> > > -		ret = cal_camerarx_start(phy);
> > > -	else
> > > -		cal_camerarx_stop(phy);
> > > +	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
> > > +						    &other_pad, &other_stream);
> > > +	if (ret)
> > > +		return ret;
> > >
> > > -	v4l2_subdev_unlock_state(state);
> > > +	return cal_camerarx_start(phy, other_pad, other_stream);
> > > +}
> > >
> > > -	return ret;
> > > +static int cal_camerarx_sd_disable_streams(struct v4l2_subdev *sd,
> > > +					   struct v4l2_subdev_state *state,
> > > +					   u32 pad, u64 streams_mask)
> > > +{
> > > +	struct cal_camerarx *phy = to_cal_camerarx(sd);
> > > +	u32 other_pad, other_stream;
> > > +	int ret;
> > > +
> > > +	if (WARN_ON(streams_mask != 1))
> > > +		return -EINVAL;
> > > +
> > > +	ret = v4l2_subdev_routing_find_opposite_end(&state->routing, pad, 0,
> > > +						    &other_pad, &other_stream);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	cal_camerarx_stop(phy, other_pad, other_stream);
> > > +
> > > +	return 0;
> > >   }
> > >
> > >   static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
> > >   					  struct v4l2_subdev_state *state,
> > >   					  struct v4l2_subdev_mbus_code_enum *code)
> > >   {
> > > -	struct cal_camerarx *phy = to_cal_camerarx(sd);
> > > -
> > >   	/* No transcoding, source and sink codes must match. */
> > >   	if (cal_rx_pad_is_source(code->pad)) {
> > >   		struct v4l2_mbus_framefmt *fmt;
> > > @@ -657,8 +736,12 @@ static int cal_camerarx_sd_enum_mbus_code(struct v4l2_subdev *sd,
> > >   		if (code->index > 0)
> > >   			return -EINVAL;
> > >
> > > -		fmt = v4l2_subdev_get_pad_format(&phy->subdev, state,
> > > -						 CAL_CAMERARX_PAD_SINK);
> > > +		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
> > > +								   code->pad,
> > > +								   code->stream);
> > > +		if (!fmt)
> > > +			return -EINVAL;
> > > +
> > >   		code->code = fmt->code;
> > >   	} else {
> > >   		if (code->index >= cal_num_formats)
> > > @@ -683,8 +766,12 @@ static int cal_camerarx_sd_enum_frame_size(struct v4l2_subdev *sd,
> > >   	if (cal_rx_pad_is_source(fse->pad)) {
> > >   		struct v4l2_mbus_framefmt *fmt;
> > >
> > > -		fmt = v4l2_subdev_get_pad_format(sd, state,
> > > -						 CAL_CAMERARX_PAD_SINK);
> > > +		fmt = v4l2_subdev_state_get_opposite_stream_format(state,
> > > +								   fse->pad,
> > > +								   fse->stream);
> > > +		if (!fmt)
> > > +			return -EINVAL;
> > > +
> > >   		if (fse->code != fmt->code)
> > >   			return -EINVAL;
> > >
> > > @@ -740,57 +827,96 @@ static int cal_camerarx_sd_set_fmt(struct v4l2_subdev *sd,
> > >
> > >   	/* Store the format and propagate it to the source pad. */
> > >
> > > -	fmt = v4l2_subdev_get_pad_format(sd, state, CAL_CAMERARX_PAD_SINK);
> > > +	fmt = v4l2_subdev_state_get_stream_format(state, format->pad,
> > > +						  format->stream);
> > > +	if (!fmt)
> > > +		return -EINVAL;
> > > +
> > >   	*fmt = format->format;
> > >
> > > -	fmt = v4l2_subdev_get_pad_format(sd, state,
> > > -					 CAL_CAMERARX_PAD_FIRST_SOURCE);
> > > +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> > > +							   format->stream);
> > > +	if (!fmt)
> > > +		return -EINVAL;
> > > +
> > >   	*fmt = format->format;
> > >
> > >   	return 0;
> > >   }
> > >
> > > +static int _cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
> > > +					struct v4l2_subdev_state *state,
> > > +					struct v4l2_subdev_krouting *routing)
> >
> > Functions starting with _ are a bit unusual :)
>
> Hmm, well... I don't have any ideas for a good name =).
> cal_camerarx_sd_set_routing_impl() ?

Or just cal_camerarx_set_routing() with the _sd_ version being the
operation callback ? Too similar as a name and the risk is to confuse
readers maybe ?

>
> > > +{
> > > +	static const struct v4l2_mbus_framefmt format = {
> > > +		.width = 640,
> > > +		.height = 480,
> > > +		.code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > +		.field = V4L2_FIELD_NONE,
> > > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > > +		.ycbcr_enc = V4L2_YCBCR_ENC_601,
> > > +		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
> > > +		.xfer_func = V4L2_XFER_FUNC_SRGB,
> > > +	};
> > > +	int ret;
> > > +
> > > +	ret = v4l2_subdev_routing_validate(sd, routing, V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = v4l2_subdev_set_routing_with_fmt(sd, state, routing, &format);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int cal_camerarx_sd_set_routing(struct v4l2_subdev *sd,
> > > +				       struct v4l2_subdev_state *state,
> > > +				       enum v4l2_subdev_format_whence which,
> > > +				       struct v4l2_subdev_krouting *routing)
> > > +{
> > > +	return _cal_camerarx_sd_set_routing(sd, state, routing);
> > > +}
> > > +
> > >   static int cal_camerarx_sd_init_cfg(struct v4l2_subdev *sd,
> > >   				    struct v4l2_subdev_state *state)
> > >   {
> > > -	struct v4l2_subdev_format format = {
> > > -		.which = state ? V4L2_SUBDEV_FORMAT_TRY
> > > -		: V4L2_SUBDEV_FORMAT_ACTIVE,
> > > -		.pad = CAL_CAMERARX_PAD_SINK,
> > > -		.format = {
> > > -			.width = 640,
> > > -			.height = 480,
> > > -			.code = MEDIA_BUS_FMT_UYVY8_2X8,
> > > -			.field = V4L2_FIELD_NONE,
> > > -			.colorspace = V4L2_COLORSPACE_SRGB,
> > > -			.ycbcr_enc = V4L2_YCBCR_ENC_601,
> > > -			.quantization = V4L2_QUANTIZATION_LIM_RANGE,
> > > -			.xfer_func = V4L2_XFER_FUNC_SRGB,
> > > -		},
> > > +	struct v4l2_subdev_route routes[] = { {
> > > +		.sink_pad = 0,
> > > +		.sink_stream = 0,
> > > +		.source_pad = 1,
> > > +		.source_stream = 0,
> > > +		.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > > +	} };
> > > +
> > > +	struct v4l2_subdev_krouting routing = {
> > > +		.num_routes = 1,
> > > +		.routes = routes,
> > >   	};
> > >
> > > -	return cal_camerarx_sd_set_fmt(sd, state, &format);
> > > +	/* Initialize routing to single route to the fist source pad */
> > > +	return _cal_camerarx_sd_set_routing(sd, state, &routing);
> > >   }
> > >
> > > -static const struct v4l2_subdev_video_ops cal_camerarx_video_ops = {
> > > -	.s_stream = cal_camerarx_sd_s_stream,
> > > -};
> > > -
> > >   static const struct v4l2_subdev_pad_ops cal_camerarx_pad_ops = {
> > > +	.enable_streams = cal_camerarx_sd_enable_streams,
> > > +	.disable_streams = cal_camerarx_sd_disable_streams,
> > >   	.init_cfg = cal_camerarx_sd_init_cfg,
> > >   	.enum_mbus_code = cal_camerarx_sd_enum_mbus_code,
> > >   	.enum_frame_size = cal_camerarx_sd_enum_frame_size,
> > >   	.get_fmt = v4l2_subdev_get_fmt,
> > >   	.set_fmt = cal_camerarx_sd_set_fmt,
> > > +	.set_routing = cal_camerarx_sd_set_routing,
> > >   };
> > >
> > >   static const struct v4l2_subdev_ops cal_camerarx_subdev_ops = {
> > > -	.video = &cal_camerarx_video_ops,
> > >   	.pad = &cal_camerarx_pad_ops,
> > >   };
> > >
> > >   static struct media_entity_operations cal_camerarx_media_ops = {
> > >   	.link_validate = v4l2_subdev_link_validate,
> > > +	.has_pad_interdep = v4l2_subdev_has_pad_interdep,
> > >   };
> > >
> > >   /* ------------------------------------------------------------------
> > > @@ -842,11 +968,12 @@ struct cal_camerarx *cal_camerarx_create(struct cal_dev *cal,
> > >   	sd = &phy->subdev;
> > >   	v4l2_subdev_init(sd, &cal_camerarx_subdev_ops);
> > >   	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> > > -	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +	sd->flags = V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS;
> > >   	snprintf(sd->name, sizeof(sd->name), "CAMERARX%u", instance);
> > >   	sd->dev = cal->dev;
> > >
> > >   	phy->pads[CAL_CAMERARX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > > +
> >
> > Intentional ?
> >
> > >   	for (i = CAL_CAMERARX_PAD_FIRST_SOURCE; i < CAL_CAMERARX_NUM_PADS; ++i)
> > >   		phy->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > >   	sd->entity.ops = &cal_camerarx_media_ops;
> > > @@ -879,7 +1006,9 @@ void cal_camerarx_destroy(struct cal_camerarx *phy)
> > >   		return;
> > >
> > >   	v4l2_device_unregister_subdev(&phy->subdev);
> > > +
> > >   	v4l2_subdev_cleanup(&phy->subdev);
> > > +
> >
> > ditto
>
> I can drop these.

As you wish, I was just making sure they were intentional

>
> > >   	media_entity_cleanup(&phy->subdev.entity);
> > >   	of_node_put(phy->source_ep_node);
> > >   	of_node_put(phy->source_node);
> > > diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
> > > index d363e123d4bb..71578bfc97ba 100644
> > > --- a/drivers/media/platform/ti/cal/cal-video.c
> > > +++ b/drivers/media/platform/ti/cal/cal-video.c
> > > @@ -119,12 +119,13 @@ static int __subdev_get_format(struct cal_ctx *ctx,
> > >   {
> > >   	struct v4l2_subdev_format sd_fmt;
> > >   	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
> > > +	struct v4l2_subdev *sd = ctx->phy->source;
> > >   	int ret;
> > >
> > >   	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >   	sd_fmt.pad = 0;
> > >
> > > -	ret = v4l2_subdev_call(ctx->phy->source, pad, get_fmt, NULL, &sd_fmt);
> > > +	ret = v4l2_subdev_call_state_active(sd, pad, get_fmt, &sd_fmt);
> > >   	if (ret)
> > >   		return ret;
> > >
> > > @@ -141,13 +142,14 @@ static int __subdev_set_format(struct cal_ctx *ctx,
> > >   {
> > >   	struct v4l2_subdev_format sd_fmt;
> > >   	struct v4l2_mbus_framefmt *mbus_fmt = &sd_fmt.format;
> > > +	struct v4l2_subdev *sd = ctx->phy->source;
> > >   	int ret;
> > >
> > >   	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >   	sd_fmt.pad = 0;
> > >   	*mbus_fmt = *fmt;
> > >
> > > -	ret = v4l2_subdev_call(ctx->phy->source, pad, set_fmt, NULL, &sd_fmt);
> > > +	ret = v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
> > >   	if (ret)
> > >   		return ret;
> > >
> > > @@ -189,6 +191,7 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
> > >   				      struct v4l2_format *f)
> > >   {
> > >   	struct cal_ctx *ctx = video_drvdata(file);
> > > +	struct v4l2_subdev *sd = ctx->phy->source;
> > >   	const struct cal_format_info *fmtinfo;
> > >   	struct v4l2_subdev_frame_size_enum fse;
> > >   	int found;
> > > @@ -213,8 +216,8 @@ static int cal_legacy_try_fmt_vid_cap(struct file *file, void *priv,
> > >   	for (fse.index = 0; ; fse.index++) {
> > >   		int ret;
> > >
> > > -		ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size,
> > > -				       NULL, &fse);
> > > +		ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size,
> > > +						    &fse);
> > >   		if (ret)
> > >   			break;
> > >
> > > @@ -250,6 +253,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
> > >   				    struct v4l2_format *f)
> > >   {
> > >   	struct cal_ctx *ctx = video_drvdata(file);
> > > +	struct v4l2_subdev *sd = &ctx->phy->subdev;
> > >   	struct vb2_queue *q = &ctx->vb_vidq;
> > >   	struct v4l2_subdev_format sd_fmt = {
> > >   		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> > > @@ -289,7 +293,7 @@ static int cal_legacy_s_fmt_vid_cap(struct file *file, void *priv,
> > >   	ctx->v_fmt.fmt.pix.field = sd_fmt.format.field;
> > >   	cal_calc_format_size(ctx, fmtinfo, &ctx->v_fmt);
> > >
> > > -	v4l2_subdev_call(&ctx->phy->subdev, pad, set_fmt, NULL, &sd_fmt);
> > > +	v4l2_subdev_call_state_active(sd, pad, set_fmt, &sd_fmt);
> > >
> > >   	ctx->fmtinfo = fmtinfo;
> > >   	*f = ctx->v_fmt;
> > > @@ -301,6 +305,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
> > >   				      struct v4l2_frmsizeenum *fsize)
> > >   {
> > >   	struct cal_ctx *ctx = video_drvdata(file);
> > > +	struct v4l2_subdev *sd = ctx->phy->source;
> > >   	const struct cal_format_info *fmtinfo;
> > >   	struct v4l2_subdev_frame_size_enum fse;
> > >   	int ret;
> > > @@ -318,8 +323,7 @@ static int cal_legacy_enum_framesizes(struct file *file, void *fh,
> > >   	fse.code = fmtinfo->code;
> > >   	fse.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > >
> > > -	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_size, NULL,
> > > -			       &fse);
> > > +	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_size, &fse);
> > >   	if (ret)
> > >   		return ret;
> > >
> > > @@ -361,6 +365,7 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
> > >   					  struct v4l2_frmivalenum *fival)
> > >   {
> > >   	struct cal_ctx *ctx = video_drvdata(file);
> > > +	struct v4l2_subdev *sd = ctx->phy->source;
> > >   	const struct cal_format_info *fmtinfo;
> > >   	struct v4l2_subdev_frame_interval_enum fie = {
> > >   		.index = fival->index,
> > > @@ -375,8 +380,8 @@ static int cal_legacy_enum_frameintervals(struct file *file, void *priv,
> > >   		return -EINVAL;
> > >
> > >   	fie.code = fmtinfo->code;
> > > -	ret = v4l2_subdev_call(ctx->phy->source, pad, enum_frame_interval,
> > > -			       NULL, &fie);
> > > +
> > > +	ret = v4l2_subdev_call_state_active(sd, pad, enum_frame_interval, &fie);
> > >   	if (ret)
> > >   		return ret;
> > >   	fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
> > > @@ -694,8 +699,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
> > >   {
> > >   	const struct cal_format_info *rx_fmtinfo;
> > >   	const struct v4l2_mbus_framefmt *format;
> > > -	struct v4l2_subdev_state *state;
> > >   	struct media_pad *remote_pad;
> > > +	struct v4l2_subdev_state *state;
> > >   	int ret = 0;
> > >
> > >   	remote_pad = media_pad_remote_pad_first(&ctx->pad);
> > > @@ -704,7 +709,8 @@ static int cal_video_check_format(struct cal_ctx *ctx)
> > >
> > >   	state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
> > >
> > > -	format = v4l2_subdev_get_pad_format(&ctx->phy->subdev, state, remote_pad->index);
> > > +	format = v4l2_subdev_state_get_stream_format(state, remote_pad->index,
> > > +						     0);
> > >   	if (!format) {
> > >   		ret = -EINVAL;
> > >   		goto out;
> > > @@ -733,10 +739,52 @@ static int cal_video_check_format(struct cal_ctx *ctx)
> > >   static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> > >   {
> > >   	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
> > > +	struct media_pad *remote_pad;
> > >   	struct cal_buffer *buf;
> > >   	dma_addr_t addr;
> > >   	int ret;
> > >
> > > +	remote_pad = media_pad_remote_pad_first(&ctx->pad);
> > > +	if (!remote_pad) {
> > > +		ctx_err(ctx, "Context not connected\n");
> > > +		ret = -ENODEV;
> > > +		goto error_release_buffers;
> > > +	}
> > > +
> > > +	if (cal_mc_api) {
> > > +		struct v4l2_subdev_route *route = NULL;
> > > +		struct v4l2_subdev_route *r;
> > > +		struct v4l2_subdev_state *state;
> > > +
> > > +		/* Find the PHY connected to this video device */
> > > +
> > > +		ctx->phy = cal_camerarx_get_phy_from_entity(remote_pad->entity);
> > > +
> > > +		state = v4l2_subdev_lock_and_get_active_state(&ctx->phy->subdev);
> > > +
> > > +		/* Find the stream */
> > > +
> > > +		for_each_active_route(&state->routing, r) {
> > > +			if (r->source_pad != remote_pad->index)
> > > +				continue;
> > > +
> > > +			route = r;
> > > +
> > > +			break;
> > > +		}
> > > +
> > > +		if (!route) {
> > > +			v4l2_subdev_unlock_state(state);
> > > +			ctx_err(ctx, "Failed to find route\n");
> > > +			ret = -ENODEV;
> > > +			goto error_release_buffers;
> > > +		}
> >
> > If I got it right: we inspect the PHY routing table, pick the route that ends in
> > the pad connected to the this video device and store the sink-stream
> > id. The sink-stream id is used for... digging into the frame_desc
> > handling I have a slightly unrelated question if the PHY shouldn't
> > implement .get_frame_desc() instead of filtering what is returned from
> > the deser's implementation of .get_frame_desc() which is called by
> > cal_camerarx_get_remote_frame_desc(). If I'm not mitaken in that case
> > you could just call the PHY's .get_frame_desc() instead of inspecting its
> > routing table here. Does it make sense..
>
> Indeed, this looks a bit odd. This patch has evolved along the stream
> series, so it has gone though almost total rewrites a few times. And there
> seem to be some odd designs like this.
>
> I think adding .get_frame_desc() to the camerarx makes sense.

I presume it will help if camerarx will ever be used without CAL (if
that's even possible..)

Thanks!

>
> > > +
> > > +		ctx->stream = route->sink_stream;
> > > +
> > > +		v4l2_subdev_unlock_state(state);
> > > +	}
> > > +
> > >   	ret = video_device_pipeline_alloc_start(&ctx->vdev);
> > >   	if (ret < 0) {
> > >   		ctx_err(ctx, "Failed to start media pipeline: %d\n", ret);
> > > @@ -775,7 +823,8 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> > >   	cal_ctx_set_dma_addr(ctx, addr);
> > >   	cal_ctx_start(ctx);
> > >
> > > -	ret = v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 1);
> > > +	ret = v4l2_subdev_enable_streams(&ctx->phy->subdev, remote_pad->index,
> > > +					 BIT(0));
> > >   	if (ret)
> > >   		goto error_stop;
> > >
> > > @@ -800,10 +849,14 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> > >   static void cal_stop_streaming(struct vb2_queue *vq)
> > >   {
> > >   	struct cal_ctx *ctx = vb2_get_drv_priv(vq);
> > > +	struct media_pad *remote_pad;
> > >
> > >   	cal_ctx_stop(ctx);
> > >
> > > -	v4l2_subdev_call(&ctx->phy->subdev, video, s_stream, 0);
> > > +	remote_pad = media_pad_remote_pad_first(&ctx->pad);
> > > +
> > > +	v4l2_subdev_disable_streams(&ctx->phy->subdev, remote_pad->index,
> > > +				    BIT(0));
> > >
> > >   	pm_runtime_put_sync(ctx->cal->dev);
> > >
> > > @@ -812,6 +865,9 @@ static void cal_stop_streaming(struct vb2_queue *vq)
> > >   	cal_release_buffers(ctx, VB2_BUF_STATE_ERROR);
> > >
> > >   	video_device_pipeline_stop(&ctx->vdev);
> > > +
> > > +	if (cal_mc_api)
> > > +		ctx->phy = NULL;
> > >   }
> > >
> > >   static const struct vb2_ops cal_video_qops = {
> > > @@ -845,6 +901,7 @@ static int cal_ctx_v4l2_init_formats(struct cal_ctx *ctx)
> > >   	const struct cal_format_info *fmtinfo;
> > >   	unsigned int i, j, k;
> > >   	int ret = 0;
> > > +	struct v4l2_subdev *sd = ctx->phy->source;
> >
> > Move it up ? :P
>
> Sure.
>
> > The rest (for my undestanding) seems right. By dropping the !mc
> > support things would look much simpler :)
>
> Yes, I'd love to drop !mc =).
>
> Thanks!
>
>  Tomi
>

Re: [PATCH v1 3/3] media: ti: cal: add multiplexed streams support

[Sakari Ailus]

Hi Jacopo,

On Fri, Feb 24, 2023 at 04:48:55PM +0100, Jacopo Mondi wrote:
> Hi Tomi
> 
> On Wed, Feb 22, 2023 at 02:56:30PM +0200, Tomi Valkeinen wrote:
> > Add routing and stream_config support to CAL driver.
> >
> > Add multiplexed streams support. CAL has 8 dma-engines and can capture 8
> > separate streams at the same time.
> >
> > Add 8 video device nodes, each representing a single dma-engine, and set
> > the number of source pads on camerarx to 8. Each video node can be
> > connected to any of the source pads on either of the camerarx instances
> > using media links. Camerarx internal routing is used to route the
> > incoming CSI-2 streams to one of the 8 source pads.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  drivers/media/platform/ti/cal/cal-camerarx.c | 233 ++++++++++++++-----
> >  drivers/media/platform/ti/cal/cal-video.c    | 146 +++++++++---
> >  drivers/media/platform/ti/cal/cal.c          |  65 ++++--
> >  drivers/media/platform/ti/cal/cal.h          |   4 +-
> >  4 files changed, 342 insertions(+), 106 deletions(-)
> >
> > diff --git a/drivers/media/platform/ti/cal/cal-camerarx.c b/drivers/media/platform/ti/cal/cal-camerarx.c
> > index faafbd0e9240..49ae29065cd1 100644
> > --- a/drivers/media/platform/ti/cal/cal-camerarx.c
> > +++ b/drivers/media/platform/ti/cal/cal-camerarx.c
> > @@ -49,21 +49,41 @@ static s64 cal_camerarx_get_ext_link_freq(struct cal_camerarx *phy)
> >  {
> >  	struct v4l2_mbus_config_mipi_csi2 *mipi_csi2 = &phy->endpoint.bus.mipi_csi2;
> >  	u32 num_lanes = mipi_csi2->num_data_lanes;
> > -	const struct cal_format_info *fmtinfo;
> >  	struct v4l2_subdev_state *state;
> > -	struct v4l2_mbus_framefmt *fmt;
> >  	u32 bpp;
> >  	s64 freq;
> >
> > -	state = v4l2_subdev_get_locked_active_state(&phy->subdev);
> > +	/*
> > +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> > +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> > +	 *
> > +	 * With multistream input there is no single pixel rate, and thus we
> > +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> > +	 * causes v4l2_get_link_freq() to return an error if it falls back to
> > +	 * V4L2_CID_PIXEL_RATE.
> > +	 */
> 
> To recap a bit of our offline discussion:
> - max9286 GMSL deserializer (as a comparison for a multiplexed
>   transmitter) use PIXEL_RATE to report the cumulative pixel rate of
>   enabled transmitters. This is because the R-Car CSI-2 receiver on
>   which use PIXEL_RATE to compute the link freq [1]
> 
> - according to [2]
>   pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample (on D-PHY)
> 
>   from which:
>   link_freq = pixel_rate * bits_per_sample / (2 * nr_of_lanes)
> 
>   This works as long the reported pixel rate includes visible and
>   blankings, something I'm not sure how many transmitters handle
>   correctly as PIXEL_RATE control is meant to report the visible pixel
>   sampling rate on the pixel array.
> 
> I guess we should go towards mandating LINK_FREQ for transmitters.

Yes, this is the intention and new drivers use LINK_FREQ (but this could
occasionally be missed in review). I don't think there's a specific
requirement for this in documentation. One should be added to document the
status quo.

Would you like to send a patch? :-)

For CSI-2 this is obvious. What about the parallel buses?

-- 
Regards,

Sakari Ailus