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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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