[PATCH v2, 03/12] media: mtk-vcodec: get capture queue buffer size from scp

[yunfei.dong at mediatek.com]

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 <yunfei.dong at 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.  
> > +/**
> > + * 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