Message-ID: <3dd23b1e-debd-bd50-e8a1-25837d2fd01e@xs4all.nl>
Date:   Wed, 26 Aug 2020 17:23:03 +0200
From:   Hans Verkuil <hverkuil-cisco@...all.nl>
To:     Ezequiel Garcia <ezequiel@...guardiasur.com.ar>,
        Alexandre Courbot <gnurou@...il.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Nicolas Dufresne <nicolas.dufresne@...labora.com>,
        linux-media <linux-media@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] media: v4l2-mem2mem: simplify poll logic a bit

On 26/08/2020 16:32, Ezequiel Garcia wrote:
> On Wed, 26 Aug 2020 at 08:19, Alexandre Courbot <gnurou@...il.com> wrote:
>>
>> Hi Ezequiel, thanks for the review!
>>
>> On Wed, Aug 26, 2020 at 1:15 PM Ezequiel Garcia
>> <ezequiel@...guardiasur.com.ar> wrote:
>>>
>>> Hi Alexandre,
>>>
>>> On Tue, 25 Aug 2020 at 11:56, Alexandre Courbot <gnurou@...il.com> 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@...labora.com>
>>>
>>> See below a nitpick.
>>>
>>>> Signed-off-by: Alexandre Courbot <gnurou@...il.com>
>>>> ---
>>>>  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
>>>>

Powered by blists - more mailing lists