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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Thu, Jun 19, 2025 at 08:00:30PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.

Thanks for the review.

> 
> 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) ?

Indeed. That only comes in the last patch, but I think some of these can be
squashed, too.

> 
> > 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.

I think I'll do just that. Indeed, this soon won't be even usable here in
any case.

> 
> > +	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,

Sakari Ailus