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@xxxxxxxxxxx>
> ---
> 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
