[PATCH 0/5] media: mtk-vcodec: venc: variouse bug fixes

[PATCH 0/5] media: mtk-vcodec: venc: variouse bug fixes

[Dafna Hirschfeld]

Some bug fixes mainly in case of error handling

Dafna Hirschfeld (5):
  media: mtk-vcodec: enter ABORT state if encoding failed
  media: mtk-vcodec: call v4l2_m2m_ctx_release first when file is
    released
  media: mtk-vcodec: change the venc handler funcs to return int
  media: mtk-vcodec: Add two error cases upon vpu irq handling
  media: mtk-vcodec: venc: Fail if a msg sent to VPU was not signaled

 .../platform/mtk-vcodec/mtk_vcodec_enc.c      |  1 +
 .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c  |  2 +-
 .../media/platform/mtk-vcodec/venc_vpu_if.c   | 27 +++++++++++++------
 3 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH 1/5] media: mtk-vcodec: enter ABORT state if encoding failed

[Dafna Hirschfeld]

In case the encoding failed, we should set
ctx->state = MTK_STATE_ABORT, since this indicates
a fatal error and there is no point to continue
trying to encode in that case.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
index 416f356af363..1678c31bc9aa 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
@@ -1109,6 +1109,7 @@ static void mtk_venc_worker(struct work_struct *work)
 		dst_buf->vb2_buf.planes[0].bytesused = 0;
 		v4l2_m2m_buf_done(dst_buf, VB2_BUF_STATE_ERROR);
 		mtk_v4l2_err("venc_if_encode failed=%d", ret);
+		ctx->state = MTK_STATE_ABORT;
 	} else {
 		v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_DONE);
 		dst_buf->vb2_buf.planes[0].bytesused = enc_result.bs_size;
-- 
2.17.1

[PATCH 2/5] media: mtk-vcodec: call v4l2_m2m_ctx_release first when file is released

[Dafna Hirschfeld]

The func v4l2_m2m_ctx_release waits for currently running jobs
to finish and then stop streaming both queues and frees the buffers.
All this should be done before the call to mtk_vcodec_enc_release
which frees the encoder handler. This fixes use-after-free bug.

Fixes: 4e855a6efa547 ("[media] vcodec: mediatek: Add Mediatek V4L2 Video Encoder Driver")
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
index 45d1870c83dd..4ced20ca647b 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
@@ -218,11 +218,11 @@ static int fops_vcodec_release(struct file *file)
 	mtk_v4l2_debug(1, "[%d] encoder", ctx->id);
 	mutex_lock(&dev->dev_mutex);
 
+	v4l2_m2m_ctx_release(ctx->m2m_ctx);
 	mtk_vcodec_enc_release(ctx);
 	v4l2_fh_del(&ctx->fh);
 	v4l2_fh_exit(&ctx->fh);
 	v4l2_ctrl_handler_free(&ctx->ctrl_hdl);
-	v4l2_m2m_ctx_release(ctx->m2m_ctx);
 
 	list_del_init(&ctx->list);
 	kfree(ctx);
-- 
2.17.1

[PATCH 3/5] media: mtk-vcodec: change the venc handler funcs to return int

[Dafna Hirschfeld]

Currently the functions handle_enc_init_msg, handle_enc_encode_msg
return void and set vpu->failure to 1 in case of failure.
Instead, change the functions to return non 0 in case of failure
and then the vpu->failure is updated to their return value.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/mtk-vcodec/venc_vpu_if.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
index be6d8790a41e..32dc844d16f9 100644
--- a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
+++ b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
@@ -9,7 +9,7 @@
 #include "venc_ipi_msg.h"
 #include "venc_vpu_if.h"
 
-static void handle_enc_init_msg(struct venc_vpu_inst *vpu, const void *data)
+static int handle_enc_init_msg(struct venc_vpu_inst *vpu, const void *data)
 {
 	const struct venc_vpu_ipi_msg_init *msg = data;
 
@@ -19,7 +19,7 @@ static void handle_enc_init_msg(struct venc_vpu_inst *vpu, const void *data)
 
 	/* Firmware version field value is unspecified on MT8173. */
 	if (vpu->ctx->dev->venc_pdata->chip == MTK_MT8173)
-		return;
+		return 0;
 
 	/* Check firmware version. */
 	mtk_vcodec_debug(vpu, "firmware version: 0x%x\n",
@@ -30,18 +30,19 @@ static void handle_enc_init_msg(struct venc_vpu_inst *vpu, const void *data)
 	default:
 		mtk_vcodec_err(vpu, "unhandled firmware version 0x%x\n",
 			       msg->venc_abi_version);
-		vpu->failure = 1;
-		break;
+		return -EINVAL;
 	}
+	return 0;
 }
 
