[PATCH] [media] s5p-mfc: Don't try to resubmit VP8 bitstream buffer for decode.

[PATCH] [media] s5p-mfc: Don't try to resubmit VP8 bitstream buffer for decode.

[Arun Kumar K]

From: Pawel Osciak <posciak@chromium.org>

Currently, for formats that are not H264, MFC driver will check
the consumed stream size returned by the firmware and, based on that,
will try to decide whether the bitstream buffer contained more than
one frame. If the size of the buffer is larger than the consumed
stream, it assumes that there are more frames in the buffer and that the
buffer should be resubmitted for decode. This rarely works though and
actually introduces problems, because:

- v7 firmware will always return consumed stream size equal to whatever
the driver passed to it when running decode (which is the size of the whole
buffer), which means we will never try to resubmit, because the firmware
will always tell us that it consumed all the data we passed to it;

- v6 firmware will return the number of consumed bytes, but will not
include the padding ("stuffing") bytes that are allowed after the frame
in VP8. Since there is no way of figuring out how many of those bytes
follow the frame without getting the frame size from IVF headers (or
somewhere else, but not from the stream itself), the driver tries to guess that
padding size is not larger than 4 bytes, which is not always true;

The only way to make it work is to queue only one frame per buffer from
userspace and the check in the kernel is useless and wrong for VP8.
MPEG4 still seems to require it, so keep it only for that format.

Signed-off-by: Pawel Osciak <posciak@chromium.org>
Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index e2aac59..66c1775 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -360,7 +360,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx *ctx,
 								list);
 		ctx->consumed_stream += s5p_mfc_hw_call(dev->mfc_ops,
 						get_consumed_stream, dev);
