Hi Tomi,
Thank you for the patch.
On Fri, May 30, 2025 at 04:50:40PM +0300, Tomi Valkeinen wrote:
> Call get_frame_desc to find out VC & DT, instead of hardcoding the VC
> routing and deducing the DT based on the mbus format.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/renesas/rcar-isp/csisp.c | 108 +++++++++++++++++-------
> 1 file changed, 77 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> index a04cbf96b809..887d8eb21a3a 100644
> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> @@ -225,24 +225,86 @@ static void risp_power_off(struct rcar_isp *isp)
> pm_runtime_put(isp->dev);
> }
>
> -static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> +static int risp_configure_routing(struct rcar_isp *isp,
> + struct v4l2_subdev_state *state)
> {
> - const struct v4l2_mbus_framefmt *fmt;
> - const struct rcar_isp_format *format;
> - unsigned int vc;
> - u32 sel_csi = 0;
> + struct v4l2_mbus_frame_desc source_fd;
> + struct v4l2_subdev_route *route;
> int ret;
>
> - fmt = v4l2_subdev_state_get_format(state, RCAR_ISP_SINK, 0);
> - if (!fmt)
> - return -EINVAL;
> + ret = v4l2_subdev_call(isp->remote, pad, get_frame_desc,
> + isp->remote_pad, &source_fd);
> + if (ret)
> + return ret;
>
> - format = risp_code_to_fmt(fmt->code);
> - if (!format) {
> - dev_err(isp->dev, "Unsupported bus format\n");
> - return -EINVAL;
> + /* Clear the channel registers */
> + for (unsigned int ch = 0; ch < 12; ++ch) {
A macro for the number of channels would be nice.
> + risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), 0);
> + risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch), 0);
> }
>
> + /* Clear the proc mode registers */
> + for (unsigned int dt = 0; dt < 64; ++dt)
> + risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), 0);
Do we really need to clear those ? These registers seem to be used to
select how to process a particular DT, likely to allow overriding the
default processing method. 0 means RAW8, so it's not a magic disable
value as far as I can tell. I think we can leave the registers as-is.
> +
> + for_each_active_route(&state->routing, route) {
> + struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
> + const struct rcar_isp_format *format;
> + const struct v4l2_mbus_framefmt *fmt;
> + unsigned int i;
> + u8 vc, dt, ch;
> + u32 v;
> +
> + for (i = 0; i < source_fd.num_entries; i++) {
> + if (source_fd.entry[i].stream == route->sink_stream) {
> + source_entry = &source_fd.entry[i];
> + break;
> + }
> + }
> +
> + if (!source_entry) {
> + dev_err(isp->dev,
> + "Failed to find stream from source frame desc\n");
Isn't it rather "Failed to find source frame desc for stream" ?
> + return -EPIPE;
> + }
> +
> + vc = source_entry->bus.csi2.vc;
> + dt = source_entry->bus.csi2.dt;
> + /* Channels 4 - 11 go to VIN */
> + ch = route->source_pad - 1 + 4;
> +
> + fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
> + route->sink_stream);
> + if (!fmt)
> + return -EINVAL;
> +
> + format = risp_code_to_fmt(fmt->code);
> + if (!format) {
> + dev_err(isp->dev, "Unsupported bus format\n");
> + return -EINVAL;
> + }
> +
> + /* VC Filtering */
> + risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> +
> + /* DT Filtering */
> + risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
> + ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> +
> + /* Proc mode */
> + v = risp_read_cs(isp, ISPPROCMODE_DT_REG(dt));
> + v |= ISPPROCMODE_DT_PROC_MODE_VCn(vc, format->procmode);
> + risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), v);
If we want to minimize the register writes, we could store the
ISPPROCMODE_DT_REG values in a local variable and write all of them in
one go. Possible/probably overkill.
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> + }
> +
> + return 0;
> +}
> +
> +static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> +{
> + u32 sel_csi = 0;
> + int ret;
> +
> ret = risp_power_on(isp);
> if (ret) {
> dev_err(isp->dev, "Failed to power on ISP\n");
> @@ -256,25 +318,9 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> risp_write_cs(isp, ISPINPUTSEL0_REG,
> risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
>
> - /* Configure Channel Selector. */
> - for (vc = 0; vc < 4; vc++) {
> - u8 ch = vc + 4;
> - u8 dt = format->datatype;
> -
> - risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> - risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
> - ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> - ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> - ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> - ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> - }
> -
> - /* Setup processing method. */
> - risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
> - ISPPROCMODE_DT_PROC_MODE_VCn(3, format->procmode) |
> - ISPPROCMODE_DT_PROC_MODE_VCn(2, format->procmode) |
> - ISPPROCMODE_DT_PROC_MODE_VCn(1, format->procmode) |
> - ISPPROCMODE_DT_PROC_MODE_VCn(0, format->procmode));
> + ret = risp_configure_routing(isp, state);
> + if (ret)
> + return ret;
>
> /* Start ISP. */
> risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
--
Regards,
Laurent Pinchart
