Re: [PATCH v2 3/6] media: nxp: imx8-isi: Fix per-stream reference counting for multiplexed streams

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

On Mon, Jul 20, 2026 at 11:34:05AM +0800, Guoniu Zhou wrote:
> The ISI crossbar fails to properly enable multiple streams from different
> virtual channels on the same input pad. 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 (BUG!)
>     -> 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.
> 
> Fix this by tracking per-stream state using:
> - enabled_streams (u64 bitmask): tracks which streams are currently enabled
> - enabled_count[] (array): per-stream reference counter to support the same
>   stream being enabled/disabled multiple times
> 
> Now each stream is independently enabled/disabled in hardware based on the
> enabled_streams bitmask, while enabled_count[] provides reference counting
> for scenarios where the same stream is enabled multiple times, such as
> duplicate cases in the ISI stream.
> 
> Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
> Cc: stable@vger.kernel.org

As far as I understand, only newer SoCs support multi-stream for ISI
inputs (please tell me if that's incorrect). Those SoCs were not
supported when the ISI driver was merged, so I don't think this should
be backported to stable kernels. The subject line should then read
"Implement per-stream ..." instead of "Fix per-stream ..." and the
commit message adapted accordingly.

> Signed-off-by: Guoniu Zhou <guoniu.zhou@oss.nxp.com>
> ---
> 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    |  7 +-
>  .../platform/nxp/imx8-isi/imx8-isi-crossbar.c      | 76 ++++++++++++++++------
>  2 files changed, 63 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 7547a6559d4c..9adbe2fe7cf8 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.h
> @@ -184,8 +184,13 @@ struct mxc_isi_dma_buffer {
>  	dma_addr_t			dma;
>  };
>  
> +/* V4L2 subdev max stream ID is 63, need 64 counters (0-63) */
> +#define MXC_ISI_MAX_STREAMS		64
> +
>  struct mxc_isi_input {
> -	unsigned int			enable_count;
> +	u64				enabled_streams;
> +	/* Per-stream reference counter */
> +	unsigned int			enabled_count[MXC_ISI_MAX_STREAMS];

You can save some memory here by replacing unsigned int with u8. The
counter can never go above the number of ISI outputs, as streams can be
enabled once only.

>  };
>  
>  struct mxc_isi_crossbar {
> 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 328d08a278ea..64576f6bd45c 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
> @@ -337,6 +337,8 @@ static int mxc_isi_crossbar_enable_streams(struct v4l2_subdev *sd,
>  	struct mxc_isi_crossbar *xbar = to_isi_crossbar(sd);
>  	struct v4l2_subdev *remote_sd;
>  	struct mxc_isi_input *input;
> +	u64 streams_to_enable;
> +	unsigned long stream;

unsigned int is enough, this is a counter, not a bitmask.

>  	u64 sink_streams;
>  	u32 sink_pad;
>  	u32 remote_pad;
> @@ -350,30 +352,47 @@ 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) {

Let's keep a short comment here:

	/* Enable the gasket when the first stream is enabled for this input. */

> +	if (!input->enabled_streams) {
>  		ret = mxc_isi_crossbar_gasket_enable(xbar, state, remote_sd,
>  						     remote_pad, sink_pad);
>  		if (ret)
>  			return ret;
> +	}
>  
> +	/*
> +	 * Track per-stream enable counts to support multiplexed streams.
> +	 * 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++;
> +	/* Increment reference count for all requested streams */
> +	for (stream = 0; stream < MXC_ISI_MAX_STREAMS; stream++) {

	for (unsigned int stream = 0; stream < MXC_ISI_MAX_STREAMS; stream++) {

and drop the local variable above.

> +		if (!(sink_streams & BIT_ULL(stream)))
> +			continue;
> +
> +		input->enabled_count[stream]++;
> +	}

Each ISI pipe processes a single stream (the driver uses the
V4L2_SUBDEV_ROUTING_NO_N_TO_1 validation flag, and patch 2/6 ensures
that only stream 0 is accepted on the source side of the crossbar). This
means that that, after translating streams_mask to the sink side,
sink_streams should have a single bit set, and streams_to_enable will
have either 0 or 1 bit set.

If that explanation is correct (please let me know if you disagree),
then this loop can be replaced with

	input->enabled_count[__ffs64(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,
> @@ -383,6 +402,8 @@ static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
>  	struct mxc_isi_crossbar *xbar = to_isi_crossbar(sd);
>  	struct v4l2_subdev *remote_sd;
>  	struct mxc_isi_input *input;
> +	u64 streams_to_disable = 0;
> +	unsigned long stream;

unsigned int here too.

>  	u64 sink_streams;
>  	u32 sink_pad;
>  	u32 remote_pad;
> @@ -396,19 +417,36 @@ static int mxc_isi_crossbar_disable_streams(struct v4l2_subdev *sd,
>  
>  	input = &xbar->inputs[sink_pad];
>  
> -	input->enable_count--;
> +	/*
> +	 * Decrease the enable count for each stream. Only disable streams
> +	 * whose count reaches zero.
> +	 */
> +	for (stream = 0; stream < MXC_ISI_MAX_STREAMS; stream++) {

	for (unsigned int stream = 0; stream < MXC_ISI_MAX_STREAMS; stream++) {

> +		if (!(sink_streams & BIT_ULL(stream)))
> +			continue;
>  
> -	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);
> +		if (!(input->enabled_streams & BIT_ULL(stream)))
> +			continue;
>  
> -		mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
> +		if (--input->enabled_count[stream] == 0)
> +			streams_to_disable |= BIT_ULL(stream);
>  	}

Similarly here, the whole loop could be replaced by

	if (--input->enabled_count[__ffs64(stream)] == 0)
		streams_to_disable |= BIT_ULL(stream);

Another option is to store the information in the mxc_isi_pipe structure
in the form of two new fields:

	unsigned int input;
	unsigned int input_stream;

The fields would be set to the input index and the sink stream mask in
mxc_isi_crossbar_enable_streams():


	struct mxc_isi_pipe *pipe = &xbar->isi->pipes[pad - xbar->num_sources];

	...

	pipe->input = sink_pad;
	pipe->input_stream = sink_streams;

In this function you would then do (completely untested)

	struct mxc_isi_pipe *pipe = &xbar->isi->pipes[pad - xbar->num_sources];

	...

	pipe->input = 0;
	pipe->input_stream = 0;

	/*
	 * Check if any other pipe receives the same input stream. If so we
	 * can't disable it yet, so return immediately.
	 */
	for (i = 0; i < xbar->isi->pdata->num_channels; ++i) {
		struct mxc_isi_pipe *pipe = &xbar->isi->pipes[i];

		if (pipe->input == sink_pad &&
		    pipe->input_stream == sink_streams)
			return 0;
	}

	input->enabled_streams &= ~sink_streams;

The advantage of this is that we won't have to add a 64 entries array to
each input. I'm not too concerned about the memory usage (even if
reducing memory usage is always nice, more for the improvement in cache
locality than for the memory saving itself), but the 64 entries feel a
bit arbitrary.

What do you think is best ?

> +	if (!streams_to_disable)
> +		return 0;
> +
> +	ret = v4l2_subdev_disable_streams(remote_sd, remote_pad,
> +					  streams_to_disable);
> +	if (ret)
> +		dev_err(xbar->isi->dev,
> +			"failed to disable streams 0x%llx on '%s':%u: %d\n",
> +			streams_to_disable, remote_sd->name, remote_pad, ret);
> +
> +	input->enabled_streams &= ~streams_to_disable;
> +
> +	if (!input->enabled_streams)
> +		mxc_isi_crossbar_gasket_disable(xbar, sink_pad);
> +
>  	return ret;
>  }
>  

-- 
Regards,

Laurent Pinchart