Hi Tomi,
Thank you for the patch.
On Fri, May 30, 2025 at 04:50:42PM +0300, Tomi Valkeinen wrote:
> Call get_frame_desc to find out VC & DT, for Gen3 platforms, instead of
> hardcoding the VC routing and deducing the DT based on the mbus format.
>
> If the source subdevice doesn't implement .get_frame_desc, we use a
> fallback case where we assume there's a single stream with VC = 0 and DT
> based on the mbus format.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/renesas/rcar-csi2.c | 113 +++++++++++++++++++----------
> 1 file changed, 76 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index b9f83aae725a..8f708196ef49 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -71,10 +71,7 @@ struct rcar_csi2;
> #define FLD_REG 0x1c
> #define FLD_FLD_NUM(n) (((n) & 0xff) << 16)
> #define FLD_DET_SEL(n) (((n) & 0x3) << 4)
> -#define FLD_FLD_EN4 BIT(3)
> -#define FLD_FLD_EN3 BIT(2)
> -#define FLD_FLD_EN2 BIT(1)
> -#define FLD_FLD_EN BIT(0)
> +#define FLD_FLD_EN(ch) BIT(ch)
>
> /* Automatic Standby Control */
> #define ASTBY_REG 0x20
> @@ -1066,52 +1063,94 @@ 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 rcar_csi2_format *format;
> - u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> - const struct v4l2_mbus_framefmt *fmt;
> + u32 phycnt, vcdt = 0, vcdt2 = 0;
> + u32 fld = FLD_DET_SEL(1);
> + struct v4l2_mbus_frame_desc source_fd;
> + struct v4l2_subdev_route *route;
> unsigned int lanes;
> - unsigned int i;
> int mbps, ret;
> + u8 ch = 0;
>
> - /* Use the format on the sink pad to compute the receiver config. */
> - fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
> + ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
> + priv->remote_pad, &source_fd);
> + if (ret && ret != -ENOIOCTLCMD) {
> + return ret;
> + } else if (ret == -ENOIOCTLCMD) {
if (ret && ret != -ENOIOCTLCMD)
return ret;
if (ret == -ENOIOCTLCMD) {
> + /* Create a fallback source_fd */
> + struct v4l2_mbus_frame_desc *fd = &source_fd;
> + const struct rcar_csi2_format *format;
> + struct v4l2_mbus_framefmt *fmt;
>
> - dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> - fmt->width, fmt->height,
> - fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
> + fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
> + if (!fmt)
> + return -EINVAL;
>
> - /* Code is validated in set_fmt. */
> - format = rcsi2_code_to_fmt(fmt->code);
> - if (!format)
> - return -EINVAL;
> + format = rcsi2_code_to_fmt(fmt->code);
> + if (!format)
> + return -EINVAL;
>
> - /*
> - * Enable all supported CSI-2 channels with virtual channel and
> - * data type matching.
> - *
> - * NOTE: It's not possible to get individual datatype for each
> - * source virtual channel. Once this is possible in V4L2
> - * it should be used here.
> - */
> - for (i = 0; i < priv->info->num_channels; i++) {
> + memset(fd, 0, sizeof(*fd));
> +
> + fd->num_entries = 1;
> + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> + fd->entry[0].stream = 0;
> + fd->entry[0].pixelcode = fmt->code;
> + fd->entry[0].bus.csi2.vc = 0;
> + fd->entry[0].bus.csi2.dt = format->datatype;
> + }
> +
> + for_each_active_route(&state->routing, route) {
> + struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
const
> + const struct v4l2_mbus_framefmt *fmt;
> + const struct rcar_csi2_format *format;
> + unsigned int i;
> + u8 vc, dt;
> u32 vcdt_part;
>
> - if (priv->channel_vc[i] < 0)
> - continue;
> + for (i = 0; i < source_fd.num_entries; i++) {
> + if (source_fd.entry[i].stream == route->sink_stream) {
No need to check the pad ?
> + source_entry = &source_fd.entry[i];
> + break;
> + }
> + }
> +
> + if (!source_entry) {
> + dev_err(priv->dev,
> + "Failed to find stream from source frame desc\n");
> + return -EPIPE;
> + }
>
> - vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | VCDT_VCDTN_EN |
> - VCDT_SEL_DTN_ON | VCDT_SEL_DT(format->datatype);
> + vc = source_entry->bus.csi2.vc;
> + dt = source_entry->bus.csi2.dt;
> +
> + vcdt_part = VCDT_SEL_VC(vc) | VCDT_VCDTN_EN |
> + VCDT_SEL_DTN_ON | VCDT_SEL_DT(dt);
I would drop the vc and dt variables and write
vcdt_part = VCDT_SEL_VC(source_entry->bus.csi2.vc)
| VCDT_VCDTN_EN | VCDT_SEL_DTN_ON
| VCDT_SEL_DT(source_entry->bus.csi2.dt);
>
> /* Store in correct reg and offset. */
> - if (i < 2)
> - vcdt |= vcdt_part << ((i % 2) * 16);
> + if (ch < 2)
> + vcdt |= vcdt_part << ((ch % 2) * 16);
> else
> - vcdt2 |= vcdt_part << ((i % 2) * 16);
> - }
> + vcdt2 |= vcdt_part << ((ch % 2) * 16);
> +
> + fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK,
> + route->sink_stream);
> + if (!fmt)
> + return -EINVAL;
> +
> + dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> + fmt->width, fmt->height,
> + fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
>
> - if (fmt->field == V4L2_FIELD_ALTERNATE)
> - fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> - | FLD_FLD_EN;
> + /* Code is validated in set_fmt. */
Then why don't you drop the !format check ?
> + format = rcsi2_code_to_fmt(fmt->code);
> + if (!format)
> + return -EINVAL;
> +
> + if (fmt->field == V4L2_FIELD_ALTERNATE)
> + fld |= FLD_FLD_EN(ch);
> +
> + ch++;
> + }
>
> /*
> * Get the number of active data lanes inspecting the remote mbus
--
Regards,
Laurent Pinchart