-static void handle_enc_encode_msg(struct venc_vpu_inst *vpu, const void *data)
+static int handle_enc_encode_msg(struct venc_vpu_inst *vpu, const void *data)
 {
 	const struct venc_vpu_ipi_msg_enc *msg = data;
 
 	vpu->state = msg->state;
 	vpu->bs_size = msg->bs_size;
 	vpu->is_key_frm = msg->is_key_frm;
+	return 0;
 }
 
 static void vpu_enc_ipi_handler(void *data, unsigned int len, void *priv)
@@ -60,12 +61,12 @@ static void vpu_enc_ipi_handler(void *data, unsigned int len, void *priv)
 
 	switch (msg->msg_id) {
 	case VPU_IPIMSG_ENC_INIT_DONE:
-		handle_enc_init_msg(vpu, data);
+		vpu->failure = handle_enc_init_msg(vpu, data);
 		break;
 	case VPU_IPIMSG_ENC_SET_PARAM_DONE:
 		break;
 	case VPU_IPIMSG_ENC_ENCODE_DONE:
-		handle_enc_encode_msg(vpu, data);
+		vpu->failure = handle_enc_encode_msg(vpu, data);
 		break;
 	case VPU_IPIMSG_ENC_DEINIT_DONE:
 		break;
-- 
2.17.1

[PATCH 4/5] media: mtk-vcodec: Add two error cases upon vpu irq handling

[Dafna Hirschfeld]

1. Fail if the function mtk_vcodec_fw_map_dm_addr
returns ERR pointer.
2. Fail if the state from the vpu msg is either
VEN_IPI_MSG_ENC_STATE_ERROR or VEN_IPI_MSG_ENC_STATE_PART

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/mtk-vcodec/venc_vpu_if.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
index 32dc844d16f9..234705ba7cd6 100644
--- a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
+++ b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
@@ -17,6 +17,8 @@ static int handle_enc_init_msg(struct venc_vpu_inst *vpu, const void *data)
 	vpu->vsi = mtk_vcodec_fw_map_dm_addr(vpu->ctx->dev->fw_handler,
 					     msg->vpu_inst_addr);
 
+	if (IS_ERR(vpu->vsi))
+		return PTR_ERR(vpu->vsi);
 	/* Firmware version field value is unspecified on MT8173. */
 	if (vpu->ctx->dev->venc_pdata->chip == MTK_MT8173)
 		return 0;
@@ -42,6 +44,12 @@ static int handle_enc_encode_msg(struct venc_vpu_inst *vpu, const void *data)
 	vpu->state = msg->state;
 	vpu->bs_size = msg->bs_size;
 	vpu->is_key_frm = msg->is_key_frm;
+	if (vpu->state == VEN_IPI_MSG_ENC_STATE_ERROR ||
+	    vpu->state == VEN_IPI_MSG_ENC_STATE_PART) {
+		mtk_vcodec_err(vpu, "bad ipi-enc-state: %s",
+			       vpu->state == VEN_IPI_MSG_ENC_STATE_ERROR ? "ERR" : "PART");
+		return -EINVAL;
+	}
 	return 0;
 }
 
-- 
2.17.1

Re: [PATCH 4/5] media: mtk-vcodec: Add two error cases upon vpu irq handling

[Irui Wang (王瑞)]

On Wed, 2021-08-04 at 16:27 +0200, Dafna Hirschfeld wrote:
> 1. Fail if the function mtk_vcodec_fw_map_dm_addr
> returns ERR pointer.
> 2. Fail if the state from the vpu msg is either
> VEN_IPI_MSG_ENC_STATE_ERROR or VEN_IPI_MSG_ENC_STATE_PART
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/media/platform/mtk-vcodec/venc_vpu_if.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> index 32dc844d16f9..234705ba7cd6 100644
> --- a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> +++ b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> @@ -17,6 +17,8 @@ static int handle_enc_init_msg(struct venc_vpu_inst
> *vpu, const void *data)
>  	vpu->vsi = mtk_vcodec_fw_map_dm_addr(vpu->ctx->dev->fw_handler,
>  					     msg->vpu_inst_addr);
>  
> +	if (IS_ERR(vpu->vsi))
> +		return PTR_ERR(vpu->vsi);
>  	/* Firmware version field value is unspecified on MT8173. */
>  	if (vpu->ctx->dev->venc_pdata->chip == MTK_MT8173)
>  		return 0;
> @@ -42,6 +44,12 @@ static int handle_enc_encode_msg(struct
> venc_vpu_inst *vpu, const void *data)
>  	vpu->state = msg->state;
>  	vpu->bs_size = msg->bs_size;
>  	vpu->is_key_frm = msg->is_key_frm;
> +	if (vpu->state == VEN_IPI_MSG_ENC_STATE_ERROR ||
> +	    vpu->state == VEN_IPI_MSG_ENC_STATE_PART) {
> +		mtk_vcodec_err(vpu, "bad ipi-enc-state: %s",
> +			       vpu->state ==
> VEN_IPI_MSG_ENC_STATE_ERROR ? "ERR" : "PART");
> +		return -EINVAL;
> +	}

