[PATCH] media: venus: fix reported size of 0-length buffers

[PATCH] media: venus: fix reported size of 0-length buffers

[Alexandre Courbot]

The last buffer is often signaled by an empty buffer with the
V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
bytesused field set to the full size of the OPB, which leads
user-space to believe that the buffer actually contains useful data. Fix
this by passing the number of bytes reported used by the firmware.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 189ec975c6bb..282de21cf2e1 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -885,10 +885,8 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 	vbuf->field = V4L2_FIELD_NONE;
 
 	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
-		unsigned int opb_sz = venus_helper_get_opb_size(inst);
-
 		vb = &vbuf->vb2_buf;
-		vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
+		vb2_set_plane_payload(vb, 0, bytesused);
 		vb->planes[0].data_offset = data_offset;
 		vb->timestamp = timestamp_us * NSEC_PER_USEC;
 		vbuf->sequence = inst->sequence_cap++;
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Alexandre Courbot]

On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
>
>
> Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@chromium.org> a écrit :
>>
>> The last buffer is often signaled by an empty buffer with the
>> V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
>> bytesused field set to the full size of the OPB, which leads
>> user-space to believe that the buffer actually contains useful data. Fix
>> this by passing the number of bytes reported used by the firmware.
>
>
> That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?

Sorry, I don't understand what you mean. EPIPE is supposed to be
returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
dequeue. This patch amends the code that prepares this LAST-flagged
buffer. How could we avoid a roundtrip in this case?

>
>>
>> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
>> ---
>>  drivers/media/platform/qcom/venus/vdec.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>> index 189ec975c6bb..282de21cf2e1 100644
>> --- a/drivers/media/platform/qcom/venus/vdec.c
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -885,10 +885,8 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>>         vbuf->field = V4L2_FIELD_NONE;
>>
>>         if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>> -               unsigned int opb_sz = venus_helper_get_opb_size(inst);
>> -
>>                 vb = &vbuf->vb2_buf;
>> -               vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
>> +               vb2_set_plane_payload(vb, 0, bytesused);
>>                 vb->planes[0].data_offset = data_offset;
>>                 vb->timestamp = timestamp_us * NSEC_PER_USEC;
>>                 vbuf->sequence = inst->sequence_cap++;
>> --
>> 2.19.1.930.g4563a0d9d0-goog
>>

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

Le mercredi 14 novembre 2018 à 13:12 +0900, Alexandre Courbot a écrit :
> On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > 
> > 
> > Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@chromium.org> a écrit :
> > > The last buffer is often signaled by an empty buffer with the
> > > V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
> > > bytesused field set to the full size of the OPB, which leads
> > > user-space to believe that the buffer actually contains useful data. Fix
> > > this by passing the number of bytes reported used by the firmware.
> > 
> > That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?
> 
> Sorry, I don't understand what you mean. EPIPE is supposed to be
> returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
> dequeue. This patch amends the code that prepares this LAST-flagged
> buffer. How could we avoid a roundtrip in this case?

Maybe it has changed, but when this was introduced, we found that some
firmware (Exynos MFC) could not know which one is last. Instead, it
gets an event saying there will be no more buffers.

Sending buffers with payload size to 0 just for the sake of setting the
V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
every polls should return EPIPE. So in the end, we decided the it
should just unblock the userspace and return EPIPE.

If you look at the related GStreamer code, it completely ignores the
LAST flag. With fake buffer of size 0, userspace will endup dequeuing
and throwing away. This is not useful to the process of terminating the
decoding. To me, this LAST flag is not useful in this context.

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

Le jeudi 15 novembre 2018 à 11:49 -0500, Nicolas Dufresne a écrit :
> Sending buffers with payload size to 0 just for the sake of setting the
> V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
> every polls should return EPIPE. So in the end, we decided the it
> should just unblock the userspace and return EPIPE.

errata, DQBUF returns EPIPE, not sure why I keep saying poll.

sorry for that,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Tomasz Figa]

