On 26/08/2020 16:32, Ezequiel Garcia wrote:
> On Wed, 26 Aug 2020 at 08:19, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
>>
>> Hi Ezequiel, thanks for the review!
>>
>> On Wed, Aug 26, 2020 at 1:15 PM Ezequiel Garcia
>> <ezequiel@xxxxxxxxxxxxxxxxxxxx> wrote:
>>>
>>> Hi Alexandre,
>>>
>>> On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@xxxxxxxxx> wrote:
>>>>
>>>> Factorize redundant checks into a single code block, remove the early
>>>> return, and declare variables in their innermost block. Hopefully this
>>>> makes this code a little bit easier to follow.
>>>>
>>>
>>> This _definitely_ makes the poll handling more readable.
>>>
>>> Reviewed-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
>>>
>>> See below a nitpick.
>>>
>>>> Signed-off-by: Alexandre Courbot <gnurou@xxxxxxxxx>
>>>> ---
>>>> drivers/media/v4l2-core/v4l2-mem2mem.c | 35 +++++++++++---------------
>>>> 1 file changed, 15 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
>>>> index 0d0192119af20..aeac9707123d0 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;
>>>>
>>>> @@ -863,33 +862,29 @@ static __poll_t v4l2_m2m_poll_for_data(struct file *file,
>>>> return EPOLLERR;
>>>>
>>>> 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;
>>>> + if (!list_empty(&src_q->done_list)) {
>>>> + struct vb2_buffer *src_vb = list_first_entry(
>>>> + &src_q->done_list, struct vb2_buffer, done_entry);
>>>> + if (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)) {
>>>> + if (!list_empty(&dst_q->done_list)) {
>>>> + struct vb2_buffer *dst_vb = list_first_entry(
>>>> + &dst_q->done_list, struct vb2_buffer, done_entry);
>>>> + if (dst_vb->state == VB2_BUF_STATE_DONE ||
>>>> + dst_vb->state == VB2_BUF_STATE_ERROR)
>>>> + rc |= EPOLLIN | EPOLLRDNORM;
>>>> + } else if (dst_q->last_buffer_dequeued) {
>>>> /*
>>>> * If the last buffer was dequeued from the capture queue,
>>>> * return immediately. DQBUF will return -EPIPE.
>>>> */
>>>
>>> The part about "returning immediately" doesn't make
>>> much sense now. Could we rephrase this, keeping the -EPIPE
>>> comment?
>>
>> I understood this sentence as referring to the system call and not
>> just this function, but maybe we can rephrase this as "... make
>> user-space wake up immediately"?
>>
>
> But is this really about user-space wakeup? I am under the impression
> that past poll_wait on both queues, we are already about to return
> (and wakeup).
>
> The way I see it, the original commit intention was to skip any
> done_list handling, returning immediately on the last buffer condition.
>
> How about just
>
> """
> If the last buffer was dequeued from the capture queue,
> signal userspace. DQBUF will return -EPIPE.
I'd write 'DQBUF(CAPTURE)' here to emphasize that only the capture
queue will return -EPIPE when you try to dequeue from it.
Also note that the original text was a copy-and-paste from vb2_core_poll().
The phrase 'return immediately' makes sense in that context since that
poll code deals with a single queue. In this case you have two queues,
and 'return immediately' no longer applies (in fact, that effectively is
the bug that being fixed here!).
Regards,
Hans
> """
>
> ?
>
>>>
>>> Thanks,
>>> Ezequiel
>>>
>>>> - if (dst_q->last_buffer_dequeued) {
>>>> - spin_unlock_irqrestore(&dst_q->done_lock, flags);
>>>> - rc |= EPOLLIN | EPOLLRDNORM;
>>>> - return rc;
>>>> - }
>>>> - }
>>>> -
>>>> - 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))
>>>> rc |= EPOLLIN | EPOLLRDNORM;
>>>> + }
>>>> spin_unlock_irqrestore(&dst_q->done_lock, flags);
>>>>
>>>> return rc;
>>>> --
>>>> 2.28.0
>>>>
