Hi Tomi,
Thank you for the patch.
On Fri, May 30, 2025 at 04:50:32PM +0300, Tomi Valkeinen wrote:
> With multiple streams the operation to enable the ISP hardware and to
> call {enable|disable}_streams() on upstream subdev will need to be
> handled separately.
>
> Prepare for that by moving {enable|disable}_streams() calls out from
> risp_start() and risp_stop().
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/renesas/rcar-isp/csisp.c | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> index 8fb2cc3b5650..2337c5d44c40 100644
> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> @@ -268,18 +268,11 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> /* Start ISP. */
> risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
>
> - ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
> - BIT_ULL(0));
> - if (ret)
> - risp_power_off(isp);
> -
> - return ret;
> + return 0;
> }
>
> static void risp_stop(struct rcar_isp *isp)
> {
> - v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
> -
> /* Stop ISP. */
> risp_write_cs(isp, ISPSTART_REG, ISPSTART_STOP);
>
> @@ -305,6 +298,13 @@ static int risp_enable_streams(struct v4l2_subdev *sd,
> return ret;
> }
>
> + ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
> + BIT_ULL(0));
You're now potentially calling v4l2_subdev_disable_streams() multiple
times on the same pad and stream, as this call isn't covered by the
stream_count check anymore. Is that correct ? Maybe because
risp_enable_streams() is guaranteed to never be called multiple times,
with stream_count never becoming larger than 1 ? If so that should be
explained in the commit message, and stream_count should probably be
dropped.
Same when stopping.
> + if (ret) {
> + risp_stop(isp);
This is also not covered by the stream_count, while risp_start() is.
> + return ret;
> + }
> +
> isp->stream_count += 1;
>
> return ret;
> @@ -322,6 +322,8 @@ static int risp_disable_streams(struct v4l2_subdev *sd,
> if (!isp->remote)
> return -ENODEV;
>
> + v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
> +
> if (isp->stream_count == 1)
> risp_stop(isp);
>
--
Regards,
Laurent Pinchart
