From: Yunfei Dong <[email protected]>
Different capture buffer format has different buffer size, need to get
real buffer size according to buffer type from scp.
Signed-off-by: Yunfei Dong <[email protected]>
---
.../platform/mtk-vcodec/mtk_vcodec_dec.c | 2 +
.../platform/mtk-vcodec/mtk_vcodec_drv.h | 2 +
.../media/platform/mtk-vcodec/vdec_ipi_msg.h | 36 +++++++++++++
.../media/platform/mtk-vcodec/vdec_vpu_if.c | 51 +++++++++++++++++++
.../media/platform/mtk-vcodec/vdec_vpu_if.h | 15 ++++++
5 files changed, 106 insertions(+)
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;
enum v4l2_colorspace colorspace;
enum v4l2_ycbcr_encoding ycbcr_enc;
diff --git a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
index 5daca8d52ebb..d00e555cf27a 100644
--- a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
+++ b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
@@ -20,6 +20,7 @@ enum vdec_ipi_msgid {
AP_IPIMSG_DEC_RESET = 0xA004,
AP_IPIMSG_DEC_CORE = 0xA005,
AP_IPIMSG_DEC_CORE_END = 0xA006,
+ AP_IPIMSG_DEC_GET_PARAM = 0xA007,
VPU_IPIMSG_DEC_INIT_ACK = 0xB000,
VPU_IPIMSG_DEC_START_ACK = 0xB001,
@@ -28,6 +29,7 @@ enum vdec_ipi_msgid {
VPU_IPIMSG_DEC_RESET_ACK = 0xB004,
VPU_IPIMSG_DEC_CORE_ACK = 0xB005,
VPU_IPIMSG_DEC_CORE_END_ACK = 0xB006,
+ VPU_IPIMSG_DEC_GET_PARAM_ACK = 0xB007,
};
/**
@@ -114,4 +116,38 @@ struct vdec_vpu_ipi_init_ack {
uint32_t inst_id;
};
+/**
+ * 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;
+};
+
+
+/**
+ * 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;
+};
+
#endif
diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
index 21f6d9c5a371..6f9bcc2b0bb9 100644
--- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
+++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
@@ -54,6 +54,27 @@ static void handle_init_ack_msg(const struct vdec_vpu_ipi_init_ack *msg)
}
}
+static void handle_get_param_msg_ack(
+ const struct vdec_vpu_ipi_get_param_ack *msg)
+{
+ struct vdec_vpu_inst *vpu = (struct vdec_vpu_inst *)
+ (unsigned long)msg->ap_inst_addr;
+
+ mtk_vcodec_debug(vpu, "+ ap_inst_addr = 0x%llx", msg->ap_inst_addr);
+
+ /* param_type is enum vdec_get_param_type */
+ switch(msg->param_type) {
+ case 2:
+ vpu->fb_sz[0] = msg->data[0];
+ vpu->fb_sz[1] = msg->data[1];
+ break;
+ default:
+ mtk_vcodec_err(vpu, "invalid get param type=%d", msg->param_type);
+ vpu->failure = 1;
+ break;
+ }
+}
+
/*
* vpu_dec_ipi_handler - Handler for VPU ipi message.
*
@@ -89,6 +110,9 @@ static void vpu_dec_ipi_handler(void *data, unsigned int len, void *priv)
case VPU_IPIMSG_DEC_CORE_END_ACK:
break;
+ case VPU_IPIMSG_DEC_GET_PARAM_ACK:
+ handle_get_param_msg_ack(data);
+ break;
default:
mtk_vcodec_err(vpu, "invalid msg=%X", msg->msg_id);
break;
@@ -217,6 +241,33 @@ int vpu_dec_start(struct vdec_vpu_inst *vpu, uint32_t *data, unsigned int len)
return err;
}
+int vpu_dec_get_param(struct vdec_vpu_inst *vpu, uint32_t *data,
+ unsigned int len, unsigned int param_type)
+{
+ struct vdec_ap_ipi_get_param msg;
+ int i;
+ int err;
+
+ mtk_vcodec_debug_enter(vpu);
+
+ if (len > ARRAY_SIZE(msg.data)) {
+ mtk_vcodec_err(vpu, "invalid len = %d\n", len);
+ return -EINVAL;
+ }
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_id = AP_IPIMSG_DEC_GET_PARAM;
+ msg.inst_id = vpu->inst_id;
+ for (i = 0; i < len; i++)
+ msg.data[i] = data[i];
+ msg.param_type = param_type;
+ msg.codec_type = vpu->codec_type;
+
+ err = vcodec_vpu_send_msg(vpu, (void *)&msg, sizeof(msg));
+ mtk_vcodec_debug(vpu, "- ret=%d", err);
+ return err;
+}
+
int vpu_dec_core(struct vdec_vpu_inst *vpu)
{
return vcodec_send_ap_ipi(vpu, AP_IPIMSG_DEC_CORE);
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];
};
/**
@@ -104,4 +108,15 @@ int vpu_dec_core(struct vdec_vpu_inst *vpu);
*/
int vpu_dec_core_end(struct vdec_vpu_inst *vpu);
+/**
+ * vpu_dec_get_param - get param from scp
+ *
+ * @vpu : instance for vdec_vpu_inst
+ * @data: meta data to pass bitstream info to VPU decoder
+ * @len : meta data length
+ * @param_type : get param type
+ */
+int vpu_dec_get_param(struct vdec_vpu_inst *vpu, uint32_t *data,
+ unsigned int len, unsigned int param_type);
+
#endif
--
2.25.1
On Tue, Dec 28, 2021 at 05:41:37PM +0800, Yunfei Dong wrote:
> From: Yunfei Dong <[email protected]>
[...]
> 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.
> +/**
> + * 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?
> +/**
> + * 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?
What is the purpose of the "reserved" field?
> diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
[...]
> +static void handle_get_param_msg_ack(
> + const struct vdec_vpu_ipi_get_param_ack *msg)
> +{
> + struct vdec_vpu_inst *vpu = (struct vdec_vpu_inst *)
> + (unsigned long)msg->ap_inst_addr;
Does it really need to cast twice?
> +
> + mtk_vcodec_debug(vpu, "+ ap_inst_addr = 0x%llx", msg->ap_inst_addr);
> +
> + /* param_type is enum vdec_get_param_type */
> + switch(msg->param_type) {
> + case 2:
What is 2? Is it GET_PARAM_PIC_INFO?
> +int vpu_dec_get_param(struct vdec_vpu_inst *vpu, uint32_t *data,
> + unsigned int len, unsigned int param_type)
> +{
> + struct vdec_ap_ipi_get_param msg;
> + int i;
> + int err;
> +
> + mtk_vcodec_debug_enter(vpu);
> +
> + if (len > ARRAY_SIZE(msg.data)) {
> + mtk_vcodec_err(vpu, "invalid len = %d\n", len);
> + return -EINVAL;
> + }
> +
> + memset(&msg, 0, sizeof(msg));
> + msg.msg_id = AP_IPIMSG_DEC_GET_PARAM;
> + msg.inst_id = vpu->inst_id;
> + for (i = 0; i < len; i++)
> + msg.data[i] = data[i];
Would it be more concise if using memcpy?
> 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.
Does fb_sz take effect in later patches?
Hi Tzung-Bi,
Thanks for your suggestion.
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 <[email protected]>
>
> [...]
> > 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.
> > +/**
> > + * 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.
> > +/**
> > + * 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.> What is the purpose of the "reserved" field?
>
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
>
"Reserved" is used to let the size of struct is 64 alinged. And maybe
used in the future to extend.
> [...]
> > +static void handle_get_param_msg_ack(
> > + const struct vdec_vpu_ipi_get_param_ack *msg)
> > +{
> > + struct vdec_vpu_inst *vpu = (struct vdec_vpu_inst *)
> > + (unsigned long)msg-
> > >ap_inst_addr;
>
> Does it really need to cast twice?
>
I will check whether it's possible to remove: (unsigned long)
> > .+
> > + mtk_vcodec_debug(vpu, "+ ap_inst_addr = 0x%llx", msg-
> > >ap_inst_addr);
> > +
> > + /* param_type is enum vdec_get_param_type */
> > + switch(msg->param_type) {
> > + case 2:
>
> What is 2? Is it GET_PARAM_PIC_INFO?
>
Yes, it's GET_PARAM_PIC_INFO.
> > +int vpu_dec_get_param(struct vdec_vpu_inst *vpu, uint32_t *data,
> > + unsigned int len, unsigned int param_type)
> > +{
> > + struct vdec_ap_ipi_get_param msg;
> > + int i;
> > + int err;
> > +
> > + mtk_vcodec_debug_enter(vpu);
> > +
> > + if (len > ARRAY_SIZE(msg.data)) {
> > + mtk_vcodec_err(vpu, "invalid len = %d\n", len);
> > + return -EINVAL;
> > + }
> > +
> > + memset(&msg, 0, sizeof(msg));
> > + msg.msg_id = AP_IPIMSG_DEC_GET_PARAM;
> > + msg.inst_id = vpu->inst_id;
> > + for (i = 0; i < len; i++)
> > + msg.data[i] = data[i];
>
> Would it be more concise if using memcpy?
>
Can be fixed.
> > 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.
> Does fb_sz take effect in later patches?
Don't have effect to later patches.
Thanks,
Yunfei Dong
On Wed, Dec 29, 2021 at 2:52 PM [email protected]
<[email protected]> 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 <[email protected]>
> >
> > [...]
> > > 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.