[PATCH] venus: vdec: fix decoded data size

[PATCH] venus: vdec: fix decoded data size

[Vikash Garodia]

Exisiting code returns the max of the decoded
size and buffer size. It turns out that buffer
size is always greater due to hardware alignment
requirement. As a result, payload size given to
client is incorrect. This change ensures that
the bytesused is assigned to actual payload size.

Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index d079aeb..ada1d2f 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 
 		vb = &vbuf->vb2_buf;
 		vb->planes[0].bytesused =
-			max_t(unsigned int, opb_sz, bytesused);
+			min_t(unsigned int, opb_sz, bytesused);
 		vb->planes[0].data_offset = data_offset;
 		vb->timestamp = timestamp_us * NSEC_PER_USEC;
 		vbuf->sequence = inst->sequence_cap++;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] venus: vdec: fix decoded data size

[Alexandre Courbot]

On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>
> Exisiting code returns the max of the decoded

s/Exisiting/Existing

Also the lines of your commit message look pretty short - I think the
standard for kernel log messges is 72 chars?

> size and buffer size. It turns out that buffer
> size is always greater due to hardware alignment
> requirement. As a result, payload size given to
> client is incorrect. This change ensures that
> the bytesused is assigned to actual payload size.
>
> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index d079aeb..ada1d2f 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>
>                 vb = &vbuf->vb2_buf;
>                 vb->planes[0].bytesused =
> -                       max_t(unsigned int, opb_sz, bytesused);
> +                       min_t(unsigned int, opb_sz, bytesused);

Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
Tested-by: Alexandre Courbot <acourbot@chromium.org>

This indeed reports the correct size to the client. If bytesused were
larger than the size of the buffer we would be having some trouble
anyway.

Actually in my tree I was using the following patch:

--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -924,13 +924,12 @@ static void vdec_buf_done(struct venus_inst
*inst, unsigned int buf_type,

               vb = &vbuf->vb2_buf;
               vb->planes[0].bytesused =
-                       max_t(unsigned int, opb_sz, bytesused);
+                       min_t(unsigned int, opb_sz, bytesused);
               vb->planes[0].data_offset = data_offset;
               vb->timestamp = timestamp_us * NSEC_PER_USEC;
               vbuf->sequence = inst->sequence_cap++;
               if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
                       const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
-                       vb->planes[0].bytesused = bytesused;
                       v4l2_event_queue_fh(&inst->fh, &ev);

Given that we are now taking the minimum of these two values, it seems
to me that we don't need to set bytesused again in case we are dealing
with the last buffer? Stanimir, what do you think?

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Alex,

On 07/02/2018 11:51 AM, Alexandre Courbot wrote:
> On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
>>
>> Exisiting code returns the max of the decoded
> 
> s/Exisiting/Existing
> 
> Also the lines of your commit message look pretty short - I think the
> standard for kernel log messges is 72 chars?
> 
>> size and buffer size. It turns out that buffer
>> size is always greater due to hardware alignment
>> requirement. As a result, payload size given to
>> client is incorrect. This change ensures that
>> the bytesused is assigned to actual payload size.
>>
>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>> ---
>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
>> index d079aeb..ada1d2f 100644
>> --- a/drivers/media/platform/qcom/venus/vdec.c
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>>
>>                 vb = &vbuf->vb2_buf;
>>                 vb->planes[0].bytesused =
>> -                       max_t(unsigned int, opb_sz, bytesused);
>> +                       min_t(unsigned int, opb_sz, bytesused);
> 
> Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
> Tested-by: Alexandre Courbot <acourbot@chromium.org>
> 
> This indeed reports the correct size to the client. If bytesused were
> larger than the size of the buffer we would be having some trouble
> anyway.
> 
> Actually in my tree I was using the following patch:
> 
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -924,13 +924,12 @@ static void vdec_buf_done(struct venus_inst
> *inst, unsigned int buf_type,
> 
>                vb = &vbuf->vb2_buf;
>                vb->planes[0].bytesused =
> -                       max_t(unsigned int, opb_sz, bytesused);
> +                       min_t(unsigned int, opb_sz, bytesused);
>                vb->planes[0].data_offset = data_offset;
>                vb->timestamp = timestamp_us * NSEC_PER_USEC;
>                vbuf->sequence = inst->sequence_cap++;
>                if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
>                        const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
> -                       vb->planes[0].bytesused = bytesused;

Actually this line doesn't exist in mainline driver. And I don't see a
reason why to set bytesused twice.

>                        v4l2_event_queue_fh(&inst->fh, &ev);
> 
> Given that we are now taking the minimum of these two values, it seems
> to me that we don't need to set bytesused again in case we are dealing
> with the last buffer? Stanimir, what do you think?
> 

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Alexandre Courbot]

On Sat, Jul 7, 2018, 00:12 Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Alex,
>
> On 07/02/2018 11:51 AM, Alexandre Courbot wrote:
> > On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <vgarodia@codeaurora.org> wrote:
> >>
> >> Exisiting code returns the max of the decoded
> >
> > s/Exisiting/Existing
> >
> > Also the lines of your commit message look pretty short - I think the
> > standard for kernel log messges is 72 chars?
> >
> >> size and buffer size. It turns out that buffer
> >> size is always greater due to hardware alignment
> >> requirement. As a result, payload size given to
> >> client is incorrect. This change ensures that
> >> the bytesused is assigned to actual payload size.
> >>
> >> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> >> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> >> index d079aeb..ada1d2f 100644
> >> --- a/drivers/media/platform/qcom/venus/vdec.c
> >> +++ b/drivers/media/platform/qcom/venus/vdec.c
> >> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
> >>
> >>                 vb = &vbuf->vb2_buf;
> >>                 vb->planes[0].bytesused =
> >> -                       max_t(unsigned int, opb_sz, bytesused);
> >> +                       min_t(unsigned int, opb_sz, bytesused);
> >
> > Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
> > Tested-by: Alexandre Courbot <acourbot@chromium.org>
> >
> > This indeed reports the correct size to the client. If bytesused were
> > larger than the size of the buffer we would be having some trouble
> > anyway.
> >
> > Actually in my tree I was using the following patch:
> >
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -924,13 +924,12 @@ static void vdec_buf_done(struct venus_inst
> > *inst, unsigned int buf_type,
> >
> >                vb = &vbuf->vb2_buf;
> >                vb->planes[0].bytesused =
> > -                       max_t(unsigned int, opb_sz, bytesused);
> > +                       min_t(unsigned int, opb_sz, bytesused);
> >                vb->planes[0].data_offset = data_offset;
> >                vb->timestamp = timestamp_us * NSEC_PER_USEC;
> >                vbuf->sequence = inst->sequence_cap++;
> >                if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
> >                        const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
> > -                       vb->planes[0].bytesused = bytesused;
>
> Actually this line doesn't exist in mainline driver. And I don't see a
> reason why to set bytesused twice.

Apologies for being careless - this came from an out-of-tree patch.

>
> >                        v4l2_event_queue_fh(&inst->fh, &ev);
> >
> > Given that we are now taking the minimum of these two values, it seems
> > to me that we don't need to set bytesused again in case we are dealing
> > with the last buffer? Stanimir, what do you think?
> >
>
> --
> regards,
> Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Vikash,

On  2.07.2018 10:44, Vikash Garodia wrote:
> Exisiting code returns the max of the decoded
> size and buffer size. It turns out that buffer
> size is always greater due to hardware alignment
> requirement. As a result, payload size given to
> client is incorrect. This change ensures that
> the bytesused is assigned to actual payload size.
> 
> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>   drivers/media/platform/qcom/venus/vdec.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index d079aeb..ada1d2f 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>   
>   		vb = &vbuf->vb2_buf;
>   		vb->planes[0].bytesused =
> -			max_t(unsigned int, opb_sz, bytesused);
> +			min_t(unsigned int, opb_sz, bytesused);

I cannot remember the exact reason to make it this way, might be an 
issue with vp8 or some mpeg2/4 on v1 which I workaround by this way. 
I'll test the patch on v1 & v3 and come back.

regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Vikash,

On 07/02/2018 10:44 AM, Vikash Garodia wrote:
> Exisiting code returns the max of the decoded
> size and buffer size. It turns out that buffer
> size is always greater due to hardware alignment
> requirement. As a result, payload size given to
> client is incorrect. This change ensures that
> the bytesused is assigned to actual payload size.
> 
> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index d079aeb..ada1d2f 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  
>  		vb = &vbuf->vb2_buf;
>  		vb->planes[0].bytesused =
> -			max_t(unsigned int, opb_sz, bytesused);
> +			min_t(unsigned int, opb_sz, bytesused);

Most probably my intension was to avoid bytesused == 0, but that is
allowed from v4l2 driver -> userspace direction

Could you drop min/max_t macros at all and use bytesused directly i.e.

vb2_set_plane_payload(vb, 0, bytesused)

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Nicolas Dufresne]

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

Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
> Hi Vikash,
> 
> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
> > Exisiting code returns the max of the decoded
> > size and buffer size. It turns out that buffer
> > size is always greater due to hardware alignment
> > requirement. As a result, payload size given to
> > client is incorrect. This change ensures that
> > the bytesused is assigned to actual payload size.
> > 
> > Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> > Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> > ---
> >  drivers/media/platform/qcom/venus/vdec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/qcom/venus/vdec.c
> > b/drivers/media/platform/qcom/venus/vdec.c
> > index d079aeb..ada1d2f 100644
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
> > *inst, unsigned int buf_type,
> >  
> >  		vb = &vbuf->vb2_buf;
> >  		vb->planes[0].bytesused =
> > -			max_t(unsigned int, opb_sz, bytesused);
> > +			min_t(unsigned int, opb_sz, bytesused);
> 
> Most probably my intension was to avoid bytesused == 0, but that is
> allowed from v4l2 driver -> userspace direction

It remains bad practice since it was used by decoders to indicate the
last buffer. Some userspace (some GStreamer versions) will stop working
if you start returning 0.

> 
> Could you drop min/max_t macros at all and use bytesused directly
> i.e.
> 
> vb2_set_plane_payload(vb, 0, bytesused)

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

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi,

On 07/18/2018 04:26 PM, Nicolas Dufresne wrote:
> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
>> Hi Vikash,
>>
>> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
>>> Exisiting code returns the max of the decoded
>>> size and buffer size. It turns out that buffer
>>> size is always greater due to hardware alignment
>>> requirement. As a result, payload size given to
>>> client is incorrect. This change ensures that
>>> the bytesused is assigned to actual payload size.
>>>
>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>> ---
>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>>> b/drivers/media/platform/qcom/venus/vdec.c
>>> index d079aeb..ada1d2f 100644
>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
>>> *inst, unsigned int buf_type,
>>>  
>>>  		vb = &vbuf->vb2_buf;
>>>  		vb->planes[0].bytesused =
>>> -			max_t(unsigned int, opb_sz, bytesused);
>>> +			min_t(unsigned int, opb_sz, bytesused);
>>
>> Most probably my intension was to avoid bytesused == 0, but that is
>> allowed from v4l2 driver -> userspace direction
> 
> It remains bad practice since it was used by decoders to indicate the
> last buffer. Some userspace (some GStreamer versions) will stop working
> if you start returning 0.

I think it is legal v4l2 driver to return bytesused = 0 when userspace
issues streamoff on both queues before EOS, no? Simply because the
capture buffers are empty.

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Hans Verkuil]