Hi Dafna,

This state check is useless, the enc result will check in
"vpu_enc_ipi_handler".

Thanks

>  	return 0;
>  }
>  

Re: [PATCH 4/5] media: mtk-vcodec: Add two error cases upon vpu irq handling

[Dafna Hirschfeld]

On 06.08.21 08:58, Irui Wang (王瑞) wrote:
> On Wed, 2021-08-04 at 16:27 +0200, Dafna Hirschfeld wrote:
>> 1. Fail if the function mtk_vcodec_fw_map_dm_addr
>> returns ERR pointer.
>> 2. Fail if the state from the vpu msg is either
>> VEN_IPI_MSG_ENC_STATE_ERROR or VEN_IPI_MSG_ENC_STATE_PART
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/media/platform/mtk-vcodec/venc_vpu_if.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> index 32dc844d16f9..234705ba7cd6 100644
>> --- a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> +++ b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> @@ -17,6 +17,8 @@ static int handle_enc_init_msg(struct venc_vpu_inst
>> *vpu, const void *data)
>>   	vpu->vsi = mtk_vcodec_fw_map_dm_addr(vpu->ctx->dev->fw_handler,
>>   					     msg->vpu_inst_addr);
>>   
>> +	if (IS_ERR(vpu->vsi))
>> +		return PTR_ERR(vpu->vsi);
>>   	/* Firmware version field value is unspecified on MT8173. */
>>   	if (vpu->ctx->dev->venc_pdata->chip == MTK_MT8173)
>>   		return 0;
>> @@ -42,6 +44,12 @@ static int handle_enc_encode_msg(struct
>> venc_vpu_inst *vpu, const void *data)
>>   	vpu->state = msg->state;
>>   	vpu->bs_size = msg->bs_size;
>>   	vpu->is_key_frm = msg->is_key_frm;
>> +	if (vpu->state == VEN_IPI_MSG_ENC_STATE_ERROR ||
>> +	    vpu->state == VEN_IPI_MSG_ENC_STATE_PART) {
>> +		mtk_vcodec_err(vpu, "bad ipi-enc-state: %s",
>> +			       vpu->state ==
>> VEN_IPI_MSG_ENC_STATE_ERROR ? "ERR" : "PART");
>> +		return -EINVAL;
>> +	}
> 
> Hi Dafna,
> 
> This state check is useless, the enc result will check in
> "vpu_enc_ipi_handler".
> 

Hi, thanks for reviewing. I see that the vpu_enc_ipi_handler only test the msg->status
and I see that the states are not tested anywhere except of "skip" state in the h264 enc.

Can't there be a scenario where msg->status is ok but the state is error?
I am testing the vp8 encoder on chromeos and at some point the encoder interrupts stop arriving
so I try to figure out why and report any possible error.

Thanks,
Dafna

> Thanks
> 
>>   	return 0;
>>   }
>>   

[PATCH 5/5] media: mtk-vcodec: venc: Fail if a msg sent to VPU was not signaled

[Dafna Hirschfeld]