-		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
+		if (ctx->codec_mode == S5P_MFC_CODEC_MPEG4_DEC &&
 			ctx->consumed_stream + STUFF_BYTE <
 			src_buf->b->v4l2_planes[0].bytesused) {
 			/* Run MFC again on the same buffer */
-- 
1.7.9.5

RE: [PATCH] [media] s5p-mfc: Don't try to resubmit VP8 bitstream buffer for decode.

[Kamil Debski]

Hi Arun,

> From: Arun Kumar K [mailto:arunkk.samsung@gmail.com] On Behalf Of Arun
> Kumar K
> Sent: Friday, March 07, 2014 9:26 AM
> 
> From: Pawel Osciak <posciak@chromium.org>
> 
> Currently, for formats that are not H264, MFC driver will check the
> consumed stream size returned by the firmware and, based on that, will
> try to decide whether the bitstream buffer contained more than one
> frame. If the size of the buffer is larger than the consumed stream, it
> assumes that there are more frames in the buffer and that the buffer
> should be resubmitted for decode. This rarely works though and actually
> introduces problems, because:
> 
> - v7 firmware will always return consumed stream size equal to whatever
> the driver passed to it when running decode (which is the size of the
> whole buffer), which means we will never try to resubmit, because the
> firmware will always tell us that it consumed all the data we passed to
> it;

This does sound like a hardware bug/feature. So in v7 the buffer is never
resubmitted, yes? And this patch makes no difference for v7?

> 
> - v6 firmware will return the number of consumed bytes, but will not
> include the padding ("stuffing") bytes that are allowed after the frame
> in VP8. Since there is no way of figuring out how many of those bytes
> follow the frame without getting the frame size from IVF headers (or
> somewhere else, but not from the stream itself), the driver tries to
> guess that padding size is not larger than 4 bytes, which is not always
> true;

How about v5 of MFC? I need to do some additional testing, as I don't want
to introduce any regressions. I remember that this check was a result of a
fair amount of work and testing with v5.
 
> The only way to make it work is to queue only one frame per buffer from
> userspace and the check in the kernel is useless and wrong for VP8.
> MPEG4 still seems to require it, so keep it only for that format.

Is your goal to give more than one frame in a single buffer and have the 
buffer resubmitted? Or the opposite - you are getting the frame resubmitted
without the need? By the contents of this patch I guess the latter, on the
other hand I do remember that at some point the idea was to be able to queue
more than one frame per buffer. I don't remember exactly who was opting for
the ability to queue more frames in a single buffer...

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

> 
> Signed-off-by: Pawel Osciak <posciak@chromium.org>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index e2aac59..66c1775 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -360,7 +360,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
> *ctx,
>  								list);
>  		ctx->consumed_stream += s5p_mfc_hw_call(dev->mfc_ops,
>  						get_consumed_stream, dev);
> -		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
> +		if (ctx->codec_mode == S5P_MFC_CODEC_MPEG4_DEC &&
>  			ctx->consumed_stream + STUFF_BYTE <
>  			src_buf->b->v4l2_planes[0].bytesused) {
>  			/* Run MFC again on the same buffer */
> --
> 1.7.9.5

Re: [PATCH] [media] s5p-mfc: Don't try to resubmit VP8 bitstream buffer for decode.

[Arun Kumar K]

Hi Kamil,

On Tue, Mar 11, 2014 at 4:59 PM, Kamil Debski <k.debski@samsung.com> wrote:
> Hi Arun,
>
>> From: Arun Kumar K [mailto:arunkk.samsung@gmail.com] On Behalf Of Arun
>> Kumar K
>> Sent: Friday, March 07, 2014 9:26 AM
>>
>> From: Pawel Osciak <posciak@chromium.org>
>>
>> Currently, for formats that are not H264, MFC driver will check the
>> consumed stream size returned by the firmware and, based on that, will
>> try to decide whether the bitstream buffer contained more than one
>> frame. If the size of the buffer is larger than the consumed stream, it
>> assumes that there are more frames in the buffer and that the buffer
>> should be resubmitted for decode. This rarely works though and actually
>> introduces problems, because:
>>
>> - v7 firmware will always return consumed stream size equal to whatever
>> the driver passed to it when running decode (which is the size of the
>> whole buffer), which means we will never try to resubmit, because the
>> firmware will always tell us that it consumed all the data we passed to
>> it;
>
> This does sound like a hardware bug/feature. So in v7 the buffer is never
> resubmitted, yes? And this patch makes no difference for v7?
>

To give some background to this - I have added this patch [1] so that
multi-frame input buffer is supported by the driver.
[1] https://patchwork.linuxtv.org/patch/15448/

This patch was added to address one test case in VP8 decoding where
2 frames used to come in a single buffer. Without this patch, it used to
skip decoding of the 2nd frame even though firmware returned
consumed bytes as only 1 frame size.

Now this concept gave issues when we tried to decode the VP8 stream
encoded using v7 firmware. In that case, an arbitrary amount of padding
bytes were added by the firmware after every frame which is allowed as
per VP8 standard. While decoding this using v6, firmware returns less
consumed bytes than the input buffer, but the remaining bytes are just
padding. So firmware used to throw error if we re-submit this stuffing bytes
as a new frame. In v7 firmware, this problem doesnt exist as it consumes
(atleast indicates that it consumed) all the input buffer.

So we came to the conclusion that the testcase of giving multiple frames
in one input buffer itself was wrong and hence it was removed.
Now this concept is needed only for MPEG4 packed PB case which this
code for resubmitting input buffer was meant for (before my patch made
it generic).

>>
>> - v6 firmware will return the number of consumed bytes, but will not
>> include the padding ("stuffing") bytes that are allowed after the frame
>> in VP8. Since there is no way of figuring out how many of those bytes
>> follow the frame without getting the frame size from IVF headers (or
>> somewhere else, but not from the stream itself), the driver tries to
>> guess that padding size is not larger than 4 bytes, which is not always
>> true;
>
> How about v5 of MFC? I need to do some additional testing, as I don't want
> to introduce any regressions. I remember that this check was a result of a
> fair amount of work and testing with v5.
>

I hope it wont give any issues in v5 also as it was never meant to handle
multiple frames in one input buffer.


>> The only way to make it work is to queue only one frame per buffer from
>> userspace and the check in the kernel is useless and wrong for VP8.
>> MPEG4 still seems to require it, so keep it only for that format.
>
> Is your goal to give more than one frame in a single buffer and have the
> buffer resubmitted? Or the opposite - you are getting the frame resubmitted
> without the need? By the contents of this patch I guess the latter, on the
> other hand I do remember that at some point the idea was to be able to queue
> more than one frame per buffer. I don't remember exactly who was opting for
> the ability to queue more frames in a single buffer...
>

I hope now it is clear. We want to disallow multiple frames in one buffer as the
behavior is not consistent in v6 and its not allowed anyway from v7 onwards.

Regards
Arun

> Best wishes,
> --
> Kamil Debski
> Samsung R&D Institute Poland
>
>>
>> Signed-off-by: Pawel Osciak <posciak@chromium.org>
>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> ---
>>  drivers/media/platform/s5p-mfc/s5p_mfc.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> index e2aac59..66c1775 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> @@ -360,7 +360,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
>> *ctx,
>>                                                               list);
>>               ctx->consumed_stream += s5p_mfc_hw_call(dev->mfc_ops,
>>                                               get_consumed_stream, dev);
>> -             if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
>> +             if (ctx->codec_mode == S5P_MFC_CODEC_MPEG4_DEC &&
>>                       ctx->consumed_stream + STUFF_BYTE <
>>                       src_buf->b->v4l2_planes[0].bytesused) {
>>                       /* Run MFC again on the same buffer */
>> --
>> 1.7.9.5
>

RE: [PATCH] [media] s5p-mfc: Don't try to resubmit VP8 bitstream buffer for decode.

[Kamil Debski]

Hi,


> From: Arun Kumar K [mailto:arunkk.samsung@gmail.com] On Behalf Of Arun
> Kumar K
> Sent: Friday, March 07, 2014 9:26 AM
> 
> From: Pawel Osciak <posciak@chromium.org>
> 
> Currently, for formats that are not H264, MFC driver will check the
> consumed stream size returned by the firmware and, based on that, will
> try to decide whether the bitstream buffer contained more than one
> frame. If the size of the buffer is larger than the consumed stream, it
> assumes that there are more frames in the buffer and that the buffer
> should be resubmitted for decode. This rarely works though and actually
> introduces problems, because:
> 
> - v7 firmware will always return consumed stream size equal to whatever
> the driver passed to it when running decode (which is the size of the
> whole buffer), which means we will never try to resubmit, because the
> firmware will always tell us that it consumed all the data we passed to
> it;
> 
> - v6 firmware will return the number of consumed bytes, but will not
> include the padding ("stuffing") bytes that are allowed after the frame
> in VP8. Since there is no way of figuring out how many of those bytes
> follow the frame without getting the frame size from IVF headers (or
> somewhere else, but not from the stream itself), the driver tries to
> guess that padding size is not larger than 4 bytes, which is not always
> true;
> 
> The only way to make it work is to queue only one frame per buffer from
> userspace and the check in the kernel is useless and wrong for VP8.
> MPEG4 still seems to require it, so keep it only for that format.
> 
> Signed-off-by: Pawel Osciak <posciak@chromium.org>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index e2aac59..66c1775 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -360,7 +360,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
> *ctx,
>  								list);
>  		ctx->consumed_stream += s5p_mfc_hw_call(dev->mfc_ops,
>  						get_consumed_stream, dev);
> -		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
> +		if (ctx->codec_mode == S5P_MFC_CODEC_MPEG4_DEC &&
>  			ctx->consumed_stream + STUFF_BYTE <
>  			src_buf->b->v4l2_planes[0].bytesused) {
>  			/* Run MFC again on the same buffer */

I expressed my doubts to this patch in my previous email.
I think that packed PB can also be found in other codecs such as H263.
So please change to the following if this is a workaround for VP8 only.
(The title says that it only changes behavior of VP8 decoding, so it is
misleading).

-		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
+		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
+		    ctx->codec_mode != S5P_MFC_CODEC_VP8_DEC &&


Did you try to revert your patch https://patchwork.linuxtv.org/patch/15448/
and checking if this fixes the problem for VP8?

> --
> 1.7.9.5

Best wishes,
-- 
Kamil Debski
Samsung R&D Institute Poland

Re: [PATCH] [media] s5p-mfc: Don't try to resubmit VP8 bitstream buffer for decode.

[Arun Kumar K]

Hi Kamil,

On 05/08/14 21:52, Kamil Debski wrote:
> Hi,
> 
> 
>> From: Arun Kumar K [mailto:arunkk.samsung@gmail.com] On Behalf Of Arun
>> Kumar K
>> Sent: Friday, March 07, 2014 9:26 AM
>>
>> From: Pawel Osciak <posciak@chromium.org>
>>
>> Currently, for formats that are not H264, MFC driver will check the
>> consumed stream size returned by the firmware and, based on that, will
>> try to decide whether the bitstream buffer contained more than one
>> frame. If the size of the buffer is larger than the consumed stream, it
>> assumes that there are more frames in the buffer and that the buffer
>> should be resubmitted for decode. This rarely works though and actually
>> introduces problems, because:
>>
>> - v7 firmware will always return consumed stream size equal to whatever
>> the driver passed to it when running decode (which is the size of the
>> whole buffer), which means we will never try to resubmit, because the
>> firmware will always tell us that it consumed all the data we passed to
>> it;
>>
>> - v6 firmware will return the number of consumed bytes, but will not
>> include the padding ("stuffing") bytes that are allowed after the frame
>> in VP8. Since there is no way of figuring out how many of those bytes
>> follow the frame without getting the frame size from IVF headers (or
>> somewhere else, but not from the stream itself), the driver tries to
>> guess that padding size is not larger than 4 bytes, which is not always
>> true;
>>
>> The only way to make it work is to queue only one frame per buffer from
>> userspace and the check in the kernel is useless and wrong for VP8.
>> MPEG4 still seems to require it, so keep it only for that format.
>>
>> Signed-off-by: Pawel Osciak <posciak@chromium.org>
>> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> ---
>>  drivers/media/platform/s5p-mfc/s5p_mfc.c |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> index e2aac59..66c1775 100644
>> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
>> @@ -360,7 +360,7 @@ static void s5p_mfc_handle_frame(struct s5p_mfc_ctx
>> *ctx,
>>  								list);
>>  		ctx->consumed_stream += s5p_mfc_hw_call(dev->mfc_ops,
>>  						get_consumed_stream, dev);
>> -		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
>> +		if (ctx->codec_mode == S5P_MFC_CODEC_MPEG4_DEC &&
>>  			ctx->consumed_stream + STUFF_BYTE <
>>  			src_buf->b->v4l2_planes[0].bytesused) {
>>  			/* Run MFC again on the same buffer */
> 
> I expressed my doubts to this patch in my previous email.
> I think that packed PB can also be found in other codecs such as H263.
> So please change to the following if this is a workaround for VP8 only.
> (The title says that it only changes behavior of VP8 decoding, so it is
> misleading).
> 

Yes it is seen as affecting only VP8 decoding.

> -		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
> +		if (ctx->codec_mode != S5P_MFC_CODEC_H264_DEC &&
> +		    ctx->codec_mode != S5P_MFC_CODEC_VP8_DEC &&
> 

Ok. I will change it this way.

> 
> Did you try to revert your patch https://patchwork.linuxtv.org/patch/15448/
> and checking if this fixes the problem for VP8?
> 

I am afraid it will not solve the VP8 issue as before that patch, it
used to check for ctx->codec_mode != S5P_MFC_CODEC_H264_DEC and frame
type is S5P_FIMV_DECODE_FRAME_P_FRAME, buffer was sent again to decode.
This condition can be triggered in VP8 case causing erroneous behaviour.

So I will make the change as you suggested above.

Regards
Arun

>> --
>> 1.7.9.5
> 
> Best wishes,
>