linux-kernel - Re: [PATCH] venus: vdec: fix decoded data size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Sep 2018 19:32:46 +0900
From:   Alexandre Courbot <acourbot@...omium.org>
To:     Hans Verkuil <hverkuil@...all.nl>
Cc:     Stanimir Varbanov <stanimir.varbanov@...aro.org>,
        Nicolas Dufresne <nicolas@...fresne.ca>,
        vgarodia@...eaurora.org,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        linux-arm-msm@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] venus: vdec: fix decoded data size

On Mon, Sep 17, 2018 at 11:33 PM Hans Verkuil <hverkuil@...all.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@...eaurora.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?