Re: [PATCH 07/13] media: ipu6: Set up CSI-2 receiver at correct moment

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

Hi Sakari,

Thank you for the patch.

On Thu, Jun 19, 2025 at 11:15:40AM +0300, Sakari Ailus wrote:
> Enable the CSI-2 receiver before the first stream is started and disable
> it when the last stream is stopped. Before this patch, every time a stream
> was started, the CSI-2 receiver was enabled and similarly, it was disabled
> when any stream was stopped.

You mentioned in 03/13 that we should only enable the source when all
video capture devices have been started. Shouldn't you enable the CSI-2
receiver when *all* streams are enabled, not when the first one is
enabled (and similarly when stopping) ?

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 25 ++++++++++++-------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> index 6030bd23b4b9..3b837e9ccffe 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> @@ -348,7 +348,7 @@ static int ipu6_isys_csi2_enable_streams(struct v4l2_subdev *sd,
>  	struct ipu6_isys_csi2_timing timing = { };
>  	struct v4l2_subdev *remote_sd;
>  	struct media_pad *remote_pad;
> -	u64 sink_streams;
> +	u64 sink_streams, already_enabled;
>  	int ret;
>  
>  	remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
> @@ -358,13 +358,17 @@ static int ipu6_isys_csi2_enable_streams(struct v4l2_subdev *sd,
>  		v4l2_subdev_state_xlate_streams(state, pad, CSI2_PAD_SINK,
>  						&streams_mask);
>  
> -	ret = ipu6_isys_csi2_calc_timing(csi2, &timing, CSI2_ACCINV);
> -	if (ret)
> -		return ret;
> +	already_enabled = v4l2_subdev_state_streams_enabled(sd, state,
> +							    CSI2_PAD_SINK);

It would be cheaper to store a bitmask of enabled streams on the csi2
structure.

> +	if (!already_enabled) {
> +		ret = ipu6_isys_csi2_calc_timing(csi2, &timing, CSI2_ACCINV);
> +		if (ret)
> +			return ret;
>  
> -	ret = ipu6_isys_csi2_set_stream(sd, &timing, csi2->nlanes, true);
> -	if (ret)
> -		return ret;
> +		ret = ipu6_isys_csi2_set_stream(sd, &timing, csi2->nlanes, true);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = v4l2_subdev_enable_streams(remote_sd, remote_pad->index,
>  					 sink_streams);
> @@ -382,7 +386,7 @@ static int ipu6_isys_csi2_disable_streams(struct v4l2_subdev *sd,
>  {
>  	struct v4l2_subdev *remote_sd;
>  	struct media_pad *remote_pad;
> -	u64 sink_streams;
> +	u64 sink_streams, still_enabled;
>  
>  	sink_streams =
>  		v4l2_subdev_state_xlate_streams(state, pad, CSI2_PAD_SINK,
> @@ -391,7 +395,10 @@ static int ipu6_isys_csi2_disable_streams(struct v4l2_subdev *sd,
>  	remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
>  	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
>  
> -	ipu6_isys_csi2_set_stream(sd, NULL, 0, false);
> +	still_enabled = v4l2_subdev_state_streams_enabled(sd, state,
> +							  CSI2_PAD_SINK);
> +	if (still_enabled == sink_streams)
> +		ipu6_isys_csi2_set_stream(sd, NULL, 0, false);
>  
>  	v4l2_subdev_disable_streams(remote_sd, remote_pad->index, sink_streams);
>  

-- 
Regards,

Laurent Pinchart