* [PATCH v2 0/2] media: v4l2-mem2mem: fix poll() bug @ 2020-08-27 12:49 Alexandre Courbot 2020-08-27 12:49 ` [PATCH v2 1/2] media: v4l2-mem2mem: always consider OUTPUT queue during poll Alexandre Courbot 2020-08-27 12:49 ` [PATCH v2 2/2] media: v4l2-mem2mem: simplify poll logic Alexandre Courbot 0 siblings, 2 replies; 5+ messages in thread From: Alexandre Courbot @ 2020-08-27 12:49 UTC (permalink / raw) To: Mauro Carvalho Chehab, Hans Verkuil, Nicolas Dufresne, Ezequiel Garcia Cc: linux-media, linux-kernel, Alexandre Courbot Thanks to the feedback on the first version, this one removes even more code. I have simplified patch 1/1 to just address the issue without moving code, since this is easier to understand and needs to be done for 2/2 anyway. As a consequence the code has changed a bit and I did not carry the Reviewed-by tags. Changes from v1: * Simplify patch 1/1, * Remove unneeded checks for DONE or ERROR status, * Rephrased comment about exiting early. Alexandre Courbot (2): media: v4l2-mem2mem: always consider OUTPUT queue during poll media: v4l2-mem2mem: simplify poll logic drivers/media/v4l2-core/v4l2-mem2mem.c | 28 +++++--------------------- 1 file changed, 5 insertions(+), 23 deletions(-) -- 2.28.0 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] media: v4l2-mem2mem: always consider OUTPUT queue during poll 2020-08-27 12:49 [PATCH v2 0/2] media: v4l2-mem2mem: fix poll() bug Alexandre Courbot @ 2020-08-27 12:49 ` Alexandre Courbot 2020-08-28 15:17 ` Ezequiel Garcia 2020-08-27 12:49 ` [PATCH v2 2/2] media: v4l2-mem2mem: simplify poll logic Alexandre Courbot 1 sibling, 1 reply; 5+ messages in thread From: Alexandre Courbot @ 2020-08-27 12:49 UTC (permalink / raw) To: Mauro Carvalho Chehab, Hans Verkuil, Nicolas Dufresne, Ezequiel Garcia Cc: linux-media, linux-kernel, Alexandre Courbot If poll() is called on a m2m device with the EPOLLOUT event after the last buffer of the CAPTURE queue is dequeued, any buffer available on OUTPUT queue will never be signaled because v4l2_m2m_poll_for_data() starts by checking whether dst_q->last_buffer_dequeued is set and returns EPOLLIN in this case, without looking at the state of the OUTPUT queue. Fix this by not early returning so we keep checking the state of the OUTPUT queue afterwards. Signed-off-by: Alexandre Courbot <gnurou@gmail.com> --- drivers/media/v4l2-core/v4l2-mem2mem.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c index 95a8f2dc5341d..fe90c3c0e4128 100644 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c @@ -868,10 +868,8 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, * If the last buffer was dequeued from the capture queue, * return immediately. DQBUF will return -EPIPE. */ - if (dst_q->last_buffer_dequeued) { - spin_unlock_irqrestore(&dst_q->done_lock, flags); - return EPOLLIN | EPOLLRDNORM; - } + if (dst_q->last_buffer_dequeued) + rc |= EPOLLIN | EPOLLRDNORM; } spin_unlock_irqrestore(&dst_q->done_lock, flags); -- 2.28.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 1/2] media: v4l2-mem2mem: always consider OUTPUT queue during poll 2020-08-27 12:49 ` [PATCH v2 1/2] media: v4l2-mem2mem: always consider OUTPUT queue during poll Alexandre Courbot @ 2020-08-28 15:17 ` Ezequiel Garcia 0 siblings, 0 replies; 5+ messages in thread From: Ezequiel Garcia @ 2020-08-28 15:17 UTC (permalink / raw) To: Alexandre Courbot Cc: Mauro Carvalho Chehab, Hans Verkuil, Nicolas Dufresne, linux-media, Linux Kernel Mailing List On Thu, 27 Aug 2020 at 09:50, Alexandre Courbot <gnurou@gmail.com> wrote: > > If poll() is called on a m2m device with the EPOLLOUT event after the > last buffer of the CAPTURE queue is dequeued, any buffer available on > OUTPUT queue will never be signaled because v4l2_m2m_poll_for_data() > starts by checking whether dst_q->last_buffer_dequeued is set and > returns EPOLLIN in this case, without looking at the state of the OUTPUT > queue. > > Fix this by not early returning so we keep checking the state of the > OUTPUT queue afterwards. > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/media/v4l2-core/v4l2-mem2mem.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c > index 95a8f2dc5341d..fe90c3c0e4128 100644 > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c > @@ -868,10 +868,8 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, > * If the last buffer was dequeued from the capture queue, > * return immediately. DQBUF will return -EPIPE. > */ > - if (dst_q->last_buffer_dequeued) { > - spin_unlock_irqrestore(&dst_q->done_lock, flags); > - return EPOLLIN | EPOLLRDNORM; > - } > + if (dst_q->last_buffer_dequeued) > + rc |= EPOLLIN | EPOLLRDNORM; > } > spin_unlock_irqrestore(&dst_q->done_lock, flags); > > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] media: v4l2-mem2mem: simplify poll logic 2020-08-27 12:49 [PATCH v2 0/2] media: v4l2-mem2mem: fix poll() bug Alexandre Courbot 2020-08-27 12:49 ` [PATCH v2 1/2] media: v4l2-mem2mem: always consider OUTPUT queue during poll Alexandre Courbot @ 2020-08-27 12:49 ` Alexandre Courbot 2020-08-28 15:18 ` Ezequiel Garcia 1 sibling, 1 reply; 5+ messages in thread From: Alexandre Courbot @ 2020-08-27 12:49 UTC (permalink / raw) To: Mauro Carvalho Chehab, Hans Verkuil, Nicolas Dufresne, Ezequiel Garcia Cc: linux-media, linux-kernel, Alexandre Courbot Factorize redundant checks into a single code block, remove unneeded checks (a buffer in done_list is necessarily in the DONE or ERROR state), and we end up with a much simpler version of this function. Signed-off-by: Alexandre Courbot <gnurou@gmail.com> --- drivers/media/v4l2-core/v4l2-mem2mem.c | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c index fe90c3c0e4128..af8138c5d7ced 100644 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c @@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, struct poll_table_struct *wait) { struct vb2_queue *src_q, *dst_q; - struct vb2_buffer *src_vb = NULL, *dst_vb = NULL; __poll_t rc = 0; unsigned long flags; @@ -862,32 +861,17 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, list_empty(&dst_q->queued_list))) return EPOLLERR; - spin_lock_irqsave(&dst_q->done_lock, flags); - if (list_empty(&dst_q->done_list)) { - /* - * If the last buffer was dequeued from the capture queue, - * return immediately. DQBUF will return -EPIPE. - */ - if (dst_q->last_buffer_dequeued) - rc |= EPOLLIN | EPOLLRDNORM; - } - spin_unlock_irqrestore(&dst_q->done_lock, flags); - spin_lock_irqsave(&src_q->done_lock, flags); if (!list_empty(&src_q->done_list)) - src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, - done_entry); - if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE - || src_vb->state == VB2_BUF_STATE_ERROR)) rc |= EPOLLOUT | EPOLLWRNORM; spin_unlock_irqrestore(&src_q->done_lock, flags); spin_lock_irqsave(&dst_q->done_lock, flags); - if (!list_empty(&dst_q->done_list)) - dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer, - done_entry); - if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE - || dst_vb->state == VB2_BUF_STATE_ERROR)) + /* + * If the last buffer was dequeued from the capture queue, signal + * userspace. DQBUF(CAPTURE) will return -EPIPE. + */ + if (!list_empty(&dst_q->done_list) || dst_q->last_buffer_dequeued) rc |= EPOLLIN | EPOLLRDNORM; spin_unlock_irqrestore(&dst_q->done_lock, flags); -- 2.28.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] media: v4l2-mem2mem: simplify poll logic 2020-08-27 12:49 ` [PATCH v2 2/2] media: v4l2-mem2mem: simplify poll logic Alexandre Courbot @ 2020-08-28 15:18 ` Ezequiel Garcia 0 siblings, 0 replies; 5+ messages in thread From: Ezequiel Garcia @ 2020-08-28 15:18 UTC (permalink / raw) To: Alexandre Courbot Cc: Mauro Carvalho Chehab, Hans Verkuil, Nicolas Dufresne, linux-media, Linux Kernel Mailing List On Thu, 27 Aug 2020 at 09:50, Alexandre Courbot <gnurou@gmail.com> wrote: > > Factorize redundant checks into a single code block, remove unneeded > checks (a buffer in done_list is necessarily in the DONE or ERROR > state), and we end up with a much simpler version of this function. > > Signed-off-by: Alexandre Courbot <gnurou@gmail.com> This is really good, thanks! Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > --- > drivers/media/v4l2-core/v4l2-mem2mem.c | 26 +++++--------------------- > 1 file changed, 5 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c > index fe90c3c0e4128..af8138c5d7ced 100644 > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c > @@ -841,7 +841,6 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, > struct poll_table_struct *wait) > { > struct vb2_queue *src_q, *dst_q; > - struct vb2_buffer *src_vb = NULL, *dst_vb = NULL; > __poll_t rc = 0; > unsigned long flags; > > @@ -862,32 +861,17 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file, > list_empty(&dst_q->queued_list))) > return EPOLLERR; > > - spin_lock_irqsave(&dst_q->done_lock, flags); > - if (list_empty(&dst_q->done_list)) { > - /* > - * If the last buffer was dequeued from the capture queue, > - * return immediately. DQBUF will return -EPIPE. > - */ > - if (dst_q->last_buffer_dequeued) > - rc |= EPOLLIN | EPOLLRDNORM; > - } > - spin_unlock_irqrestore(&dst_q->done_lock, flags); > - > spin_lock_irqsave(&src_q->done_lock, flags); > if (!list_empty(&src_q->done_list)) > - src_vb = list_first_entry(&src_q->done_list, struct vb2_buffer, > - done_entry); > - if (src_vb && (src_vb->state == VB2_BUF_STATE_DONE > - || src_vb->state == VB2_BUF_STATE_ERROR)) > rc |= EPOLLOUT | EPOLLWRNORM; > spin_unlock_irqrestore(&src_q->done_lock, flags); > > spin_lock_irqsave(&dst_q->done_lock, flags); > - if (!list_empty(&dst_q->done_list)) > - dst_vb = list_first_entry(&dst_q->done_list, struct vb2_buffer, > - done_entry); > - if (dst_vb && (dst_vb->state == VB2_BUF_STATE_DONE > - || dst_vb->state == VB2_BUF_STATE_ERROR)) > + /* > + * If the last buffer was dequeued from the capture queue, signal > + * userspace. DQBUF(CAPTURE) will return -EPIPE. > + */ > + if (!list_empty(&dst_q->done_list) || dst_q->last_buffer_dequeued) > rc |= EPOLLIN | EPOLLRDNORM; > spin_unlock_irqrestore(&dst_q->done_lock, flags); > > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-08-28 15:19 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-08-27 12:49 [PATCH v2 0/2] media: v4l2-mem2mem: fix poll() bug Alexandre Courbot 2020-08-27 12:49 ` [PATCH v2 1/2] media: v4l2-mem2mem: always consider OUTPUT queue during poll Alexandre Courbot 2020-08-28 15:17 ` Ezequiel Garcia 2020-08-27 12:49 ` [PATCH v2 2/2] media: v4l2-mem2mem: simplify poll logic Alexandre Courbot 2020-08-28 15:18 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox