Re: [PATCH v6 07/11] media: rcar-csi2: Add .get_frame_desc op

[Niklas Söderlund]

Hi Tomi,

Thanks for your work.

On 2026-06-17 14:55:00 +0300, Tomi Valkeinen wrote:
> Add v4l2_subdev_pad_ops.get_frame_desc() implementation.
> 
> We also implement a fallback for the case where the upstream subdevice
> does not implement .get_frame_desc. It assumes a single stream with VC =
> 0 and DT based on the configured stream mbus format.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
> ---
>  drivers/media/platform/renesas/rcar-csi2.c | 74 ++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index 91f713e1f8c2..0d9e730fa677 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -1935,12 +1935,86 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int rcsi2_get_frame_desc_fallback(struct v4l2_subdev *sd,
> +					 unsigned int pad,
> +					 struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct v4l2_subdev_route *route;
> +	const struct rcar_csi2_format *format;
> +	struct v4l2_subdev_state *state;
> +	struct v4l2_mbus_framefmt *fmt;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> +	if (state->routing.num_routes != 1) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	route = &state->routing.routes[0];
> +
> +	if (route->source_pad != pad) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
> +					   route->sink_stream);
> +	if (!fmt) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	format = rcsi2_code_to_fmt(fmt->code);
> +	if (!format) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	fd->num_entries = 1;
> +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +	fd->entry[0].stream = route->source_stream;
> +	fd->entry[0].pixelcode = fmt->code;
> +	fd->entry[0].bus.csi2.vc = 0;
> +	fd->entry[0].bus.csi2.dt = format->datatype;
> +
> +out:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
> +}
> +
> +static int rcsi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +	int ret;
> +
> +	/*
> +	 * This should only get called on gen4, with ISP, so yell here
> +	 * if that is not the case to highlight a driver bug.
> +	 */

nit: Not your fault but as this driver grew organically to support Gen4 
which only have one source pad compared to Gen3 which have 4. Could we 
extend this comment to carry more detail?

    Providing a frame descriptor on the source pad only make sens on 
    Gen4. On Gen3 the CSI-2 IP is used to demux the CSI streams to each 
    VIN instance, while on Gen4 this job is done by the Channel Selector 
    of the ISP, and that needs the frame descriptors.

Or something similar? With that fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>

> +	if (WARN_ON(!priv->info->use_isp))
> +		return -ENOTTY;
> +
> +	if (WARN_ON(pad != RCAR_CSI2_SOURCE_VC0))
> +		return -EINVAL;
> +
> +	ret = v4l2_subdev_get_frame_desc_passthrough(sd, pad, fd);
> +	if (ret == -ENOIOCTLCMD)
> +		ret = rcsi2_get_frame_desc_fallback(sd, pad, fd);
> +	return ret;
> +}
> +
>  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
>  	.enable_streams = rcsi2_enable_streams,
>  	.disable_streams = rcsi2_disable_streams,
>  
>  	.set_fmt = rcsi2_set_pad_format,
>  	.get_fmt = v4l2_subdev_get_fmt,
> +
> +	.get_frame_desc = rcsi2_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> 
> -- 
> 2.43.0
> 

-- 
Kind Regards,
Niklas Söderlund