Re: [PATCH v5 3/5] [media] davinci: vpif_capture: fix start/stop streaming locking

[Kevin Hilman]

Laurent Pinchart  writes:

> Hi Kevin,
>
> Thank you for the patch.
>
> On Tuesday 06 Dec 2016 21:08:24 Kevin Hilman wrote:
>> Video capture subdevs may be over I2C and may sleep during xfer, so we
>> cannot do IRQ-disabled locking when calling the subdev.
>> 
>> The IRQ-disabled locking is meant to protect the DMA queue list
>> throughout the rest of the driver, so update the locking in
>> [start|stop]_streaming to protect just this list.
>> 
>> Suggested-by: Laurent Pinchart 
>> Signed-off-by: Kevin Hilman 
>
> I would also add a comment to document the irqlock field as protecting the 
> dma_queue list only. Something like
>
> - /* Used in video-buf */
> + /* Protects the dma_queue field */
>   spinlock_t irqlock;
>
> With that,
>
> Reviewed-by: Laurent Pinchart 

OK, will update the comment.  Thanks for the review,

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

Re: [PATCH v5 3/5] [media] davinci: vpif_capture: fix start/stop streaming locking

[Laurent Pinchart]

Hi Kevin,

Thank you for the patch.

On Tuesday 06 Dec 2016 21:08:24 Kevin Hilman wrote:
> Video capture subdevs may be over I2C and may sleep during xfer, so we
> cannot do IRQ-disabled locking when calling the subdev.
> 
> The IRQ-disabled locking is meant to protect the DMA queue list
> throughout the rest of the driver, so update the locking in
> [start|stop]_streaming to protect just this list.
> 
> Suggested-by: Laurent Pinchart 
> Signed-off-by: Kevin Hilman 

I would also add a comment to document the irqlock field as protecting the 
dma_queue list only. Something like

-   /* Used in video-buf */
+   /* Protects the dma_queue field */
spinlock_t irqlock;

With that,

Reviewed-by: Laurent Pinchart 

> ---
>  drivers/media/platform/davinci/vpif_capture.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/davinci/vpif_capture.c
> b/drivers/media/platform/davinci/vpif_capture.c index
> c24049acd40a..f72a719efb3d 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -179,8 +179,6 @@ static int vpif_start_streaming(struct vb2_queue *vq,
> unsigned int count) unsigned long addr, flags;
>   int ret;
> 
> - spin_lock_irqsave(>irqlock, flags);
> -
>   /* Initialize field_id */
>   ch->field_id = 0;
> 
> @@ -211,6 +209,7 @@ static int vpif_start_streaming(struct vb2_queue *vq,
> unsigned int count) vpif_config_addr(ch, ret);
> 
>   /* Get the next frame from the buffer queue */
> + spin_lock_irqsave(>irqlock, flags);
>   common->cur_frm = common->next_frm = list_entry(common-
>dma_queue.next,
>   struct vpif_cap_buffer, list);
>   /* Remove buffer from the buffer queue */
> @@ -244,6 +243,7 @@ static int vpif_start_streaming(struct vb2_queue *vq,
> unsigned int count) return 0;
> 
>  err:
> + spin_lock_irqsave(>irqlock, flags);
>   list_for_each_entry_safe(buf, tmp, >dma_queue, list) {
>   list_del(>list);
>   vb2_buffer_done(>vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> @@ -287,7 +287,6 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
>   vpif_dbg(1, debug, "stream off failed in subdev\n");
> 
>   /* release all active buffers */
> - spin_lock_irqsave(>irqlock, flags);
>   if (common->cur_frm == common->next_frm) {
>   vb2_buffer_done(>cur_frm->vb.vb2_buf,
>   VB2_BUF_STATE_ERROR);
> @@ -300,6 +299,7 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
>   VB2_BUF_STATE_ERROR);
>   }
> 
> + spin_lock_irqsave(>irqlock, flags);
>   while (!list_empty(>dma_queue)) {
>   common->next_frm = list_entry(common->dma_queue.next,
>   struct vpif_cap_buffer, list);

-- 
Regards,

Laurent Pinchart

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

[PATCH v5 3/5] [media] davinci: vpif_capture: fix start/stop streaming locking

[Kevin Hilman]

Video capture subdevs may be over I2C and may sleep during xfer, so we
cannot do IRQ-disabled locking when calling the subdev.

The IRQ-disabled locking is meant to protect the DMA queue list
throughout the rest of the driver, so update the locking in
[start|stop]_streaming to protect just this list.

Suggested-by: Laurent Pinchart 
Signed-off-by: Kevin Hilman 
---
 drivers/media/platform/davinci/vpif_capture.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index c24049acd40a..f72a719efb3d 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -179,8 +179,6 @@ static int vpif_start_streaming(struct vb2_queue *vq, 
unsigned int count)
unsigned long addr, flags;
int ret;
 
-   spin_lock_irqsave(>irqlock, flags);
-
/* Initialize field_id */
ch->field_id = 0;
 
@@ -211,6 +209,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, 
unsigned int count)
vpif_config_addr(ch, ret);
 
/* Get the next frame from the buffer queue */
+   spin_lock_irqsave(>irqlock, flags);
common->cur_frm = common->next_frm = list_entry(common->dma_queue.next,
struct vpif_cap_buffer, list);
/* Remove buffer from the buffer queue */
@@ -244,6 +243,7 @@ static int vpif_start_streaming(struct vb2_queue *vq, 
unsigned int count)
return 0;
 
 err:
+   spin_lock_irqsave(>irqlock, flags);
list_for_each_entry_safe(buf, tmp, >dma_queue, list) {
list_del(>list);
vb2_buffer_done(>vb.vb2_buf, VB2_BUF_STATE_QUEUED);
@@ -287,7 +287,6 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
vpif_dbg(1, debug, "stream off failed in subdev\n");
 
/* release all active buffers */
-   spin_lock_irqsave(>irqlock, flags);
if (common->cur_frm == common->next_frm) {
vb2_buffer_done(>cur_frm->vb.vb2_buf,
VB2_BUF_STATE_ERROR);
@@ -300,6 +299,7 @@ static void vpif_stop_streaming(struct vb2_queue *vq)
VB2_BUF_STATE_ERROR);
}
 
+   spin_lock_irqsave(>irqlock, flags);
while (!list_empty(>dma_queue)) {
common->next_frm = list_entry(common->dma_queue.next,
struct vpif_cap_buffer, list);
-- 
2.9.3

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