[PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer

[PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer

[Andriy Gelman]

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);
 	}
 
-- 
2.25.1

Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer

[Nicolas Dufresne]

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).

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.

>  	}
>  

Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer

[Hans Verkuil]

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.
> 
>>  	}
>>  
> 

Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer

[Hans Verkuil]

Added Sylwester and Tomasz.

I'd like to have an Ack of a driver maintainer before merging.

Regards,

	Hans

On 02/05/2020 21:40, Andriy Gelman wrote:
> 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);
>  	}
>  
> 

Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer

[Tomasz Figa]

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.
>
> >       }
> >
>

Re: [PATCH] media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer

[Hans Verkuil]

Hi all,

I'm going through a bunch of (very) old patches in my patchwork TODO list
that for one reason or another I never processed. This patch is one of
them.

I don't feel comfortable merging this, given the follow-ups that were posted.

If someone wants to get this in anyway, then please make a new patch. I'm
marking it as 'Changes Requested' in patchwork.

Regards,

	Hans

On 02/05/2020 21:40, Andriy Gelman wrote:
> 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);
>  	}
>