* [PATCH] videobuf2/vb2_buffer_done: Changing the position of spinlock to protect only the required code
[not found] <CGME20180727082146epcas5p10374c04f0767dbbe409c8171c49d7c9a@epcas5p1.samsung.com>
@ 2018-07-27 8:21 ` Satendra Singh Thakur
2018-07-31 7:46 ` Sakari Ailus
0 siblings, 1 reply; 2+ messages in thread
From: Satendra Singh Thakur @ 2018-07-27 8:21 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Hans Verkuil,
Satendra Singh Thakur, Al Viro, linux-media, linux-kernel
Cc: vineet.j, hemanshu.s, sst2005
1.Currently, in the func vb2_buffer_done, spinlock protects
following code
vb->state = VB2_BUF_STATE_QUEUED;
list_add_tail(&vb->done_entry, &q->done_list);
spin_unlock_irqrestore(&q->done_lock, flags);
vb->state = state;
atomic_dec(&q->owned_by_drv_count);
2.The spinlock is mainly needed to protect list related ops and
vb->state = STATE_ERROR or STATE_DONE as in other funcs
vb2_discard_done
__vb2_get_done_vb
vb2_core_poll.
3. Therefore, spinlock is mainly needed for
list_add, list_del, list_first_entry ops
and state = STATE_DONE and STATE_ERROR to protect
done_list queue.
3. Hence, state = STATE_QUEUED doesn't need spinlock protection.
4. Also atomic_dec dones't require the same as its already atomic.
Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
---
drivers/media/common/videobuf2/videobuf2-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index f32ec73..968b403 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -923,17 +923,17 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
call_void_memop(vb, finish, vb->planes[plane].mem_priv);
}
- spin_lock_irqsave(&q->done_lock, flags);
if (state == VB2_BUF_STATE_QUEUED ||
state == VB2_BUF_STATE_REQUEUEING) {
vb->state = VB2_BUF_STATE_QUEUED;
} else {
/* Add the buffer to the done buffers list */
+ spin_lock_irqsave(&q->done_lock, flags);
list_add_tail(&vb->done_entry, &q->done_list);
vb->state = state;
+ spin_unlock_irqrestore(&q->done_lock, flags);
}
atomic_dec(&q->owned_by_drv_count);
- spin_unlock_irqrestore(&q->done_lock, flags);
trace_vb2_buf_done(q, vb);
--
2.7.4
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] videobuf2/vb2_buffer_done: Changing the position of spinlock to protect only the required code
2018-07-27 8:21 ` [PATCH] videobuf2/vb2_buffer_done: Changing the position of spinlock to protect only the required code Satendra Singh Thakur
@ 2018-07-31 7:46 ` Sakari Ailus
0 siblings, 0 replies; 2+ messages in thread
From: Sakari Ailus @ 2018-07-31 7:46 UTC (permalink / raw)
To: Satendra Singh Thakur
Cc: Mauro Carvalho Chehab, Hans Verkuil, Al Viro, linux-media,
linux-kernel, vineet.j, hemanshu.s, sst2005
Hi Satendra,
Thanks for the patch.
On Fri, Jul 27, 2018 at 01:51:36PM +0530, Satendra Singh Thakur wrote:
> 1.Currently, in the func vb2_buffer_done, spinlock protects
> following code
> vb->state = VB2_BUF_STATE_QUEUED;
> list_add_tail(&vb->done_entry, &q->done_list);
> spin_unlock_irqrestore(&q->done_lock, flags);
> vb->state = state;
> atomic_dec(&q->owned_by_drv_count);
> 2.The spinlock is mainly needed to protect list related ops and
> vb->state = STATE_ERROR or STATE_DONE as in other funcs
> vb2_discard_done
> __vb2_get_done_vb
> vb2_core_poll.
> 3. Therefore, spinlock is mainly needed for
> list_add, list_del, list_first_entry ops
> and state = STATE_DONE and STATE_ERROR to protect
> done_list queue.
> 3. Hence, state = STATE_QUEUED doesn't need spinlock protection.
> 4. Also atomic_dec dones't require the same as its already atomic.
>
> Signed-off-by: Satendra Singh Thakur <satendra.t@samsung.com>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index f32ec73..968b403 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -923,17 +923,17 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> }
>
> - spin_lock_irqsave(&q->done_lock, flags);
> if (state == VB2_BUF_STATE_QUEUED ||
> state == VB2_BUF_STATE_REQUEUEING) {
> vb->state = VB2_BUF_STATE_QUEUED;
> } else {
You could move flags here as well. I wonder what others think.
> /* Add the buffer to the done buffers list */
> + spin_lock_irqsave(&q->done_lock, flags);
> list_add_tail(&vb->done_entry, &q->done_list);
> vb->state = state;
The state could be assigned without holding the lock here.
> + spin_unlock_irqrestore(&q->done_lock, flags);
> }
> atomic_dec(&q->owned_by_drv_count);
> - spin_unlock_irqrestore(&q->done_lock, flags);
>
> trace_vb2_buf_done(q, vb);
>
--
Kind regards,
Sakari Ailus
sakari.ailus@linux.intel.com
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-07-31 7:46 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20180727082146epcas5p10374c04f0767dbbe409c8171c49d7c9a@epcas5p1.samsung.com>
2018-07-27 8:21 ` [PATCH] videobuf2/vb2_buffer_done: Changing the position of spinlock to protect only the required code Satendra Singh Thakur
2018-07-31 7:46 ` Sakari Ailus
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.