Each message sent to the VPU should raise a signal. The signal
handler sets vpu->signaled. Test the field and fail
if it is 0.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/mtk-vcodec/venc_vpu_if.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
index 234705ba7cd6..8331b1bd1971 100644
--- a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
+++ b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
@@ -92,6 +92,7 @@ static int vpu_enc_send_msg(struct venc_vpu_inst *vpu, void *msg,
 {
 	int status;
 
+	vpu->signaled = 0;
 	mtk_vcodec_debug_enter(vpu);
 
 	if (!vpu->ctx->dev->fw_handler) {
@@ -106,6 +107,8 @@ static int vpu_enc_send_msg(struct venc_vpu_inst *vpu, void *msg,
 			       *(uint32_t *)msg, len, status);
 		return -EINVAL;
 	}
+	if (!vpu->signaled)
+		return -EINVAL;
 	if (vpu->failure)
 		return -EINVAL;
 
@@ -122,7 +125,6 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
 	mtk_vcodec_debug_enter(vpu);
 
 	init_waitqueue_head(&vpu->wq_hd);
-	vpu->signaled = 0;
 	vpu->failure = 0;
 
 	status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler, vpu->id,
-- 
2.17.1

Re: [PATCH 5/5] media: mtk-vcodec: venc: Fail if a msg sent to VPU was not signaled

[Irui Wang (王瑞)]

On Wed, 2021-08-04 at 16:27 +0200, Dafna Hirschfeld wrote:
> Each message sent to the VPU should raise a signal. The signal
> handler sets vpu->signaled. Test the field and fail
> if it is 0.

I suppose you want to handle the message execution result, if ipi
message can't send or acked successfully, the returned "status" of
"mtk_vcodec_fw_ipi_send" will return, so I think you don't need to
check "signaled" again.

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/media/platform/mtk-vcodec/venc_vpu_if.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> index 234705ba7cd6..8331b1bd1971 100644
> --- a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> +++ b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
> @@ -92,6 +92,7 @@ static int vpu_enc_send_msg(struct venc_vpu_inst
> *vpu, void *msg,
>  {
>  	int status;
>  
> +	vpu->signaled = 0;
>  	mtk_vcodec_debug_enter(vpu);
>  
>  	if (!vpu->ctx->dev->fw_handler) {
> @@ -106,6 +107,8 @@ static int vpu_enc_send_msg(struct venc_vpu_inst
> *vpu, void *msg,
>  			       *(uint32_t *)msg, len, status);
>  		return -EINVAL;
>  	}
> +	if (!vpu->signaled)
> +		return -EINVAL;
>  	if (vpu->failure)
>  		return -EINVAL;
>  
> @@ -122,7 +125,6 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
>  	mtk_vcodec_debug_enter(vpu);
>  
>  	init_waitqueue_head(&vpu->wq_hd);
> -	vpu->signaled = 0;
>  	vpu->failure = 0;
>  
>  	status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler,
> vpu->id,

Re: [PATCH 5/5] media: mtk-vcodec: venc: Fail if a msg sent to VPU was not signaled

[Dafna Hirschfeld]

On 06.08.21 08:50, Irui Wang (王瑞) wrote:
> On Wed, 2021-08-04 at 16:27 +0200, Dafna Hirschfeld wrote:
>> Each message sent to the VPU should raise a signal. The signal
>> handler sets vpu->signaled. Test the field and fail
>> if it is 0.
> 
> I suppose you want to handle the message execution result, if ipi
> message can't send or acked successfully, the returned "status" of
> "mtk_vcodec_fw_ipi_send" will return, so I think you don't need to
> check "signaled" again.

in that case, the field 'signaled' is not needed and can be removed
So I can send a patch to remove it.

Thanks,
Dafna

> 
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/media/platform/mtk-vcodec/venc_vpu_if.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> index 234705ba7cd6..8331b1bd1971 100644
>> --- a/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> +++ b/drivers/media/platform/mtk-vcodec/venc_vpu_if.c
>> @@ -92,6 +92,7 @@ static int vpu_enc_send_msg(struct venc_vpu_inst
>> *vpu, void *msg,
>>   {
>>   	int status;
>>   
>> +	vpu->signaled = 0;
>>   	mtk_vcodec_debug_enter(vpu);
>>   
>>   	if (!vpu->ctx->dev->fw_handler) {
>> @@ -106,6 +107,8 @@ static int vpu_enc_send_msg(struct venc_vpu_inst
>> *vpu, void *msg,
>>   			       *(uint32_t *)msg, len, status);
>>   		return -EINVAL;
>>   	}
>> +	if (!vpu->signaled)
>> +		return -EINVAL;
>>   	if (vpu->failure)
>>   		return -EINVAL;
>>   
>> @@ -122,7 +125,6 @@ int vpu_enc_init(struct venc_vpu_inst *vpu)
>>   	mtk_vcodec_debug_enter(vpu);
>>   
>>   	init_waitqueue_head(&vpu->wq_hd);
>> -	vpu->signaled = 0;
>>   	vpu->failure = 0;
>>   
>>   	status = mtk_vcodec_fw_ipi_register(vpu->ctx->dev->fw_handler,
>> vpu->id,