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 <[email protected]>
---
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
On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Tested-by: Alexandre Courbot <[email protected]>
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?
Hi Alex,
On 07/02/2018 11:51 AM, Alexandre Courbot wrote:
> On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
> Tested-by: Alexandre Courbot <[email protected]>
>
> 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
On Sat, Jul 7, 2018, 00:12 Stanimir Varbanov
<[email protected]> wrote:
>
> Hi Alex,
>
> On 07/02/2018 11:51 AM, Alexandre Courbot wrote:
> > On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> > Tested-by: Alexandre Courbot <[email protected]>
> >
> > 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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)
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
>>>>> ---
>>>>> 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
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 <[email protected]>
>>>>>> ---
>>>>>> 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
On Mon, Sep 17, 2018 at 11:33 PM Hans Verkuil <[email protected]> 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 <[email protected]>
> >>>>>> ---
> >>>>>> 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?
Hi Alex,
On 09/19/2018 01:32 PM, Alexandre Courbot wrote:
> On Mon, Sep 17, 2018 at 11:33 PM Hans Verkuil <[email protected]> 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 <[email protected]>
>>>>>>>> ---
>>>>>>>> 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
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
On Thu, Sep 20, 2018 at 12:53 AM Nicolas Dufresne <[email protected]> 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
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
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 <[email protected]> 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 <[email protected]>
>>>>>>>>> ---
>>>>>>>>> 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