Re: [PATCH] media: mtk-vcodec: Always signal source change event on format change

[李務誠]

Reviewed-by: Wu-Cheng Li 

On Tue, Jan 9, 2018 at 4:42 PM, Tomasz Figa  wrote:
> Currently the driver signals the source change event only in case of
> a midstream resolution change, however the initial format detection
> is also defined as a source change by the V4L2 codec API specification.
> Fix this by signaling the event after the initial header is parsed as
> well.
>
> Signed-off-by: Tomasz Figa 
> ---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c 
> b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> index 843510979ad8..86f0a7134365 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
> @@ -1224,6 +1224,8 @@ static void vb2ops_vdec_buf_queue(struct vb2_buffer *vb)
> ctx->dpb_size = dpbsize;
> ctx->state = MTK_STATE_HEADER;
> mtk_v4l2_debug(1, "[%d] dpbsize=%d", ctx->id, ctx->dpb_size);
> +
> +   mtk_vdec_queue_res_chg_event(ctx);
>  }
>
>  static void vb2ops_vdec_buf_finish(struct vb2_buffer *vb)
> --
> 2.16.0.rc0.223.g4a4ac83678-goog
>

Re: [PATCH] [media] mtk-vcodec: fix vp9 decode error

[李務誠]

Reviewed-by: Wu-Cheng Li 
Tested-by: Wu-Cheng Li 