On Fri, Nov 16, 2018 at 1:50 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le mercredi 14 novembre 2018 à 13:12 +0900, Alexandre Courbot a écrit :
> > On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > >
> > >
> > > Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@chromium.org> a écrit :
> > > > The last buffer is often signaled by an empty buffer with the
> > > > V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
> > > > bytesused field set to the full size of the OPB, which leads
> > > > user-space to believe that the buffer actually contains useful data. Fix
> > > > this by passing the number of bytes reported used by the firmware.
> > >
> > > That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?
> >
> > Sorry, I don't understand what you mean. EPIPE is supposed to be
> > returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
> > dequeue. This patch amends the code that prepares this LAST-flagged
> > buffer. How could we avoid a roundtrip in this case?
>
> Maybe it has changed, but when this was introduced, we found that some
> firmware (Exynos MFC) could not know which one is last. Instead, it
> gets an event saying there will be no more buffers.
>

It was never the case with the MFC (firmware/driver) we were using on
Chrome OS and it doesn't seem to be the case for the current upstream
s5p-mfc driver.

> Sending buffers with payload size to 0 just for the sake of setting the
> V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
> every polls should return EPIPE. So in the end, we decided the it
> should just unblock the userspace and return EPIPE.
>
> If you look at the related GStreamer code, it completely ignores the
> LAST flag. With fake buffer of size 0, userspace will endup dequeuing
> and throwing away. This is not useful to the process of terminating the
> decoding. To me, this LAST flag is not useful in this context.

Except that -EPIPE is actually signaled by the vb2 core and it happens
after the user space dequeues a buffer with the LAST flag set:
https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/media/common/videobuf2/videobuf2-core.c#L1634
https://elixir.bootlin.com/linux/v4.20-rc2/source/drivers/media/common/videobuf2/videobuf2-v4l2.c#L555

Best regards,
Tomasz

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Alexandre Courbot]

On Fri, Nov 16, 2018 at 1:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le mercredi 14 novembre 2018 à 13:12 +0900, Alexandre Courbot a écrit :
> > On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > >
> > >
> > > Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@chromium.org> a écrit :
> > > > The last buffer is often signaled by an empty buffer with the
> > > > V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
> > > > bytesused field set to the full size of the OPB, which leads
> > > > user-space to believe that the buffer actually contains useful data. Fix
> > > > this by passing the number of bytes reported used by the firmware.
> > >
> > > That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?
> >
> > Sorry, I don't understand what you mean. EPIPE is supposed to be
> > returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
> > dequeue. This patch amends the code that prepares this LAST-flagged
> > buffer. How could we avoid a roundtrip in this case?
>
> Maybe it has changed, but when this was introduced, we found that some
> firmware (Exynos MFC) could not know which one is last. Instead, it
> gets an event saying there will be no more buffers.
>
> Sending buffers with payload size to 0 just for the sake of setting the
> V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
> every polls should return EPIPE. So in the end, we decided the it
> should just unblock the userspace and return EPIPE.
>
> If you look at the related GStreamer code, it completely ignores the
> LAST flag. With fake buffer of size 0, userspace will endup dequeuing
> and throwing away. This is not useful to the process of terminating the
> decoding. To me, this LAST flag is not useful in this context.

Note that this patch does not interfere with DQBUF returning -EPIPE
after the last buffer has been dequeued. It just fixes an invalid size
that was returned for the last buffer.

Note also that if I understand the doc properly, the kernel driver
*must* set the V4L2_BUF_FLAG_LAST on the last buffer. With Venus the
last buffer is signaled by the firmware with an empty buffer. That's
not something we can change or predict earlier, so in order to respect
the specification we need to return that empty buffer. After that
DQBUF will behave as expected (returning -EPIPE), so GStreamer should
be happy as well.

Without the proposed fix however, GStreamer would receive the last
buffer with an incorrect size, and thus interpret random data as a
frame.

So to me this fix seems to be both correct, and needed. Isn't it?

Cheers,
Alex.

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Nicolas Dufresne]

