| Message ID | 1530517447-29296-1-git-send-email-vgarodia@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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)
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)
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.
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
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).
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
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?
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.
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 <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
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.
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 --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++;
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(-)