v4l2-mem2mem: use CAPTURE queue lock

PATCH In review John Sheu 4 participants 8 messages Feb 7, 2013 – Mar 25, 2013

Subsystems (2)

Media (V4L/DVB) Driver core

lore

Share & tools

lore

John Sheu

author

2013-02-07 00:03 UTC #0 lore raw

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

From: John Sheu <[email protected]>

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 <[email protected]>

---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/media/v4l2-core/v4l2-mem2mem.c +3 -0

index 438ea45..c52a2c5 100644
@@ -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

Signature · 1 line

1.8.1

John Sheu

author

2013-02-07 00:09 UTC #1 lore raw

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

From: John Sheu <[email protected]>

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 <[email protected]>

---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

drivers/media/v4l2-core/v4l2-mem2mem.c +28 -3

index c52a2c5..c5c9d24 100644
@@ -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);

Signature · 1 line

1.8.1

Pawel Osciak

Acked-by on v1 2/3

2013-02-28 15:46 UTC #2 lore raw

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

Quoted 10 lines · John Sheu

From: John Sheu <[email protected]>
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 <[email protected]>

Acked-by Pawel Osciak <[email protected]>

Quoted 50 lines

---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

drivers/media/v4l2-core/v4l2-mem2mem.c +28 -4

index c52a2c5..c5c9d24 100644
@@ -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

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Best regards,
Pawel Osciak

John Sheu

author

2013-02-07 00:03 UTC #3 lore raw

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

In reply to John Sheu

From: John Sheu <[email protected]>

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 <[email protected]>

---
 drivers/base/dma-buf.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

drivers/base/dma-buf.c +14 -4

index a3f79c4..01daf9c 100644
@@ -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);

Signature · 1 line

1.8.1

Sumit Semwal

2013-02-07 10:24 UTC #4 lore raw

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

Hi John,

Quoted 5 lines · John Sheu

From: John Sheu <[email protected]>
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.

Quoted 47 lines

Signed-off-by John Sheu <[email protected]>

---
  drivers/base/dma-buf.c | 18 ++++++++++++++----
  1 file changed, 14 insertions(+), 4 deletions(-)

drivers/base/dma-buf.c +14 -4

index a3f79c4..01daf9c 100644
@@ -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);

Mauro Carvalho Chehab

2013-03-18 23:49 UTC #5 lore raw

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

Em Thu, 07 Feb 2013 15:54:52 +0530
Sumit Semwal <[email protected]> escreveu:

Quoted 11 lines

Hi John,
Quoted 5 lines · John Sheu
From: John Sheu <[email protected]>
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

Quoted 61 lines

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.
Quoted 37 lines
Signed-off-by John Sheu <[email protected]>
---
  drivers/base/dma-buf.c | 18 ++++++++++++++----
  1 file changed, 14 insertions(+), 4 deletions(-)
drivers/base/dma-buf.c +7 -4
index a3f79c4..01daf9c 100644
@@ -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 */
drivers/base/dma-buf.c int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, · line 482
Quoted 10 lines
drivers/base/dma-buf.c +7 -0
+		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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Cheers,
Mauro

Sumit Semwal

2013-03-25 11:25 UTC #6 lore raw

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

Quoted 11 lines · Mauro Carvalho Chehab

Em Thu, 07 Feb 2013 15:54:52 +0530
Sumit Semwal <[email protected]> escreveu:
Quoted 8 lines
Hi John,
Quoted 5 lines · John Sheu
From: John Sheu <[email protected]>
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.

Quoted 7 lines

Quoted 3 lines
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 :)

Quoted 66 lines

Thanks!
Mauro
Quoted 53 lines
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.
Quoted 44 lines
Signed-off-by John Sheu <[email protected]>
---
  drivers/base/dma-buf.c | 18 ++++++++++++++----
  1 file changed, 14 insertions(+), 4 deletions(-)
drivers/base/dma-buf.c +14 -4
index a3f79c4..01daf9c 100644
@@ -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;
  }
Quoted 8 lines
drivers/base/dma-buf.c int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma, · line 482
Quoted 3 lines
drivers/base/dma-buf.c +0 -0
  EXPORT_SYMBOL_GPL(dma_buf_mmap);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines

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

Pawel Osciak

Acked-by on v1 1/3

2013-02-28 15:45 UTC #7 lore raw

In reply to John Sheu

Quoted 7 lines · John Sheu

From: John Sheu <[email protected]>
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 <[email protected]>

Acked-by Pawel Osciak <[email protected]>

Quoted 31 lines

---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/media/v4l2-core/v4l2-mem2mem.c +3 -1

index 438ea45..c52a2c5 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines

Best regards,
Pawel Osciak