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 <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Kevin Hilman <khilman@xxxxxxxxxxxx>
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 <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> 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(&common->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(&common->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(&common->irqlock, flags);
> list_for_each_entry_safe(buf, tmp, &common->dma_queue, list) {
> list_del(&buf->list);
> vb2_buffer_done(&buf->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(&common->irqlock, flags);
> if (common->cur_frm == common->next_frm) {
> vb2_buffer_done(&common->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(&common->irqlock, flags);
> while (!list_empty(&common->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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
