Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540131 > unrolled thread

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

Started byRicky Liang <jcliang@chromium.org>
First post2016-12-12 05:40 +0100
Last post2016-12-14 08:30 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver Ricky Liang <jcliang@chromium.org> - 2016-12-12 05:40 +0100
    Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder  Driver Rick Chang <rick.chang@mediatek.com> - 2016-12-12 10:10 +0100
      Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver Ricky Liang <jcliang@chromium.org> - 2016-12-14 08:30 +0100

#1540131 — Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

FromRicky Liang <jcliang@chromium.org>
Date2016-12-12 05:40 +0100
SubjectRe: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver
Message-ID<sNqz7-89k-9@gated-at.bofh.it>
Hi Rick,

On Wed, Nov 30, 2016 at 11:08 AM, Rick Chang <rick.chang@mediatek.com> wrote:
> Add v4l2 driver for Mediatek JPEG Decoder
>
> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.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...>

[toc] | [next] | [standalone]

#1540197 — Re: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver

FromRick Chang <rick.chang@mediatek.com>
Date2016-12-12 10:10 +0100
SubjectRe: [PATCH v8 2/4] vcodec: mediatek: Add Mediatek JPEG Decoder Driver
Message-ID<sNuMq-2rs-33@gated-at.bofh.it>
In reply to#1540131
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@mediatek.com> wrote:
> > Add v4l2 driver for Mediatek JPEG Decoder
> >
> > Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> > Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.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...>

[toc] | [prev] | [next] | [standalone]

#1541731

FromRicky Liang <jcliang@chromium.org>
Date2016-12-14 08:30 +0100
Message-ID<sOcaJ-3AW-5@gated-at.bofh.it>
In reply to#1540197
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@mediatek.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@mediatek.com> wrote:
>> > Add v4l2 driver for Mediatek JPEG Decoder
>> >
>> > Signed-off-by: Rick Chang <rick.chang@mediatek.com>
>> > Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.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...>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web