[PATCH 1/3] [media] v4l2-mem2mem: use CAPTURE queue lock

[PATCH 1/3] [media] v4l2-mem2mem: use CAPTURE queue lock

[John Sheu]

From: John Sheu <sheu@chromium.org>

In v4l2_m2m_try_schedule(), use the CAPTURE queue lock when accessing
the CAPTURE queue, instead of relying on just holding the OUTPUT queue
lock.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 438ea45..c52a2c5 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -230,12 +230,15 @@ static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 		dprintk("No input buffers available\n");
 		return;
 	}
+	spin_lock_irqsave(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags);
 	if (list_empty(&m2m_ctx->cap_q_ctx.rdy_queue)) {
+		spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags);
 		spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock, flags);
 		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("No output buffers available\n");
 		return;
 	}
+	spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags);
 	spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock, flags);
 
 	if (m2m_dev->m2m_ops->job_ready
-- 
1.8.1

[PATCH 2/3] [media]: v4l2-mem2mem: drop rdy_queue on STREAMOFF

[John Sheu]

From: John Sheu <sheu@chromium.org>

When a v4l2-mem2mem context gets a STREAMOFF call on either its CAPTURE
or OUTPUT queues, we should:
* Drop the corresponding rdy_queue, since a subsequent STREAMON expects
  an empty queue.
* Deschedule the context, as it now has at least one empty queue and
  cannot run.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index c52a2c5..c5c9d24 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -408,10 +408,35 @@ EXPORT_SYMBOL_GPL(v4l2_m2m_streamon);
 int v4l2_m2m_streamoff(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 		       enum v4l2_buf_type type)
 {
-	struct vb2_queue *vq;
+	struct v4l2_m2m_dev *m2m_dev;
+	struct v4l2_m2m_queue_ctx *q_ctx;
+	unsigned long flags_job, flags;
+	int ret;
 
-	vq = v4l2_m2m_get_vq(m2m_ctx, type);
-	return vb2_streamoff(vq, type);
+	q_ctx = get_queue_ctx(m2m_ctx, type);
+	ret = vb2_streamoff(&q_ctx->q, type);
+	if (ret)
+		return ret;
+
+	m2m_dev = m2m_ctx->m2m_dev;
+	spin_lock_irqsave(&m2m_dev->job_spinlock, flags_job);
+	/* We should not be scheduled anymore, since we're dropping a queue. */
+	INIT_LIST_HEAD(&m2m_ctx->queue);
+	m2m_ctx->job_flags = 0;
+
+	spin_lock_irqsave(&q_ctx->rdy_spinlock, flags);
+	/* Drop queue, since streamoff returns device to the same state as after
+	 * calling reqbufs. */
+	INIT_LIST_HEAD(&q_ctx->rdy_queue);
+	spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
+
+	if (m2m_dev->curr_ctx == m2m_ctx) {
+		m2m_dev->curr_ctx = NULL;
+		wake_up(&m2m_ctx->finished);
+	}
+	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_streamoff);
 
-- 
1.8.1

Re: [PATCH 2/3] [media]: v4l2-mem2mem: drop rdy_queue on STREAMOFF

[Pawel Osciak]

On Wed, Feb 6, 2013 at 4:03 PM, John Sheu <sheu@google.com> wrote:
> From: John Sheu <sheu@chromium.org>
>
> When a v4l2-mem2mem context gets a STREAMOFF call on either its CAPTURE
> or OUTPUT queues, we should:
> * Drop the corresponding rdy_queue, since a subsequent STREAMON expects
>   an empty queue.
> * Deschedule the context, as it now has at least one empty queue and
>   cannot run.
>
> Signed-off-by: John Sheu <sheu@google.com>

Acked-by: Pawel Osciak <pawel@osciak.com>

> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 31 ++++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index c52a2c5..c5c9d24 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -408,10 +408,35 @@ EXPORT_SYMBOL_GPL(v4l2_m2m_streamon);
>  int v4l2_m2m_streamoff(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>                        enum v4l2_buf_type type)
>  {
> -       struct vb2_queue *vq;
> +       struct v4l2_m2m_dev *m2m_dev;
> +       struct v4l2_m2m_queue_ctx *q_ctx;
> +       unsigned long flags_job, flags;
> +       int ret;
>
> -       vq = v4l2_m2m_get_vq(m2m_ctx, type);
> -       return vb2_streamoff(vq, type);
> +       q_ctx = get_queue_ctx(m2m_ctx, type);
> +       ret = vb2_streamoff(&q_ctx->q, type);
> +       if (ret)
> +               return ret;
> +
> +       m2m_dev = m2m_ctx->m2m_dev;
> +       spin_lock_irqsave(&m2m_dev->job_spinlock, flags_job);
> +       /* We should not be scheduled anymore, since we're dropping a queue. */
> +       INIT_LIST_HEAD(&m2m_ctx->queue);
> +       m2m_ctx->job_flags = 0;
> +
> +       spin_lock_irqsave(&q_ctx->rdy_spinlock, flags);
> +       /* Drop queue, since streamoff returns device to the same state as after
> +        * calling reqbufs. */
> +       INIT_LIST_HEAD(&q_ctx->rdy_queue);
> +       spin_unlock_irqrestore(&q_ctx->rdy_spinlock, flags);
> +
> +       if (m2m_dev->curr_ctx == m2m_ctx) {
> +               m2m_dev->curr_ctx = NULL;
> +               wake_up(&m2m_ctx->finished);
> +       }
> +       spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
> +
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_streamoff);
>
> --
> 1.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
Best regards,
Pawel Osciak

[PATCH 3/3] dma-buf: restore args on failure of dma_buf_mmap

[John Sheu]

From: John Sheu <sheu@chromium.org>

Callers to dma_buf_mmap expect to fput() the vma struct's vm_file
themselves on failure.  Not restoring the struct's data on failure
causes a double-decrement of the vm_file's refcount.

Signed-off-by: John Sheu <sheu@google.com>
---
 drivers/base/dma-buf.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
index a3f79c4..01daf9c 100644
--- a/drivers/base/dma-buf.c
+++ b/drivers/base/dma-buf.c
@@ -446,6 +446,9 @@ EXPORT_SYMBOL_GPL(dma_buf_kunmap);
 int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 		 unsigned long pgoff)
 {
+	struct file *oldfile;
+	int ret;
+
 	if (WARN_ON(!dmabuf || !vma))
 		return -EINVAL;
 
@@ -459,14 +462,21 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
 		return -EINVAL;
 
 	/* readjust the vma */
-	if (vma->vm_file)
-		fput(vma->vm_file);
-
+	oldfile = vma->vm_file;
 	vma->vm_file = get_file(dmabuf->file);
 
 	vma->vm_pgoff = pgoff;
 
-	return dmabuf->ops->mmap(dmabuf, vma);
+	ret = dmabuf->ops->mmap(dmabuf, vma);
+	if (ret) {
+		/* restore old parameters on failure */
+		vma->vm_file = oldfile;
+		fput(dmabuf->file);
+	} else {
+		if (oldfile)
+			fput(oldfile);
+	}
+	return ret;
 }
 EXPORT_SYMBOL_GPL(dma_buf_mmap);
 
