* Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer
2020-06-02 15:09 ` [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer Nicolas Dufresne
@ 2020-09-17 8:48 ` Hans Verkuil
2020-09-17 12:52 ` Tomasz Figa
1 sibling, 0 replies; 5+ messages in thread
From: Hans Verkuil @ 2020-09-17 8:48 UTC (permalink / raw)
To: Nicolas Dufresne, Andriy Gelman
Cc: Andrzej Hajda, Kamil Debski, linux-kernel, Jeongtae Park,
Kyungmin Park, Mauro Carvalho Chehab, linux-arm-kernel,
linux-media
On 02/06/2020 17:09, Nicolas Dufresne wrote:
> Hi Andriy,
>
> thanks for you patch.
>
> Le samedi 02 mai 2020 à 15:40 -0400, Andriy Gelman a écrit :
>> From: Andriy Gelman <andriy.gelman@gmail.com>
>>
>> As per V4L2 api, the final buffer should set V4L2_BUF_FLAG_LAST flag.
>>
>> Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
>> ---
>> drivers/media/platform/s5p-mfc/s5p_mfc.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> index 5c2a23b953a4..b3d9b3a523fe 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> @@ -614,6 +614,7 @@ static void s5p_mfc_handle_stream_complete(struct s5p_mfc_ctx *ctx)
>> list_del(&mb_entry->list);
>> ctx->dst_queue_cnt--;
>> vb2_set_plane_payload(&mb_entry->b->vb2_buf, 0, 0);
>> + mb_entry->b->flags |= V4L2_BUF_FLAG_LAST;
>> vb2_buffer_done(&mb_entry->b->vb2_buf, VB2_BUF_STATE_DONE);
>
> The empty buffer is only there for backward compatibility. As the spec
> says, userspace should completely ignore this buffer. I bet it will
> still have the effect to set last_buffer_dequeued = true in vb2,
Actually, no. It only tests for V4L2_BUF_FLAG_LAST, not for empty buffers.
Regards,
Hans
> unblocking poll() operations and allowing for the queue to unblock and
> return EPIPE on further DQBUF.
>
> Perhaps you should make sure the if both the src and dst queues are
> empty/done by the time cmd_stop is called it will still work. Other
> drivers seems to handle this, but this one does not seems to have a
> special case for that, which may hang userspace in a different way.
>
> What you should do to verify this patch is correct, and that your
> userpace does not rely on legacy path is that it should always be able
> to detect the end of the drain with a EPIPE on DQBUF. LAST_BUF is just
> an early signalling, but may not occur if there was nothing left to
> produce (except for MFC which maybe be crafting a buffer in all cases,
> but that's going a roundtrip through the HW, which is not clear will
> work if the dst queue was empty).
>
> As shared on IRC, you have sent these patch to FFMPEG:
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-2-andriy.gelman@gmail.com/
>
> This should have been clarified as supporting legacy drivers / older
> kernel with Samsung driver. Seems like a fair patch. And you added:
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-1-andriy.gelman@gmail.com/
>
> This one should maybe add the clarification that this is an
> optimization to skip an extra poll/dqbuf dance, but that end of
> draining will ultimately be catched by EPIPE on dqbuf for the described
> cases. Remains valid enhancement to ffmpeg imho.
>
>> }
>>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer
2020-06-02 15:09 ` [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer Nicolas Dufresne
2020-09-17 8:48 ` Hans Verkuil
@ 2020-09-17 12:52 ` Tomasz Figa
1 sibling, 0 replies; 5+ messages in thread
From: Tomasz Figa @ 2020-09-17 12:52 UTC (permalink / raw)
To: Nicolas Dufresne
Cc: Andriy Gelman, Jeongtae Park, Kamil Debski,
Linux Kernel Mailing List, Andrzej Hajda, Kyungmin Park,
Mauro Carvalho Chehab,
list@263.net:IOMMU DRIVERS
<iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>, ,
Linux Media Mailing List
On Tue, Jun 2, 2020 at 5:09 PM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Hi Andriy,
>
> thanks for you patch.
>
> Le samedi 02 mai 2020 à 15:40 -0400, Andriy Gelman a écrit :
> > From: Andriy Gelman <andriy.gelman@gmail.com>
> >
> > As per V4L2 api, the final buffer should set V4L2_BUF_FLAG_LAST flag.
> >
> > Signed-off-by: Andriy Gelman <andriy.gelman@gmail.com>
> > ---
> > drivers/media/platform/s5p-mfc/s5p_mfc.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > index 5c2a23b953a4..b3d9b3a523fe 100644
> > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> > @@ -614,6 +614,7 @@ static void s5p_mfc_handle_stream_complete(struct s5p_mfc_ctx *ctx)
> > list_del(&mb_entry->list);
> > ctx->dst_queue_cnt--;
> > vb2_set_plane_payload(&mb_entry->b->vb2_buf, 0, 0);
> > + mb_entry->b->flags |= V4L2_BUF_FLAG_LAST;
> > vb2_buffer_done(&mb_entry->b->vb2_buf, VB2_BUF_STATE_DONE);
>
> The empty buffer is only there for backward compatibility. As the spec
> says, userspace should completely ignore this buffer. I bet it will
> still have the effect to set last_buffer_dequeued = true in vb2,
> unblocking poll() operations and allowing for the queue to unblock and
> return EPIPE on further DQBUF.
>
> Perhaps you should make sure the if both the src and dst queues are
> empty/done by the time cmd_stop is called it will still work. Other
> drivers seems to handle this, but this one does not seems to have a
> special case for that, which may hang userspace in a different way.
>
> What you should do to verify this patch is correct, and that your
> userpace does not rely on legacy path is that it should always be able
> to detect the end of the drain with a EPIPE on DQBUF. LAST_BUF is just
> an early signalling, but may not occur if there was nothing left to
> produce (except for MFC which maybe be crafting a buffer in all cases,
> but that's going a roundtrip through the HW, which is not clear will
> work if the dst queue was empty).
The spec guarantees that a buffer with the LAST_BUF flag is returned
to the userspace. In fact, handling entirely by the DQBUF return code
may be buggy, because the LAST_BUF flag may also be set for other
reasons, like a resolution change happening after a drain request was
already initiated. The proper way to handle a drain is to look for the
LAST_BUF flag and then try to dequeue an event to check what the
LAST_BUF flag is associated with. It might be worth adding a relevant
note to the drain sequence documentation in the spec.
As for the patch itself, I think it's valid, but it's a bit concerning
that the code is inside a conditional block executed only when there
is a buffer in the CAPTURE queue [1]. As I mentioned above, the driver
needs to signal the LAST_BUF flag, so if there is no buffer to signal
it on, it should be signaled when a buffer is queued. Of course it's
well possible that the condition can never happen, e.g. the function
is called only as a result of a hardware request that can be scheduled
only when there is at least 1 CAPTURE buffer in the queue. Looking at
[2], it might be the case indeed, but someone should validate that.
[1] https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc.c#L611
[2] https://elixir.bootlin.com/linux/v5.9-rc5/source/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c#L222
Best regards,
Tomasz
>
> As shared on IRC, you have sent these patch to FFMPEG:
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-2-andriy.gelman@gmail.com/
>
> This should have been clarified as supporting legacy drivers / older
> kernel with Samsung driver. Seems like a fair patch. And you added:
>
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200429212942.28797-1-andriy.gelman@gmail.com/
>
> This one should maybe add the clarification that this is an
> optimization to skip an extra poll/dqbuf dance, but that end of
> draining will ultimately be catched by EPIPE on dqbuf for the described
> cases. Remains valid enhancement to ffmpeg imho.
>
> > }
> >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 5+ messages in thread