On Wed, Jul 19, 2017 at 5:22 PM, Tiffany Lin  wrote:
> Fix The camera has a blurry screen phenomenon when
> we video chat with apprtc using vp9 codec
>
> Signed-off-by: Tiffany Lin 
> ---
>  .../media/platform/mtk-vcodec/vdec/vdec_vp9_if.c   | 37 
> --
>  1 file changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c 
> b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
> index 1daee12..bc8349b 100644
> --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
> +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
> @@ -31,6 +31,7 @@
>  #define MAX_NUM_REF_FRAMES 8
>  #define VP9_MAX_FRM_BUF_NUM 9
>  #define VP9_MAX_FRM_BUF_NODE_NUM (VP9_MAX_FRM_BUF_NUM * 2)
> +#define VP9_SEG_ID_SZ 0x12000
>
>  /**
>   * struct vp9_dram_buf - contains buffer info for vpu
> @@ -132,6 +133,7 @@ struct vp9_sf_ref_fb {
>   * @frm_num : decoded frame number, include sub-frame count (AP-R, VPU-W)
>   * @mv_buf : motion vector working buffer (AP-W, VPU-R)
>   * @frm_refs : maintain three reference buffer info (AP-R/W, VPU-R/W)
> + * @seg_id_buf : segmentation map working buffer (AP-W, VPU-R)
>   */
>  struct vdec_vp9_vsi {
> unsigned char sf_bs_buf[VP9_SUPER_FRAME_BS_SZ];
> @@ -167,11 +169,14 @@ struct vdec_vp9_vsi {
> struct vp9_dram_buf mv_buf;
>
> struct vp9_ref_buf frm_refs[REFS_PER_FRAME];
> +   struct vp9_dram_buf seg_id_buf;
> +
>  };
>
>  /*
>   * struct vdec_vp9_inst - vp9 decode instance
>   * @mv_buf : working buffer for mv
> + * @seg_id_buf : working buffer for segmentation map
>   * @dec_fb : vdec_fb node to link fb to different fb_xxx_list
>   * @available_fb_node_list : current available vdec_fb node
>   * @fb_use_list : current used or referenced vdec_fb
> @@ -187,6 +192,7 @@ struct vdec_vp9_vsi {
>   */
>  struct vdec_vp9_inst {
> struct mtk_vcodec_mem mv_buf;
> +   struct mtk_vcodec_mem seg_id_buf;
>
> struct vdec_fb_node dec_fb[VP9_MAX_FRM_BUF_NODE_NUM];
> struct list_head available_fb_node_list;
> @@ -388,13 +394,11 @@ static bool vp9_alloc_work_buf(struct vdec_vp9_inst 
> *inst)
> vsi->buf_h);
>
> mem = >mv_buf;
> -
> if (mem->va)
> mtk_vcodec_mem_free(inst->ctx, mem);
>
> mem->size = ((vsi->buf_w / 64) *
> (vsi->buf_h / 64) + 2) * 36 * 16;
> -
> result = mtk_vcodec_mem_alloc(inst->ctx, mem);
> if (result) {
> mem->size = 0;
> @@ -406,6 +410,24 @@ static bool vp9_alloc_work_buf(struct vdec_vp9_inst 
> *inst)
> vsi->mv_buf.pa = (unsigned long)mem->dma_addr;
> vsi->mv_buf.sz = (unsigned int)mem->size;
>
> +
> +   mem = >seg_id_buf;
> +   if (mem->va)
> +   mtk_vcodec_mem_free(inst->ctx, mem);
> +
> +   mem->size = VP9_SEG_ID_SZ;
> +   result = mtk_vcodec_mem_alloc(inst->ctx, mem);
> +   if (result) {
> +   mem->size = 0;
> +   mtk_vcodec_err(inst, "Cannot allocate seg_id_buf");
> +   return false;
> +   }
> +   /* Set the va again */
> +   vsi->seg_id_buf.va = (unsigned long)mem->va;
> +   vsi->seg_id_buf.pa = (unsigned long)mem->dma_addr;
> +   vsi->seg_id_buf.sz = (unsigned int)mem->size;
> +
> +
> vp9_free_all_sf_ref_fb(inst);
> vsi->sf_next_ref_fb_idx = vp9_get_sf_ref_fb(inst);
>
> @@ -653,6 +675,12 @@ static void vp9_reset(struct vdec_vp9_inst *inst)
> inst->vsi->mv_buf.va = (unsigned long)inst->mv_buf.va;
> inst->vsi->mv_buf.pa = (unsigned long)inst->mv_buf.dma_addr;
> inst->vsi->mv_buf.sz = (unsigned long)inst->mv_buf.size;
> +
> +   /* Set the va again, since vpu_dec_reset will clear seg_id_buf in vpu 
> */
> +   inst->vsi->seg_id_buf.va = (unsigned long)inst->seg_id_buf.va;
> +   inst->vsi->seg_id_buf.pa = (unsigned long)inst->seg_id_buf.dma_addr;
> +   inst->vsi->seg_id_buf.sz = (unsigned long)inst->seg_id_buf.size;
> +
>  }
>
>  static void init_all_fb_lists(struct vdec_vp9_inst *inst)
> @@ -752,6 +780,10 @@ static void vdec_vp9_deinit(unsigned long h_vdec)
> if (mem->va)
> mtk_vcodec_mem_free(inst->ctx, mem);
>
> +   mem = >seg_id_buf;
> +   if (mem->va)
> +   mtk_vcodec_mem_free(inst->ctx, mem);
> +
> vp9_free_all_sf_ref_fb(inst);
> vp9_free_inst(inst);
>  }
> @@ -848,6 +880,7 @@ static int vdec_vp9_decode(unsigned long h_vdec, struct 
> mtk_vcodec_mem *bs,
> vsi->sf_frm_sz[idx]);
> }
> }
> +   memset(inst->seg_id_buf.va, 0, inst->seg_id_buf.size);
> ret = vpu_dec_start(>vpu, data, 3);
> if (ret) {
> mtk_vcodec_err(inst, 

Re: [PATCH] [media] mtk-mdp: Fix g_/s_selection capture/compose logic

[李務誠]

Reviewed-by: Wu-Cheng Li 

On Thu, Apr 13, 2017 at 12:18 PM, Minghsiu Tsai
 wrote:
> From: Daniel Kurtz 
>
> Experiments show that the:
>  (1) mtk-mdp uses the _MPLANE form of CAPTURE/OUTPUT
>  (2) CAPTURE types use CROP targets, and OUTPUT types use COMPOSE targets
>
> Signed-off-by: Daniel Kurtz 
> Signed-off-by: Minghsiu Tsai 
>
> ---
>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 18 +-
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c 
> b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 13afe48..8ab7ca0 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -837,12 +837,12 @@ static int mtk_mdp_m2m_g_selection(struct file *file, 
> void *fh,
> struct mtk_mdp_ctx *ctx = fh_to_ctx(fh);
> bool valid = false;
>
> -   if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> -   if (mtk_mdp_is_target_compose(s->target))
> -   valid = true;
> -   } else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +   if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> if (mtk_mdp_is_target_crop(s->target))
> valid = true;
> +   } else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +   if (mtk_mdp_is_target_compose(s->target))
> +   valid = true;
> }
> if (!valid) {
> mtk_mdp_dbg(1, "[%d] invalid type:%d,%u", ctx->id, s->type,
> @@ -907,12 +907,12 @@ static int mtk_mdp_m2m_s_selection(struct file *file, 
> void *fh,
> int ret;
> bool valid = false;
>
> -   if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> -   if (s->target == V4L2_SEL_TGT_COMPOSE)
> -   valid = true;
> -   } else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT) {
> +   if (s->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> if (s->target == V4L2_SEL_TGT_CROP)
> valid = true;
> +   } else if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +   if (s->target == V4L2_SEL_TGT_COMPOSE)
> +   valid = true;
> }
> if (!valid) {
> mtk_mdp_dbg(1, "[%d] invalid type:%d,%u", ctx->id, s->type,
> @@ -925,7 +925,7 @@ static int mtk_mdp_m2m_s_selection(struct file *file, 
> void *fh,
> if (ret)
> return ret;
>
> -   if (mtk_mdp_is_target_crop(s->target))
> +   if (mtk_mdp_is_target_compose(s->target))
> frame = >s_frame;
> else
> frame = >d_frame;
> --
> 1.9.1
>

Re: [PATCH] media: mtk-jpeg: fix continuous log "Context is NULL"

[李務誠]

On Tue, Mar 14, 2017 at 10:21 PM, Minghsiu Tsai
 wrote:
> The symptom is continuous log "mtk-jpeg 18004000.jpegdec: Context is NULL"
> in kernel log. It is becauese the error handling in irq doesn't clear
> interrupt.
>
> The calling flow like as below when issue happen
> mtk_jpeg_device_run()
> mtk_jpeg_job_abort()
>   v4l2_m2m_job_finish() -> m2m_dev->curr_ctx = NULL;
> mtk_jpeg_dec_irq()
>   v4l2_m2m_get_curr_priv()
>  -> m2m_dev->curr_ctx == NULL
>  -> return NULL
> log "Context is NULL"
>
> There is race condition between job_abort() and irq. In order to simplify
> code, don't want to add extra flag to maintain state, empty job_abort() and
> clear interrupt before v4l2_m2m_get_curr_priv() in irq.
>
> Signed-off-by: Minghsiu Tsai 
Reviewed-by: Wu-Cheng Li 
Tested-by: Wu-Cheng Li 
> ---
>  drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 14 ++
>  1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c 
> b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> index b10183f..c02bc7f 100644
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> @@ -862,15 +862,6 @@ static int mtk_jpeg_job_ready(void *priv)
>
>  static void mtk_jpeg_job_abort(void *priv)
>  {
> -   struct mtk_jpeg_ctx *ctx = priv;
> -   struct mtk_jpeg_dev *jpeg = ctx->jpeg;
> -   struct vb2_buffer *src_buf, *dst_buf;
> -
> -   src_buf = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx);
> -   dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> -   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf), VB2_BUF_STATE_ERROR);
> -   v4l2_m2m_buf_done(to_vb2_v4l2_buffer(dst_buf), VB2_BUF_STATE_ERROR);
> -   v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
>  }
>
>  static struct v4l2_m2m_ops mtk_jpeg_m2m_ops = {
> @@ -941,6 +932,8 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
> u32 dec_ret;
> int i;
>
> +   dec_ret = mtk_jpeg_dec_get_int_status(jpeg->dec_reg_base);
> +   dec_irq_ret = mtk_jpeg_dec_enum_result(dec_ret);
> ctx = v4l2_m2m_get_curr_priv(jpeg->m2m_dev);
> if (!ctx) {
> v4l2_err(>v4l2_dev, "Context is NULL\n");
> @@ -951,9 +944,6 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
> dst_buf = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(src_buf);
>
> -   dec_ret = mtk_jpeg_dec_get_int_status(jpeg->dec_reg_base);
> -   dec_irq_ret = mtk_jpeg_dec_enum_result(dec_ret);
> -
> if (dec_irq_ret >= MTK_JPEG_DEC_RESULT_UNDERFLOW)
> mtk_jpeg_dec_reset(jpeg->dec_reg_base);
>
> --
> 1.9.1
>

Re: [PATCH 1/1] mtk-vcodec: check the vp9 decoder buffer index from VPU.

[李務誠]

On Tue, Mar 7, 2017 at 3:59 PM, Tiffany Lin  wrote:
> On Tue, 2017-03-07 at 14:03 +0800, Wu-Cheng Li wrote:
>> From: Wu-Cheng Li 
>>
>> VPU firmware has a bug and may return invalid buffer index for
>> some vp9 videos. Check the buffer indexes before accessing the
>> buffer.
>>
>> Signed-off-by: Wu-Cheng Li 
>> ---
>>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c |  6 +
>>  .../media/platform/mtk-vcodec/vdec/vdec_vp9_if.c   | 26 
>> ++
>>  drivers/media/platform/mtk-vcodec/vdec_drv_if.h|  2 ++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c 
>> b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
>> index 502877a4b1df..7ebcf9e57ac7 100644
>> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
>> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c
>> @@ -1176,6 +1176,12 @@ static void vb2ops_vdec_buf_queue(struct vb2_buffer 
>> *vb)
>>  "[%d] vdec_if_decode() src_buf=%d, size=%zu, 
>> fail=%d, res_chg=%d",
>>  ctx->id, src_buf->index,
>>  src_mem.size, ret, res_chg);
>> +
>> + if (ret == -EIO) {
>> + mtk_v4l2_err("[%d] Unrecoverable error in 
>> vdec_if_decode.",
>> + ctx->id);
>> + ctx->state = MTK_STATE_ABORT;
>> + }
> Could we use v4l2_m2m_buf_done(to_vb2_v4l2_buffer(src_buf),
> VB2_BUF_STATE_ERROR); instead ctx->state = MTK_STATE_ABORT;
> In this case, the behavior will be same as vdec_if_decode called in
> mtk_vdec_worker.
If we use VB2_BUF_STATE_ERROR, dqbuf will return V4L2_BUF_FLAG_ERROR.
It means a recoverable error.

"The driver may also set V4L2_BUF_FLAG_ERROR in the flags field. It indicates
a non-critical (recoverable) streaming error. In such case the application may
continue as normal, but should be aware that data in the dequeued buffer might
be corrupted."
https://static.lwn.net/kerneldoc/media/uapi/v4l/vidioc-qbuf.html
> And we could also get information about what output buffer make vpu
> crash.
>
> best regards,
> Tiffany
>>   return;
>>   }
>>
>> diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c 
>> b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
>> index e91a3b425b0c..5539b1853f16 100644
>> --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
>> +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c
>> @@ -718,6 +718,26 @@ static void get_free_fb(struct vdec_vp9_inst *inst, 
>> struct vdec_fb **out_fb)
>>   *out_fb = fb;
>>  }
>>
>> +static int validate_vsi_array_indexes(struct vdec_vp9_inst *inst,
>> + struct vdec_vp9_vsi *vsi) {
>> + if (vsi->sf_frm_idx >= VP9_MAX_FRM_BUF_NUM - 1) {
>> + mtk_vcodec_err(inst, "Invalid vsi->sf_frm_idx=%u.",
>> + vsi->sf_frm_idx);
>> + return -EIO;
>> + }
>> + if (vsi->frm_to_show_idx >= VP9_MAX_FRM_BUF_NUM) {
>> + mtk_vcodec_err(inst, "Invalid vsi->frm_to_show_idx=%u.",
>> + vsi->frm_to_show_idx);
>> + return -EIO;
>> + }
>> + if (vsi->new_fb_idx >= VP9_MAX_FRM_BUF_NUM) {
>> + mtk_vcodec_err(inst, "Invalid vsi->new_fb_idx=%u.",
>> + vsi->new_fb_idx);
>> + return -EIO;
>> + }
>> + return 0;
>> +}
>> +
>>  static void vdec_vp9_deinit(unsigned long h_vdec)
>>  {
>>   struct vdec_vp9_inst *inst = (struct vdec_vp9_inst *)h_vdec;
>> @@ -834,6 +854,12 @@ static int vdec_vp9_decode(unsigned long h_vdec, struct 
>> mtk_vcodec_mem *bs,
>>   goto DECODE_ERROR;
>>   }
>>
>> + ret = validate_vsi_array_indexes(inst, vsi);
>> + if (ret) {
>> + mtk_vcodec_err(inst, "Invalid values from VPU.");
>> + goto DECODE_ERROR;
>> + }
>> +
>>   if (vsi->resolution_changed) {
>>   if (!vp9_alloc_work_buf(inst)) {
>>   ret = -EINVAL;
>> diff --git a/drivers/media/platform/mtk-vcodec/vdec_drv_if.h 
>> b/drivers/media/platform/mtk-vcodec/vdec_drv_if.h
>> index db6b5205ffb1..ded1154481cd 100644
>> --- a/drivers/media/platform/mtk-vcodec/vdec_drv_if.h
>> +++ b/drivers/media/platform/mtk-vcodec/vdec_drv_if.h
>> @@ -85,6 +85,8 @@ void vdec_if_deinit(struct mtk_vcodec_ctx *ctx);
>>   * @res_chg  : [out] resolution change happens if current bs have different
>>   *   picture width/height
>>   * Note: To flush the decoder when reaching EOF, set input bitstream as 
>> NULL.
>> + *
>> + * Return: 0 on success. -EIO on unrecoverable error.
>>   */
>>  int vdec_if_decode(struct mtk_vcodec_ctx *ctx, struct mtk_vcodec_mem *bs,
>>  struct vdec_fb *fb, bool *res_chg);
>
>

Re: V4L2_DEC_CMD_STOP and last_buffer_dequeued

[李務誠]

We found videobuf2-core.h has a function
vb2_clear_last_buffer_dequeued to clear last_buffer_dequeued. We call
vb2_clear_last_buffer_dequeued in the driver when it gets CMD_START.
Everything works now.

On Mon, Oct 17, 2016 at 9:46 PM, Nicolas Dufresne <nico...@ndufresne.ca> wrote:
> Le samedi 15 octobre 2016 à 08:16 +0800, Wu-Cheng Li (李務誠) a écrit :
>> last_buffer_dequeued is only cleared to false when CAPTURE queue is
>> STREAMOFF (#1). Queuing a header to OUTPUT queue won't clear
>> last_buffer_dequeued of CAPTURE queue. It looks to me that v4l2 core
>> needs to intercept CMD_START and clear last_buffer_dequeued. What do
>> you think?
We found
>>
>> http://lxr.free-electrons.com/source/drivers/media/v4l2-core/videobuf2-core.c#L1951
>
> That sounds reasonable, assuming it does not break drivers.
>
>> >
>> >
>> > Note that for many a flush is the action of getting rid of the pending
>> > images and achieve by using STREAMOFF. While the effect of CMD_STOP is
>> > to signal the decoder that no more encoded image will be queued, hence
>> > remaining images should be delivered to userspace. They will
>> > differentiate as a flush operation vs as drain operation. This is no
>> > rocket science of course.
>>
>> I see. What I want is drain operation. In Chromium terms, CMD_STOP
>> maps to flush and STREAMOFF maps to reset.
>
> Yes, that's the reason I was mentioning. This was a great source of
> confusion during a workshop with some Google/Chromium folks.
>
> A question on top of this, what are the use cases for you to drain
> without flushing afteward ? Is it really needed ?
>
> regards,
> Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: V4L2_DEC_CMD_STOP and last_buffer_dequeued

[李務誠]

On Sat, Oct 15, 2016 at 2:20 AM, Nicolas Dufresne
<nicolas.dufre...@gmail.com> wrote:
>
> Le mercredi 12 octobre 2016 à 23:33 +0800, Wu-Cheng Li (李務誠) a écrit :
> > I'm trying to use V4L2_DEC_CMD_STOP to implement flush. First the
> > userspace sent V4L2_DEC_CMD_STOP to initiate the flush. The driver
> > set
> > V4L2_BUF_FLAG_LAST on the last CAPTURE buffer. I thought implementing
> > V4L2_DEC_CMD_START in the driver was enough to start the decoder. But
> > last_buffer_dequeued had been set to true in v4l2 core. I couldn't
> > clear last_buffer_dequeued without calling STREAMOFF from the
> > userspace. If I need to call STREAMOFF/STREAMON after
> > V4L2_DEC_CMD_STOP, it looks like V4L2_DEC_CMD_START is not useful.
> > Did
> > I miss anything?
>
> It's likely what the driver do is slightly off what the spec say. All
> user space code so far seems to only drain at EOS. As the next buffer
> is a new stream, it make sense to completely reset the encoder. We'd
> need to review that, but using CMD_START should work if you queue a
> header first.

last_buffer_dequeued is only cleared to false when CAPTURE queue is
STREAMOFF (#1). Queuing a header to OUTPUT queue won't clear
last_buffer_dequeued of CAPTURE queue. It looks to me that v4l2 core
needs to intercept CMD_START and clear last_buffer_dequeued. What do
you think?

http://lxr.free-electrons.com/source/drivers/media/v4l2-core/videobuf2-core.c#L1951
>
>
> Note that for many a flush is the action of getting rid of the pending
> images and achieve by using STREAMOFF. While the effect of CMD_STOP is
> to signal the decoder that no more encoded image will be queued, hence
> remaining images should be delivered to userspace. They will
> differentiate as a flush operation vs as drain operation. This is no
> rocket science of course.

I see. What I want is drain operation. In Chromium terms, CMD_STOP
maps to flush and STREAMOFF maps to reset.
>
>
> regards,
> Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

V4L2_DEC_CMD_STOP and last_buffer_dequeued

[李務誠]

Hi,
I'm trying to use V4L2_DEC_CMD_STOP to implement flush. First the
userspace sent V4L2_DEC_CMD_STOP to initiate the flush. The driver set
V4L2_BUF_FLAG_LAST on the last CAPTURE buffer. I thought implementing
V4L2_DEC_CMD_START in the driver was enough to start the decoder. But
last_buffer_dequeued had been set to true in v4l2 core. I couldn't
clear last_buffer_dequeued without calling STREAMOFF from the
userspace. If I need to call STREAMOFF/STREAMON after
V4L2_DEC_CMD_STOP, it looks like V4L2_DEC_CMD_START is not useful. Did
I miss anything?

Regards,
Wu-Cheng
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/9] DocBook/v4l: Add compressed video formats used on MT8173 codec driver

[李務誠]

On Wed, Jul 13, 2016 at 3:14 AM, Nicolas Dufresne
<nicolas.dufre...@gmail.com> wrote:
> Le mardi 12 juillet 2016 à 15:08 -0400, Nicolas Dufresne a écrit :
>> Le mardi 12 juillet 2016 à 16:16 +0800, Wu-Cheng Li (李務誠) a écrit :
>> > Decoder hardware produces MT21 (compressed). Image processor can
>> > convert it to a format that can be input of display driver.
>> > Tiffany.
>> > When do you plan to upstream image processor (mtk-mdp)?
>> > >
>> > > It can be as input format for encoder, MDP and display drivers in
>> > our
>> > > platform.
>> > I remember display driver can only accept uncompressed MT21. Right?
>> > Basically V4L2_PIX_FMT_MT21 is compressed and is like an opaque
>> > format. It's not usable until it's decompressed and converted by
>> > image
>> > processor.
>>
>> Previously it was described as MediaTek block mode, and now as a
>> MediaTek compressed format. It makes me think you have no idea what
>> this pixel format really is. Is that right ?
>>
>> The main reason why I keep asking, is that we often find similarities
>> between what vendor like to call their proprietary formats. Doing the
>> proper research helps not creating a mess like in Android where you
>> have a lot of formats that all point to the same format. I believe
>> there was the same concern when Samsung wanted to introduce their Z-
>> flip-Z NV12 tile format. In the end they simply provided sufficient
>> documentation so we could document it and implement software
>> converters
>> for test and validation purpose.
>
> Here's the kind of information we want in the documentation.
>
> https://chromium.googlesource.com/chromium/src/media/+/master/base/vide
> o_types.h#40
That is the documentation of decompressed MT21. Originally MT21 was meant
to be a YUV format and we can map it in CPU to use it. The name was changed
to mean a compressed format. The current design is only MTK image processor
can convert it. Software cannot decompress it. I'm not sure if we
should document
the format inside if we cannot decompress in software. For chromium, I'll update
the code to explain MT21 is an opaque compressed format.
>
>   // MediaTek proprietary format. MT21 is similar to NV21 except the memory
>   // layout and pixel layout (swizzles). 12bpp with Y plane followed by a 2x2
>   // interleaved VU plane. Each image contains two buffers -- Y plane and VU
>   // plane. Two planes can be non-contiguous in memory. The starting addresses
>   // of Y plane and VU plane are 4KB alignment.
>   // Suppose image dimension is (width, height). For both Y plane and VU 
> plane:
>   // Row pitch = ((width+15)/16) * 16.
>   // Plane size = Row pitch * (((height+31)/32)*32)
>
> Now obviously this is incomplete, as the swizzling need to be documented of 
> course.
>
>>
>> regards,
>> Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/9] DocBook/v4l: Add compressed video formats used on MT8173 codec driver

[李務誠]

On Mon, Jul 11, 2016 at 10:56 AM, tiffany lin  wrote:
> Hi Hans,
>
> On Fri, 2016-07-08 at 12:23 +0200, Hans Verkuil wrote:
>> On 05/30/2016 02:29 PM, Tiffany Lin wrote:
>> > Add V4L2_PIX_FMT_MT21 documentation
>> >
>> > Signed-off-by: Tiffany Lin 
>> > ---
>> >  Documentation/DocBook/media/v4l/pixfmt.xml |6 ++
>> >  1 file changed, 6 insertions(+)
>> >
>> > diff --git a/Documentation/DocBook/media/v4l/pixfmt.xml 
>> > b/Documentation/DocBook/media/v4l/pixfmt.xml
>> > index 5a08aee..d40e0ce 100644
>> > --- a/Documentation/DocBook/media/v4l/pixfmt.xml
>> > +++ b/Documentation/DocBook/media/v4l/pixfmt.xml
>> > @@ -1980,6 +1980,12 @@ array. Anything what's in between the UYVY lines is 
>> > JPEG data and should be
>> >  concatenated to form the JPEG stream. 
>> >  
>> >   
>> > + 
>> > +   V4L2_PIX_FMT_MT21
>> > +   'MT21'
>> > +   Compressed two-planar YVU420 format used by Mediatek MT8173
>> > +   codec driver.
>>
>> Can you give a few more details? The encoder driver doesn't seem to produce 
>> this
>> format, so who is creating this? Where is this format documented?
Decoder hardware produces MT21 (compressed). Image processor can
convert it to a format that can be input of display driver. Tiffany.
When do you plan to upstream image processor (mtk-mdp)?
>
> It can be as input format for encoder, MDP and display drivers in our
> platform.
I remember display driver can only accept uncompressed MT21. Right?
Basically V4L2_PIX_FMT_MT21 is compressed and is like an opaque
format. It's not usable until it's decompressed and converted by image
processor.
> This private format is only available in our platform.
> So I put it in "Reserved Format Identifiers" sections.
>
>
> best regards,
> Tiffany
>
>> Regards,
>>
>>   Hans
>>
>> > + 
>> > 
>> >
>> >  
>> >
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver

[李務誠]

+Andrew. Last I heard the license was ready and Andrew was preparing
the VPU firmware. Andrew. Is the firmware ready to submit?

On Fri, Jul 1, 2016 at 6:11 PM, Hans Verkuil  wrote:
> On 06/16/2016 12:54 PM, Mauro Carvalho Chehab wrote:
>> Em Tue, 14 Jun 2016 19:08:08 +0800
>> tiffany lin  escreveu:
>>
>>> Hi Mauro,
>>>
>>>
>>> On Wed, 2016-06-08 at 07:13 +0900, Hans Verkuil wrote:

 On 06/07/2016 11:22 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 30 May 2016 20:29:14 +0800
> Tiffany Lin  escreveu:
>
>> ==
>>   Introduction
>> ==
>>
>> The purpose of this series is to add the driver for video codec hw 
>> embedded in the Mediatek's MT8173 SoCs.
>> Mediatek Video Codec is able to handle video decoding of in a range of 
>> formats.
>>
>> This patch series add Mediatek block format V4L2_PIX_FMT_MT21, the 
>> decoder driver will decoded bitstream to
>> V4L2_PIX_FMT_MT21 format.
>>
>> This patch series rely on MTK VPU driver in patch series "Add MT8173 
>> Video Encoder Driver and VPU Driver"[1]
>> and patch "CHROMIUM: v4l: Add V4L2_PIX_FMT_VP9 definition"[2] for VP9 
>> support.
>> Mediatek Video Decoder driver rely on VPU driver to load, communicate 
>> with VPU.
>>
>> Internally the driver uses videobuf2 framework and MTK IOMMU and MTK SMI 
>> both have been merged in v4.6-rc1.
>>
>> [1]https://patchwork.linuxtv.org/patch/33734/
>> [2]https://chromium-review.googlesource.com/#/c/245241/
>
> Hmm... I'm not seeing the firmware for this driver at the
> linux-firmware tree:
>
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/log/
>
> Nor I'm seeing any pull request for them. Did you send it?
> I'll only merge the driver upstream after seeing such pull request.

>>> Sorry, I am not familiar with how to upstream firmware.
>>> Do you mean we need to upstream vpu firmware first before merge encoder
>>> driver upstream?
>>
>> Please look at this page:
>>   
>> https://linuxtv.org/wiki/index.php/Development:_How_to_submit_patches#Firmware_submission
>>
>> The information here can also be useful:
>>   https://www.kernel.org/doc/readme/firmware-README.AddingFirmware
>>
>> In summary, you need to provide redistribution rights for the
>> firmware blob. You can either submit it to me or directly to
>> linux-firmware. In the latter, please c/c me on such patch.
>
> Tiffany, what is the status of the firmware submission?
>
> Regards,
>
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/9] Add MT8173 Video Decoder Driver

[李務誠]

On Wed, Jun 8, 2016 at 6:13 AM, Hans Verkuil  wrote:
>
>
> On 06/07/2016 11:22 PM, Mauro Carvalho Chehab wrote:
>>
>> Em Mon, 30 May 2016 20:29:14 +0800
>> Tiffany Lin  escreveu:
>>
>>> ==
>>>   Introduction
>>> ==
>>>
>>> The purpose of this series is to add the driver for video codec hw
>>> embedded in the Mediatek's MT8173 SoCs.
>>> Mediatek Video Codec is able to handle video decoding of in a range of
>>> formats.
>>>
>>> This patch series add Mediatek block format V4L2_PIX_FMT_MT21, the
>>> decoder driver will decoded bitstream to
>>> V4L2_PIX_FMT_MT21 format.
>>>
>>> This patch series rely on MTK VPU driver in patch series "Add MT8173
>>> Video Encoder Driver and VPU Driver"[1]
>>> and patch "CHROMIUM: v4l: Add V4L2_PIX_FMT_VP9 definition"[2] for VP9
>>> support.
>>> Mediatek Video Decoder driver rely on VPU driver to load, communicate
>>> with VPU.
>>>
>>> Internally the driver uses videobuf2 framework and MTK IOMMU and MTK SMI
>>> both have been merged in v4.6-rc1.
>>>
>>> [1]https://patchwork.linuxtv.org/patch/33734/
>>> [2]https://chromium-review.googlesource.com/#/c/245241/
>>
>>
>> Hmm... I'm not seeing the firmware for this driver at the
>> linux-firmware tree:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/log/
Tiffany. Can you check the license and add the firmware to linux-firmware?

For the information, both encoder and decoder drivers require the
firmware to work.
>>
>> Nor I'm seeing any pull request for them. Did you send it?
>> I'll only merge the driver upstream after seeing such pull request.
>
>
> Mauro, are you confusing the decoder and encoder driver? I haven't
> thoroughly reviewed the decoder driver
> yet, so there is no pull request for the decoder driver.
>
> The only pull request I made was for the encoder driver.
>
> Regards,
>
> Hans
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 5/8] [media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver

[李務誠]

> >
> > ...
> >
> > > +static int fops_vcodec_open(struct file *file)
> > > +{
> > > +   struct video_device *vfd = video_devdata(file);
> > > +   struct mtk_vcodec_dev *dev = video_drvdata(file);
> > > +   struct mtk_vcodec_ctx *ctx = NULL;
> > > +   int ret = 0;
> > > +
> > > +   if (dev->instance_mask == ~0UL) {
> > > +   /* ffz Undefined if no zero exists, err handling here */
> > > +   mtk_v4l2_err("Too many open contexts");
> > > +   ret = -EBUSY;
> > > +   goto err_alloc;
> >
> > I'm not happy seeing this here. You should always be able to open the 
> > device.
> > I would expect to see a check like this in e.g. start_streaming, since 
> > that's
> > where you start to use the hardware for real, and checking if you have 
> > enough
> > resources there is perfectly fine.
> >
> > If this is an artificial constraint (i.e. not based on a real hardware 
> > limitation),
> > then it should perhaps just be dropped. Such constraints tend to be 
> > pointless.
> > If you want to encode 20 streams simultaneously, then why not? It will be 
> > very
> > slow, but that's not this driver's problem :-)
> >
> We use ffz to get instance index.
> This only make sure that instance id is correct since in ffz
> description,
> "Undefined if no zero exists, so code should check against ~0UL first."
> In this case, it may not be able to move to start_streaming.
> Any suggestion that how we do this?
The instance index is only used for printing the debug information.
No? If that's the case, you can remove instance index and print
mtk_vcodec_ctx address for debugging.
>
>
> > > +   }
> > > +
> > > +   mutex_lock(>dev_mutex);
> > > +
> > > +   ctx = devm_kzalloc(>plat_dev->dev, sizeof(*ctx), GFP_KERNEL);
> >
> > Why is this a devm_ call? It is not managed by a device, so it seems to me 
> > that
> > a regular kzalloc is good enough here.
> >
> > > +   if (!ctx) {
> > > +   ret = -ENOMEM;
> > > +   goto err_alloc;
> > > +   }
> > > +
> > > +   ctx->idx = ffz(dev->instance_mask);
> > > +   v4l2_fh_init(>fh, video_devdata(file));
> > > +   file->private_data = >fh;
> > > +   v4l2_fh_add(>fh);
> > > +   INIT_LIST_HEAD(>list);
> > > +   ctx->dev = dev;
> > > +   init_waitqueue_head(>queue);
> > > +
> > > +   if (vfd == dev->vfd_enc) {
> > > +   ctx->type = MTK_INST_ENCODER;
> > > +   ret = mtk_vcodec_enc_ctrls_setup(ctx);
> > > +   if (ret) {
> > > +   mtk_v4l2_err("Failed to setup controls() (%d)",
> > > +  ret);
> > > +   goto err_ctrls_setup;
> > > +   }
> > > +   ctx->m2m_ctx = v4l2_m2m_ctx_init(dev->m2m_dev_enc, ctx,
> > > +_vcodec_enc_queue_init);
> > > +   if (IS_ERR(ctx->m2m_ctx)) {
> > > +   ret = PTR_ERR(ctx->m2m_ctx);
> > > +   mtk_v4l2_err("Failed to v4l2_m2m_ctx_init() (%d)",
> > > +  ret);
> > > +   goto err_m2m_ctx_init;
> > > +   }
> > > +   mtk_vcodec_enc_set_default_params(ctx);
> > > +   } else {
> > > +   mtk_v4l2_err("Invalid vfd !");
> >
> > This shouldn't be possible at all. I would just drop the 'if (vfd == 
> > dev->vfd_enc)' check.
> Got it, will remove in next version.
>
> >
> > > +   ret = -ENOENT;
> > > +   goto err_m2m_ctx_init;
> > > +   }
> > > +
> > > +   if (v4l2_fh_is_singular(>fh)) {
> > > +   ret = vpu_load_firmware(dev->vpu_plat_dev);
> > > +   if (ret < 0) {
> > > +   /*
> > > + * Return 0 if downloading firmware successfully,
> > > + * otherwise it is failed
> > > + */
> > > +   mtk_v4l2_err("vpu_load_firmware failed!");
> > > +   goto err_load_fw;
> > > +   }
> >
> > The fw load seems to be a one-time thing, but here it is done every time
> > someone opens the device and nobody else had it open.
> >
> > If this is a one time thing, then using a bool 'loaded_fw' makes more sense.
> >
> More than one module use vpu firmware, encoder/decoder/mdp...etc.
> If this is first encode instance, we need to check and load vpu
> firmware.
> vpu_load_firmware will check and load firmware when necessary.
>
> best regards,
> Tiffany
> > > +
> > > +   dev->enc_capability =
> > > +   vpu_get_venc_hw_capa(dev->vpu_plat_dev);
> > > +   mtk_v4l2_debug(0, "encoder capability %x", 
> > > dev->enc_capability);
> > > +   }
> > > +
> > > +   mtk_v4l2_debug(2, "Create instance [%d]@%p m2m_ctx=%p ",
> > > +ctx->idx, ctx, ctx->m2m_ctx);
> > > +   set_bit(ctx->idx, >instance_mask);
> > > +   dev->num_instances++;
> > > +   list_add(>list, >ctx_list);
> > > +
> > > +   mutex_unlock(>dev_mutex);
> > > +   mtk_v4l2_debug(0, "%s encoder [%d]", dev_name(>plat_dev->dev),
> > > +  ctx->idx);
> > > +