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

[Alexandre Courbot]

On Sat, Jul 7, 2018, 00:12 Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> wrote:
>
> Hi Alex,
>
> On 07/02/2018 11:51 AM, Alexandre Courbot wrote:
> > On Mon, Jul 2, 2018 at 4:44 PM Vikash Garodia <vgarodia@xxxxxxxxxxxxxx> wrote:
> >>
> >> Exisiting code returns the max of the decoded
> >
> > s/Exisiting/Existing
> >
> > Also the lines of your commit message look pretty short - I think the
> > standard for kernel log messges is 72 chars?
> >
> >> size and buffer size. It turns out that buffer
> >> size is always greater due to hardware alignment
> >> requirement. As a result, payload size given to
> >> client is incorrect. This change ensures that
> >> the bytesused is assigned to actual payload size.
> >>
> >> Change-Id: Ie6f3429c0cb23f682544748d181fa4fa63ca2e28
> >> Signed-off-by: Vikash Garodia <vgarodia@xxxxxxxxxxxxxx>
> >> ---
> >>  drivers/media/platform/qcom/venus/vdec.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> >> index d079aeb..ada1d2f 100644
> >> --- a/drivers/media/platform/qcom/venus/vdec.c
> >> +++ b/drivers/media/platform/qcom/venus/vdec.c
> >> @@ -890,7 +890,7 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
> >>
> >>                 vb = &vbuf->vb2_buf;
> >>                 vb->planes[0].bytesused =
> >> -                       max_t(unsigned int, opb_sz, bytesused);
> >> +                       min_t(unsigned int, opb_sz, bytesused);
> >
> > Reviewed-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
> > Tested-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
> >
> > 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