Re: [PATCH 03/13] media: ipu6: Enable and disable each stream at CSI-2 subdev source pad

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

On Thu, Jun 19, 2025 at 04:19:00PM +0300, Laurent Pinchart wrote:
> On Thu, Jun 19, 2025 at 01:10:48PM +0000, Sakari Ailus wrote:
> > On Thu, Jun 19, 2025 at 03:48:55PM +0300, Laurent Pinchart wrote:
> > > On Thu, Jun 19, 2025 at 11:15:36AM +0300, Sakari Ailus wrote:
> > > > On UAPI, streaming is started a video device at a time. The IPU6 ISYS
> > > > driver only starts streaming on the source sub-device when all relevant
> > > > video devices have been set streaming. This also needs to be reflected in
> > > > the sub-device pads, hence set them all streaming, one at a time.
> > > 
> > > Why ? What's wrong with enabling/disabling streams on each pad as the
> > > corresponding video device is started/stopped ?
> > 
> > Streaming is started once all the related video nodes are set streaming,
> > otherwise there's a chance of losing data.
> 
> Yes, but that's something that can be handled in the subdev, to only
> start the ISYS and its source when all the streams that are part of the
> pipeline have been started.

I agree, this is done in the CSI-2 receiver driver actually.

There's some work to do in cleaning up the IPU6 driver. The combination of
videobuf2 and streams isn't very driver-friendly currently. Things
generally need to happen in certain order and this may not be always veyr
convenient for drivers.

> 
> > I've squashed this with the last patch after posting the set.
> 
> Maybe it will become clearer when I review the rest of the series.
> 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 43 ++++++++++++-------
> > > >  1 file changed, 28 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > > > index 24a2ef93474c..54006b5e2ccd 100644
> > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> > > > @@ -990,6 +990,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> > > >  	struct device *dev = &av->isys->adev->auxdev.dev;
> > > >  	struct v4l2_subdev *sd;
> > > >  	struct media_pad *r_pad;
> > > > +	unsigned int i;
> > > >  	u32 sink_pad, sink_stream;
> > > >  	u64 r_stream;
> > > >  	u64 stream_mask = 0;
> > > > @@ -1018,14 +1019,15 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> > > >  		stop_streaming_firmware(av);
> > > >  
> > > >  		/* stop sub-device which connects with video */
> > > > -		dev_dbg(dev, "stream off entity %s pad:%d mask:0x%llx\n",
> > > > -			sd->name, r_pad->index, stream_mask);
> > > > -		ret = v4l2_subdev_disable_streams(sd, r_pad->index,
> > > > -						  stream_mask);
> > > > -		if (ret) {
> > > > -			dev_err(dev, "stream off %s failed with %d\n", sd->name,
> > > > -				ret);
> > > > -			return ret;
> > > > +		for (i = CSI2_PAD_SRC; i < NR_OF_CSI2_SRC_PADS; i++) {
> > > > +			if (!media_pad_pipeline(&sd->entity.pads[i]))
> > > > +				continue;
> > > > +			ret = v4l2_subdev_disable_streams(sd, i, 1U);
> > > > +			if (ret) {
> > > > +				dev_err(dev, "stream off %s failed with %d\n",
> > > > +					sd->name, ret);
> > > > +				return ret;
> > > > +			}
> > > >  		}
> > > >  		close_streaming_firmware(av);
> > > >  	} else {
> > > > @@ -1036,13 +1038,15 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> > > >  		}
> > > >  
> > > >  		/* start sub-device which connects with video */
> > > > -		dev_dbg(dev, "stream on %s pad %d mask 0x%llx\n", sd->name,
> > > > -			r_pad->index, stream_mask);
> > > > -		ret = v4l2_subdev_enable_streams(sd, r_pad->index, stream_mask);
> > > > -		if (ret) {
> > > > -			dev_err(dev, "stream on %s failed with %d\n", sd->name,
> > > > -				ret);
> > > > -			goto out_media_entity_stop_streaming_firmware;
> > > > +		for (i = CSI2_PAD_SRC; i < NR_OF_CSI2_SRC_PADS; i++) {
> > > > +			if (!media_pad_pipeline(&sd->entity.pads[i]))
> > > > +				continue;
> > > > +			ret = v4l2_subdev_enable_streams(sd, i, 1U);
> > > > +			if (ret) {
> > > > +				dev_err(dev, "stream on %s failed with %d\n",
> > > > +					sd->name, ret);
> > > > +				goto out_media_entity_stop_streaming_firmware;
> > > > +			}
> > > >  		}
> > > >  	}
> > > >  
> > > > @@ -1051,6 +1055,15 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> > > >  	return 0;
> > > >  
> > > >  out_media_entity_stop_streaming_firmware:
> > > > +	while (i-- > CSI2_PAD_SRC) {
> > > > +		int ret2;
> > > > +
> > > > +		if (!media_pad_pipeline(&sd->entity.pads[i]))
> > > > +			continue;
> > > > +		ret2 = v4l2_subdev_disable_streams(sd, i, 1U);
> > > > +		dev_err(dev, "stream off %s failed with %d\n", sd->name, ret2);
> > > > +	}
> > > > +
> > > >  	stop_streaming_firmware(av);
> > > >  
> > > >  	return ret;
> 

-- 
Regards,

Sakari Ailus