Re: [PATCH v5 06/10] media: rcar-csi2: Add .get_frame_desc op

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Jun 16, 2026 at 02:30:05PM +0300, Tomi Valkeinen wrote:
> On 18/03/2026 23:16, Laurent Pinchart wrote:
> > On Wed, Mar 11, 2026 at 03:53:19PM +0200, 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@ideasonboard.com>
> >> ---
> >>   drivers/media/platform/renesas/rcar-csi2.c | 70 ++++++++++++++++++++++++++++++
> >>   1 file changed, 70 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> >> index ad62c95c8f9a..b8baf7c65e90 100644
> >> --- a/drivers/media/platform/renesas/rcar-csi2.c
> >> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> >> @@ -1935,12 +1935,82 @@ 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;
> >> +
> >> +	if (WARN_ON(!priv->info->use_isp))
> >> +		return -ENOTTY;
> > 
> > Why is that, can't the get frame desc operation be supported on Gen3 ?
> 
> It can, and it is, in this patch. The thing here is that 
> rcsi2_get_frame_desc() is the implementation for 
> v4l2_subdev_pad_ops.get_frame_desc(). On gen4, csisp calls it, but on 
> gen3, there's no one to call it as on gen3 the csi2 does the demuxing.
> 
> So the above check is just a "yell if our drivers do a totally wrong thing".

A comment would be useful.

> >> +
> >> +	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);
> > 
> > A dev_warn_once() would be good here, to get people to fix the source
> > device driver.
> 
> Perhaps at some point, but do we want to add it already?
> 
> Hmm, actually, this will go away with Sakari's series that adds 
> framework level fallback handling, so I think warning now about a thing 
> that is no longer a thing in the future serves no purpose.

Or you could consider that adding a dev_warn_once() isn't an issue as
the code will be replaced soon :-) Up to you.

> >> +	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 = {
> >>

-- 
Regards,

Laurent Pinchart