On 07/18/2018 04:37 PM, Stanimir Varbanov wrote:
> Hi,
> 
> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote:
>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
>>> Hi Vikash,
>>>
>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
>>>> Exisiting code returns the max of the decoded
>>>> size and buffer size. It turns out that buffer
>>>> size is always greater due to hardware alignment
>>>> requirement. As a result, payload size given to
>>>> client is incorrect. This change ensures that
>>>> the bytesused is assigned to actual payload size.
>>>>
>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>>> ---
>>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>>>> b/drivers/media/platform/qcom/venus/vdec.c
>>>> index d079aeb..ada1d2f 100644
>>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
>>>> *inst, unsigned int buf_type,
>>>>  
>>>>  		vb = &vbuf->vb2_buf;
>>>>  		vb->planes[0].bytesused =
>>>> -			max_t(unsigned int, opb_sz, bytesused);
>>>> +			min_t(unsigned int, opb_sz, bytesused);
>>>
>>> Most probably my intension was to avoid bytesused == 0, but that is
>>> allowed from v4l2 driver -> userspace direction
>>
>> It remains bad practice since it was used by decoders to indicate the
>> last buffer. Some userspace (some GStreamer versions) will stop working
>> if you start returning 0.
> 
> I think it is legal v4l2 driver to return bytesused = 0 when userspace
> issues streamoff on both queues before EOS, no? Simply because the
> capture buffers are empty.
> 

Going through some of the older pending patches I found this one:

So is this patch right or wrong?

It's not clear to me.

Regards,

	Hans

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Hans,

