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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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,
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 <[email protected]>
> ---
> 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;
> }
>
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 <[email protected]>
>> ---
>> 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;
>> }
>>
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 <[email protected]>
>> ---
>> 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,