Re: [PATCH v6, 19/24] media: mediatek: vcodec: disable wait interrupt for svp mode

["Yunfei Dong (董云飞)" <Yunfei.Dong@mediatek.com>]

Hi CK,

Sorry to reply your suggestion too late.

Thanks for your help to review this patch.
On Fri, 2024-05-17 at 09:53 +0000, CK Hu (胡俊光) wrote:
> Hi, Yunfei:
> 
> On Thu, 2024-05-16 at 20:20 +0800, Yunfei Dong wrote:
> > Waiting interrupt in optee-os for svp mode, need to disable it in
> > kernel
> > in case of interrupt is cleaned.
> > 
> > Signed-off-by: Yunfei Dong <yunfei.dong@mediatek.com>
> > ---
> >  .../vcodec/decoder/mtk_vcodec_dec_hw.c        | 34 +++++------
> >  .../vcodec/decoder/mtk_vcodec_dec_pm.c        |  6 +-
> >  .../decoder/vdec/vdec_h264_req_multi_if.c     | 57 +++++++++++--
> > ------
> >  3 files changed, 54 insertions(+), 43 deletions(-)
> > 
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.
> > c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.
> > c
> > index 881d5de41e05..1982c088c6da 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.
> > c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.
> > c
> > @@ -72,26 +72,28 @@ static irqreturn_t mtk_vdec_hw_irq_handler(int
> > irq, void *priv)
> >  
> >  	ctx = mtk_vcodec_get_curr_ctx(dev->main_dev, dev->hw_idx);
> >  
> > -	/* check if HW active or not */
> > -	cg_status = readl(dev->reg_base[VDEC_HW_SYS] +
> > VDEC_HW_ACTIVE_ADDR);
> > -	if (cg_status & VDEC_HW_ACTIVE_MASK) {
> > -		mtk_v4l2_vdec_err(ctx, "vdec active is not 0x0
> > (0x%08x)", cg_status);
> > -		return IRQ_HANDLED;
> > -	}
> > +	if (!ctx->is_secure_playback) {
> > +		/* check if HW active or not */
> > +		cg_status = readl(dev->reg_base[VDEC_HW_SYS] +
> > VDEC_HW_ACTIVE_ADDR);
> > +		if (cg_status & VDEC_HW_ACTIVE_MASK) {
> > +			mtk_v4l2_vdec_err(ctx, "vdec active is not 0x0
> > (0x%08x)", cg_status);
> > +			return IRQ_HANDLED;
> > +		}
> >  
> > -	dec_done_status = readl(vdec_misc_addr);
> > -	if ((dec_done_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS) !=
> > -	    MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)
> > -		return IRQ_HANDLED;
> > +		dec_done_status = readl(vdec_misc_addr);
> > +		if ((dec_done_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)
> > !=
> > +		    MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)
> > +			return IRQ_HANDLED;
> >  
> > -	/* clear interrupt */
> > -	writel(dec_done_status | VDEC_IRQ_CFG, vdec_misc_addr);
> > -	writel(dec_done_status & ~VDEC_IRQ_CLR, vdec_misc_addr);
> > +		/* clear interrupt */
> > +		writel(dec_done_status | VDEC_IRQ_CFG, vdec_misc_addr);
> > +		writel(dec_done_status & ~VDEC_IRQ_CLR,
> > vdec_misc_addr);
> >  
> > -	wake_up_dec_ctx(ctx, MTK_INST_IRQ_RECEIVED, dev->hw_idx);
> > +		wake_up_dec_ctx(ctx, MTK_INST_IRQ_RECEIVED, dev-
> > >hw_idx);
> >  
> > -	mtk_v4l2_vdec_dbg(3, ctx, "wake up ctx %d, dec_done_status=%x",
> > -			  ctx->id, dec_done_status);
> > +		mtk_v4l2_vdec_dbg(3, ctx, "wake up ctx %d,
> > dec_done_status=%x",
> > +				  ctx->id, dec_done_status);
> > +	}
> >  
> >  	return IRQ_HANDLED;
> >  }
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_pm.
> > c
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_pm.
> > c
> > index aefd3e9e3061..a94eda936f16 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_pm.
> > c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_pm.
> > c
> > @@ -238,7 +238,8 @@ void mtk_vcodec_dec_enable_hardware(struct
> > mtk_vcodec_dec_ctx *ctx, int hw_idx)
> >  		mtk_vcodec_dec_child_dev_on(ctx->dev, MTK_VDEC_LAT0);
> >  	mtk_vcodec_dec_child_dev_on(ctx->dev, hw_idx);
> >  
> > -	mtk_vcodec_dec_enable_irq(ctx->dev, hw_idx);
> > +	if (!ctx->is_secure_playback)
> > +		mtk_vcodec_dec_enable_irq(ctx->dev, hw_idx);
> >  
> >  	if (IS_VDEC_INNER_RACING(ctx->dev->dec_capability))
> >  		mtk_vcodec_load_racing_info(ctx);
> > @@ -250,7 +251,8 @@ void mtk_vcodec_dec_disable_hardware(struct
> > mtk_vcodec_dec_ctx *ctx, int hw_idx)
> >  	if (IS_VDEC_INNER_RACING(ctx->dev->dec_capability))
> >  		mtk_vcodec_record_racing_info(ctx);
> >  
> > -	mtk_vcodec_dec_disable_irq(ctx->dev, hw_idx);
> > +	if (!ctx->is_secure_playback)
> > +		mtk_vcodec_dec_disable_irq(ctx->dev, hw_idx);
> >  
> >  	mtk_vcodec_dec_child_dev_off(ctx->dev, hw_idx);
> >  	if (IS_VDEC_LAT_ARCH(ctx->dev->vdec_pdata->hw_arch) &&
> > diff --git
> > a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req
> > _multi_if.c
> > b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req
> > _multi_if.c
> > index f27d326f00ba..3186777dcc13 100644
> > ---
> > a/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req
> > _multi_if.c
> > +++
> > b/drivers/media/platform/mediatek/vcodec/decoder/vdec/vdec_h264_req
> > _multi_if.c
> > @@ -593,14 +593,16 @@ static int vdec_h264_slice_core_decode(struct
> > vdec_lat_buf *lat_buf)
> >  		goto vdec_dec_end;
> >  	}
> >  
> > -	/* wait decoder done interrupt */
> > -	timeout = mtk_vcodec_wait_for_done_ctx(inst->ctx,
> > MTK_INST_IRQ_RECEIVED,
> > -					       WAIT_INTR_TIMEOUT_MS,
> > MTK_VDEC_CORE);
> > -	if (timeout)
> > -		mtk_vdec_err(ctx, "core decode timeout: pic_%d", ctx-
> > >decoded_frame_cnt);
> > -	inst->vsi_core->dec.timeout = !!timeout;
> > -
> > -	vpu_dec_core_end(vpu);
> > +	if (!ctx->is_secure_playback) {
> > +		/* wait decoder done interrupt */
> > +		timeout = mtk_vcodec_wait_for_done_ctx(inst->ctx,
> > MTK_INST_IRQ_RECEIVED,
> > +						       WAIT_INTR_TIMEOU
> > T_MS, MTK_VDEC_CORE);
> > +		if (timeout)
> > +			mtk_vdec_err(ctx, "core decode timeout:
> > pic_%d", ctx->decoded_frame_cnt);
> > +		inst->vsi_core->dec.timeout = !!timeout;
> > +
> > +		vpu_dec_core_end(vpu);
> > +	}
> 
> In svp mode, optee driver would wait the irq, but why optee driver
> does not notify normal world driver? The normal world driver here
> does not wait anything and keep going, so it would get the frame
> which has not been processed done and pass it to next module such as
> mdp? The normal world driver does not wait here, how does it make
> sure the frame is processed done?
> 
Normal and secure world can receive the interrupt at the same time when
play secure video.

The driver process the interrupt and clean the interrupt in optee-os
when play secure video, not normal world. So the driver needn't to wait
interrupt and clean the interrupt in normal world
> Regards,
> 
> CK
> 
Best Regards,
Yunfei Dong
> >  	mtk_vdec_debug(ctx, "pic[%d] crc: 0x%x 0x%x 0x%x 0x%x 0x%x 0x%x
> > 0x%x 0x%x",
> >  		       ctx->decoded_frame_cnt,
> >  		       inst->vsi_core->dec.crc[0], inst->vsi_core-
> > >dec.crc[1],
> > @@ -724,14 +726,16 @@ static int vdec_h264_slice_lat_decode(void
> > *h_vdec, struct mtk_vcodec_mem *bs,
> >  		vdec_msg_queue_qbuf(&inst->ctx->msg_queue.core_ctx,
> > lat_buf);
> >  	}
> >  
> > -	/* wait decoder done interrupt */
> > -	timeout = mtk_vcodec_wait_for_done_ctx(inst->ctx,
> > MTK_INST_IRQ_RECEIVED,
> > -					       WAIT_INTR_TIMEOUT_MS,
> > MTK_VDEC_LAT0);
> > -	if (timeout)
> > -		mtk_vdec_err(inst->ctx, "lat decode timeout: pic_%d",
> > inst->slice_dec_num);
> > -	inst->vsi->dec.timeout = !!timeout;
> > +	if (!inst->ctx->is_secure_playback) {
> > +		/* wait decoder done interrupt */
> > +		timeout = mtk_vcodec_wait_for_done_ctx(inst->ctx,
> > MTK_INST_IRQ_RECEIVED,
> > +						       WAIT_INTR_TIMEOU
> > T_MS, MTK_VDEC_LAT0);
> > +		if (timeout)
> > +			mtk_vdec_err(inst->ctx, "lat decode timeout:
> > pic_%d", inst->slice_dec_num);
> > +		inst->vsi->dec.timeout = !!timeout;
> >  
> > -	err = vpu_dec_end(vpu);
> > +		err = vpu_dec_end(vpu);
> > +	}
> >  	if (err == SLICE_HEADER_FULL || err == TRANS_BUFFER_FULL) {
> >  		if (!IS_VDEC_INNER_RACING(inst->ctx->dev-
> > >dec_capability))
> >  			vdec_msg_queue_qbuf(&inst->ctx-
> > >msg_queue.lat_ctx, lat_buf);
> > @@ -831,16 +835,19 @@ static int vdec_h264_slice_single_decode(void
> > *h_vdec, struct mtk_vcodec_mem *bs
> >  	if (err)
> >  		goto err_free_fb_out;
> >  
> > -	/* wait decoder done interrupt */
> > -	err = mtk_vcodec_wait_for_done_ctx(inst->ctx,
> > MTK_INST_IRQ_RECEIVED,
> > -					   WAIT_INTR_TIMEOUT_MS,
> > MTK_VDEC_CORE);
> > -	if (err)
> > -		mtk_vdec_err(inst->ctx, "decode timeout: pic_%d", inst-
> > >ctx->decoded_frame_cnt);
> > -
> > -	inst->vsi->dec.timeout = !!err;
> > -	err = vpu_dec_end(vpu);
> > -	if (err)
> > -		goto err_free_fb_out;
> > +	if (!inst->ctx->is_secure_playback) {
> > +		/* wait decoder done interrupt */
> > +		err = mtk_vcodec_wait_for_done_ctx(inst->ctx,
> > MTK_INST_IRQ_RECEIVED,
> > +						   WAIT_INTR_TIMEOUT_MS
> > , MTK_VDEC_CORE);
> > +		if (err)
> > +			mtk_vdec_err(inst->ctx, "decode timeout:
> > pic_%d",
> > +				     inst->ctx->decoded_frame_cnt);
> > +
> > +		inst->vsi->dec.timeout = !!err;
> > +		err = vpu_dec_end(vpu);
> > +		if (err)
> > +			goto err_free_fb_out;
> > +	}
> >  
> >  	memcpy(&inst->vsi_ctx, inst->vpu.vsi, sizeof(inst->vsi_ctx));
> >  	mtk_vdec_debug(inst->ctx, "pic[%d] crc: 0x%x 0x%x 0x%x 0x%x
> > 0x%x 0x%x 0x%x 0x%x",