-- 
1.8.1

Re: [PATCH 3/3] dma-buf: restore args on failure of dma_buf_mmap

[Sumit Semwal]

Hi John,

On Thursday 07 February 2013 05:33 AM, John Sheu wrote:
> From: John Sheu <sheu@chromium.org>
>
> Callers to dma_buf_mmap expect to fput() the vma struct's vm_file
> themselves on failure.  Not restoring the struct's data on failure
> causes a double-decrement of the vm_file's refcount.
Thanks for your patch; could you please re-send it to the correct, 
relevant lists and me (as the maintainer of dma-buf) rather than just to 
linux-media ml?

I just chanced to see this patch, otherwise it could easily have slipped 
past me (and other interested parties).

You could run scripts/get_maintainer.pl on your patch to find out the 
right lists / email IDs to CC.

Thanks and best regards,
~Sumit.
>
> Signed-off-by: John Sheu <sheu@google.com>
> ---
>   drivers/base/dma-buf.c | 18 ++++++++++++++----
>   1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> index a3f79c4..01daf9c 100644
> --- a/drivers/base/dma-buf.c
> +++ b/drivers/base/dma-buf.c
> @@ -446,6 +446,9 @@ EXPORT_SYMBOL_GPL(dma_buf_kunmap);
>   int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>   		 unsigned long pgoff)
>   {
> +	struct file *oldfile;
> +	int ret;
> +
>   	if (WARN_ON(!dmabuf || !vma))
>   		return -EINVAL;
>
> @@ -459,14 +462,21 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>   		return -EINVAL;
>
>   	/* readjust the vma */
> -	if (vma->vm_file)
> -		fput(vma->vm_file);
> -
> +	oldfile = vma->vm_file;
>   	vma->vm_file = get_file(dmabuf->file);
>
>   	vma->vm_pgoff = pgoff;
>
> -	return dmabuf->ops->mmap(dmabuf, vma);
> +	ret = dmabuf->ops->mmap(dmabuf, vma);
> +	if (ret) {
> +		/* restore old parameters on failure */
> +		vma->vm_file = oldfile;
> +		fput(dmabuf->file);
> +	} else {
> +		if (oldfile)
> +			fput(oldfile);
> +	}
> +	return ret;
>   }
>   EXPORT_SYMBOL_GPL(dma_buf_mmap);
>
>

