Re: [PATCH v3 07/13] media: ipu6: Close firmware streams on streaming enable failure

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

Hi Bingbu,

On Mon, Jan 05, 2026 at 11:07:02AM +0800, Bingbu Cao wrote:
> Sakari,
> 
> On 1/2/26 3:58 AM, Sakari Ailus wrote:
> > Hi Bingbu,
> > 
> > On Wed, Dec 31, 2025 at 02:11:40PM +0800, Bingbu Cao wrote:
> >> Sakari,
> >>
> >> Thanks for the patch.
> >>
> >> On 12/30/25 9:10 PM, Sakari Ailus wrote:
> >>> When enabling streaming fails, the stream is stopped in firmware but not
> >>> closed. Do this to release resources on firmware side.
> >>>
> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>> ---
> >>>  drivers/media/pci/intel/ipu6/ipu6-isys-video.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> >>> index dec8f5ffcfa5..919b77107cef 100644
> >>> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> >>> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-video.c
> >>> @@ -1066,6 +1066,7 @@ int ipu6_isys_video_set_streaming(struct ipu6_isys_video *av, int state,
> >>>  
> >>>  out_media_entity_stop_streaming_firmware:
> >>>  	stop_streaming_firmware(av);
> >>> +	close_streaming_firmware(av);
> >>
> >> It looks the close_streaming_firmware() has no chance to run if
> >> v4l2_subdev_disable_streams() above failed. Beside, the
> >> stop_streaming_firmware() is better called after subdev
> >> disable_streams().
> > 
> > Do you mean we wouldn't need to call stop_streaming_firmware() at all here?
> > That would be actually aligned with start_streaming_firmware() error
> > handling.
> >
> 
> I mean the code above:
>   	if (!state) {
> 		...
> 		ret = v4l2_subdev_disable_streams(sd, r_pad->index,
> 						  stream_mask);
> 		if (ret) {
> 			...
> 			return ret;
> 		}
> 		close_streaming_firmware(av);
> 	} else {
> 
> close_streaming_firmware() should be called anyway.

Ah, good point. I guess in this case it'd be probably best to just ignore
the return value -- but maybe complain -- from disable_streams() above. I
can post a patch for that.

Presumably, the same stream wasn't disabled on the source (as we've been
called) so it'd take a bug somewhere for disable_streams() to return an
error.

-- 
Kind regards,

Sakari Ailus