On 09/17/2018 01:00 PM, Hans Verkuil wrote:
> On 07/18/2018 04:37 PM, Stanimir Varbanov wrote:
>> Hi,
>>
>> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote:
>>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
>>>> Hi Vikash,
>>>>
>>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
>>>>> Exisiting code returns the max of the decoded
>>>>> size and buffer size. It turns out that buffer
>>>>> size is always greater due to hardware alignment
>>>>> requirement. As a result, payload size given to
>>>>> client is incorrect. This change ensures that
>>>>> the bytesused is assigned to actual payload size.
>>>>>
>>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>>>> ---
>>>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>>>>> b/drivers/media/platform/qcom/venus/vdec.c
>>>>> index d079aeb..ada1d2f 100644
>>>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
>>>>> *inst, unsigned int buf_type,
>>>>>  
>>>>>  		vb = &vbuf->vb2_buf;
>>>>>  		vb->planes[0].bytesused =
>>>>> -			max_t(unsigned int, opb_sz, bytesused);
>>>>> +			min_t(unsigned int, opb_sz, bytesused);
>>>>
>>>> Most probably my intension was to avoid bytesused == 0, but that is
>>>> allowed from v4l2 driver -> userspace direction
>>>
>>> It remains bad practice since it was used by decoders to indicate the
>>> last buffer. Some userspace (some GStreamer versions) will stop working
>>> if you start returning 0.
>>
>> I think it is legal v4l2 driver to return bytesused = 0 when userspace
>> issues streamoff on both queues before EOS, no? Simply because the
>> capture buffers are empty.
>>
> 
> Going through some of the older pending patches I found this one:
> 
> So is this patch right or wrong?

I'm not sure either, let's not applying it for now (if Nicolas is right
this will break gstreamer plugin).

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Hans Verkuil]

On 09/17/2018 04:30 PM, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 09/17/2018 01:00 PM, Hans Verkuil wrote:
>> On 07/18/2018 04:37 PM, Stanimir Varbanov wrote:
>>> Hi,
>>>
>>> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote:
>>>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
>>>>> Hi Vikash,
>>>>>
>>>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
>>>>>> Exisiting code returns the max of the decoded
>>>>>> size and buffer size. It turns out that buffer
>>>>>> size is always greater due to hardware alignment
>>>>>> requirement. As a result, payload size given to
>>>>>> client is incorrect. This change ensures that
>>>>>> the bytesused is assigned to actual payload size.
>>>>>>
>>>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
>>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>>>>> ---
>>>>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>>>>>> b/drivers/media/platform/qcom/venus/vdec.c
>>>>>> index d079aeb..ada1d2f 100644
>>>>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
>>>>>> *inst, unsigned int buf_type,
>>>>>>  
>>>>>>  		vb = &vbuf->vb2_buf;
>>>>>>  		vb->planes[0].bytesused =
>>>>>> -			max_t(unsigned int, opb_sz, bytesused);
>>>>>> +			min_t(unsigned int, opb_sz, bytesused);
>>>>>
>>>>> Most probably my intension was to avoid bytesused == 0, but that is
>>>>> allowed from v4l2 driver -> userspace direction
>>>>
>>>> It remains bad practice since it was used by decoders to indicate the
>>>> last buffer. Some userspace (some GStreamer versions) will stop working
>>>> if you start returning 0.
>>>
>>> I think it is legal v4l2 driver to return bytesused = 0 when userspace
>>> issues streamoff on both queues before EOS, no? Simply because the
>>> capture buffers are empty.
>>>
>>
>> Going through some of the older pending patches I found this one:
>>
>> So is this patch right or wrong?
> 
> I'm not sure either, let's not applying it for now (if Nicolas is right
> this will break gstreamer plugin).
> 

OK, I marked this as Rejected. If you change your mind it can be reposted :-)

Regards,

	Hans

Re: [PATCH] venus: vdec: fix decoded data size

[Alexandre Courbot]

On Mon, Sep 17, 2018 at 11:33 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 09/17/2018 04:30 PM, Stanimir Varbanov wrote:
> > Hi Hans,
> >
> > On 09/17/2018 01:00 PM, Hans Verkuil wrote:
> >> On 07/18/2018 04:37 PM, Stanimir Varbanov wrote:
> >>> Hi,
> >>>
> >>> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote:
> >>>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
> >>>>> Hi Vikash,
> >>>>>
> >>>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
> >>>>>> Exisiting code returns the max of the decoded
> >>>>>> size and buffer size. It turns out that buffer
> >>>>>> size is always greater due to hardware alignment
> >>>>>> requirement. As a result, payload size given to
> >>>>>> client is incorrect. This change ensures that
> >>>>>> the bytesused is assigned to actual payload size.
> >>>>>>
> >>>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> >>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> >>>>>> ---
> >>>>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
> >>>>>> b/drivers/media/platform/qcom/venus/vdec.c
> >>>>>> index d079aeb..ada1d2f 100644
> >>>>>> --- a/drivers/media/platform/qcom/venus/vdec.c
> >>>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
> >>>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
> >>>>>> *inst, unsigned int buf_type,
> >>>>>>
> >>>>>>                  vb = &vbuf->vb2_buf;
> >>>>>>                  vb->planes[0].bytesused =
> >>>>>> -                        max_t(unsigned int, opb_sz, bytesused);
> >>>>>> +                        min_t(unsigned int, opb_sz, bytesused);
> >>>>>
> >>>>> Most probably my intension was to avoid bytesused == 0, but that is
> >>>>> allowed from v4l2 driver -> userspace direction
> >>>>
> >>>> It remains bad practice since it was used by decoders to indicate the
> >>>> last buffer. Some userspace (some GStreamer versions) will stop working
> >>>> if you start returning 0.
> >>>
> >>> I think it is legal v4l2 driver to return bytesused = 0 when userspace
> >>> issues streamoff on both queues before EOS, no? Simply because the
> >>> capture buffers are empty.
> >>>
> >>
> >> Going through some of the older pending patches I found this one:
> >>
> >> So is this patch right or wrong?
> >
> > I'm not sure either, let's not applying it for now (if Nicolas is right
> > this will break gstreamer plugin).
> >
>
> OK, I marked this as Rejected. If you change your mind it can be reposted :-)