Le jeudi 22 novembre 2018 à 17:31 +0900, Alexandre Courbot a écrit :
> On Fri, Nov 16, 2018 at 1:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > Le mercredi 14 novembre 2018 à 13:12 +0900, Alexandre Courbot a écrit :
> > > On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > > 
> > > > Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@chromium.org> a écrit :
> > > > > The last buffer is often signaled by an empty buffer with the
> > > > > V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
> > > > > bytesused field set to the full size of the OPB, which leads
> > > > > user-space to believe that the buffer actually contains useful data. Fix
> > > > > this by passing the number of bytes reported used by the firmware.
> > > > 
> > > > That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?
> > > 
> > > Sorry, I don't understand what you mean. EPIPE is supposed to be
> > > returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
> > > dequeue. This patch amends the code that prepares this LAST-flagged
> > > buffer. How could we avoid a roundtrip in this case?
> > 
> > Maybe it has changed, but when this was introduced, we found that some
> > firmware (Exynos MFC) could not know which one is last. Instead, it
> > gets an event saying there will be no more buffers.
> > 
> > Sending buffers with payload size to 0 just for the sake of setting the
> > V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
> > every polls should return EPIPE. So in the end, we decided the it
> > should just unblock the userspace and return EPIPE.
> > 
> > If you look at the related GStreamer code, it completely ignores the
> > LAST flag. With fake buffer of size 0, userspace will endup dequeuing
> > and throwing away. This is not useful to the process of terminating the
> > decoding. To me, this LAST flag is not useful in this context.
> 
> Note that this patch does not interfere with DQBUF returning -EPIPE
> after the last buffer has been dequeued. It just fixes an invalid size
> that was returned for the last buffer.
> 
> Note also that if I understand the doc properly, the kernel driver
> *must* set the V4L2_BUF_FLAG_LAST on the last buffer. With Venus the
> last buffer is signaled by the firmware with an empty buffer. That's
> not something we can change or predict earlier, so in order to respect
> the specification we need to return that empty buffer. After that
> DQBUF will behave as expected (returning -EPIPE), so GStreamer should
> be happy as well.
> 
> Without the proposed fix however, GStreamer would receive the last
> buffer with an incorrect size, and thus interpret random data as a
> frame.
> 
> So to me this fix seems to be both correct, and needed. Isn't it?

Totally, thanks for the extra clarification.

> 
> Cheers,
> Alex.

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Alexandre Courbot]

On Fri, Nov 23, 2018 at 8:53 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le jeudi 22 novembre 2018 à 17:31 +0900, Alexandre Courbot a écrit :
> > On Fri, Nov 16, 2018 at 1:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > Le mercredi 14 novembre 2018 à 13:12 +0900, Alexandre Courbot a écrit :
> > > > On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
> > > > >
> > > > > Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@chromium.org> a écrit :
> > > > > > The last buffer is often signaled by an empty buffer with the
> > > > > > V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
> > > > > > bytesused field set to the full size of the OPB, which leads
> > > > > > user-space to believe that the buffer actually contains useful data. Fix
> > > > > > this by passing the number of bytes reported used by the firmware.
> > > > >
> > > > > That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?
> > > >
> > > > Sorry, I don't understand what you mean. EPIPE is supposed to be
> > > > returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
> > > > dequeue. This patch amends the code that prepares this LAST-flagged
> > > > buffer. How could we avoid a roundtrip in this case?
> > >
> > > Maybe it has changed, but when this was introduced, we found that some
> > > firmware (Exynos MFC) could not know which one is last. Instead, it
> > > gets an event saying there will be no more buffers.
> > >
> > > Sending buffers with payload size to 0 just for the sake of setting the
> > > V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
> > > every polls should return EPIPE. So in the end, we decided the it
> > > should just unblock the userspace and return EPIPE.
> > >
> > > If you look at the related GStreamer code, it completely ignores the
> > > LAST flag. With fake buffer of size 0, userspace will endup dequeuing
> > > and throwing away. This is not useful to the process of terminating the
> > > decoding. To me, this LAST flag is not useful in this context.
> >
> > Note that this patch does not interfere with DQBUF returning -EPIPE
> > after the last buffer has been dequeued. It just fixes an invalid size
> > that was returned for the last buffer.
> >
> > Note also that if I understand the doc properly, the kernel driver
> > *must* set the V4L2_BUF_FLAG_LAST on the last buffer. With Venus the
> > last buffer is signaled by the firmware with an empty buffer. That's
> > not something we can change or predict earlier, so in order to respect
> > the specification we need to return that empty buffer. After that
> > DQBUF will behave as expected (returning -EPIPE), so GStreamer should
> > be happy as well.
> >
> > Without the proposed fix however, GStreamer would receive the last
> > buffer with an incorrect size, and thus interpret random data as a
> > frame.
> >
> > So to me this fix seems to be both correct, and needed. Isn't it?
>
> Totally, thanks for the extra clarification.

