On 23/08/2023 04:38, Ming Qian wrote:
> decoder firmware may notify host some debug message,
> it can help analyze the state of the firmware in case of error
>
> Fixes: 9f599f351e86 ("media: amphion: add vpu core driver")
> Signed-off-by: Ming Qian <ming.qian@xxxxxxx>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> ---
> v2
> - force to terminate string message data
> drivers/media/platform/amphion/vpu_defs.h | 1 +
> drivers/media/platform/amphion/vpu_helpers.c | 1 +
> drivers/media/platform/amphion/vpu_malone.c | 1 +
> drivers/media/platform/amphion/vpu_msgs.c | 9 +++++++++
> drivers/media/platform/amphion/vpu_rpc.c | 3 ++-
> 5 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/amphion/vpu_defs.h b/drivers/media/platform/amphion/vpu_defs.h
> index 667637eedb5d..7320852668d6 100644
> --- a/drivers/media/platform/amphion/vpu_defs.h
> +++ b/drivers/media/platform/amphion/vpu_defs.h
> @@ -71,6 +71,7 @@ enum {
> VPU_MSG_ID_TIMESTAMP_INFO,
> VPU_MSG_ID_FIRMWARE_XCPT,
> VPU_MSG_ID_PIC_SKIPPED,
> + VPU_MSG_ID_DBG_MSG,
> };
>
> enum VPU_ENC_MEMORY_RESOURSE {
> diff --git a/drivers/media/platform/amphion/vpu_helpers.c b/drivers/media/platform/amphion/vpu_helpers.c
> index af3b336e5dc3..d12310af9ebc 100644
> --- a/drivers/media/platform/amphion/vpu_helpers.c
> +++ b/drivers/media/platform/amphion/vpu_helpers.c
> @@ -489,6 +489,7 @@ const char *vpu_id_name(u32 id)
> case VPU_MSG_ID_UNSUPPORTED: return "unsupported";
> case VPU_MSG_ID_FIRMWARE_XCPT: return "exception";
> case VPU_MSG_ID_PIC_SKIPPED: return "skipped";
> + case VPU_MSG_ID_DBG_MSG: return "debug msg";
> }
> return "<unknown>";
> }
> diff --git a/drivers/media/platform/amphion/vpu_malone.c b/drivers/media/platform/amphion/vpu_malone.c
> index f771661980c0..d3425de7bccd 100644
> --- a/drivers/media/platform/amphion/vpu_malone.c
> +++ b/drivers/media/platform/amphion/vpu_malone.c
> @@ -745,6 +745,7 @@ static struct vpu_pair malone_msgs[] = {
> {VPU_MSG_ID_UNSUPPORTED, VID_API_EVENT_UNSUPPORTED_STREAM},
> {VPU_MSG_ID_FIRMWARE_XCPT, VID_API_EVENT_FIRMWARE_XCPT},
> {VPU_MSG_ID_PIC_SKIPPED, VID_API_EVENT_PIC_SKIPPED},
> + {VPU_MSG_ID_DBG_MSG, VID_API_EVENT_DBG_MSG_DEC},
> };
>
> static void vpu_malone_pack_fs_alloc(struct vpu_rpc_event *pkt,
> diff --git a/drivers/media/platform/amphion/vpu_msgs.c b/drivers/media/platform/amphion/vpu_msgs.c
> index d0ead051f7d1..999efeaebfd5 100644
> --- a/drivers/media/platform/amphion/vpu_msgs.c
> +++ b/drivers/media/platform/amphion/vpu_msgs.c
> @@ -180,6 +180,14 @@ static void vpu_session_handle_pic_skipped(struct vpu_inst *inst, struct vpu_rpc
> vpu_inst_unlock(inst);
> }
>
> +static void vpu_session_handle_dbg_msg(struct vpu_inst *inst, struct vpu_rpc_event *pkt)
> +{
> + char *str = (char *)pkt->data;
> +
> + if (strlen(str))
This can be replaced by: 'if (*str)', it's faster than strlen.
> + dev_info(inst->dev, "instance %d firmware dbg msg : %s\n", inst->id, str);
> +}
> +
> static struct vpu_msg_handler handlers[] = {
> {VPU_MSG_ID_START_DONE, vpu_session_handle_start_done},
> {VPU_MSG_ID_STOP_DONE, vpu_session_handle_stop_done},
> @@ -196,6 +204,7 @@ static struct vpu_msg_handler handlers[] = {
> {VPU_MSG_ID_UNSUPPORTED, vpu_session_handle_error},
> {VPU_MSG_ID_FIRMWARE_XCPT, vpu_session_handle_firmware_xcpt},
> {VPU_MSG_ID_PIC_SKIPPED, vpu_session_handle_pic_skipped},
> + {VPU_MSG_ID_DBG_MSG, vpu_session_handle_dbg_msg},
> };
>
> static int vpu_session_handle_msg(struct vpu_inst *inst, struct vpu_rpc_event *msg)
> diff --git a/drivers/media/platform/amphion/vpu_rpc.c b/drivers/media/platform/amphion/vpu_rpc.c
> index f626a9f835e0..8234370afee5 100644
> --- a/drivers/media/platform/amphion/vpu_rpc.c
> +++ b/drivers/media/platform/amphion/vpu_rpc.c
> @@ -144,7 +144,7 @@ static int vpu_rpc_receive_msg_buf(struct vpu_shared_addr *shared, struct vpu_rp
> msg->hdr.num = (msgword >> 16) & 0xff;
> msg->hdr.id = msgword & 0x3fff;
>
> - if (msg->hdr.num > ARRAY_SIZE(msg->data))
> + if (msg->hdr.num >= ARRAY_SIZE(msg->data))
Is this right? You are changing the max length just to be able to set the
last byte to 0, which makes sense for VPU_MSG_ID_DBG_MSG but perhaps not for
others.
I think it would be better to leave this code as-is and instead set the
last byte to 0 in vpu_session_handle_dbg_msg(). That's where it is actually
needed.
Or am I missing something here?
Regards,
Hans
> return -EINVAL;
>
> for (i = 0; i < msg->hdr.num; i++) {
> @@ -156,6 +156,7 @@ static int vpu_rpc_receive_msg_buf(struct vpu_shared_addr *shared, struct vpu_rp
> data = shared->msg_mem_vir;
> }
> }
> + msg->data[msg->hdr.num] = 0;
>
> /*update rptr after data is read*/
> mb();
