On Wed, Dec 29, 2021 at 2:52 PM yunfei.dong@mediatek.com yunfei.dong@mediatek.com wrote:
On Wed, 2021-12-29 at 13:36 +0800, Tzung-Bi Shih wrote:
On Tue, Dec 28, 2021 at 05:41:37PM +0800, Yunfei Dong wrote:
From: Yunfei Dong yunfei.dong@mediatek.corp-partner.google.com
[...]
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c index 130ecef2e766..87891ebd7246 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c @@ -466,6 +466,8 @@ static int vidioc_vdec_s_fmt(struct file *file, void *priv, } ctx->state = MTK_STATE_INIT; }
} else {
ctx->capture_fourcc = fmt->fourcc;}
/*
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h index a23a7646437c..95e07cf2cd3e 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h @@ -277,6 +277,7 @@ struct vdec_pic_info {
to be used with encoder and stateful decoder.- @is_flushing: set to true if flushing is in progress.
- @current_codec: current set input codec, in V4L2 pixel format
- @capture_fourcc: capture queue type in V4L2 pixel format
- @colorspace: enum v4l2_colorspace; supplemental to pixelformat
- @ycbcr_enc: enum v4l2_ycbcr_encoding, Y'CbCr encoding
@@ -322,6 +323,7 @@ struct mtk_vcodec_ctx { bool is_flushing;
u32 current_codec;
- u32 capture_fourcc;
What is the purpose of capture_fourcc? It is not used.
Need to calculate each plane size according to capture fourcc type from scp. The plane size of MM21 is different with MT21C. And the capture fourcc type of different codec maybe different.
Purpose of capture_fourcc in the context is not obvious and looks irrelevant to the patch. Could it move to somewhere patch that makes more sense?
+/**
- struct vdec_ap_ipi_get_param - for AP_IPIMSG_SET_PARAM
- @msg_id : AP_IPIMSG_DEC_START
- @inst_id : instance ID. Used if the ABI version >= 2.
- @data : picture information
- @param_type : get param type
- @codec_type : Codec fourcc
- */
+struct vdec_ap_ipi_get_param {
- uint32_t msg_id;
- uint32_t inst_id;
- uint32_t data[4];
- uint32_t param_type;
- uint32_t codec_type;
+};
Are AP_IPIMSG_SET_PARAM and AP_IPIMSG_DEC_START typos?
It's getting message from scp side. It's looks much better to add one new path from ap to scp.
Pardon me, I failed to understand it. I thought the struct could be for AP_IPIMSG_DEC_GET_PARAM.
+/**
- struct vdec_vpu_ipi_init_ack - for VPU_IPIMSG_DEC_INIT_ACK
- @msg_id : VPU_IPIMSG_DEC_INIT_ACK
- @status : VPU exeuction result
- @ap_inst_addr : AP vcodec_vpu_inst instance address
- @data : picture information from SCP.
- @param_type : get param type
- */
+struct vdec_vpu_ipi_get_param_ack {
- uint32_t msg_id;
- int32_t status;
- uint64_t ap_inst_addr;
- uint32_t data[4];
- uint32_t param_type;
- uint32_t reserved;
+};
Same here: is VPU_IPIMSG_DEC_INIT_ACK a typo?
It's getting message from scp side. It's looks much better to add one new path from ap to scp.
Same here: I thought it was for VPU_IPIMSG_DEC_GET_PARAM_ACK.
diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h index 4cb3c7f5a3ad..963f8d4877b7 100644 --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.h @@ -28,6 +28,8 @@ struct mtk_vcodec_ctx;
- @wq : wait queue to wait VPU message ack
- @handler : ipi handler for each decoder
- @codec_type : use codec type to separate different codecs
- @capture_type : used capture type to separate different
capture format
*/
- @fb_sz : frame buffer size of each plane
struct vdec_vpu_inst { int id; @@ -42,6 +44,8 @@ struct vdec_vpu_inst { wait_queue_head_t wq; mtk_vcodec_ipi_handler handler; unsigned int codec_type;
- unsigned int capture_type;
- unsigned int fb_sz[2];
};
capture_type is not used in the patch.
Capture type will be used in scp to get capture plane size according to capture buffer type.
Pardon me, I failed to understand it. I may misunderstand, however, it doesn't look like a shared memory structure between AP and SCP.
At least to me, capture_type is not used in the patch. It could be better to move it to somewhere patch that makes more sense.
Does fb_sz take effect in later patches?
Don't have effect to later patches.
Is the fb_sz used somewhere "later"? The patch gets fb_sz from SCP, however, fb_sz is never used in some control or configuration paths.