Currently, if streamon/streamoff calls are imbalanced we can either end up
with a negative ISI m2m usage_count (if streamoff() is called more times
than streamon()) in which case we'll not be able to restart the ISI pipe
next time, or the usage_count never gets to 0 and the pipe is never
switched off.

So, to avoid that, add an 'in_use' flag in the ctx structure that will
keep track whether the output/capture queues have been started or not,
and use it to avoid decrementing/incrementing the usage_count
unnecessarily.

Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver")
Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
---
v2:
 * Changed the way 'usage_count' is incremented/decremented by taking
   into account the context the streamon/streamoff functions are called
   from;
 * Changed the commit message and subject to reflect the changes;

 .../platform/nxp/imx8-isi/imx8-isi-m2m.c      | 24 +++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
index 9745d6219a166..3f06ae1349e53 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
@@ -65,6 +65,7 @@ struct mxc_isi_m2m_ctx {
 	} ctrls;
 
 	bool chained;
+	bool in_use[2];
 };
 
 static inline struct mxc_isi_m2m_buffer *
@@ -491,6 +492,7 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 	const struct mxc_isi_format_info *cap_info = ctx->queues.cap.info;
 	const struct mxc_isi_format_info *out_info = ctx->queues.out.info;
 	struct mxc_isi_m2m *m2m = ctx->m2m;
+	bool already_in_use;
 	bool bypass;
 
 	int ret;
@@ -502,6 +504,8 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 		goto unlock;
 	}
 
+	already_in_use = ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE];
+
 	bypass = cap_pix->width == out_pix->width &&
 		 cap_pix->height == out_pix->height &&
 		 cap_info->encoding == out_info->encoding;
@@ -520,7 +524,10 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 		mxc_isi_channel_get(m2m->pipe);
 	}
 
-	m2m->usage_count++;
+	if (!already_in_use) {
+		m2m->usage_count++;
+		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = true;
+	}
 
 	/*
 	 * Allocate resources for the channel, counting how many users require
@@ -555,7 +562,12 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 	ctx->chained = false;
 
 deinit:
-	if (--m2m->usage_count == 0) {
+	if (!already_in_use) {
+		m2m->usage_count--;
+		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
+	}
+
+	if (m2m->usage_count == 0) {
 		mxc_isi_channel_put(m2m->pipe);
 		mxc_isi_channel_release(m2m->pipe);
 	}
@@ -575,6 +587,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
 
 	mutex_lock(&m2m->lock);
 
+	if (!ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE])
+		goto unlock;
+
 	/*
 	 * If the last context is this one, reset it to make sure the device
 	 * will be reconfigured when streaming is restarted.
@@ -587,6 +602,8 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
 		mxc_isi_channel_unchain(m2m->pipe);
 	ctx->chained = false;
 
+	ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
+
 	/* Turn off the light with the last user. */
 	if (--m2m->usage_count == 0) {
 		mxc_isi_channel_disable(m2m->pipe);
@@ -594,8 +611,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
 		mxc_isi_channel_release(m2m->pipe);
 	}
 
-	WARN_ON(m2m->usage_count < 0);
-
+unlock:
 	mutex_unlock(&m2m->lock);
 
 	return 0;
-- 
2.34.1

On Fri, Sep 27, 2024 at 11:04:20AM +0300, Laurentiu Palcu wrote:
> Currently, if streamon/streamoff calls are imbalanced we can either end up
> with a negative ISI m2m usage_count (if streamoff() is called more times
> than streamon()) in which case we'll not be able to restart the ISI pipe
> next time, or the usage_count never gets to 0 and the pipe is never
> switched off.
> 
> So, to avoid that, add an 'in_use' flag in the ctx structure that will
> keep track whether the output/capture queues have been started or not,
> and use it to avoid decrementing/incrementing the usage_count
> unnecessarily.
> 
> Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver")
> Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> ---
> v2:
>  * Changed the way 'usage_count' is incremented/decremented by taking
>    into account the context the streamon/streamoff functions are called
>    from;
>  * Changed the commit message and subject to reflect the changes;
> 
>  .../platform/nxp/imx8-isi/imx8-isi-m2m.c      | 24 +++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> index 9745d6219a166..3f06ae1349e53 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> @@ -65,6 +65,7 @@ struct mxc_isi_m2m_ctx {
>  	} ctrls;
>  
>  	bool chained;
> +	bool in_use[2];

