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

[Laurent Pinchart]

On Tue, Jun 16, 2026 at 02:22:10PM +0300, Tomi Valkeinen wrote:
> 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@xxxxxxxxxxxxxxxx>
> >> ---
> >>   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...

I tend to avoid them, but sometimes wonder if relying on invariants
creates ticking time bombs when core code is refactored. We always make
assumptions, for instance this function doesn't check if the sd or state
pointer is NULL. Is not checking the return value of
v4l2_subdev_state_get_opposite_stream_format() here worse ?

> > 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@xxxxxxxxxxxxxxxx>
> > 
> >>   
> >> -	/* 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;

-- 
Regards,

Laurent Pinchart