linux-kernel - Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

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] [day] [month] [year] [list]
Date:   Wed, 14 Dec 2016 15:29:00 +0800
From:   Ricky Liang <jcliang@...omium.org>
To:     Rick Chang <rick.chang@...iatek.com>
Cc:     Hans Verkuil <hans.verkuil@...co.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        open list <linux-kernel@...r.kernel.org>,
        linux-media@...r.kernel.org, srv_heupstream@...iatek.com,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-mediatek@...ts.infradead.org>,
        "moderated list:ARM/Mediatek SoC..." 
        <linux-arm-kernel@...ts.infradead.org>,
        "open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>,
        Minghsiu Tsai <minghsiu.tsai@...iatek.com>,
        Bin Liu <bin.liu@...iatek.com>
Subject: Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

Hi Rick,

Can you upload patchset v9 to address the issue? Thanks!

On Mon, Dec 12, 2016 at 5:07 PM, Rick Chang <rick.chang@...iatek.com> wrote:
> Hi Ricky,
>
> Thanks for your feedback. We will fix the problem in another patch.
>
> On Mon, 2016-12-12 at 12:34 +0800, Ricky Liang wrote:
>> Hi Rick,
>>
>> On Wed, Nov 30, 2016 at 11:08 AM, Rick Chang <rick.chang@...iatek.com> wrote:
>> > Add v4l2 driver for Mediatek JPEG Decoder
>> >
>> > Signed-off-by: Rick Chang <rick.chang@...iatek.com>
>> > Signed-off-by: Minghsiu Tsai <minghsiu.tsai@...iatek.com>
>>
>> <snip...>
>>
>> > +static bool mtk_jpeg_check_resolution_change(struct mtk_jpeg_ctx *ctx,
>> > +                                            struct mtk_jpeg_dec_param *param)
>> > +{
>> > +       struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>> > +       struct mtk_jpeg_q_data *q_data;
>> > +
>> > +       q_data = &ctx->out_q;
>> > +       if (q_data->w != param->pic_w || q_data->h != param->pic_h) {
>> > +               v4l2_dbg(1, debug, &jpeg->v4l2_dev, "Picture size change\n");
>> > +               return true;
>> > +       }
>> > +
>> > +       q_data = &ctx->cap_q;
>> > +       if (q_data->fmt != mtk_jpeg_find_format(ctx, param->dst_fourcc,
>> > +                                               MTK_JPEG_FMT_TYPE_CAPTURE)) {
>> > +               v4l2_dbg(1, debug, &jpeg->v4l2_dev, "format change\n");
>> > +               return true;
>> > +       }
>> > +       return false;
>>
>> <snip...>
>>
>> > +static void mtk_jpeg_device_run(void *priv)
>> > +{
>> > +       struct mtk_jpeg_ctx *ctx = priv;
>> > +       struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>> > +       struct vb2_buffer *src_buf, *dst_buf;
>> > +       enum vb2_buffer_state buf_state = VB2_BUF_STATE_ERROR;
>> > +       unsigned long flags;
>> > +       struct mtk_jpeg_src_buf *jpeg_src_buf;
>> > +       struct mtk_jpeg_bs bs;
>> > +       struct mtk_jpeg_fb fb;
>> > +       int i;
>> > +
>> > +       src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>> > +       dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
>> > +       jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(src_buf);
>> > +
>> > +       if (jpeg_src_buf->flags & MTK_JPEG_BUF_FLAGS_LAST_FRAME) {
>> > +               for (i = 0; i < dst_buf->num_planes; i++)
>> > +                       vb2_set_plane_payload(dst_buf, i, 0);
>> > +               buf_state = VB2_BUF_STATE_DONE;
>> > +               goto dec_end;
>> > +       }
>> > +
>> > +       if (mtk_jpeg_check_resolution_change(ctx, &jpeg_src_buf->dec_param)) {
>> > +               mtk_jpeg_queue_src_chg_event(ctx);
>> > +               ctx->state = MTK_JPEG_SOURCE_CHANGE;
>> > +               v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>> > +               return;
>> > +       }
>>
>> This only detects source change if multiple OUPUT buffers are queued.
>> It does not catch the source change in the following scenario:
>>
>> - OUPUT buffers for jpeg1 enqueued
>> - OUTPUT queue STREAMON
>> - userspace creates CAPTURE buffers
>> - CAPTURE buffers enqueued
>> - CAPTURE queue STREAMON
>> - decode
>> - OUTPUT queue STREAMOFF
>> - userspace recreates OUTPUT buffers for jpeg2
>> - OUTPUT buffers for jpeg2 enqueued
>> - OUTPUT queue STREAMON
>>
>> In the above sequence if jpeg2's decoded size is larger than jpeg1 the
>> function fails to detect that the existing CAPTURE buffers are not big
>> enough to hold the decoded data.
>>
>> A possible fix is to pass *dst_buf to
>> mtk_jpeg_check_resolution_change(), and check in the function that all
>> the dst_buf planes are large enough to hold the decoded data.
>>
>> > +
>> > +       mtk_jpeg_set_dec_src(ctx, src_buf, &bs);
>> > +       if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, dst_buf, &fb))
>> > +               goto dec_end;
>> > +
>> > +       spin_lock_irqsave(&jpeg->hw_lock, flags);
>> > +       mtk_jpeg_dec_reset(jpeg->dec_reg_base);
>> > +       mtk_jpeg_dec_set_config(jpeg->dec_reg_base,
>> > +                               &jpeg_src_buf->dec_param, &bs, &fb);
>> > +
>> > +       mtk_jpeg_dec_start(jpeg->dec_reg_base);
>> > +       spin_unlock_irqrestore(&jpeg->hw_lock, flags);
>> > +       return;
>> > +
>> > +dec_end:
>> > +       v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
>> > +       v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
>> > +       v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf), buf_state);
>> > +       v4l2_m2m_buf_done(to_vb2_v4l2_buffer(dst_buf), buf_state);
>> > +       v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>> > +}
>>
>> <snip...>
>
>