| Message ID | 20200502194052.485-1-andriy.gelman@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | media: s5p-mfc: set V4L2_BUF_FLAG_LAST flag on final buffer | expand |
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. > >> } >> >
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); > } > >
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. > > > } > > >
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); }