I think you can store this in mxc_isi_m2m_ctx_queue_data instead as a

	bool streaming;

>  };
>  
>  static inline struct mxc_isi_m2m_buffer *
> @@ -491,6 +492,7 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  	const struct mxc_isi_format_info *cap_info = ctx->queues.cap.info;
>  	const struct mxc_isi_format_info *out_info = ctx->queues.out.info;
>  	struct mxc_isi_m2m *m2m = ctx->m2m;
> +	bool already_in_use;
>  	bool bypass;
>  
>  	int ret;
> @@ -502,6 +504,8 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  		goto unlock;
>  	}
>  
> +	already_in_use = ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE];
> +

As the streamon and streamoff operation are covered by the same queue
lock for all contexts, you can do all this outside of the m2m->lock
sections. I think the following patch (untested) should do and would be
simpler:

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
index 9745d6219a16..cd6c52e9d158 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
@@ -43,6 +43,7 @@ struct mxc_isi_m2m_ctx_queue_data {
 	struct v4l2_pix_format_mplane format;
 	const struct mxc_isi_format_info *info;
 	u32 sequence;
+	bool streaming;
 };

 struct mxc_isi_m2m_ctx {
@@ -486,15 +487,18 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 				enum v4l2_buf_type type)
 {
 	struct mxc_isi_m2m_ctx *ctx = to_isi_m2m_ctx(fh);
+	struct mxc_isi_m2m_ctx_queue_data *q = mxc_isi_m2m_ctx_qdata(ctx, type);
 	const struct v4l2_pix_format_mplane *out_pix = &ctx->queues.out.format;
 	const struct v4l2_pix_format_mplane *cap_pix = &ctx->queues.cap.format;
 	const struct mxc_isi_format_info *cap_info = ctx->queues.cap.info;
 	const struct mxc_isi_format_info *out_info = ctx->queues.out.info;
 	struct mxc_isi_m2m *m2m = ctx->m2m;
 	bool bypass;
-
 	int ret;

+	if (q->streaming)
+		return 0;
+
 	mutex_lock(&m2m->lock);

 	if (m2m->usage_count == INT_MAX) {
@@ -547,6 +551,8 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
 		goto unchain;
 	}

+	q->streaming = true;
+
 	return 0;

 unchain:
@@ -569,10 +575,14 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
 				 enum v4l2_buf_type type)
 {
 	struct mxc_isi_m2m_ctx *ctx = to_isi_m2m_ctx(fh);
+	struct mxc_isi_m2m_ctx_queue_data *q = mxc_isi_m2m_ctx_qdata(ctx, type);
 	struct mxc_isi_m2m *m2m = ctx->m2m;

 	v4l2_m2m_ioctl_streamoff(file, fh, type);

+	if (!q->streaming)
+		return 0;
+
 	mutex_lock(&m2m->lock);

 	/*
@@ -598,6 +608,8 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,

 	mutex_unlock(&m2m->lock);

+	q->streaming = false;
+
 	return 0;
 }

If this works for you, could you turn it into a v3 ?

>  	bypass = cap_pix->width == out_pix->width &&
>  		 cap_pix->height == out_pix->height &&
>  		 cap_info->encoding == out_info->encoding;
> @@ -520,7 +524,10 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  		mxc_isi_channel_get(m2m->pipe);
>  	}
>  
> -	m2m->usage_count++;
> +	if (!already_in_use) {
> +		m2m->usage_count++;
> +		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = true;
> +	}
>  
>  	/*
>  	 * Allocate resources for the channel, counting how many users require
> @@ -555,7 +562,12 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  	ctx->chained = false;
>  
>  deinit:
> -	if (--m2m->usage_count == 0) {
> +	if (!already_in_use) {
> +		m2m->usage_count--;
> +		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
> +	}
> +
> +	if (m2m->usage_count == 0) {
>  		mxc_isi_channel_put(m2m->pipe);
>  		mxc_isi_channel_release(m2m->pipe);
>  	}
> @@ -575,6 +587,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
>  
>  	mutex_lock(&m2m->lock);
>  
> +	if (!ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE])
> +		goto unlock;
> +
>  	/*
>  	 * If the last context is this one, reset it to make sure the device
>  	 * will be reconfigured when streaming is restarted.
> @@ -587,6 +602,8 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
>  		mxc_isi_channel_unchain(m2m->pipe);
>  	ctx->chained = false;
>  
> +	ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
> +
>  	/* Turn off the light with the last user. */
>  	if (--m2m->usage_count == 0) {
>  		mxc_isi_channel_disable(m2m->pipe);
> @@ -594,8 +611,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
>  		mxc_isi_channel_release(m2m->pipe);
>  	}
>  
> -	WARN_ON(m2m->usage_count < 0);
> -
> +unlock:
>  	mutex_unlock(&m2m->lock);
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On Tue, Oct 22, 2024 at 02:05:19AM +0300, Laurent Pinchart wrote:
> On Fri, Sep 27, 2024 at 11:04:20AM +0300, Laurentiu Palcu wrote:
> > Currently, if streamon/streamoff calls are imbalanced we can either end up
> > with a negative ISI m2m usage_count (if streamoff() is called more times
> > than streamon()) in which case we'll not be able to restart the ISI pipe
> > next time, or the usage_count never gets to 0 and the pipe is never
> > switched off.
> > 
> > So, to avoid that, add an 'in_use' flag in the ctx structure that will
> > keep track whether the output/capture queues have been started or not,
> > and use it to avoid decrementing/incrementing the usage_count
> > unnecessarily.
> > 
> > Fixes: cf21f328fcafac ("media: nxp: Add i.MX8 ISI driver")
> > Signed-off-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > ---
> > v2:
> >  * Changed the way 'usage_count' is incremented/decremented by taking
> >    into account the context the streamon/streamoff functions are called
> >    from;
> >  * Changed the commit message and subject to reflect the changes;
> > 
> >  .../platform/nxp/imx8-isi/imx8-isi-m2m.c      | 24 +++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > index 9745d6219a166..3f06ae1349e53 100644
> > --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> > @@ -65,6 +65,7 @@ struct mxc_isi_m2m_ctx {
> >  	} ctrls;
> >  
> >  	bool chained;
> > +	bool in_use[2];
> 
> I think you can store this in mxc_isi_m2m_ctx_queue_data instead as a
> 
> 	bool streaming;
> 
> >  };
> >  
> >  static inline struct mxc_isi_m2m_buffer *
> > @@ -491,6 +492,7 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
> >  	const struct mxc_isi_format_info *cap_info = ctx->queues.cap.info;
> >  	const struct mxc_isi_format_info *out_info = ctx->queues.out.info;
> >  	struct mxc_isi_m2m *m2m = ctx->m2m;
> > +	bool already_in_use;
> >  	bool bypass;
> >  
> >  	int ret;
> > @@ -502,6 +504,8 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
> >  		goto unlock;
> >  	}
> >  
> > +	already_in_use = ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE];
> > +
> 
> As the streamon and streamoff operation are covered by the same queue
> lock for all contexts, you can do all this outside of the m2m->lock
> sections. I think the following patch (untested) should do and would be
> simpler:
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> index 9745d6219a16..cd6c52e9d158 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-m2m.c
> @@ -43,6 +43,7 @@ struct mxc_isi_m2m_ctx_queue_data {
>  	struct v4l2_pix_format_mplane format;
>  	const struct mxc_isi_format_info *info;
>  	u32 sequence;
> +	bool streaming;
>  };
> 
>  struct mxc_isi_m2m_ctx {
> @@ -486,15 +487,18 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  				enum v4l2_buf_type type)
>  {
>  	struct mxc_isi_m2m_ctx *ctx = to_isi_m2m_ctx(fh);
> +	struct mxc_isi_m2m_ctx_queue_data *q = mxc_isi_m2m_ctx_qdata(ctx, type);
>  	const struct v4l2_pix_format_mplane *out_pix = &ctx->queues.out.format;
>  	const struct v4l2_pix_format_mplane *cap_pix = &ctx->queues.cap.format;
>  	const struct mxc_isi_format_info *cap_info = ctx->queues.cap.info;
>  	const struct mxc_isi_format_info *out_info = ctx->queues.out.info;
>  	struct mxc_isi_m2m *m2m = ctx->m2m;
>  	bool bypass;
> -
>  	int ret;
> 
> +	if (q->streaming)
> +		return 0;
> +
>  	mutex_lock(&m2m->lock);
> 
>  	if (m2m->usage_count == INT_MAX) {
> @@ -547,6 +551,8 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
>  		goto unchain;
>  	}
> 
> +	q->streaming = true;
> +
>  	return 0;
> 
>  unchain:
> @@ -569,10 +575,14 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
>  				 enum v4l2_buf_type type)
>  {
>  	struct mxc_isi_m2m_ctx *ctx = to_isi_m2m_ctx(fh);
> +	struct mxc_isi_m2m_ctx_queue_data *q = mxc_isi_m2m_ctx_qdata(ctx, type);
>  	struct mxc_isi_m2m *m2m = ctx->m2m;
> 
>  	v4l2_m2m_ioctl_streamoff(file, fh, type);
> 
> +	if (!q->streaming)
> +		return 0;
> +
>  	mutex_lock(&m2m->lock);
> 
>  	/*
> @@ -598,6 +608,8 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
> 
>  	mutex_unlock(&m2m->lock);
> 
> +	q->streaming = false;
> +
>  	return 0;
>  }
> 
> If this works for you, could you turn it into a v3 ?
>

It's funny that in my first tries to fix this I added a 'streaming' flag
to mxc_isi_m2m_ctx_queue_data too. But I must've messed up something
else as I dropped it and chose the other, more complicated, solution. :/

Anyway, I gave your patch a test and it seems to work fine so far.

I'll send a v3 with your suggestion.

Thanks,
Laurentiu

> >  	bypass = cap_pix->width == out_pix->width &&
> >  		 cap_pix->height == out_pix->height &&
> >  		 cap_info->encoding == out_info->encoding;
> > @@ -520,7 +524,10 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
> >  		mxc_isi_channel_get(m2m->pipe);
> >  	}
> >  
> > -	m2m->usage_count++;
> > +	if (!already_in_use) {
> > +		m2m->usage_count++;
> > +		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = true;
> > +	}
> >  
> >  	/*
> >  	 * Allocate resources for the channel, counting how many users require
> > @@ -555,7 +562,12 @@ static int mxc_isi_m2m_streamon(struct file *file, void *fh,
> >  	ctx->chained = false;
> >  
> >  deinit:
> > -	if (--m2m->usage_count == 0) {
> > +	if (!already_in_use) {
> > +		m2m->usage_count--;
> > +		ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
> > +	}
> > +
> > +	if (m2m->usage_count == 0) {
> >  		mxc_isi_channel_put(m2m->pipe);
> >  		mxc_isi_channel_release(m2m->pipe);
> >  	}
> > @@ -575,6 +587,9 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
> >  
> >  	mutex_lock(&m2m->lock);
> >  
> > +	if (!ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE])
> > +		goto unlock;
> > +
> >  	/*
> >  	 * If the last context is this one, reset it to make sure the device
> >  	 * will be reconfigured when streaming is restarted.
> > @@ -587,6 +602,8 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
> >  		mxc_isi_channel_unchain(m2m->pipe);
> >  	ctx->chained = false;
> >  
> > +	ctx->in_use[type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE] = false;
> > +
> >  	/* Turn off the light with the last user. */
> >  	if (--m2m->usage_count == 0) {
> >  		mxc_isi_channel_disable(m2m->pipe);
> > @@ -594,8 +611,7 @@ static int mxc_isi_m2m_streamoff(struct file *file, void *fh,
> >  		mxc_isi_channel_release(m2m->pipe);
> >  	}
> >  
> > -	WARN_ON(m2m->usage_count < 0);
> > -
> > +unlock:
> >  	mutex_unlock(&m2m->lock);
> >  
> >  	return 0;
> 
> -- 
> Regards,
> 
> Laurent Pinchart