Re: [PATCH v5 04/10] media: rcar-csi2: Switch to Streams API

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

Hi,

On 18/03/2026 23:04, Laurent Pinchart wrote:
> On Wed, Mar 11, 2026 at 03:53:17PM +0200, Tomi Valkeinen wrote:
>> Switch to Streams API with a single hardcoded route.
>>
>> For single-stream use case there should be no change in behavior.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>   drivers/media/platform/renesas/rcar-csi2.c | 64 +++++++++++++++++++++++-------
>>   1 file changed, 50 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
>> index 158fa447e668..ad62c95c8f9a 100644
>> --- a/drivers/media/platform/renesas/rcar-csi2.c
>> +++ b/drivers/media/platform/renesas/rcar-csi2.c
>> @@ -1023,17 +1023,24 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>>   	 */
>>   	freq = v4l2_get_link_freq(remote_pad, 0, 0);
>>   	if (freq < 0) {
>> +		const struct v4l2_subdev_route *route;
>>   		const struct rcar_csi2_format *format;
>>   		const struct v4l2_mbus_framefmt *fmt;
>>   		unsigned int lanes;
>>   		unsigned int bpp;
>>   		int ret;
>>   
>> +		if (state->routing.num_routes != 1)
>> +			return -EINVAL;
>> +
>>   		ret = rcsi2_get_active_lanes(priv, &lanes);
>>   		if (ret)
>>   			return ret;
>>   
>> -		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
>> +		route = &state->routing.routes[0];
>> +
>> +		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
>> +						   route->sink_stream);
>>   		if (!fmt)
>>   			return -EINVAL;
>>   
>> @@ -1062,6 +1069,7 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>>   static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
>>   				     struct v4l2_subdev_state *state)
>>   {
>> +	const struct v4l2_subdev_route *route;
>>   	const struct rcar_csi2_format *format;
>>   	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>>   	const struct v4l2_mbus_framefmt *fmt;
>> @@ -1070,7 +1078,16 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
>>   	int mbps, ret;
>>   
>>   	/* Use the format on the sink pad to compute the receiver config. */
>> -	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
>> +
>> +	if (state->routing.num_routes != 1)
>> +		return -EINVAL;
>> +
>> +	route = &state->routing.routes[0];
>> +
>> +	fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
>> +					   route->sink_stream);
>> +	if (!fmt)
>> +		return -EINVAL;
>>   
>>   	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
>>   		fmt->width, fmt->height,
>> @@ -1892,8 +1909,7 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
>>   				struct v4l2_subdev_state *state,
>>   				struct v4l2_subdev_format *format)
>>   {
>> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
>> -	unsigned int num_pads = rcsi2_num_pads(priv);
>> +	struct v4l2_mbus_framefmt *fmt;
>>   
>>   	if (format->pad > RCAR_CSI2_SINK)
>>   		return v4l2_subdev_get_fmt(sd, state, format);
>> @@ -1901,11 +1917,20 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
>>   	if (!rcsi2_code_to_fmt(format->format.code))
>>   		format->format.code = rcar_csi2_formats[0].code;
>>   
>> -	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
>> +	/* Set sink format. */
>> +	fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream);
>> +	if (!fmt)
>> +		return -EINVAL;
> 
> Can the call return NULL, isn't it checked by the subdev core already ?
> 
>> +
>> +	*fmt = format->format;
>> +
>> +	/* Propagate the format to the source pad. */
>> +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
>> +							   format->stream);
>> +	if (!fmt)
>> +		return -EINVAL;
> 
> I wonder if this error check could be omitted too. If there's a format
> for the sink stream, it means there's a route, so the opposite stream
> format should be guaranteed to exist. Or maybe it's too late and I
> should go to bed :-)

I've thought about that every now and then, and afaics we should always 
have a stream fmt. But not checking the return feels like you're doing 
something nasty, so I've just added them as usually it's not troublesome 
to return an error...

  Tomi

> Looking at what other drivers do, they all check the return value of the
> function, so let's keep it as-is for now.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>>   
>> -	/* Propagate the format to the source pads. */
>> -	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < num_pads; i++)
>> -		*v4l2_subdev_state_get_format(state, i) = format->format;
>> +	*fmt = format->format;
>>   
>>   	return 0;
>>   }
>> @@ -1925,8 +1950,15 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
>>   static int rcsi2_init_state(struct v4l2_subdev *sd,
>>   			    struct v4l2_subdev_state *state)
>>   {
>> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
>> -	unsigned int num_pads = rcsi2_num_pads(priv);
>> +	static struct v4l2_subdev_route routes[] = {
>> +		{
>> +			.sink_pad = RCAR_CSI2_SINK,
>> +			.sink_stream = 0,
>> +			.source_pad = RCAR_CSI2_SOURCE_VC0,
>> +			.source_stream = 0,
>> +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
>> +		},
>> +	};
>>   
>>   	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
>>   		.width		= 1920,
>> @@ -1939,10 +1971,13 @@ static int rcsi2_init_state(struct v4l2_subdev *sd,
>>   		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
>>   	};
>>   
>> -	for (unsigned int i = RCAR_CSI2_SINK; i < num_pads; i++)
>> -		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
>> +	static const struct v4l2_subdev_krouting routing = {
>> +		.num_routes = ARRAY_SIZE(routes),
>> +		.routes = routes,
>> +	};
>>   
>> -	return 0;
>> +	return v4l2_subdev_set_routing_with_fmt(sd, state, &routing,
>> +						&rcar_csi2_default_fmt);
>>   }
>>   
>>   static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
>> @@ -2599,7 +2634,8 @@ static int rcsi2_probe(struct platform_device *pdev)
>>   	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
>>   	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
>>   		 KBUILD_MODNAME, dev_name(&pdev->dev));
>> -	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	priv->subdev.flags = V4L2_SUBDEV_FL_HAS_DEVNODE |
>> +			     V4L2_SUBDEV_FL_STREAMS;
>>   
>>   	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
>>   	priv->subdev.entity.ops = &rcar_csi2_entity_ops;
>>
>