Re: [PATCH 3/3] dma-buf: restore args on failure of dma_buf_mmap

[Mauro Carvalho Chehab]

Em Thu, 07 Feb 2013 15:54:52 +0530
Sumit Semwal <sumit.semwal@linaro.org> escreveu:

> Hi John,
> 
> On Thursday 07 February 2013 05:33 AM, John Sheu wrote:
> > From: John Sheu <sheu@chromium.org>
> >
> > Callers to dma_buf_mmap expect to fput() the vma struct's vm_file
> > themselves on failure.  Not restoring the struct's data on failure
> > causes a double-decrement of the vm_file's refcount.
> Thanks for your patch; could you please re-send it to the correct, 
> relevant lists and me (as the maintainer of dma-buf) rather than just to 
> linux-media ml?

Yes, it doesn't make sense to apply this one via the media tree ;)

I'm applying patches 1 and 2, as they should go through the media tree.

Thanks!
Mauro
> 
> I just chanced to see this patch, otherwise it could easily have slipped 
> past me (and other interested parties).
> 
> You could run scripts/get_maintainer.pl on your patch to find out the 
> right lists / email IDs to CC.
> 
> Thanks and best regards,
> ~Sumit.
> >
> > Signed-off-by: John Sheu <sheu@google.com>
> > ---
> >   drivers/base/dma-buf.c | 18 ++++++++++++++----
> >   1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
> > index a3f79c4..01daf9c 100644
> > --- a/drivers/base/dma-buf.c
> > +++ b/drivers/base/dma-buf.c
> > @@ -446,6 +446,9 @@ EXPORT_SYMBOL_GPL(dma_buf_kunmap);
> >   int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
> >   		 unsigned long pgoff)
> >   {
> > +	struct file *oldfile;
> > +	int ret;
> > +
> >   	if (WARN_ON(!dmabuf || !vma))
> >   		return -EINVAL;
> >
> > @@ -459,14 +462,21 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
> >   		return -EINVAL;
> >
> >   	/* readjust the vma */
> > -	if (vma->vm_file)
> > -		fput(vma->vm_file);
> > -
> > +	oldfile = vma->vm_file;
> >   	vma->vm_file = get_file(dmabuf->file);
> >
> >   	vma->vm_pgoff = pgoff;
> >
> > -	return dmabuf->ops->mmap(dmabuf, vma);
> > +	ret = dmabuf->ops->mmap(dmabuf, vma);
> > +	if (ret) {
> > +		/* restore old parameters on failure */
> > +		vma->vm_file = oldfile;
> > +		fput(dmabuf->file);
> > +	} else {
> > +		if (oldfile)
> > +			fput(oldfile);
> > +	}
> > +	return ret;
> >   }
> >   EXPORT_SYMBOL_GPL(dma_buf_mmap);
> >
> >
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 

Cheers,
Mauro

Re: [PATCH 3/3] dma-buf: restore args on failure of dma_buf_mmap

[Sumit Semwal]