Mmm I'm not saying it has to be done in the current form, but at the
moment the returned bytesused seems to be wrong (at least Chrome is
not happy). We are returning the total size of the buffer instead of
the actually useful payload.

If the intent is to avoid returning bytesused == 0 except for the
special case of the last buffer, how about the following?

--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -943,8 +943,7 @@ static void vdec_buf_done(struct venus_inst *inst,
unsigned int buf_type,
               unsigned int opb_sz = venus_helper_get_opb_size(inst);

               vb = &vbuf->vb2_buf;
-               vb->planes[0].bytesused =
-                       max_t(unsigned int, opb_sz, bytesused);
+                vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
               vb->planes[0].data_offset = data_offset;
               vb->timestamp = timestamp_us * NSEC_PER_USEC;
               vbuf->sequence = inst->sequence_cap++;

It works fine for me, and should not return 0 more often than it did
before (i.e. never). In practice I also never see the firmware
reporting a payload of zero on SDM845, but maybe older chips differ?

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Alex,

On 09/19/2018 01:32 PM, Alexandre Courbot wrote:
> On Mon, Sep 17, 2018 at 11:33 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 09/17/2018 04:30 PM, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> On 09/17/2018 01:00 PM, Hans Verkuil wrote:
>>>> On 07/18/2018 04:37 PM, Stanimir Varbanov wrote:
>>>>> Hi,
>>>>>
>>>>> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote:
>>>>>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
>>>>>>> Hi Vikash,
>>>>>>>
>>>>>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
>>>>>>>> Exisiting code returns the max of the decoded
>>>>>>>> size and buffer size. It turns out that buffer
>>>>>>>> size is always greater due to hardware alignment
>>>>>>>> requirement. As a result, payload size given to
>>>>>>>> client is incorrect. This change ensures that
>>>>>>>> the bytesused is assigned to actual payload size.
>>>>>>>>
>>>>>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
>>>>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>>>>>>> ---
>>>>>>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>> b/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>> index d079aeb..ada1d2f 100644
>>>>>>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
>>>>>>>> *inst, unsigned int buf_type,
>>>>>>>>
>>>>>>>>                  vb = &vbuf->vb2_buf;
>>>>>>>>                  vb->planes[0].bytesused =
>>>>>>>> -                        max_t(unsigned int, opb_sz, bytesused);
>>>>>>>> +                        min_t(unsigned int, opb_sz, bytesused);
>>>>>>>
>>>>>>> Most probably my intension was to avoid bytesused == 0, but that is
>>>>>>> allowed from v4l2 driver -> userspace direction
>>>>>>
>>>>>> It remains bad practice since it was used by decoders to indicate the
>>>>>> last buffer. Some userspace (some GStreamer versions) will stop working
>>>>>> if you start returning 0.
>>>>>
>>>>> I think it is legal v4l2 driver to return bytesused = 0 when userspace
>>>>> issues streamoff on both queues before EOS, no? Simply because the
>>>>> capture buffers are empty.
>>>>>
>>>>
>>>> Going through some of the older pending patches I found this one:
>>>>
>>>> So is this patch right or wrong?
>>>
>>> I'm not sure either, let's not applying it for now (if Nicolas is right
>>> this will break gstreamer plugin).
>>>
>>
>> OK, I marked this as Rejected. If you change your mind it can be reposted :-)
> 
> Mmm I'm not saying it has to be done in the current form, but at the
> moment the returned bytesused seems to be wrong (at least Chrome is
> not happy). We are returning the total size of the buffer instead of
> the actually useful payload.
> 
> If the intent is to avoid returning bytesused == 0 except for the
> special case of the last buffer, how about the following?
> 
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -943,8 +943,7 @@ static void vdec_buf_done(struct venus_inst *inst,
> unsigned int buf_type,
>                unsigned int opb_sz = venus_helper_get_opb_size(inst);
> 
>                vb = &vbuf->vb2_buf;
> -               vb->planes[0].bytesused =
> -                       max_t(unsigned int, opb_sz, bytesused);
> +                vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
>                vb->planes[0].data_offset = data_offset;
>                vb->timestamp = timestamp_us * NSEC_PER_USEC;
>                vbuf->sequence = inst->sequence_cap++;
> 
> It works fine for me, and should not return 0 more often than it did
> before (i.e. never). In practice I also never see the firmware
> reporting a payload of zero on SDM845, but maybe older chips differ?

yes, it looks fine. Let me test it with older versions.

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Nicolas Dufresne]

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

