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