Awesome, thanks for confirming!

Stanimir, can we have your thoughts about this change?

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Stanimir Varbanov]

Hi Alex,

On 11/22/18 10:31 AM, Alexandre Courbot wrote:
> On Fri, Nov 16, 2018 at 1:49 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>
>> Le mercredi 14 novembre 2018 à 13:12 +0900, Alexandre Courbot a écrit :
>>> On Wed, Nov 14, 2018 at 3:54 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>>>
>>>>
>>>> Le mar. 13 nov. 2018 04 h 30, Alexandre Courbot <acourbot@chromium.org> a écrit :
>>>>> The last buffer is often signaled by an empty buffer with the
>>>>> V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
>>>>> bytesused field set to the full size of the OPB, which leads
>>>>> user-space to believe that the buffer actually contains useful data. Fix
>>>>> this by passing the number of bytes reported used by the firmware.
>>>>
>>>> That means the driver does not know on time which one is last. Why not just returned EPIPE to userspace on DQBUF and ovoid this useless roundtrip ?
>>>
>>> Sorry, I don't understand what you mean. EPIPE is supposed to be
>>> returned after a buffer with V4L2_BUF_FLAG_LAST is made available for
>>> dequeue. This patch amends the code that prepares this LAST-flagged
>>> buffer. How could we avoid a roundtrip in this case?
>>
>> Maybe it has changed, but when this was introduced, we found that some
>> firmware (Exynos MFC) could not know which one is last. Instead, it
>> gets an event saying there will be no more buffers.
>>
>> Sending buffers with payload size to 0 just for the sake of setting the
>> V4L2_BUF_FLAG_LAST was considered a waste. Specially that after that,
>> every polls should return EPIPE. So in the end, we decided the it
>> should just unblock the userspace and return EPIPE.
>>
>> If you look at the related GStreamer code, it completely ignores the
>> LAST flag. With fake buffer of size 0, userspace will endup dequeuing
>> and throwing away. This is not useful to the process of terminating the
>> decoding. To me, this LAST flag is not useful in this context.
> 
> Note that this patch does not interfere with DQBUF returning -EPIPE
> after the last buffer has been dequeued. It just fixes an invalid size
> that was returned for the last buffer.
> 
> Note also that if I understand the doc properly, the kernel driver
> *must* set the V4L2_BUF_FLAG_LAST on the last buffer. With Venus the
> last buffer is signaled by the firmware with an empty buffer. That's

Small correction, the firmware signals EoS with HFI_BUFFERFLAG_EOS flag
with the returned buffer, then we set V4L2_BUF_FLAG_LAST.

Usually with v1 and v3 when HFI_BUFFERFLAG_EOS is set the bytesused is zero.

> not something we can change or predict earlier, so in order to respect
> the specification we need to return that empty buffer. After that
> DQBUF will behave as expected (returning -EPIPE), so GStreamer should
> be happy as well.
> 
> Without the proposed fix however, GStreamer would receive the last
> buffer with an incorrect size, and thus interpret random data as a
> frame.
> 
> So to me this fix seems to be both correct, and needed. Isn't it?
> 
> Cheers,
> Alex.
> 

-- 
regards,
Stan

Re: [PATCH] media: venus: fix reported size of 0-length buffers

[Stanimir Varbanov]

Hi Alex,

Thanks for the patch!

On 11/13/18 11:30 AM, Alexandre Courbot wrote:
> The last buffer is often signaled by an empty buffer with the
> V4L2_BUF_FLAG_LAST flag set. Such buffers were returned with the
> bytesused field set to the full size of the OPB, which leads
> user-space to believe that the buffer actually contains useful data. Fix
> this by passing the number of bytes reported used by the firmware.
> 
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 189ec975c6bb..282de21cf2e1 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -885,10 +885,8 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  	vbuf->field = V4L2_FIELD_NONE;
>  
>  	if (type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> -		unsigned int opb_sz = venus_helper_get_opb_size(inst);
> -
>  		vb = &vbuf->vb2_buf;
> -		vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
> +		vb2_set_plane_payload(vb, 0, bytesused);
>  		vb->planes[0].data_offset = data_offset;
>  		vb->timestamp = timestamp_us * NSEC_PER_USEC;
>  		vbuf->sequence = inst->sequence_cap++;
> 

-- 
regards,
Stan