RE: [PATCH v3 1/3] media: nxp: imx8-isi: Implement per-stream reference counting for multiplexed streams

[G.N. Zhou]

Hi Frank,

Thanks for the review.

> -----Original Message-----
> From: Frank Li (OSS) <frank.li@xxxxxxxxxxx>
> Sent: Friday, July 24, 2026 12:23 AM
> To: G.N. Zhou (OSS) <guoniu.zhou@xxxxxxxxxxx>
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho
> Chehab <mchehab@xxxxxxxxxx>; Frank Li <frank.li@xxxxxxx>; Sascha Hauer
> <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>;
> c.hemp@xxxxxxxxx; Stefan Riedmüller <s.riedmueller@xxxxxxxxx>; Jacopo
> Mondi <jacopo@xxxxxxxxxx>; Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>;
> Bryan O'Donoghue <bod@xxxxxxxxxx>; Aisheng Dong
> <aisheng.dong@xxxxxxx>; G.N. Zhou <guoniu.zhou@xxxxxxx>; linux-
> media@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/3] media: nxp: imx8-isi: Implement per-stream
> reference counting for multiplexed streams
> 
> On Thu, Jul 23, 2026 at 05:16:37PM +0800, Guoniu Zhou wrote:
> > The ISI crossbar needs to properly enable multiple streams from
> > different virtual channels on the same input pad. Currently only the
> > first stream gets enabled in hardware, subsequent streams are silently
> ignored.
> >
> > The driver uses a single enable_count per input to track the input state.
> > When enable_count is non-zero, the code assumes the input is already
> > active and skips calling v4l2_subdev_enable_streams() for additional streams:
> >
> >   Call 1: enable_streams(stream 0)
> >     -> enable_count == 0, enable gasket and stream 0 in hardware
> >     -> enable_count = 1
> >
> >   Call 2: enable_streams(stream 1)
> >     -> enable_count == 1, skip hardware enable
> >     -> enable_count = 2
> >     -> stream 1 never gets enabled
> >
> > Similarly on disable, when enable_count reaches zero, ALL streams are
> > disabled regardless of which streams are actually still active.
> >
> > Implement per-stream state tracking by storing the input index and
> > stream mask in the mxc_isi_pipe structure. On enable, record which
> > input and stream the pipe is receiving. On disable, clear the pipe's
> > record and check if any other pipe is still using the same input
> > stream before actually disabling it.
> >
> > Signed-off-by: Guoniu Zhou <guoniu.zhou@xxxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Change from "Fix" to "Implement" as multi-stream support is for newer
> >   SoCs not present when the driver was merged (Laurent)
> > - Remove Fixes and Cc: stable tags (Laurent)
> > - Store input/input_stream in mxc_isi_pipe instead of using per-stream
> >   counters array, avoiding arbitrary 64-entry limit (Laurent)
> > - Use UINT_MAX to mark pipe as inactive to distinguish from valid pad
> > 0
> >
> > Changes in v2:
> > - Use fixed-size array for enabled_count instead of dynamic allocation
> > - Use BIT_ULL() macro for u64 bitmask operations
> > - Use MXC_ISI_MAX_STREAMS (64) as loop boundary instead of num_sources
> > - Remove mxc_isi_stream_counters_alloc/free functions
> > ---
> >  .../media/platform/nxp/imx8-isi/imx8-isi-core.h    |  5 +-
> >  .../platform/nxp/imx8-isi/imx8-isi-crossbar.c      | 66 +++++++++++++++-------
> >  .../media/platform/nxp/imx8-isi/imx8-isi-pipe.c    |  1 +
> >  3 files changed, 52 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > index 9bb4d430d15e..c07abdd6a1f0 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> > @@ -186,7 +186,7 @@ struct mxc_isi_dma_buffer {  };
> >
> >  struct mxc_isi_input {
> > -	unsigned int			enable_count;
> > +	u64				enabled_streams;
> >  };
> >
> >  struct mxc_isi_crossbar {
> > @@ -259,6 +259,9 @@ struct mxc_isi_pipe {
> >  	u8				acquired_res;
> >  	u8				chained_res;
> >  	bool				chained;
> > +
> > +	unsigned int			input;
> > +	u64				input_stream;
> >  };
> >
> >  struct mxc_isi_m2m {
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > index 9f0231ca47a3..6bd61bfb15d6 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> > @@ -335,8 +335,10 @@ static int mxc_isi_crossbar_enable_streams(struct
> v4l2_subdev *sd,
> >  					   u32 pad, u64 streams_mask)
> >  {
> >  	struct mxc_isi_crossbar *xbar = to_isi_crossbar(sd);
> > +	struct mxc_isi_pipe *pipe = &xbar->isi->pipes[pad -
> > +xbar->num_sinks];
> >  	struct v4l2_subdev *remote_sd;
> >  	struct mxc_isi_input *input;
> > +	u64 streams_to_enable;
> >  	u64 sink_streams;
> >  	u32 sink_pad;
> >  	u32 remote_pad;
> > @@ -350,30 +352,41 @@ static int
> > mxc_isi_crossbar_enable_streams(struct v4l2_subdev *sd,
> >
> >  	input = &xbar->inputs[sink_pad];
> >
> > -	/*
> > -	 * TODO: Track per-stream enable counts to support multiplexed
> > -	 * streams.
> > -	 */
> > -	if (!input->enable_count) {
> > +	/* Enable the gasket when the first stream is enabled for this input. */
> > +	if (!input->enabled_streams) {
> 
> The similar with disable case, if other pipe already enable it, do we need skip
> call mxc_isi_crossbar_gasket_enable()?
> 
> enable() and disable() is not symmetric

The enable and disable paths are actually symmetric in behavior, just implemented
differently:

Enable path: Uses input->enabled_streams bitmask to track which streams are already
enabled. When a second pipe tries to enable the same stream:
- streams_to_enable = sink_streams & ~input->enabled_streams evaluates to 0
- So v4l2_subdev_enable_streams() is NOT called again
- Gasket is only enabled when !input->enabled_streams (first user)

Disable path: Checks if any other pipe is still using the same input stream before
actually disabling.

Both achieve the same goal: enable on first user, disable on last user. The enable path
relies on the bitmask to skip redundant calls, while the disable path needs to check pipe
state because we're about to clear this pipe's record.

That said, I agree the current approach can be confusing to readers since the two paths
use different mechanisms. I will unify the state management to use the same pattern in
both enable and disable paths.

> 
> Frank
> 
> >  		ret = mxc_isi_crossbar_gasket_enable(xbar, state, remote_sd,
> >  						     remote_pad, sink_pad);
> >  		if (ret)
> >  			return ret;
> > +	}
> > +
> > +	/* Only enable streams that are not already enabled. */
> > +	streams_to_enable = sink_streams & ~input->enabled_streams;
> >
> > +	if (streams_to_enable) {
> >  		ret = v4l2_subdev_enable_streams(remote_sd, remote_pad,
> > -						 sink_streams);
> > +						 streams_to_enable);
> >  		if (ret) {
> >  			dev_err(xbar->isi->dev,
> >  				"failed to enable streams 0x%llx on
> '%s':%u: %d\n",
> > -				sink_streams, remote_sd->name, remote_pad,
> ret);
> > -			mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
> > -			return ret;
> > +				streams_to_enable, remote_sd->name,
> remote_pad, ret);
> > +			goto err_gasket_disable;
> >  		}
> > +
> > +		input->enabled_streams |= streams_to_enable;
> >  	}
> >
> > -	input->enable_count++;
> > +	/* Record the input and stream for this pipe. */
> > +	pipe->input = sink_pad;
> > +	pipe->input_stream = sink_streams;
> >
> >  	return 0;
> > +
> > +err_gasket_disable:
> > +	if (!input->enabled_streams)
> > +		mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
> > +
> > +	return ret;
> >  }
> >
> >  static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
> > @@ -381,6 +394,7 @@ static int mxc_isi_crossbar_disable_streams(struct
> v4l2_subdev *sd,
> >  					    u32 pad, u64 streams_mask)
> >  {
> >  	struct mxc_isi_crossbar *xbar = to_isi_crossbar(sd);
> > +	struct mxc_isi_pipe *pipe = &xbar->isi->pipes[pad -
> > +xbar->num_sinks];
> >  	struct v4l2_subdev *remote_sd;
> >  	struct mxc_isi_input *input;
> >  	u64 sink_streams;
> > @@ -396,19 +410,33 @@ static int
> > mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
> >
> >  	input = &xbar->inputs[sink_pad];
> >
> > -	input->enable_count--;
> > +	/* Clear the input and stream for this pipe. */
> > +	pipe->input = UINT_MAX;
> > +	pipe->input_stream = 0;
> >
> > -	if (!input->enable_count) {
> > -		ret = v4l2_subdev_disable_streams(remote_sd, remote_pad,
> > -						  sink_streams);
> > -		if (ret)
> > -			dev_err(xbar->isi->dev,
> > -				"failed to disable streams 0x%llx on
> '%s':%u: %d\n",
> > -				sink_streams, remote_sd->name, remote_pad,
> ret);
> > +	/*
> > +	 * Check if any other pipe receives the same input stream. If so we
> > +	 * can't disable it yet, so return immediately.
> > +	 */
> > +	for (unsigned int i = 0; i < xbar->isi->pdata->num_channels; ++i) {
> > +		struct mxc_isi_pipe *pipe = &xbar->isi->pipes[i];
> >
> > -		mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
> > +		if (pipe->input == sink_pad &&
> > +		    pipe->input_stream == sink_streams)
> > +			return 0;
> >  	}
> >
> > +	ret = v4l2_subdev_disable_streams(remote_sd, remote_pad,
> sink_streams);
> > +	if (ret)
> > +		dev_err(xbar->isi->dev,
> > +			"failed to disable streams 0x%llx on '%s':%u: %d\n",
> > +			sink_streams, remote_sd->name, remote_pad, ret);
> > +
> > +	input->enabled_streams &= ~sink_streams;
> > +
> > +	if (!input->enabled_streams)
> > +		mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
> > +
> >  	return ret;
> >  }
> >
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > index 16085f23bc0b..c0ec59856374 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-pipe.c
> > @@ -816,6 +816,7 @@ int mxc_isi_pipe_init(struct mxc_isi_dev *isi,
> unsigned int id)
> >  	pipe->acquired_res = 0;
> >  	pipe->chained_res = 0;
> >  	pipe->chained = false;
> > +	pipe->input = UINT_MAX;
> >
> >  	sd = &pipe->sd;
> >  	v4l2_subdev_init(sd, &mxc_isi_pipe_subdev_ops);
> >
> > --
> > 2.34.1
> >
> >