On 19 March 2013 05:19, Mauro Carvalho Chehab <mchehab@redhat.com> wrote:
> Em Thu, 07 Feb 2013 15:54:52 +0530
> Sumit Semwal <sumit.semwal@linaro.org> escreveu:
>
>> Hi John,
>>
>> On Thursday 07 February 2013 05:33 AM, John Sheu wrote:
>> > From: John Sheu <sheu@chromium.org>
>> >
>> > Callers to dma_buf_mmap expect to fput() the vma struct's vm_file
>> > themselves on failure.  Not restoring the struct's data on failure
>> > causes a double-decrement of the vm_file's refcount.
>> Thanks for your patch; could you please re-send it to the correct,
>> relevant lists and me (as the maintainer of dma-buf) rather than just to
>> linux-media ml?
>
> Yes, it doesn't make sense to apply this one via the media tree ;)
>
> I'm applying patches 1 and 2, as they should go through the media tree.
Sure, this patch is already in mainline as of 3.9-rc1 :)
>
> Thanks!
> Mauro
>>
>> I just chanced to see this patch, otherwise it could easily have slipped
>> past me (and other interested parties).
>>
>> You could run scripts/get_maintainer.pl on your patch to find out the
>> right lists / email IDs to CC.
>>
>> Thanks and best regards,
>> ~Sumit.
>> >
>> > Signed-off-by: John Sheu <sheu@google.com>
>> > ---
>> >   drivers/base/dma-buf.c | 18 ++++++++++++++----
>> >   1 file changed, 14 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/base/dma-buf.c b/drivers/base/dma-buf.c
>> > index a3f79c4..01daf9c 100644
>> > --- a/drivers/base/dma-buf.c
>> > +++ b/drivers/base/dma-buf.c
>> > @@ -446,6 +446,9 @@ EXPORT_SYMBOL_GPL(dma_buf_kunmap);
>> >   int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>> >              unsigned long pgoff)
>> >   {
>> > +   struct file *oldfile;
>> > +   int ret;
>> > +
>> >     if (WARN_ON(!dmabuf || !vma))
>> >             return -EINVAL;
>> >
>> > @@ -459,14 +462,21 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>> >             return -EINVAL;
>> >
>> >     /* readjust the vma */
>> > -   if (vma->vm_file)
>> > -           fput(vma->vm_file);
>> > -
>> > +   oldfile = vma->vm_file;
>> >     vma->vm_file = get_file(dmabuf->file);
>> >
>> >     vma->vm_pgoff = pgoff;
>> >
>> > -   return dmabuf->ops->mmap(dmabuf, vma);
>> > +   ret = dmabuf->ops->mmap(dmabuf, vma);
>> > +   if (ret) {
>> > +           /* restore old parameters on failure */
>> > +           vma->vm_file = oldfile;
>> > +           fput(dmabuf->file);
>> > +   } else {
>> > +           if (oldfile)
>> > +                   fput(oldfile);
>> > +   }
>> > +   return ret;
>> >   }
>> >   EXPORT_SYMBOL_GPL(dma_buf_mmap);
>> >
>> >
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> --
>
> Cheers,
> Mauro



--
Thanks and regards,

Sumit Semwal

Linaro Kernel Engineer - Graphics working group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] [media] v4l2-mem2mem: use CAPTURE queue lock

[Pawel Osciak]

On Wed, Feb 6, 2013 at 4:03 PM, John Sheu <sheu@google.com> wrote:
> From: John Sheu <sheu@chromium.org>
>
> In v4l2_m2m_try_schedule(), use the CAPTURE queue lock when accessing
> the CAPTURE queue, instead of relying on just holding the OUTPUT queue
> lock.
>
> Signed-off-by: John Sheu <sheu@google.com>

Acked-by: Pawel Osciak <pawel@osciak.com>

> ---
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 438ea45..c52a2c5 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -230,12 +230,15 @@ static void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
>                 dprintk("No input buffers available\n");
>                 return;
>         }
> +       spin_lock_irqsave(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags);
>         if (list_empty(&m2m_ctx->cap_q_ctx.rdy_queue)) {
> +               spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags);
>                 spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock, flags);
>                 spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
>                 dprintk("No output buffers available\n");
>                 return;
>         }
> +       spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags);
>         spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock, flags);
>
>         if (m2m_dev->m2m_ops->job_ready
> --
> 1.8.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best regards,
Pawel Osciak