diff mbox

venus: vdec: fix decoded data size

Message ID 1530517447-29296-1-git-send-email-vgarodia@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Vikash Garodia July 2, 2018, 7:44 a.m. UTC
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(-)

Comments

Stanimir Varbanov July 7, 2018, 12:26 p.m. UTC | #1
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
Stanimir Varbanov July 18, 2018, 11:31 a.m. UTC | #2
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)
Nicolas Dufresne July 18, 2018, 1:26 p.m. UTC | #3
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)
Stanimir Varbanov July 18, 2018, 2:37 p.m. UTC | #4
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.
Hans Verkuil Sept. 17, 2018, 10 a.m. UTC | #5
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
Stanimir Varbanov Sept. 17, 2018, 2:30 p.m. UTC | #6
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).
Hans Verkuil Sept. 17, 2018, 2:32 p.m. UTC | #7
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
Alexandre Courbot Sept. 19, 2018, 10:32 a.m. UTC | #8
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?
Stanimir Varbanov Sept. 19, 2018, 3:02 p.m. UTC | #9
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.
Nicolas Dufresne Sept. 19, 2018, 3:53 p.m. UTC | #10
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
Tomasz Figa Sept. 20, 2018, 3:02 a.m. UTC | #11
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
Stanimir Varbanov Sept. 25, 2018, 9:41 a.m. UTC | #12
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.
Stanimir Varbanov Oct. 2, 2018, 7:38 a.m. UTC | #13
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?
diff mbox

Patch

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++;