Le mercredi 19 septembre 2018 à 18:02 +0300, Stanimir Varbanov a
écrit :
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -943,8 +943,7 @@ static void vdec_buf_done(struct venus_inst
> > *inst,
> > unsigned int buf_type,
> >                 unsigned int opb_sz =
> > venus_helper_get_opb_size(inst);
> > 
> >                 vb = &vbuf->vb2_buf;
> > -               vb->planes[0].bytesused =
> > -                       max_t(unsigned int, opb_sz, bytesused);
> > +                vb2_set_plane_payload(vb, 0, bytesused ? :
> > opb_sz);
> >                 vb->planes[0].data_offset = data_offset;
> >                 vb->timestamp = timestamp_us * NSEC_PER_USEC;
> >                 vbuf->sequence = inst->sequence_cap++;
> > 
> > It works fine for me, and should not return 0 more often than it
> > did
> > before (i.e. never). In practice I also never see the firmware
> > reporting a payload of zero on SDM845, but maybe older chips
> > differ?
> 
> yes, it looks fine. Let me test it with older versions.

What about removing the allow_zero_bytesused flag on this specific
queue ? Then you can leave it to 0, and the framework will change it to
the buffer size.

Nicolas

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

Re: [PATCH] venus: vdec: fix decoded data size

[Tomasz Figa]

On Thu, Sep 20, 2018 at 12:53 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le mercredi 19 septembre 2018 à 18:02 +0300, Stanimir Varbanov a
> écrit :
> > > --- a/drivers/media/platform/qcom/venus/vdec.c
> > > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > > @@ -943,8 +943,7 @@ static void vdec_buf_done(struct venus_inst
> > > *inst,
> > > unsigned int buf_type,
> > >                 unsigned int opb_sz =
> > > venus_helper_get_opb_size(inst);
> > >
> > >                 vb = &vbuf->vb2_buf;
> > > -               vb->planes[0].bytesused =
> > > -                       max_t(unsigned int, opb_sz, bytesused);
> > > +                vb2_set_plane_payload(vb, 0, bytesused ? :
> > > opb_sz);
> > >                 vb->planes[0].data_offset = data_offset;
> > >                 vb->timestamp = timestamp_us * NSEC_PER_USEC;
> > >                 vbuf->sequence = inst->sequence_cap++;
> > >
> > > It works fine for me, and should not return 0 more often than it
> > > did
> > > before (i.e. never). In practice I also never see the firmware
> > > reporting a payload of zero on SDM845, but maybe older chips
> > > differ?
> >
> > yes, it looks fine. Let me test it with older versions.
>
> What about removing the allow_zero_bytesused flag on this specific
> queue ? Then you can leave it to 0, and the framework will change it to
> the buffer size.

First of all, why we would ever have 0 in bytesused?

That should never happen normally in the middle of decoding and if it
happens, then perhaps such buffer should be returned by the driver
with ERROR state or maybe just silently reused for further decoding.

The only cases where the value of 0 could happen could be EOS or end
of the drain sequence (explicit by STOP command or by resolution
change). In both cases, having 0 bytesused returned from the driver to
vb2 is perfectly fine, because such buffer would have the
V4L2_BUF_FLAG_LAST flag set anyway.

Best regards,
Tomasz

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Nicolas,

On 09/19/2018 06:53 PM, Nicolas Dufresne wrote:
> Le mercredi 19 septembre 2018 à 18:02 +0300, Stanimir Varbanov a
> écrit :
>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>> @@ -943,8 +943,7 @@ static void vdec_buf_done(struct venus_inst
>>> *inst,
>>> unsigned int buf_type,
>>>                 unsigned int opb_sz =
>>> venus_helper_get_opb_size(inst);
>>>
>>>                 vb = &vbuf->vb2_buf;
>>> -               vb->planes[0].bytesused =
>>> -                       max_t(unsigned int, opb_sz, bytesused);
>>> +                vb2_set_plane_payload(vb, 0, bytesused ? :
>>> opb_sz);
>>>                 vb->planes[0].data_offset = data_offset;
>>>                 vb->timestamp = timestamp_us * NSEC_PER_USEC;
>>>                 vbuf->sequence = inst->sequence_cap++;
>>>
>>> It works fine for me, and should not return 0 more often than it
>>> did
>>> before (i.e. never). In practice I also never see the firmware
>>> reporting a payload of zero on SDM845, but maybe older chips
>>> differ?
>>
>> yes, it looks fine. Let me test it with older versions.
> 
> What about removing the allow_zero_bytesused flag on this specific
> queue ? Then you can leave it to 0, and the framework will change it to
> the buffer size.

This is valid only for OUTPUT type buffers, but here we bother for
CAPTURE buffers.

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi,

On 09/19/2018 06:02 PM, Stanimir Varbanov wrote:
> Hi Alex,
> 
> On 09/19/2018 01:32 PM, Alexandre Courbot wrote:
>> On Mon, Sep 17, 2018 at 11:33 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>
>>> On 09/17/2018 04:30 PM, Stanimir Varbanov wrote:
>>>> Hi Hans,
>>>>
>>>> On 09/17/2018 01:00 PM, Hans Verkuil wrote:
>>>>> On 07/18/2018 04:37 PM, Stanimir Varbanov wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 07/18/2018 04:26 PM, Nicolas Dufresne wrote:
>>>>>>> Le mercredi 18 juillet 2018 à 14:31 +0300, Stanimir Varbanov a écrit :
>>>>>>>> Hi Vikash,
>>>>>>>>
>>>>>>>> On 07/02/2018 10:44 AM, Vikash Garodia wrote:
>>>>>>>>> Exisiting code returns the max of the decoded
>>>>>>>>> size and buffer size. It turns out that buffer
>>>>>>>>> size is always greater due to hardware alignment
>>>>>>>>> requirement. As a result, payload size given to
>>>>>>>>> client is incorrect. This change ensures that
>>>>>>>>> the bytesused is assigned to actual payload size.
>>>>>>>>>
>>>>>>>>> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
>>>>>>>>> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
>>>>>>>>> ---
>>>>>>>>>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>>> b/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>>> index d079aeb..ada1d2f 100644
>>>>>>>>> --- a/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>>>>>>>>> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst
>>>>>>>>> *inst, unsigned int buf_type,
>>>>>>>>>
>>>>>>>>>                  vb = &vbuf->vb2_buf;
>>>>>>>>>                  vb->planes[0].bytesused =
>>>>>>>>> -                        max_t(unsigned int, opb_sz, bytesused);
>>>>>>>>> +                        min_t(unsigned int, opb_sz, bytesused);
>>>>>>>>
>>>>>>>> Most probably my intension was to avoid bytesused == 0, but that is
>>>>>>>> allowed from v4l2 driver -> userspace direction
>>>>>>>
>>>>>>> It remains bad practice since it was used by decoders to indicate the
>>>>>>> last buffer. Some userspace (some GStreamer versions) will stop working
>>>>>>> if you start returning 0.
>>>>>>
>>>>>> I think it is legal v4l2 driver to return bytesused = 0 when userspace
>>>>>> issues streamoff on both queues before EOS, no? Simply because the
>>>>>> capture buffers are empty.
>>>>>>
>>>>>
>>>>> Going through some of the older pending patches I found this one:
>>>>>
>>>>> So is this patch right or wrong?
>>>>
>>>> I'm not sure either, let's not applying it for now (if Nicolas is right
>>>> this will break gstreamer plugin).
>>>>
>>>
>>> OK, I marked this as Rejected. If you change your mind it can be reposted :-)
>>
>> Mmm I'm not saying it has to be done in the current form, but at the
>> moment the returned bytesused seems to be wrong (at least Chrome is
>> not happy). We are returning the total size of the buffer instead of
>> the actually useful payload.
>>
>> If the intent is to avoid returning bytesused == 0 except for the
>> special case of the last buffer, how about the following?
>>
>> --- a/drivers/media/platform/qcom/venus/vdec.c
>> +++ b/drivers/media/platform/qcom/venus/vdec.c
>> @@ -943,8 +943,7 @@ static void vdec_buf_done(struct venus_inst *inst,
>> unsigned int buf_type,
>>                unsigned int opb_sz = venus_helper_get_opb_size(inst);
>>
>>                vb = &vbuf->vb2_buf;
>> -               vb->planes[0].bytesused =
>> -                       max_t(unsigned int, opb_sz, bytesused);
>> +                vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
>>                vb->planes[0].data_offset = data_offset;
>>                vb->timestamp = timestamp_us * NSEC_PER_USEC;
>>                vbuf->sequence = inst->sequence_cap++;
>>
>> It works fine for me, and should not return 0 more often than it did
>> before (i.e. never). In practice I also never see the firmware
>> reporting a payload of zero on SDM845, but maybe older chips differ?
> 
> yes, it looks fine. Let me test it with older versions.
> 

OK, I played a bit with vb2_set_plane_payload(vb, 0, bytesused)

On v1 I see a buffer with LAST flag and bytesused == 0 (when
V4L2_DEC_CMD_STOP is used), after that after session_stop (first
streamoff) is called I see the rest of the capture buffers returned with
bytesused == 0.

So I think we can go ahead.

Vikash, can you resend with such a change?

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Vikash Garodia]

Hi Stanimir,

On 2018-10-05 20:56, Stanimir Varbanov wrote:
> Hi Vikash,
> 
> please, increment the version of the patch next time. This one must be 
> v2.
> 
> On 10/03/2018 02:30 PM, Vikash Garodia wrote:
>> Exisiting code returns the max of the decoded size and buffer size.
> 
> s/Exisiting/Existing
> 

Posted v2 with above comments.

Thanks,
Vikash

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Vikash,

please, increment the version of the patch next time. This one must be v2.

On 10/03/2018 02:30 PM, Vikash Garodia wrote:
> Exisiting code returns the max of the decoded size and buffer size.

s/Exisiting/Existing

> It turns out that buffer size is always greater due to hardware
> alignment requirement. As a result, payload size given to client
> is incorrect. This change ensures that the bytesused is assigned
> to actual payload size, when available.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 991e158..189ec97 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -888,8 +888,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  		unsigned int opb_sz = venus_helper_get_opb_size(inst);
>  
>  		vb = &vbuf->vb2_buf;
> -		vb->planes[0].bytesused =
> -			max_t(unsigned int, opb_sz, bytesused);
> +		vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
>  		vb->planes[0].data_offset = data_offset;
>  		vb->timestamp = timestamp_us * NSEC_PER_USEC;
>  		vbuf->sequence = inst->sequence_cap++;
> 

-- 
regards,
Stan

Re: [PATCH] venus: vdec: fix decoded data size

[Stanimir Varbanov]

Hi Vikash,

On 10/03/2018 02:30 PM, Vikash Garodia wrote:
> Exisiting code returns the max of the decoded size and buffer size.
> It turns out that buffer size is always greater due to hardware
> alignment requirement. As a result, payload size given to client
> is incorrect. This change ensures that the bytesused is assigned
> to actual payload size, when available.
> 
> Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/vdec.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index 991e158..189ec97 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -888,8 +888,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>  		unsigned int opb_sz = venus_helper_get_opb_size(inst);
>  
>  		vb = &vbuf->vb2_buf;
> -		vb->planes[0].bytesused =
> -			max_t(unsigned int, opb_sz, bytesused);
> +		vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
>  		vb->planes[0].data_offset = data_offset;
>  		vb->timestamp = timestamp_us * NSEC_PER_USEC;
>  		vbuf->sequence = inst->sequence_cap++;
> 

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

-- 
regards,
Stan

[PATCH] venus: vdec: fix decoded data size

[Vikash Garodia]

Exisiting code returns the max of the decoded size and buffer size.
It turns out that buffer size is always greater due to hardware
alignment requirement. As a result, payload size given to client
is incorrect. This change ensures that the bytesused is assigned
to actual payload size, when available.

Signed-off-by: Vikash Garodia <vgarodia@codeaurora.org>
---
 drivers/media/platform/qcom/venus/vdec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
index 991e158..189ec97 100644
--- a/drivers/media/platform/qcom/venus/vdec.c
+++ b/drivers/media/platform/qcom/venus/vdec.c
@@ -888,8 +888,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
 		unsigned int opb_sz = venus_helper_get_opb_size(inst);
 
 		vb = &vbuf->vb2_buf;
-		vb->planes[0].bytesused =
-			max_t(unsigned int, opb_sz, bytesused);
+		vb2_set_plane_payload(vb, 0, bytesused ? : opb_sz);
 		vb->planes[0].data_offset = data_offset;
 		vb->timestamp = timestamp_us * NSEC_PER_USEC;
 		vbuf->sequence = inst->sequence_cap++;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project