Re: [PATCH v1, 7/8] media: uapi: Init VP9 stateless decode params
Hi AngeloGioacchino, Thanks for your suggestion, Separate this patch with mt8195 support, and sent it again. On Thu, 2022-01-27 at 11:35 +0100, AngeloGioacchino Del Regno wrote: > Il 27/01/22 03:55, Yunfei Dong ha scritto: > > Init some of VP9 frame decode params to default value. > > > > Signed-off-by: Yunfei Dong > > Hello Yunfei, > > This patch is not strictly related to MediaTek SoCs, since it's > modfying v4l2-core. > Can you please send this patch separately? > > Thanks, > Angelo > Best Regards, Yunfei Dong > > --- > > drivers/media/v4l2-core/v4l2-ctrls-core.c | 8 > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c > > b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > index 54abe5245dcc..b25c77b8a445 100644 > > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c > > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c > > @@ -112,6 +112,7 @@ static void std_init_compound(const struct > > v4l2_ctrl *ctrl, u32 idx, > > struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture; > > struct v4l2_ctrl_mpeg2_quantisation *p_mpeg2_quant; > > struct v4l2_ctrl_vp8_frame *p_vp8_frame; > > + struct v4l2_ctrl_vp9_frame *p_vp9_frame; > > struct v4l2_ctrl_fwht_params *p_fwht_params; > > void *p = ptr.p + idx * ctrl->elem_size; > > > > @@ -152,6 +153,13 @@ static void std_init_compound(const struct > > v4l2_ctrl *ctrl, u32 idx, > > p_vp8_frame = p; > > p_vp8_frame->num_dct_parts = 1; > > break; > > + case V4L2_CTRL_TYPE_VP9_FRAME: > > + p_vp9_frame = p; > > + p_vp9_frame->profile = 0; > > + p_vp9_frame->bit_depth = 8; > > + p_vp9_frame->flags |= V4L2_VP9_FRAME_FLAG_X_SUBSAMPLING > > | > > + V4L2_VP9_FRAME_FLAG_Y_SUBSAMPLING; > > + break; > > case V4L2_CTRL_TYPE_FWHT_PARAMS: > > p_fwht_params = p; > > p_fwht_params->version = V4L2_FWHT_VERSION; > > > >
Re: [PATCH v1, 7/8] media: uapi: Init VP9 stateless decode params
Hi Chen-Yu,
Thanks for your suggestion.
Send this patch again.
On Thu, 2022-01-27 at 17:23 +0800, Chen-Yu Tsai wrote:
> Hi,
>
> On Thu, Jan 27, 2022 at 10:56 AM Yunfei Dong <
> yunfei.d...@mediatek.com> wrote:
> >
> > Init some of VP9 frame decode params to default value.
> >
> > Signed-off-by: Yunfei Dong
>
> Maybe add
>
> Fixes: b88dbe38dca8 ("media: uapi: Add VP9 stateless decoder
> controls")
>
Best Regards,
Yunfei Dong
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls-core.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > index 54abe5245dcc..b25c77b8a445 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> > @@ -112,6 +112,7 @@ static void std_init_compound(const struct
> > v4l2_ctrl *ctrl, u32 idx,
> > struct v4l2_ctrl_mpeg2_picture *p_mpeg2_picture;
> > struct v4l2_ctrl_mpeg2_quantisation *p_mpeg2_quant;
> > struct v4l2_ctrl_vp8_frame *p_vp8_frame;
> > + struct v4l2_ctrl_vp9_frame *p_vp9_frame;
> > struct v4l2_ctrl_fwht_params *p_fwht_params;
> > void *p = ptr.p + idx * ctrl->elem_size;
> >
> > @@ -152,6 +153,13 @@ static void std_init_compound(const struct
> > v4l2_ctrl *ctrl, u32 idx,
> > p_vp8_frame = p;
> > p_vp8_frame->num_dct_parts = 1;
> > break;
> > + case V4L2_CTRL_TYPE_VP9_FRAME:
> > + p_vp9_frame = p;
> > + p_vp9_frame->profile = 0;
> > + p_vp9_frame->bit_depth = 8;
> > + p_vp9_frame->flags |=
> > V4L2_VP9_FRAME_FLAG_X_SUBSAMPLING |
> > + V4L2_VP9_FRAME_FLAG_Y_SUBSAMPLING;
> > + break;
> > case V4L2_CTRL_TYPE_FWHT_PARAMS:
> > p_fwht_params = p;
> > p_fwht_params->version = V4L2_FWHT_VERSION;
> > --
> > 2.25.1
> >
Re: [PATCH v2, 03/12] media: mtk-vcodec: get capture queue buffer size from scp
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
>
> [...]
> > 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(, 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
> > ---
Re: [PATCH v1, 12/12] media: mtk-vcodec: Add h264 slice api driver for mt8192
Hi Nicolas,
Thanks for your suggestion.
On Wed, 2021-12-15 at 10:27 -0500, Nicolas Dufresne wrote:
> Hi Yunfei,
>
> Le mercredi 15 décembre 2021 à 14:59 +0800, Yunfei Dong a écrit :
> > From: Yunfei Dong
> >
> > Adds h264 lat and core driver for mt8192.
>
> This is purely a nit, but I have first notice the usage of "slice" in
> the
> namespace and the title, which lead me to think this new platform was
> V4L2_STATELESS_H264_DECODE_MODE_SLICE_BASED. I think some structure
> which are
> clearly frame_based should probably be renamed (its the namespace
> that is
> confusing) to reduce the confusion.
>
This driver is frame_based used for mt8182 lat and core architecture.
I will fix the commit in next patch.
Thanks for your remind.
Best Regards,
Yunfei Dong
> p.s. Note that adding slice_based mode would be amazing for streaming
> with ultra
> low latency (think remote video games)
>
> regards,
> Nicolas
>
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > drivers/media/platform/mtk-vcodec/Makefile| 1 +
> > .../mtk-vcodec/vdec/vdec_h264_req_lat_if.c| 620
> > ++
> > .../media/platform/mtk-vcodec/vdec_drv_if.c | 8 +-
> > .../media/platform/mtk-vcodec/vdec_drv_if.h | 1 +
> > include/linux/remoteproc/mtk_scp.h| 2 +
> > 5 files changed, 631 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/vdec/vdec_h264_req_lat_if.c
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > b/drivers/media/platform/mtk-vcodec/Makefile
> > index 3f41d748eee5..1777d7606f0d 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -10,6 +10,7 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > vdec/vdec_vp9_if.o \
> > vdec/vdec_h264_req_if.o \
> > vdec/vdec_h264_req_common.o \
> > + vdec/vdec_h264_req_lat_if.o \
> > mtk_vcodec_dec_drv.o \
> > vdec_drv_if.o \
> > vdec_vpu_if.o \
> > diff --git a/drivers/media/platform/mtk-
> > vcodec/vdec/vdec_h264_req_lat_if.c b/drivers/media/platform/mtk-
> > vcodec/vdec/vdec_h264_req_lat_if.c
> > new file mode 100644
> > index ..403d7df00e1d
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_lat_if.c
> > @@ -0,0 +1,620 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2021 MediaTek Inc.
> > + * Author: Yunfei Dong
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "../mtk_vcodec_util.h"
> > +#include "../mtk_vcodec_dec.h"
> > +#include "../mtk_vcodec_intr.h"
> > +#include "../vdec_drv_base.h"
> > +#include "../vdec_drv_if.h"
> > +#include "../vdec_vpu_if.h"
> > +#include "vdec_h264_req_common.h"
> > +
> > +/**
> > + * enum vdec_h264_core_dec_err_type - core decode error type
> > + */
> > +enum vdec_h264_core_dec_err_type {
> > + TRANS_BUFFER_FULL = 1,
> > + SLICE_HEADER_FULL,
> > +};
> > +
> > +/**
> > + * struct vdec_h264_slice_lat_dec_param - parameters for decode
> > current frame
> > + * @sps : h264 sps syntax parameters
> > + * @pps : h264 pps syntax parameters
> > + * @slice_header: h264 slice header syntax parameters
> > + * @scaling_matrix : h264 scaling list parameters
> > + * @decode_params : decoder parameters of each frame used for
> > hardware decode
> > + * @h264_dpb_info : dpb reference list
> > + */
> > +struct vdec_h264_slice_lat_dec_param {
> > + struct mtk_h264_sps_param sps;
> > + struct mtk_h264_pps_param pps;
> > + struct mtk_h264_slice_hd_param slice_header;
> > + struct slice_api_h264_scaling_matrix scaling_matrix;
> > + struct slice_api_h264_decode_param decode_params;
> > + struct mtk_h264_dpb_info
> > h264_dpb_info[V4L2_H264_NUM_DPB_ENTRIES];
> > +};
> > +
> > +/**
> > + * struct vdec_h264_slice_info - decode information
> > + * @nal_info: nal info of current picture
> > + * @timeout : Decode timeout: 1 timeout, 0 no timeount
> > + * @bs_buf_size : bitstream size
> > + * @bs_buf_addr : bitstream buffer dma address
> > + * @y_fb_dma: Y frame buffer dma address
> > + * @c_fb_dma: C frame buffer dma address
> > + * @vdec_fb_va : VDEC frame buffer struct virtual address
> > + * @crc : Used to check whether hardware's status is right
> > + */
> > +struct vdec_h264_slice_info {
> > + uint16_t nal_info;
> > + uint16_t timeout;
> > + uint32_t bs_buf_size;
> > + uint64_t bs_buf_addr;
> > + uint64_t y_fb_dma;
> > + uint64_t c_fb_dma;
> > + uint64_t vdec_fb_va;
> > + uint32_t crc[8];
> > +};
> > +
> > +/**
> > + * struct vdec_h264_slice_vsi - shared memory for decode
> > information exchange
> > + *between VPU and Host. The memory is allocated by VPU
> > then mapping to
> > + *Host in vdec_h264_slice_init() and freed in
> > vdec_h264_slice_deinit()
> > + *by VPU. AP-W/R : AP is writer/reader on this item. VPU-
> > W/R: VPU is
>
Re: [PATCH v12, 13/19] media: mtk-vcodec: Add work queue for core hardware decode
Hi steve,
Thanks for your suggestion.
On Thu, 2021-12-09 at 15:44 -0800, Steve Cho wrote:
> On Wed, Dec 1, 2021 at 7:46 PM Yunfei Dong
> wrote:
> >
> > Add work queue to process core hardware information.
> > First, get lat_buf from message queue, then call core
> > hardware of each codec(H264/VP9/AV1) to decode, finally
> > puts lat_buf back to the message.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 16 +++-
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 3 ++
> > .../platform/mtk-vcodec/vdec_msg_queue.c | 41
> > ---
> > .../platform/mtk-vcodec/vdec_msg_queue.h | 8 ++--
> > 4 files changed, 57 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index d460703f335d..4fbff61d2334 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -341,6 +341,17 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > goto err_dec_pm;
> > }
> >
> > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) {
> > + vdec_msg_queue_init_ctx(>msg_queue_core_ctx,
> > MTK_VDEC_CORE);
> > + dev->core_workqueue =
> > alloc_ordered_workqueue("core-decoder",
> > + WQ_MEM_RECLAIM | WQ_FREEZABLE);
> > + if (!dev->core_workqueue) {
> > + mtk_v4l2_err("Failed to create core
> > workqueue");
> > + ret = -EINVAL;
> > + goto err_res;
> > + }
> > + }
> > +
> > for (i = 0; i < MTK_VDEC_HW_MAX; i++)
> > mutex_init(>dec_mutex[i]);
> > spin_lock_init(>irqlock);
> > @@ -351,7 +362,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > ret = v4l2_device_register(>dev, >v4l2_dev);
> > if (ret) {
> > mtk_v4l2_err("v4l2_device_register err=%d", ret);
> > - goto err_res;
> > + goto err_core_workq;
> > }
> >
> > init_waitqueue_head(>queue);
> > @@ -450,6 +461,9 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > video_unregister_device(vfd_dec);
> > err_dec_alloc:
> > v4l2_device_unregister(>v4l2_dev);
> > +err_core_workq:
> > + if (IS_VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch))
> > + destroy_workqueue(dev->core_workqueue);
> > err_res:
> > mtk_vcodec_release_dec_pm(>pm);
> > err_dec_pm:
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > index cbaed96dcfa2..a558cc16026d 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > @@ -27,6 +27,7 @@
> > #define MTK_VCODEC_MAX_PLANES 3
> > #define MTK_V4L2_BENCHMARK 0
> > #define WAIT_INTR_TIMEOUT_MS 1000
> > +#define IS_VDEC_LAT_ARCH(hw_arch) ((hw_arch) >=
> > MTK_VDEC_LAT_SINGLE_CORE)
>
> Basic question: What is practical meaning of this? What architectures
> are supported?
>
This definition is used to separate different architectures.
Pure single core/lat single core at current period is supported.
> >
> > /*
> > * enum mtk_hw_reg_idx - MTK hw register base index
> > @@ -464,6 +465,7 @@ struct mtk_vcodec_enc_pdata {
> > * @dec_capability: used to identify decode capability, ex: 4k
> > * @enc_capability: used to identify encode capability
> > *
> > + * @core_workqueue: queue used for core hardware decode
> > * @msg_queue_core_ctx: msg queue context used for core workqueue
> > *
> > * @subdev_dev: subdev hardware device
> > @@ -506,6 +508,7 @@ struct mtk_vcodec_dev {
> > unsigned int dec_capability;
> > unsigned int enc_capability;
> >
> > + struct workqueue_struct *core_workqueue;
> > struct vdec_msg_queue_ctx msg_queue_core_ctx;
> >
> > void *subdev_dev[MTK_VDEC_HW_MAX];
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > index 913aefa67618..24f1d03df9f1 100644
> > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > @@ -68,6 +68,9 @@ int vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx
> > *msg_ctx, struct vdec_lat_buf
> >
> > if (msg_ctx->hardware_index != MTK_VDEC_CORE)
> > wake_up_all(_ctx->ready_to_use);
> > + else
> > + queue_work(buf->ctx->dev->core_workqueue,
> > + >ctx->msg_queue.core_work);
>
> need {} for else here?
>
If condition not add "{}", else need not to add "{}" ?
> >
> > mtk_v4l2_debug(3, "enqueue buf type: %d addr: 0x%p num:
> > %d",
> > msg_ctx->hardware_index, buf,
Re: [PATCH v12, 06/19] media: mtk-vcodec: Add to support multi hardware decode
Hi Steve,
Thanks for your suggestion,
On Thu, 2021-12-09 at 15:29 -0800, Steve Cho wrote:
> Few comments and questions.
>
> On Wed, Dec 1, 2021 at 7:46 PM Yunfei Dong
> wrote:
> >
> > There are more than two hardwares for decoder: LAT0, LAT1 and CORE.
> > In order to
> > manage these hardwares, register each hardware as independent
> > platform device
> > for the larbs are different.
>
> basic question: what is "larbs"?
>
Can regard larbs as some hardware which decode depends on.
> >
> > Each hardware module controls its own information which includes
> > interrupt/power/
> > clocks/registers.
> >
> > Calling of_platform_populate in parent device, and use
> > subdev_bitmap to record
> > whether the hardwares are registered done.
>
> nit: s/registered done/registered/ ?
>
Fix in v13.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > drivers/media/platform/mtk-vcodec/Makefile| 5 +-
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 111 +++
> > .../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 172
> > ++
> > .../platform/mtk-vcodec/mtk_vcodec_dec_hw.h | 51 ++
> > .../mtk-vcodec/mtk_vcodec_dec_stateful.c | 1 +
> > .../mtk-vcodec/mtk_vcodec_dec_stateless.c | 2 +
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 19 ++
> > 7 files changed, 329 insertions(+), 32 deletions(-)
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/mtk_vcodec_dec_hw.c
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/mtk_vcodec_dec_hw.h
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > b/drivers/media/platform/mtk-vcodec/Makefile
> > index ca8e9e7a9c4e..c61bfb179bcc 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -2,7 +2,8 @@
> >
> > obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> >mtk-vcodec-enc.o \
> > - mtk-vcodec-common.o
> > + mtk-vcodec-common.o \
> > + mtk-vcodec-dec-hw.o
> >
> > mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > vdec/vdec_vp8_if.o \
> > @@ -16,6 +17,8 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > mtk_vcodec_dec_stateless.o \
> > mtk_vcodec_dec_pm.o \
> >
> > +mtk-vcodec-dec-hw-y := mtk_vcodec_dec_hw.o
> > +
> > mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> > venc/venc_h264_if.o \
> > mtk_vcodec_enc.o \
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index b7a51e96d4ba..95fbe9be3f6d 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -18,19 +18,40 @@
> >
> > #include "mtk_vcodec_drv.h"
> > #include "mtk_vcodec_dec.h"
> > +#include "mtk_vcodec_dec_hw.h"
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_intr.h"
> > #include "mtk_vcodec_util.h"
> > #include "mtk_vcodec_fw.h"
> >
> > -#define VDEC_HW_ACTIVE 0x10
> > -#define VDEC_IRQ_CFG 0x11
> > -#define VDEC_IRQ_CLR 0x10
> > -#define VDEC_IRQ_CFG_REG 0xa4
> > -
> > module_param(mtk_v4l2_dbg_level, int, 0644);
> > module_param(mtk_vcodec_dbg, bool, 0644);
> >
> > +static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_dev
> > *vdec_dev)
> > +{
> > + struct platform_device *pdev = vdec_dev->plat_dev;
> > + struct device_node *subdev_node;
> > + enum mtk_vdec_hw_id hw_idx;
> > + const struct of_device_id *of_id;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mtk_vdec_hw_match); i++) {
> > + of_id = _vdec_hw_match[i];
> > + subdev_node = of_find_compatible_node(NULL, NULL,
> > + of_id->compatible);
> > + if (!subdev_node)
> > + continue;
> > +
> > + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id-
> > >data;
> > + if (!test_bit(hw_idx, vdec_dev->subdev_bitmap)) {
> > + dev_err(>dev, "Vdec %d is not ready",
> > hw_idx);
> > + return -EAGAIN;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv)
> > {
> > struct mtk_vcodec_dev *dev = priv;
> > @@ -95,6 +116,42 @@ static int mtk_vcodec_get_reg_bases(struct
> > mtk_vcodec_dev *dev)
> > return 0;
> > }
> >
> > +static int mtk_vcodec_init_dec_resources(struct mtk_vcodec_dev
> > *dev)
> > +{
> > + struct platform_device *pdev = dev->plat_dev;
> > + int ret;
> > +
> > + ret = mtk_vcodec_get_reg_bases(dev);
> > + if (ret)
> > + return ret;
> > +
> > + if (dev->vdec_pdata->is_subdev_supported)
> > + return 0;
> > +
> > +
Re: [PATCH v12, 15/19] dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8192
Hi Rob, Thanks for your suggestion. On Fri, 2021-12-10 at 10:49 -0600, Rob Herring wrote: > On Thu, Dec 02, 2021 at 11:45:40AM +0800, Yunfei Dong wrote: > > Adds decoder dt-bindings for mt8192. > > > > Signed-off-by: Yunfei Dong > > --- > > .../media/mediatek,vcodec-subdev-decoder.yaml | 266 > > ++ > > 1 file changed, 266 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/media/mediatek,vcodec-subdev- > > decoder.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev- > > decoder.yaml > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev- > > decoder.yaml > > new file mode 100644 > > index ..67cbcf8b3373 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > subdev-decoder.yaml > > @@ -0,0 +1,266 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > + > > +%YAML 1.2 > > +--- > > +$id: " > > http://devicetree.org/schemas/media/mediatek,vcodec-subdev-decoder.yaml# > > " > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#; > > + > > +title: Mediatek Video Decode Accelerator With Multi Hardware > > + > > +maintainers: > > + - Yunfei Dong > > + > > +description: | > > + Mediatek Video Decode is the video decode hardware present in > > Mediatek > > + SoCs which supports high resolution decoding functionalities. > > Required > > + parent and child device node. > > + > > + About the Decoder Hardware Block Diagram, please check below: > > + > > ++-+--- > > -+ > > +| | > > | > > +| input -> lat HW -> lat buffer --|--> lat buffer -> core HW > > -> output | > > +||| | || > > | > > ++||---+-||-- > > ---+ > > + lat workqueue | core > > workqueue > > +-||-||-- > > > > + || || > > > > + \/\/ > > + + > > --+ > > + |enable/disable > > | > > + | clk powerirqiommu > > | > > + | (lat/lat > > soc/core0/core1)| > > + + > > --+ > > + > > + As above, there are parent and child devices, child mean each > > hardware. The child device > > + controls the information of each hardware independent which > > include clk/power/irq. > > + > > + There are two workqueues in parent device: lat workqueue and > > core workqueue. They are used > > + to lat and core hardware deocder. Lat workqueue need to get > > input bitstream and lat buffer, > > + then enable lat to decode, writing the result to lat buffer, > > dislabe hardware when lat decode > > + done. Core workqueue need to get lat buffer and output buffer, > > then enable core to decode, > > + writing the result to output buffer, disable hardware when core > > decode done. These two > > + hardwares will decode each frame cyclically. > > + > > + For the smi common may not the same for each hardware, can't > > combine all hardware in one node, > > + or leading to iommu fault when access dram data. > > + > > +properties: > > + compatible: > > +const: mediatek,mt8192-vcodec-dec > > + > > + reg: > > +maxItems: 1 > > + > > + iommus: > > +minItems: 1 > > +maxItems: 32 > > +description: | > > + List of the hardware port in respective IOMMU block for > > current Socs. > > + Refer to bindings/iommu/mediatek,iommu.yaml. > > + > > + mediatek,scp: > > +$ref: /schemas/types.yaml#/definitions/phandle > > +maxItems: 1 > > +description: | > > + The node of system control processor (SCP), using > > + the remoteproc & rpmsg framework. > > + $ref: /schemas/remoteproc/mtk,scp.yaml > > '$ref' is not valid here. Just 'See remoteproc/mtk,scp.yaml' > Remove this line. > > + > > + dma-ranges: > > +maxItems: 1 > > +description: | > > + Describes the physical address space of IOMMU maps to > > memory. > > + > > + "#address-cells": > > +const: 1 > > + > > + "#size-cells": > > +const: 1 > > + > > + ranges: true > > + > > +# Required child node: > > +patternProperties: > > + vcodec-lat: > > '^vcodec-lat@[0-9a-f]+$': > Fix in v13. > > +type: object > > + > > +properties: > > + compatible: > > +const: mediatek,mtk-vcodec-lat > > + > > + reg: > > +maxItems: 1 > > + > > + interrupts: > > +maxItems: 1 > > + > > + iommus: > > +minItems: 1 >
Re: [PATCH v11, 10/19] media: mtk-vcodec: Add msg queue feature for lat and core architecture
Hi AngeloGioacchino,
Thanks for your suggestion.
On Wed, 2021-12-01 at 13:09 +0100, AngeloGioacchino Del Regno wrote:
> Il 29/11/21 04:41, Yunfei Dong ha scritto:
> > For lat and core architecture, lat thread will send message to core
> > thread when lat decode done. Core hardware will use the message
> > from lat to decode, then free message to lat thread when decode
> > done.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > drivers/media/platform/mtk-vcodec/Makefile| 1 +
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 9 +
> > .../platform/mtk-vcodec/vdec_msg_queue.c | 257
> > ++
> > .../platform/mtk-vcodec/vdec_msg_queue.h | 148 ++
> > 4 files changed, 415 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/vdec_msg_queue.c
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/vdec_msg_queue.h
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > b/drivers/media/platform/mtk-vcodec/Makefile
> > index c61bfb179bcc..359619653a0e 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -12,6 +12,7 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > mtk_vcodec_dec_drv.o \
> > vdec_drv_if.o \
> > vdec_vpu_if.o \
> > + vdec_msg_queue.o \
> > mtk_vcodec_dec.o \
> > mtk_vcodec_dec_stateful.o \
> > mtk_vcodec_dec_stateless.o \
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > index 7fc106df039b..610b0af13879 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > @@ -15,7 +15,9 @@
> > #include
> > #include
> > #include
> > +
> > #include "mtk_vcodec_util.h"
> > +#include "vdec_msg_queue.h"
> >
> > #define MTK_VCODEC_DRV_NAME "mtk_vcodec_drv"
> > #define MTK_VCODEC_DEC_NAME "mtk-vcodec-dec"
> > @@ -282,6 +284,8 @@ struct vdec_pic_info {
> >* @decoded_frame_cnt: number of decoded frames
> >* @lock: protect variables accessed by V4L2 threads and worker
> > thread such as
> >* mtk_video_dec_buf.
> > + *
> > + * @msg_queue: msg queue used to store lat buffer information.
> >*/
> > struct mtk_vcodec_ctx {
> > enum mtk_instance_type type;
> > @@ -325,6 +329,7 @@ struct mtk_vcodec_ctx {
> > int decoded_frame_cnt;
> > struct mutex lock;
> >
> > + struct vdec_msg_queue msg_queue;
> > };
> >
> > enum mtk_chip {
> > @@ -457,6 +462,8 @@ struct mtk_vcodec_enc_pdata {
> >* @dec_capability: used to identify decode capability, ex: 4k
> >* @enc_capability: used to identify encode capability
> >*
> > + * @msg_queue_core_ctx: msg queue context used for core workqueue
> > + *
> >* @subdev_dev: subdev hardware device
> >* @subdev_bitmap: used to record hardware is ready or not
> >*/
> > @@ -497,6 +504,8 @@ struct mtk_vcodec_dev {
> > unsigned int dec_capability;
> > unsigned int enc_capability;
> >
> > + struct vdec_msg_queue_ctx msg_queue_core_ctx;
> > +
> > void *subdev_dev[MTK_VDEC_HW_MAX];
> > DECLARE_BITMAP(subdev_bitmap, MTK_VDEC_HW_MAX);
> > };
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > new file mode 100644
> > index ..da4d114f7ad0
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > @@ -0,0 +1,257 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2021 MediaTek Inc.
> > + * Author: Yunfei Dong
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include "mtk_vcodec_dec_pm.h"
> > +#include "mtk_vcodec_drv.h"
> > +#include "vdec_msg_queue.h"
> > +
> > +/* the size used to store lat slice header information */
> > +#define VDEC_LAT_SLICE_HEADER_SZ(640 * SZ_1K)
> > +
> > +/* the size used to store avc error information */
> > +#define VDEC_ERR_MAP_SZ_AVC (17 * SZ_1K)
> > +
> > +/* core will read the trans buffer which decoded by lat to decode
> > again.
> > + * The trans buffer size of FHD and 4K bitstreams are different.
> > + */
> > +static int vde_msg_queue_get_trans_size(int width, int height)
> > +{
> > + if (width > 1920 || height > 1088)
> > + return 30 * SZ_1M;
> > + else
> > + return 6 * SZ_1M;
> > +}
> > +
> > +void vdec_msg_queue_init_ctx(struct vdec_msg_queue_ctx *ctx,
> > + int hardware_index)
> > +{
> > + init_waitqueue_head(>ready_to_use);
> > + INIT_LIST_HEAD(>ready_queue);
> > + spin_lock_init(>ready_lock);
> > + ctx->ready_num = 0;
> > + ctx->hardware_index = hardware_index;
> > +}
> > +
> > +static struct list_head *vdec_get_buf_list(int hardware_index,
> > + struct vdec_lat_buf *buf)
> > +{
> > + switch (hardware_index) {
> > + case MTK_VDEC_CORE:
> > +
Re: [PATCH v11, 04/19] media: mtk-vcodec: export decoder pm functions
Hi Benjamin,
Thanks for your suggestion.
On Tue, 2021-11-30 at 14:34 +0100, Benjamin Gaignard wrote:
> Le 29/11/2021 à 04:41, Yunfei Dong a écrit :
> > Register each hardware as platform device, need to call pm
> > functions
> > to open/close power and clock from module mtk-vcodec-dec, export
> > these
> > functions.
>
> The commit message confuse me, maybe something like:
> "When mtk vcodec decoder is build as a module we need to export
> mtk-vcodec-dec pm functions to make them visible by the other
> components"
>
Fix in patch v12
> With that:
> Reviewed-by: Benjamin Gaignard
>
Best Regards,
Yunfei Dong
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > index 20bd157a855c..221cf60e9fbf 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -77,12 +77,14 @@ int mtk_vcodec_init_dec_pm(struct
> > platform_device *pdev,
> > put_device(pm->larbvdec);
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_init_dec_pm);
> >
> > void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm)
> > {
> > pm_runtime_disable(pm->dev);
> > put_device(pm->larbvdec);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_release_dec_pm);
> >
> > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> > {
> > @@ -94,6 +96,7 @@ int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm
> > *pm)
> >
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_pw_on);
> >
> > void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
> > {
> > @@ -103,6 +106,7 @@ void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm
> > *pm)
> > if (ret)
> > mtk_v4l2_err("pm_runtime_put_sync fail %d", ret);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_pw_off);
> >
> > void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm)
> > {
> > @@ -129,6 +133,7 @@ void mtk_vcodec_dec_clock_on(struct
> > mtk_vcodec_pm *pm)
> > for (i -= 1; i >= 0; i--)
> > clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_clock_on);
> >
> > void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm)
> > {
> > @@ -139,3 +144,4 @@ void mtk_vcodec_dec_clock_off(struct
> > mtk_vcodec_pm *pm)
> > for (i = dec_clk->clk_num - 1; i >= 0; i--)
> > clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_clock_off);
Re: [PATCH v11, 01/19] media: mtk-vcodec: Get numbers of register bases from DT
Hi Steve,
Thanks for your suggestion.On Wed, 2021-12-01 at 15:36 -0800, Steve Cho
wrote:
> LGTM with few nits.
>
> Thanks,Steve
>
> On Sun, Nov 28, 2021 at 7:44 PM Yunfei Dong > wrote:
> > Different platform may has different numbers of register bases.
> > Gets the
> >
> > numbers of register bases from DT (sizeof(u32) * 4 bytes for each)
> Few nits.
> s/platform/platforms/
> s/has/have/
>
> Fix, DT is dts.
> Btw, what is DT?
> >
> >
> > Reviewed-by: Tzung-Bi Shih
> >
> > Signed-off-by: Yunfei Dong
> >
> > ---
> >
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 37 ++-
> >
> >
> > 1 file changed, 28 insertions(+), 9 deletions(-)
> >
> >
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> >
> > index e6e6a8203eeb..59caf2163349 100644
> >
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> >
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> >
> > @@ -78,6 +78,30 @@ static irqreturn_t
> > mtk_vcodec_dec_irq_handler(int irq, void *priv)
> >
> > return IRQ_HANDLED;
> >
> > }
> >
> >
> >
> > +static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
> >
> > +{
> I see that dev is already checked before entering into this function,
> but null check for dev would still be nice.
>
Dev is never null in this function, whether it looks not very
reasonable?
Best Regards,Yunfei Dong
> > + struct platform_device *pdev = dev->plat_dev;
> >
> > + int reg_num, i;
> >
> > +
> >
> > + /* Sizeof(u32) * 4 bytes for each register base. */
> >
> > + reg_num = of_property_count_elems_of_size(pdev-
> > >dev.of_node, "reg",
> >
> > + sizeof(u32) * 4);
> >
> > + if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE) {
> >
> > + dev_err(>dev, "Invalid register property
> > size: %d\n", reg_num);
> >
> > + return -EINVAL;
> >
> > + }
> >
> > +
> >
> > + for (i = 0; i < reg_num; i++) {
> >
> > + dev->reg_base[i] =
> > devm_platform_ioremap_resource(pdev, i);
> >
> > + if (IS_ERR(dev->reg_base[i]))
> >
> > + return PTR_ERR(dev->reg_base[i]);
> >
> > +
> >
> > + mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev-
> > >reg_base[i]);
> >
> > + }
> >
> > +
> >
> > + return 0;
> >
> > +}
> >
> > +
> >
> > static int fops_vcodec_open(struct file *file)
> >
> > {
> >
> > struct mtk_vcodec_dev *dev = video_drvdata(file);
> >
> > @@ -206,7 +230,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> >
> > struct resource *res;
> >
> > phandle rproc_phandle;
> >
> > enum mtk_vcodec_fw_type fw_type;
> >
> > - int i, ret;
> >
> > + int ret;
> >
> >
> >
> > dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
> >
> > if (!dev)
> >
> > @@ -238,14 +262,9 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> >
> > goto err_dec_pm;
> >
> > }
> >
> >
> >
> > - for (i = 0; i < NUM_MAX_VDEC_REG_BASE; i++) {
> >
> > - dev->reg_base[i] =
> > devm_platform_ioremap_resource(pdev, i);
> >
> > - if (IS_ERR((__force void *)dev->reg_base[i])) {
> >
> > - ret = PTR_ERR((__force void *)dev-
> > >reg_base[i]);
> >
> > - goto err_res;
> >
> > - }
> >
> > - mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev-
> > >reg_base[i]);
> >
> > - }
> >
> > + ret = mtk_vcodec_get_reg_bases(dev);
> >
> > + if (ret)
> >
> > + goto err_res;
> >
> >
> >
> > res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> >
> > if (res == NULL) {
> >
Re: [PATCH v11, 01/19] media: mtk-vcodec: Get numbers of register bases from DT
Hi Benjamin,
Thanks for your suggestion.
On Tue, 2021-11-30 at 14:20 +0100, Benjamin Gaignard wrote:
> Le 29/11/2021 à 04:41, Yunfei Dong a écrit :
> > Different platform may has different numbers of register bases.
> > Gets the
> > numbers of register bases from DT (sizeof(u32) * 4 bytes for each).
> >
> > Reviewed-by: Tzung-Bi Shih
> > Signed-off-by: Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 37
> > ++-
> > 1 file changed, 28 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index e6e6a8203eeb..59caf2163349 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -78,6 +78,30 @@ static irqreturn_t
> > mtk_vcodec_dec_irq_handler(int irq, void *priv)
> > return IRQ_HANDLED;
> > }
> >
> > +static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
> > +{
> > + struct platform_device *pdev = dev->plat_dev;
> > + int reg_num, i;
> > +
> > + /* Sizeof(u32) * 4 bytes for each register base. */
> > + reg_num = of_property_count_elems_of_size(pdev->dev.of_node,
> > "reg",
> > + sizeof(u32) * 4);
>
> It looks strange for me to have a "reg" size equal to sizeof(u32) *
> 4. Usually
> we more see reg size = sizeof(u32).
>
For the definition of reg in dtsi is reg = <0 0x1600 0 0x800>,
Need to using sizeof(u32) * 4, not sizeof(u32).
> > + if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE) {
>
> If reg_num = NUM_MAX_VDEC_REG_BASE you will iterate out of bounds of
> dev->reg_base array.
> That never happens because dev->reg_base size equal
> NUM_MAX_VCODEC_REG_BASE.
> The question is what is the real needed size for dev->reg_base array
> ? NUM_MAX_VDEC_REG_BASE or
> NUM_MAX_VCODEC_REG_BASE ?
>
> NUM_MAX_VDEC_REG_BASE is used for video decoder, and
> NUM_MAX_VCODEC_REG_BASE is used for encoder.
The max register number of video decoder is NUM_MAX_VDEC_REG_BASE, and
the ranges is 0 ~ NUM_MAX_VDEC_REG_BASE - 1.
> Regards,
> Benjamin
>
Best Regards,
Yunfei Dong
> > + dev_err(>dev, "Invalid register property size:
> > %d\n", reg_num);
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < reg_num; i++) {
> > + dev->reg_base[i] = devm_platform_ioremap_resource(pdev,
> > i);
> > + if (IS_ERR(dev->reg_base[i]))
> > + return PTR_ERR(dev->reg_base[i]);
> > +
> > + mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev-
> > >reg_base[i]);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int fops_vcodec_open(struct file *file)
> > {
> > struct mtk_vcodec_dev *dev = video_drvdata(file);
> > @@ -206,7 +230,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > struct resource *res;
> > phandle rproc_phandle;
> > enum mtk_vcodec_fw_type fw_type;
> > - int i, ret;
> > + int ret;
> >
> > dev = devm_kzalloc(>dev, sizeof(*dev), GFP_KERNEL);
> > if (!dev)
> > @@ -238,14 +262,9 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > goto err_dec_pm;
> > }
> >
> > - for (i = 0; i < NUM_MAX_VDEC_REG_BASE; i++) {
> > - dev->reg_base[i] = devm_platform_ioremap_resource(pdev,
> > i);
> > - if (IS_ERR((__force void *)dev->reg_base[i])) {
> > - ret = PTR_ERR((__force void *)dev-
> > >reg_base[i]);
> > - goto err_res;
> > - }
> > - mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev-
> > >reg_base[i]);
> > - }
> > + ret = mtk_vcodec_get_reg_bases(dev);
> > + if (ret)
> > + goto err_res;
> >
> > res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > if (res == NULL) {
Re: [PATCH v10, 00/19] Support multi hardware decode using of_platform_populate
Hi Ezequiel,
On Tue, 2021-11-16 at 09:06 -0300, Ezequiel Garcia wrote:
> Hi Yunfei,
>
> On Tue, 16 Nov 2021 at 08:42, yunfei.d...@mediatek.com
> wrote:
> >
> > Hi Ezequiel,
> >
> > Thanks for you suggestion.
> > On Sun, 2021-11-14 at 19:04 -0300, Ezequiel Garcia wrote:
> > > Hi Yunfei,
> > >
> > > On Thu, 11 Nov 2021 at 01:15, Yunfei Dong <
> > > yunfei.d...@mediatek.com>
> > > wrote:
> > > >
> > > > This series adds support for multi hardware decode into mtk-
> > > > vcodec,
> > > > by first adding use
> > > > of_platform_populate to manage each hardware information:
> > > > interrupt, clock, register
> > > > bases and power. Secondly add core work queue to deal with core
> > > > hardware message,
> > > > at the same time, add msg queue for different hardware share
> > > > messages. Lastly, the
> > > > architecture of different specs are not the same, using specs
> > > > type
> > > > to separate them.
> > > >
> > > > This series has been tested with both MT8183 and MT8173.
> > > > Decoding
> > > > was working for both chips.
> > > >
> > >
> > > How are you testing Decoding? If you are running some test suite,
> > > it
> > > would
> > > be good to add such information.
> > >
> >
> > For 8183 and 8173, these patches don't have influence for them, can
> > work well.
> >
> > These patches are used for mt8192 and 8195. We do cts/gts/stress
> > test
> > to check our drivers.
>
> What is the meaning of cts/gts/stress tests?
> Could you share the sources of the test?
>
CTS is android compatibility test suite. You can get the information
from: https://source.android.com/compatibility.
GTS is google Mobile Service Test Suite.
Need to run the test suites to test vcodec drivers. Regards them as
google android certification.
> Thanks,
> Ezequiel
>
Thanks,
Yunfei Dong
> > > Are you testing some edge-cases such as parallel/concurrent
> > > decoding,
> > > removing the driver while streaming, and so on? This should help
> > > catch
> > > some typical issues.
> > >
> > > Thanks,
> > > Ezequiel
> > >
> >
> > Thanks,
> > Yunfei Dong
> > > > Patches 1~3 rewrite get register bases and power on/off
> > > > interface.
> > > > Patches 4 export decoder pm interfaces.
> > > > Patches 5 add to support 8192.
> > > > Patch 6 support multi hardware.
> > > > Patch 7 separate video encoder and decoder document
> > > > Patch 8-17 add interfaces to support core hardware.
> > > > Patch 18-19 remove mtk_vcodec_release_dec/enc_pm interfaces.
> > > > ---
> > > > changes compared with v9:
> > > > - need not to build ko, just export pm interfaces for patch
> > > > 04/19.
> > > > - fix comments for patch 06/19
> > > >
> > > > changes compared with v8:
> > > > - add new patch 18~19 to remove mtk_vcodec_release_de/enc_pm
> > > > interfaces.
> > > > - fix spelling mistakes for patch 17/19
> > > > - fix yaml comments for patch 15/19
> > > >
> > > > Changes compared with v7:
> > > > - add new patch 4 to build decoder pm file as module
> > > > - add new patch 5 to support 8192
> > > > - fix comments for patch 6/17
> > > > - change some logic for using work queue instead of create
> > > > thread
> > > > for core hardware decode for patch 10/17
> > > > - using work queue for hardware decode instead of create thread
> > > > for
> > > > patch 13/17
> > > > - add returen value for patch 14/17
> > > > - fix yaml check fail 15/17
> > > >
> > > > Changes compared with v6:
> > > > - Use of_platform_populate to manage multi hardware, not
> > > > component
> > > > framework for patch 4/15
> > > > - Re-write dtsi document for hardware architecture changed for
> > > > patch 13/15 -The dtsi will write like below in patch 13/15:
> > > > vcodec_dec: vcodec_dec@1600 {
> > > > compatible = "mediatek,mt8192-vcodec-dec";
> > > > #address-cells = <2>;
> > > > #size-cells = <2>;
> > > > ranges;
> > > > reg = <0 0x1600 0 0x100
Re: [PATCH v10, 15/19] dt-bindings: media: mtk-vcodec: Adds decoder dt-bindings for mt8192
Hi Ezequiel, Thanks for your suggestion. On Sun, 2021-11-14 at 18:56 -0300, Ezequiel Garcia wrote: > Yunfei, > > On Thu, 11 Nov 2021 at 01:15, Yunfei Dong > wrote: > > > > Adds decoder dt-bindings for mt8192. > > > > Signed-off-by: Yunfei Dong > > --- > > .../media/mediatek,vcodec-subdev-decoder.yaml | 261 > > ++ > > 1 file changed, 261 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/media/mediatek,vcodec-subdev- > > decoder.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev- > > decoder.yaml > > b/Documentation/devicetree/bindings/media/mediatek,vcodec-subdev- > > decoder.yaml > > new file mode 100644 > > index ..1886fae6e39d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec- > > subdev-decoder.yaml > > @@ -0,0 +1,261 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > + > > +%YAML 1.2 > > +--- > > +$id: " > > http://devicetree.org/schemas/media/mediatek,vcodec-subdev-decoder.yaml# > > " > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#; > > + > > +title: Mediatek Video Decode Accelerator With Multi Hardware > > + > > +maintainers: > > + - Yunfei Dong > > + > > +description: | > > + Mediatek Video Decode is the video decode hardware present in > > Mediatek > > + SoCs which supports high resolution decoding functionalities. > > Required > > + main and subdev device node. > > + > > + About the Decoder Hardware Block Diagram, please check below: > > + > > ++-+--- > > -+ > > +| | > > | > > +| input -> lat HW -> lat buffer --|--> lat buffer -> core HW > > -> output > > | > > To be completely honest, I can't really understand what is the > meaning > of the blocks > with input -> lat hw -> lat buffer, and how this means lat- and core- > are children of some parent. > I will add more detail information as below: The block diagram is data flow, need two or more hardwares to decode for each picture. input is input bitstream, lat hardware need input bitstream and lat buffer to decode, will write temp information to lat buffer when lat decode done. lat buffer is used to share lat buffer information for lat hardware and core hardware. core hardware need frame buffer and lat buffer to decode, will write decode result to frame buffer. > > +||| | || > > | > > ++||---+-||-- > > ---+ > > + || lat thread | core > > thread|| > > +-||-||-- > > -- > > + || || > > > > + \/\/ > > + + > > --+ > > + |enable/disable > > | > > + | clk powerirqiommu > > port | > > + | (lat/lat > > soc/core0/core1)| > > + + > > --+ > > + > > + As above, mean in main device, mean in subdev > > device. The information > > + of each hardware will be stored in subdev device. There are two > > workqueues in main device: > > + lat and core. Enable/disable the lat clk/power/irq when lat need > > to work through hardware > > + index, core is the same. > > + > > + Normally the smi common may not the same for each hardware, > > can't combine all hardware in > > + one node, or leading to iommu fault when access dram data. > > + > There are one parent node, regards it as main device, each child node as subdevices, one child node mean one hardware. There are two work queues in main device used for lat and core hardware to decode. The information of each hardware will be stored in subdevices. Enable/disable the lat clk/power/irq when hardware need to work. > To what extent the lat- and core- devices are really "children" > or "subdevices" of the video-codec@1600 device? > > I.e. what resources do they share? What are the details of > their bus topology? > The hardware is really very simple, just enable it to work when needed. But each hardware isn't independent, need lat and core to decode for every picture. No complex bus topology. > > + > > +examples: > > + - | > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +video-codec@1600 { > > +compatible = "mediatek,mt8192-vcodec-dec"; > > +reg = <0x1600 0x1000>; /* VDEC_SYS */ > > +mediatek,scp = <>; > > +iommus = < M4U_PORT_L4_VDEC_MC_EXT>; > > +dma-ranges = <0x1 0x0 0x0
Re: [PATCH v10, 00/19] Support multi hardware decode using of_platform_populate
Hi Ezequiel,
Thanks for you suggestion.
On Sun, 2021-11-14 at 19:04 -0300, Ezequiel Garcia wrote:
> Hi Yunfei,
>
> On Thu, 11 Nov 2021 at 01:15, Yunfei Dong
> wrote:
> >
> > This series adds support for multi hardware decode into mtk-vcodec,
> > by first adding use
> > of_platform_populate to manage each hardware information:
> > interrupt, clock, register
> > bases and power. Secondly add core work queue to deal with core
> > hardware message,
> > at the same time, add msg queue for different hardware share
> > messages. Lastly, the
> > architecture of different specs are not the same, using specs type
> > to separate them.
> >
> > This series has been tested with both MT8183 and MT8173. Decoding
> > was working for both chips.
> >
>
> How are you testing Decoding? If you are running some test suite, it
> would
> be good to add such information.
>
For 8183 and 8173, these patches don't have influence for them, can
work well.
These patches are used for mt8192 and 8195. We do cts/gts/stress test
to check our drivers.
> Are you testing some edge-cases such as parallel/concurrent decoding,
> removing the driver while streaming, and so on? This should help
> catch
> some typical issues.
>
> Thanks,
> Ezequiel
>
Thanks,
Yunfei Dong
> > Patches 1~3 rewrite get register bases and power on/off interface.
> > Patches 4 export decoder pm interfaces.
> > Patches 5 add to support 8192.
> > Patch 6 support multi hardware.
> > Patch 7 separate video encoder and decoder document
> > Patch 8-17 add interfaces to support core hardware.
> > Patch 18-19 remove mtk_vcodec_release_dec/enc_pm interfaces.
> > ---
> > changes compared with v9:
> > - need not to build ko, just export pm interfaces for patch 04/19.
> > - fix comments for patch 06/19
> >
> > changes compared with v8:
> > - add new patch 18~19 to remove mtk_vcodec_release_de/enc_pm
> > interfaces.
> > - fix spelling mistakes for patch 17/19
> > - fix yaml comments for patch 15/19
> >
> > Changes compared with v7:
> > - add new patch 4 to build decoder pm file as module
> > - add new patch 5 to support 8192
> > - fix comments for patch 6/17
> > - change some logic for using work queue instead of create thread
> > for core hardware decode for patch 10/17
> > - using work queue for hardware decode instead of create thread for
> > patch 13/17
> > - add returen value for patch 14/17
> > - fix yaml check fail 15/17
> >
> > Changes compared with v6:
> > - Use of_platform_populate to manage multi hardware, not component
> > framework for patch 4/15
> > - Re-write dtsi document for hardware architecture changed for
> > patch 13/15 -The dtsi will write like below in patch 13/15:
> > vcodec_dec: vcodec_dec@1600 {
> > compatible = "mediatek,mt8192-vcodec-dec";
> > #address-cells = <2>;
> > #size-cells = <2>;
> > ranges;
> > reg = <0 0x1600 0 0x1000>; /* VDEC_SYS */
> > mediatek,scp = <>;
> > iommus = < M4U_PORT_L4_VDEC_MC_EXT>;
> > dma-ranges = <0x1 0x0 0x0 0x4000 0x0 0xfff0>;
> > vcodec_lat {
> > compatible = "mediatek,mtk-vcodec-lat";
> > reg = <0 0x1601 0 0x800>; /*
> > VDEC_MISC */
> > reg-name = "reg-misc";
> > interrupts = ;
> > iommus = < M4U_PORT_L5_VDEC_LAT0_VLD_EXT>,
> > < M4U_PORT_L5_VDEC_LAT0_VLD2_EXT>,
> > < M4U_PORT_L5_VDEC_LAT0_AVC_MV_EXT>,
> > < M4U_PORT_L5_VDEC_LAT0_PRED_RD_EXT>,
> > < M4U_PORT_L5_VDEC_LAT0_TILE_EXT>,
> > < M4U_PORT_L5_VDEC_LAT0_WDMA_EXT>,
> > < M4U_PORT_L5_VDEC_LAT0_RG_CTRL_DMA_EXT>,
> > < M4U_PORT_L5_VDEC_UFO_ENC_EXT>;
> > clocks = < CLK_TOP_VDEC_SEL>,
> > <_soc CLK_VDEC_SOC_VDEC>,
> > <_soc CLK_VDEC_SOC_LAT>,
> > <_soc CLK_VDEC_SOC_LARB1>,
> > < CLK_TOP_MAINPLL_D4>;
> > clock-names = "vdec-sel", "vdec-soc-vdec", "vdec-soc-
> > lat",
> > "vdec-vdec", "vdec-top";
> > assigned-clocks = < CLK_TOP_VDEC_SEL>;
> > assigned-clock-parents = <
> > CLK_TOP_MAINPLL_D4>;
> > power-domains = < MT8192_POWER_DOMAIN_VDEC>;
> > };
> >
> > vcodec_core {
> > compatible = "mediatek,mtk-vcodec-core";
> > reg = <0 0x16025000 0 0x1000>; /*
> > VDEC_CORE_MISC */
> > reg-names = "reg-misc";
> > interrupts = ;
> > iommus = < M4U_PORT_L4_VDEC_MC_EXT>,
> > < M4U_PORT_L4_VDEC_UFO_EXT>,
> > < M4U_PORT_L4_VDEC_PP_EXT>,
> > < M4U_PORT_L4_VDEC_PRED_RD_EXT>,
> > < M4U_PORT_L4_VDEC_PRED_WR_EXT>,
> > < M4U_PORT_L4_VDEC_PPWRAP_EXT>,
> > < M4U_PORT_L4_VDEC_TILE_EXT>,
> > < M4U_PORT_L4_VDEC_VLD_EXT>,
> > <
Re: [PATCH v9, 06/19] media: mtk-vcodec: Manage multi hardware information
Hi Tzung-Bi,
Thanks for your suggestion.
On Wed, 2021-11-10 at 18:30 +0800, Tzung-Bi Shih wrote:
> On Tue, Nov 09, 2021 at 08:50:17PM +0800, Yunfei Dong wrote:
> > Manage each hardware information which includes irq/power/clk.
> > The hardware includes LAT0, LAT1 and CORE.
>
> The commit message doesn't explain the code. Could you provide some
> explanations about how the async mechanism works? (e.g. A bitmap for
> all sub-devices' readiness ...)
>
add more detail description for commit message.
> > Reported-by: kernel test robot
>
> Apparently wrong tag.
>
Remove
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index b7a51e96d4ba..eb2af42aa102 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -18,19 +18,49 @@
> >
> > #include "mtk_vcodec_drv.h"
> > #include "mtk_vcodec_dec.h"
> > +#include "mtk_vcodec_dec_hw.h"
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_intr.h"
> > -#include "mtk_vcodec_util.h"
>
> Why does mtk_vcodec_util.h need to remove?
>
Put #include "mtk_vcodec_util.h" in mtk_vcodec_dec_hw.h.
> > +static int mtk_vcodec_subdev_device_check(struct mtk_vcodec_ctx
> > *ctx)
> > + {
>
> Remove the extra space.
>
Fix
> > + struct mtk_vcodec_dev *vdec_dev = ctx->dev;
> > + struct platform_device *pdev = vdec_dev->plat_dev;
> > + struct device_node *subdev_node;
> > + enum mtk_vdec_hw_id hw_idx;
> > + const struct of_device_id *of_id;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mtk_vdec_hw_match); i++) {
> > + of_id = _vdec_hw_match[i];
> > + subdev_node = of_find_compatible_node(NULL, NULL,
> > + of_id->compatible);
> > + if (!subdev_node)
> > + continue;
> > +
> > + if (!of_device_is_available(subdev_node)) {
> > + of_node_put(subdev_node);
> > + dev_err(>dev, "Fail to get MMSYS
> > node\n");
>
> I am not sure if the error message makes sense about mentioning MMSYS
> here.
>
Fix with subdev node.
> > + continue;
> > + }
> > +
> > + hw_idx = (enum mtk_vdec_hw_id)(uintptr_t)of_id->data;
>
> Does it really need to cast twice?
>
yes, or will build warning.
> > + vdec_dev->subdev_node[hw_idx] = subdev_node;
> > +
> > + if (!test_bit(hw_idx, vdec_dev->hardware_bitmap)) {
> > + dev_err(>dev, "Vdec hw_idx is not ready
> > %d.",
> > + hw_idx);
>
> I would prefer "Vdec %d is not ready\n".
>
Fix
> > + return -EINVAL;
>
> -EAGAIN makes more sense.
> Fix
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
> Would it need to call of_node_put() in the error handling path?
>
yes, fix it.
> > +static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev)
> > +{
>
> I would rather not call them "params". They are more like
> "resources".
>
Fix it with resources.
> > + struct platform_device *pdev = dev->plat_dev;
> > + int ret;
> > +
> > + ret = mtk_vcodec_get_reg_bases(dev);
> > + if (ret)
> > + return ret;
> > +
> > + if (!dev->vdec_pdata->is_subdev_supported) {
> > + dev->dec_irq = platform_get_irq(pdev, 0);
> > + if (dev->dec_irq < 0) {
> > + dev_err(>dev, "failed to get irq
> > number");
> > + return dev->dec_irq;
> > + }
> > +
> > + irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
> > + ret = devm_request_irq(>dev, dev->dec_irq,
> > + mtk_vcodec_dec_irq_handler, 0, pdev->name,
> > dev);
> > + if (ret) {
> > + dev_err(>dev, "failed to install dev-
> > >dec_irq %d (%d)",
> > + dev->dec_irq, ret);
> > + return ret;
> > + }
> > +
> > + ret = mtk_vcodec_init_dec_pm(pdev, >pm);
> > + if (ret < 0) {
> > + dev_err(>dev, "failed to get mt vcodec
> > clock source");
> > + return ret;
> > + }
> > + }
>
> I would prefer:
>
> if (dev->vdec_pdata->is_subdev_supported)
> return 0;
>
Fix
> And decrease the indent level by 1 for the following blocks.
>
> > @@ -329,6 +377,13 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > goto err_event_workq;
> > }
> >
> > + ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,
> > + >dev);
> > + if (ret) {
> > + mtk_v4l2_err("Master device of_platform_populate
> > failed.");
>
> s/Master/Main/
>
> Doesn't it need to reference `is_subdev_supported` before populating?
>
Fix
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> >
Re: [PATCH v8, 03/17] media: mtk-vcodec: Refactor vcodec pm interface
Hi dafna,
Thanks for your suggestion.
On Fri, 2021-10-29 at 13:35 +0200, Dafna Hirschfeld wrote:
>
> On 29.10.21 05:55, Yunfei Dong wrote:
> > Using the needed param for pm init/release function and remove
> > unused
> > param mtkdev in 'struct mtk_vcodec_pm'.
> >
> > Reviewed-by: Tzung-Bi Shih
> > Reviewed-By: AngeloGioacchino Del Regno <
> > angelogioacchino.delre...@collabora.com>
> > Signed-off-by: Yunfei Dong
>
> Hi,
> I already commented on v7 that since the pm implementation for dec
> and enc is identical,
> you should better do the same refactor to enc and dec or better
> remove the code duplication.
>
Add patch 18 and 19 in v9 to remove decoder and encoder pm interfaces.
> Thanks,
> Dafna
>
Thanks,
Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 6 ++---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 22
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 5 +++--
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 1 -
> > .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 1 -
> > 5 files changed, 15 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index 055d50e52720..3ac4c3935e4e 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -249,7 +249,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > if (IS_ERR(dev->fw_handler))
> > return PTR_ERR(dev->fw_handler);
> >
> > - ret = mtk_vcodec_init_dec_pm(dev);
> > + ret = mtk_vcodec_init_dec_pm(dev->plat_dev, >pm);
> > if (ret < 0) {
> > dev_err(>dev, "Failed to get mt vcodec clock
> > source");
> > goto err_dec_pm;
> > @@ -378,7 +378,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > err_dec_alloc:
> > v4l2_device_unregister(>v4l2_dev);
> > err_res:
> > - mtk_vcodec_release_dec_pm(dev);
> > + mtk_vcodec_release_dec_pm(>pm);
> > err_dec_pm:
> > mtk_vcodec_fw_release(dev->fw_handler);
> > return ret;
> > @@ -418,7 +418,7 @@ static int mtk_vcodec_dec_remove(struct
> > platform_device *pdev)
> > video_unregister_device(dev->vfd_dec);
> >
> > v4l2_device_unregister(>v4l2_dev);
> > - mtk_vcodec_release_dec_pm(dev);
> > + mtk_vcodec_release_dec_pm(>pm);
> > mtk_vcodec_fw_release(dev->fw_handler);
> > return 0;
> > }
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > index 6038db96f71c..20bd157a855c 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -13,18 +13,15 @@
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_util.h"
> >
> > -int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
> > +int mtk_vcodec_init_dec_pm(struct platform_device *pdev,
> > + struct mtk_vcodec_pm *pm)
> > {
> > struct device_node *node;
> > - struct platform_device *pdev;
> > - struct mtk_vcodec_pm *pm;
> > + struct platform_device *larb_pdev;
> > struct mtk_vcodec_clk *dec_clk;
> > struct mtk_vcodec_clk_info *clk_info;
> > int i = 0, ret = 0;
> >
> > - pdev = mtkdev->plat_dev;
> > - pm = >pm;
> > - pm->mtkdev = mtkdev;
> > dec_clk = >vdec_clk;
> > node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> > if (!node) {
> > @@ -32,13 +29,12 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> > return -1;
> > }
> >
> > - pdev = of_find_device_by_node(node);
> > + larb_pdev = of_find_device_by_node(node);
> > of_node_put(node);
> > - if (WARN_ON(!pdev)) {
> > + if (WARN_ON(!larb_pdev)) {
> > return -1;
> > }
> > - pm->larbvdec = >dev;
> > - pdev = mtkdev->plat_dev;
> > + pm->larbvdec = _pdev->dev;
> > pm->dev = >dev;
> >
> > dec_clk->clk_num =
> > @@ -82,10 +78,10 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> > return ret;
> > }
> >
> > -void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
> > +void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm)
> > {
> > - pm_runtime_disable(dev->pm.dev);
> > - put_device(dev->pm.larbvdec);
> > + pm_runtime_disable(pm->dev);
> > + put_device(pm->larbvdec);
> > }
> >
> > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> > index 280aeaefdb65..a3df6aef6cb9 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> > @@ -9,8 +9,9 @@
> >
> > #include "mtk_vcodec_drv.h"
> >
> > -int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *dev);
>
Re: [PATCH v8, 04/17] media: mtk-vcodec: Build decoder pm file as module
Hi dafna,
Thanks for your suggestion.
On Fri, 2021-10-29 at 13:42 +0200, Dafna Hirschfeld wrote:
>
> On 29.10.21 05:55, Yunfei Dong wrote:
> > Need to build decoder pm file as module for master and comp
> > use the same pm interface.
>
> Do you still use the component framework in this patchset?
> In the cover letter you write: "- Use of_platform_populate to manage
> multi hardware, not component framework for patch 4/15"
remove component.
> If that frameworks is not used anymore you should also change the
> commit log, and maybe this patch is not needed anymore?
>
main device and subdev device all use the same pm interface, so build
the file as module.
> Thanks,
> Dafna
Thanks,
Yunfei Dong
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > v8: add new patch to build pm file as module
> > ---
> > drivers/media/platform/mtk-vcodec/Makefile| 6 --
> > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 9
> > +
> > 2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > b/drivers/media/platform/mtk-vcodec/Makefile
> > index ca8e9e7a9c4e..5d36e05535d7 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -2,7 +2,8 @@
> >
> > obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> >mtk-vcodec-enc.o \
> > - mtk-vcodec-common.o
> > + mtk-vcodec-common.o \
> > + mtk-vcodec-dec-common.o
> >
> > mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > vdec/vdec_vp8_if.o \
> > @@ -14,7 +15,8 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > mtk_vcodec_dec.o \
> > mtk_vcodec_dec_stateful.o \
> > mtk_vcodec_dec_stateless.o \
> > - mtk_vcodec_dec_pm.o \
> > +
> > +mtk-vcodec-dec-common-y := mtk_vcodec_dec_pm.o
> >
> > mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> > venc/venc_h264_if.o \
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > index 20bd157a855c..09a281e3065a 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -77,12 +77,14 @@ int mtk_vcodec_init_dec_pm(struct
> > platform_device *pdev,
> > put_device(pm->larbvdec);
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_init_dec_pm);
> >
> > void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm)
> > {
> > pm_runtime_disable(pm->dev);
> > put_device(pm->larbvdec);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_release_dec_pm);
> >
> > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> > {
> > @@ -94,6 +96,7 @@ int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm
> > *pm)
> >
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_pw_on);
> >
> > void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
> > {
> > @@ -103,6 +106,7 @@ void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm
> > *pm)
> > if (ret)
> > mtk_v4l2_err("pm_runtime_put_sync fail %d", ret);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_pw_off);
> >
> > void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm)
> > {
> > @@ -129,6 +133,7 @@ void mtk_vcodec_dec_clock_on(struct
> > mtk_vcodec_pm *pm)
> > for (i -= 1; i >= 0; i--)
> > clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_clock_on);
> >
> > void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm)
> > {
> > @@ -139,3 +144,7 @@ void mtk_vcodec_dec_clock_off(struct
> > mtk_vcodec_pm *pm)
> > for (i = dec_clk->clk_num - 1; i >= 0; i--)
> > clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_clock_off);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mediatek video decoder driver");
> >
Re: [PATCH v8, 07/17] dt-bindings: media: mtk-vcodec: Separate video encoder and decoder dt-bindings
Hi Dafna,
Thanks for your suggestion,
On Fri, 2021-10-29 at 13:49 +0200, Dafna Hirschfeld wrote:
>
> On 29.10.21 05:55, Yunfei Dong wrote:
> > Decoder will use component framework to manage hardware, it is big
> > difference with encoder.
> >
> > Reviewed-by: Rob Herring
> > Signed-off-by: Yunfei Dong
> > ---
> > .../media/mediatek,vcodec-decoder.yaml| 176
> > +
> > .../media/mediatek,vcodec-encoder.yaml| 187
> > ++
> > .../bindings/media/mediatek-vcodec.txt| 131
> > 3 files changed, 363 insertions(+), 131 deletions(-)
> > create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,vcodec-
> > encoder.yaml
> > delete mode 100644
> > Documentation/devicetree/bindings/media/mediatek-vcodec.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > b/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > new file mode 100644
> > index ..5de37065fbce
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > @@ -0,0 +1,176 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/media/mediatek,vcodec-decoder.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mediatek Video Decode Accelerator With Component
> > +
> > +maintainers:
> > + - Yunfei Dong
> > +
> > +description: |+
> > + Mediatek Video Decode is the video decode hardware present in
> > Mediatek
> > + SoCs which supports high resolution decoding functionalities.
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - mediatek,mt8173-vcodec-dec
> > + - mediatek,mt8183-vcodec-dec
> > +
> > + reg:
> > +maxItems: 12
> > +
> > + interrupts:
> > +maxItems: 1
> > +
> > + clocks:
> > +maxItems: 8
> > +
> > + clock-names:
> > +items:
> > + - const: vcodecpll
> > + - const: univpll_d2
> > + - const: clk_cci400_sel
> > + - const: vdec_sel
> > + - const: vdecpll
> > + - const: vencpll
> > + - const: venc_lt_sel
> > + - const: vdec_bus_clk_src
> > +
> > + assigned-clocks: true
> > +
> > + assigned-clock-parents: true
> > +
> > + assigned-clock-rates: true
> > +
> > + power-domains:
> > +maxItems: 1
> > +
> > + iommus:
> > +minItems: 1
> > +maxItems: 32
> > +description: |
> > + List of the hardware port in respective IOMMU block for
> > current Socs.
> > + Refer to bindings/iommu/mediatek,iommu.yaml.
> > +
> > + dma-ranges:
> > +maxItems: 1
> > +description: |
> > + Describes the physical address space of IOMMU maps to
> > memory.
> > +
> > + mediatek,larb:
> > +$ref: /schemas/types.yaml#/definitions/phandle
> > +maxItems: 1
> > +description: |
> > + Must contain the local arbiters in the current Socs.
> > +
> > + mediatek,vpu:
> > +$ref: /schemas/types.yaml#/definitions/phandle
> > +maxItems: 1
> > +description:
> > + Describes point to vpu.
> > +
> > + mediatek,scp:
> > +$ref: /schemas/types.yaml#/definitions/phandle
> > +maxItems: 1
> > +description:
> > + Describes point to scp.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - clock-names
> > + - iommus
> > + - assigned-clocks
> > + - assigned-clock-parents
> > +
> > +allOf:
> > + - if:
> > + properties:
> > +compatible:
> > + contains:
> > +enum:
> > + - mediatek,mt8183-vcodec-dec
> > +
> > +then:
> > + required:
> > +- mediatek,scp
> > +
> > + - if:
> > + properties:
> > +compatible:
> > + contains:
> > +enum:
> > + - mediatek,mt8173-vcodec-dec
> > +
> > +then:
> > + required:
> > +- mediatek,vpu
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +vcodec_dec: vcodec@1600 {
> > + compatible = "mediatek,mt8173-vcodec-dec";
> > + reg = <0x1600 0x100>, /*VDEC_SYS*/
> > + <0x1602 0x1000>, /*VDEC_MISC*/
> > + <0x16021000 0x800>, /*VDEC_LD*/
> > + <0x16021800 0x800>, /*VDEC_TOP*/
> > + <0x16022000 0x1000>, /*VDEC_CM*/
> > + <0x16023000 0x1000>, /*VDEC_AD*/
> > + <0x16024000 0x1000>, /*VDEC_AV*/
> > + <0x16025000 0x1000>, /*VDEC_PP*/
> > + <0x16026800 0x800>, /*VP8_VD*/
> > + <0x16027000 0x800>, /*VP6_VD*/
> > + <0x16027800 0x800>, /*VP8_VL*/
> > + <0x16028400 0x400>; /*VP9_VD*/
> > + interrupts = ;
> > + mediatek,larb = <>;
> > + iommus
Re: [PATCH v1] media: mtk-vcodec: Align width and height to 64
Hi steve,
Thanks for your suggestion.On Tue, 2021-11-02 at 09:43 -0700, Steve Cho
wrote:
> Thank you Yunfei for following up with this change.
> This change is the last missing piece to enable VD on Kukui with
> Chromium.
> This patch fixed the corruption we were seeing on Kukui with certain
> tests.
>
> One comment from me is just to use defined macro or variable instead
> of hard coding 64.
>
> "User get width and height are 64 align when set format."
>
> This sentence might need to be reworded. It is not clear to me.
>
> Maybe something like "Width and height need to be 64 bytes aligned
> when setting the format."
>
> Thanks,Steve
Fix it and send patch v2.
Thanks,Yunfei Dong
> On Fri, Oct 29, 2021 at 2:45 AM Yunfei Dong > wrote:
> > User get width and height are 64 align when set format. Need to
> > make
> >
> > sure all is 64 align when use width and height to calculate buffer
> > size.
> >
> >
> >
> > Signed-off-by: Yunfei Dong
> >
> > ---
> >
> > drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c | 4 ++--
> >
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >
> >
> > diff --git a/drivers/media/platform/mtk-
> > vcodec/vdec/vdec_h264_req_if.c b/drivers/media/platform/mtk-
> > vcodec/vdec/vdec_h264_req_if.c
> >
> > index 946c23088308..28c17204f9a1 100644
> >
> > --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> >
> > +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_req_if.c
> >
> > @@ -562,8 +562,8 @@ static void get_pic_info(struct
> > vdec_h264_slice_inst *inst,
> >
> > {
> >
> > struct mtk_vcodec_ctx *ctx = inst->ctx;
> >
> >
> >
> > - ctx->picinfo.buf_w = (ctx->picinfo.pic_w + 15) &
> > 0xFFF0;
> >
> > - ctx->picinfo.buf_h = (ctx->picinfo.pic_h + 31) &
> > 0xFFE0;
> >
> > + ctx->picinfo.buf_w = ALIGN(ctx->picinfo.pic_w, 64);
> >
> > + ctx->picinfo.buf_h = ALIGN(ctx->picinfo.pic_h, 64);
> >
> > ctx->picinfo.fb_sz[0] = ctx->picinfo.buf_w * ctx-
> > >picinfo.buf_h;
> >
> > ctx->picinfo.fb_sz[1] = ctx->picinfo.fb_sz[0] >> 1;
> >
> > inst->vsi_ctx.dec.cap_num_planes =
> >
Re: [PATCH v8, 03/17] media: mtk-vcodec: Refactor vcodec pm interface
Hi Dafna,
Thanks for your suggestion.
On Fri, 2021-10-29 at 13:35 +0200, Dafna Hirschfeld wrote:
>
> On 29.10.21 05:55, Yunfei Dong wrote:
> > Using the needed param for pm init/release function and remove
> > unused
> > param mtkdev in 'struct mtk_vcodec_pm'.
> >
> > Reviewed-by: Tzung-Bi Shih
> > Reviewed-By: AngeloGioacchino Del Regno <
> > angelogioacchino.delre...@collabora.com>
> > Signed-off-by: Yunfei Dong
>
> Hi,
> I already commented on v7 that since the pm implementation for dec
> and enc is identical,
> you should better do the same refactor to enc and dec or better
> remove the code duplication.
>
1: Can't refactor encoder and decoder to use the same interface,
first, decoder and encoder are different hardware, the interface will
have more and more differences for different complex scenarios. Second,
we are trying to separate video encoder and decoder for the hardware
having more and more differences.
2: We are trying to remove mtk_vcodec_dec_pw_on, may not fix it in this
patch series. For another colleague are doing it.
> Thanks,
> Dafna
>
Thanks,
Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 6 ++---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 22
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 5 +++--
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 1 -
> > .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 1 -
> > 5 files changed, 15 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index 055d50e52720..3ac4c3935e4e 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -249,7 +249,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > if (IS_ERR(dev->fw_handler))
> > return PTR_ERR(dev->fw_handler);
> >
> > - ret = mtk_vcodec_init_dec_pm(dev);
> > + ret = mtk_vcodec_init_dec_pm(dev->plat_dev, >pm);
> > if (ret < 0) {
> > dev_err(>dev, "Failed to get mt vcodec clock
> > source");
> > goto err_dec_pm;
> > @@ -378,7 +378,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > err_dec_alloc:
> > v4l2_device_unregister(>v4l2_dev);
> > err_res:
> > - mtk_vcodec_release_dec_pm(dev);
> > + mtk_vcodec_release_dec_pm(>pm);
> > err_dec_pm:
> > mtk_vcodec_fw_release(dev->fw_handler);
> > return ret;
> > @@ -418,7 +418,7 @@ static int mtk_vcodec_dec_remove(struct
> > platform_device *pdev)
> > video_unregister_device(dev->vfd_dec);
> >
> > v4l2_device_unregister(>v4l2_dev);
> > - mtk_vcodec_release_dec_pm(dev);
> > + mtk_vcodec_release_dec_pm(>pm);
> > mtk_vcodec_fw_release(dev->fw_handler);
> > return 0;
> > }
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > index 6038db96f71c..20bd157a855c 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -13,18 +13,15 @@
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_util.h"
> >
> > -int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
> > +int mtk_vcodec_init_dec_pm(struct platform_device *pdev,
> > + struct mtk_vcodec_pm *pm)
> > {
> > struct device_node *node;
> > - struct platform_device *pdev;
> > - struct mtk_vcodec_pm *pm;
> > + struct platform_device *larb_pdev;
> > struct mtk_vcodec_clk *dec_clk;
> > struct mtk_vcodec_clk_info *clk_info;
> > int i = 0, ret = 0;
> >
> > - pdev = mtkdev->plat_dev;
> > - pm = >pm;
> > - pm->mtkdev = mtkdev;
> > dec_clk = >vdec_clk;
> > node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> > if (!node) {
> > @@ -32,13 +29,12 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> > return -1;
> > }
> >
> > - pdev = of_find_device_by_node(node);
> > + larb_pdev = of_find_device_by_node(node);
> > of_node_put(node);
> > - if (WARN_ON(!pdev)) {
> > + if (WARN_ON(!larb_pdev)) {
> > return -1;
> > }
> > - pm->larbvdec = >dev;
> > - pdev = mtkdev->plat_dev;
> > + pm->larbvdec = _pdev->dev;
> > pm->dev = >dev;
> >
> > dec_clk->clk_num =
> > @@ -82,10 +78,10 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> > return ret;
> > }
> >
> > -void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
> > +void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm)
> > {
> > - pm_runtime_disable(dev->pm.dev);
> > - put_device(dev->pm.larbvdec);
> > + pm_runtime_disable(pm->dev);
> > + put_device(pm->larbvdec);
> > }
> >
> > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
>
Re: [PATCH v8, 04/17] media: mtk-vcodec: Build decoder pm file as module
Hi Danfa,
Thanks for your feedback.
On Fri, 2021-10-29 at 13:42 +0200, Dafna Hirschfeld wrote:
>
> On 29.10.21 05:55, Yunfei Dong wrote:
> > Need to build decoder pm file as module for master and comp
> > use the same pm interface.
>
> Do you still use the component framework in this patchset?
> In the cover letter you write: "- Use of_platform_populate to manage
> multi hardware, not component framework for patch 4/15"
> If that frameworks is not used anymore you should also change the
> commit log, and maybe this patch is not needed anymore?
>
Not use component framework.
For iommu limit, not only parent device, the child device also need to
register as platform device. So the child device need to call
module_platform_driver. The child and parent device all need to call pm
interface to open/close pm/clk/irq. So build pm file as module, and
export necessary function.
> >
Thanks
Yunfei Dong
> Thanks,
> Dafna
> > Signed-off-by: Yunfei Dong
> > ---
> > v8: add new patch to build pm file as module
> > ---
> > drivers/media/platform/mtk-vcodec/Makefile| 6 --
> > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 9
> > +
> > 2 files changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > b/drivers/media/platform/mtk-vcodec/Makefile
> > index ca8e9e7a9c4e..5d36e05535d7 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -2,7 +2,8 @@
> >
> > obj-$(CONFIG_VIDEO_MEDIATEK_VCODEC) += mtk-vcodec-dec.o \
> >mtk-vcodec-enc.o \
> > - mtk-vcodec-common.o
> > + mtk-vcodec-common.o \
> > + mtk-vcodec-dec-common.o
> >
> > mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > vdec/vdec_vp8_if.o \
> > @@ -14,7 +15,8 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > mtk_vcodec_dec.o \
> > mtk_vcodec_dec_stateful.o \
> > mtk_vcodec_dec_stateless.o \
> > - mtk_vcodec_dec_pm.o \
> > +
> > +mtk-vcodec-dec-common-y := mtk_vcodec_dec_pm.o
> >
> > mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> > venc/venc_h264_if.o \
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > index 20bd157a855c..09a281e3065a 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -77,12 +77,14 @@ int mtk_vcodec_init_dec_pm(struct
> > platform_device *pdev,
> > put_device(pm->larbvdec);
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_init_dec_pm);
> >
> > void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm)
> > {
> > pm_runtime_disable(pm->dev);
> > put_device(pm->larbvdec);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_release_dec_pm);
> >
> > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> > {
> > @@ -94,6 +96,7 @@ int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm
> > *pm)
> >
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_pw_on);
> >
> > void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
> > {
> > @@ -103,6 +106,7 @@ void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm
> > *pm)
> > if (ret)
> > mtk_v4l2_err("pm_runtime_put_sync fail %d", ret);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_pw_off);
> >
> > void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm)
> > {
> > @@ -129,6 +133,7 @@ void mtk_vcodec_dec_clock_on(struct
> > mtk_vcodec_pm *pm)
> > for (i -= 1; i >= 0; i--)
> > clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_clock_on);
> >
> > void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm)
> > {
> > @@ -139,3 +144,7 @@ void mtk_vcodec_dec_clock_off(struct
> > mtk_vcodec_pm *pm)
> > for (i = dec_clk->clk_num - 1; i >= 0; i--)
> > clk_disable_unprepare(dec_clk->clk_info[i].vcodec_clk);
> > }
> > +EXPORT_SYMBOL_GPL(mtk_vcodec_dec_clock_off);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_DESCRIPTION("Mediatek video decoder driver");
> >
Re: [PATCH v8, 07/17] dt-bindings: media: mtk-vcodec: Separate video encoder and decoder dt-bindings
Hi Dafna,
Thanks for your suggestion.
On Fri, 2021-10-29 at 13:49 +0200, Dafna Hirschfeld wrote:
>
> On 29.10.21 05:55, Yunfei Dong wrote:
> > Decoder will use component framework to manage hardware, it is big
> > difference with encoder.
> >
> > Reviewed-by: Rob Herring
> > Signed-off-by: Yunfei Dong
> > ---
> > .../media/mediatek,vcodec-decoder.yaml| 176
> > +
> > .../media/mediatek,vcodec-encoder.yaml| 187
> > ++
> > .../bindings/media/mediatek-vcodec.txt| 131
> > 3 files changed, 363 insertions(+), 131 deletions(-)
> > create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > create mode 100644
> > Documentation/devicetree/bindings/media/mediatek,vcodec-
> > encoder.yaml
> > delete mode 100644
> > Documentation/devicetree/bindings/media/mediatek-vcodec.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > b/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > new file mode 100644
> > index ..5de37065fbce
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/mediatek,vcodec-
> > decoder.yaml
> > @@ -0,0 +1,176 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +
> > +%YAML 1.2
> > +---
> > +$id:
> > http://devicetree.org/schemas/media/mediatek,vcodec-decoder.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Mediatek Video Decode Accelerator With Component
> > +
> > +maintainers:
> > + - Yunfei Dong
> > +
> > +description: |+
> > + Mediatek Video Decode is the video decode hardware present in
> > Mediatek
> > + SoCs which supports high resolution decoding functionalities.
> > +
> > +properties:
> > + compatible:
> > +enum:
> > + - mediatek,mt8173-vcodec-dec
> > + - mediatek,mt8183-vcodec-dec
> > +
> > + reg:
> > +maxItems: 12
> > +
> > + interrupts:
> > +maxItems: 1
> > +
> > + clocks:
> > +maxItems: 8
> > +
> > + clock-names:
> > +items:
> > + - const: vcodecpll
> > + - const: univpll_d2
> > + - const: clk_cci400_sel
> > + - const: vdec_sel
> > + - const: vdecpll
> > + - const: vencpll
> > + - const: venc_lt_sel
> > + - const: vdec_bus_clk_src
> > +
> > + assigned-clocks: true
> > +
> > + assigned-clock-parents: true
> > +
> > + assigned-clock-rates: true
> > +
> > + power-domains:
> > +maxItems: 1
> > +
> > + iommus:
> > +minItems: 1
> > +maxItems: 32
> > +description: |
> > + List of the hardware port in respective IOMMU block for
> > current Socs.
> > + Refer to bindings/iommu/mediatek,iommu.yaml.
> > +
> > + dma-ranges:
> > +maxItems: 1
> > +description: |
> > + Describes the physical address space of IOMMU maps to
> > memory.
> > +
> > + mediatek,larb:
> > +$ref: /schemas/types.yaml#/definitions/phandle
> > +maxItems: 1
> > +description: |
> > + Must contain the local arbiters in the current Socs.
> > +
> > + mediatek,vpu:
> > +$ref: /schemas/types.yaml#/definitions/phandle
> > +maxItems: 1
> > +description:
> > + Describes point to vpu.
> > +
> > + mediatek,scp:
> > +$ref: /schemas/types.yaml#/definitions/phandle
> > +maxItems: 1
> > +description:
> > + Describes point to scp.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - clocks
> > + - clock-names
> > + - iommus
> > + - assigned-clocks
> > + - assigned-clock-parents
> > +
> > +allOf:
> > + - if:
> > + properties:
> > +compatible:
> > + contains:
> > +enum:
> > + - mediatek,mt8183-vcodec-dec
> > +
> > +then:
> > + required:
> > +- mediatek,scp
> > +
> > + - if:
> > + properties:
> > +compatible:
> > + contains:
> > +enum:
> > + - mediatek,mt8173-vcodec-dec
> > +
> > +then:
> > + required:
> > +- mediatek,vpu
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +vcodec_dec: vcodec@1600 {
> > + compatible = "mediatek,mt8173-vcodec-dec";
> > + reg = <0x1600 0x100>, /*VDEC_SYS*/
> > + <0x1602 0x1000>, /*VDEC_MISC*/
> > + <0x16021000 0x800>, /*VDEC_LD*/
> > + <0x16021800 0x800>, /*VDEC_TOP*/
> > + <0x16022000 0x1000>, /*VDEC_CM*/
> > + <0x16023000 0x1000>, /*VDEC_AD*/
> > + <0x16024000 0x1000>, /*VDEC_AV*/
> > + <0x16025000 0x1000>, /*VDEC_PP*/
> > + <0x16026800 0x800>, /*VP8_VD*/
> > + <0x16027000 0x800>, /*VP6_VD*/
> > + <0x16027800 0x800>, /*VP8_VL*/
> > + <0x16028400 0x400>; /*VP9_VD*/
> > + interrupts = ;
> > + mediatek,larb = <>;
> > + iommus
Re: [PATCH v7, 03/15] media: mtk-vcodec: Refactor vcodec pm interface
Hi Dafna,
Thanks for your suggestion.
On Thu, 2021-10-14 at 15:44 +0200, Dafna Hirschfeld wrote:
>
> On 11.10.21 09:02, Yunfei Dong wrote:
> > Using the needed param for pm init/release function and remove
> > unused
> > param mtkdev in 'struct mtk_vcodec_pm'.
> >
>
> I see that there is a lot of code duplication between
> mtk_vcodec_release_dec_pm.c and mtk_vcodec_release_enc_pm.c
> I think if you bother to factor the decoder you should do the same
> factor to the encoder, but actually the much better thing to do
> is to unify all code duplication between these two files, just for
> example of identical functions:
>
> mtk_vcodec_enc/dec_clock_on/off
> mtk_vcodec_release_enc_pm
> mtk_vcodec_init_dec_pm
>
> In addition, the function mtk_vcodec_dec_pw_on can be remove since it
> only calls pm_runtime_resume_and_get.
> It would be much better to call pm_runtime_resume_and_get directly
> and not hide it in a different function
>
I will fix it in next patch series. It's not very reasonable in this
patch series.
> Thanks,
> Dafna
>
Thanks,
Yunfei Dong
> > Reviewed-by: Tzung-Bi Shih
> > Signed-off-by: Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 6 ++---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 22
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 5 +++--
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 1 -
> > .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 1 -
> > 5 files changed, 15 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index 8db9cdc66043..dd749d41c75a 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -249,7 +249,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > if (IS_ERR(dev->fw_handler))
> > return PTR_ERR(dev->fw_handler);
> >
> > - ret = mtk_vcodec_init_dec_pm(dev);
> > + ret = mtk_vcodec_init_dec_pm(dev->plat_dev, >pm);
> > if (ret < 0) {
> > dev_err(>dev, "Failed to get mt vcodec clock
> > source");
> > goto err_dec_pm;
> > @@ -379,7 +379,7 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > err_dec_alloc:
> > v4l2_device_unregister(>v4l2_dev);
> > err_res:
> > - mtk_vcodec_release_dec_pm(dev);
> > + mtk_vcodec_release_dec_pm(>pm);
> > err_dec_pm:
> > mtk_vcodec_fw_release(dev->fw_handler);
> > return ret;
> > @@ -422,7 +422,7 @@ static int mtk_vcodec_dec_remove(struct
> > platform_device *pdev)
> > video_unregister_device(dev->vfd_dec);
> >
> > v4l2_device_unregister(>v4l2_dev);
> > - mtk_vcodec_release_dec_pm(dev);
> > + mtk_vcodec_release_dec_pm(>pm);
> > mtk_vcodec_fw_release(dev->fw_handler);
> > return 0;
> > }
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > index 6038db96f71c..20bd157a855c 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -13,18 +13,15 @@
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_util.h"
> >
> > -int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
> > +int mtk_vcodec_init_dec_pm(struct platform_device *pdev,
> > + struct mtk_vcodec_pm *pm)
> > {
> > struct device_node *node;
> > - struct platform_device *pdev;
> > - struct mtk_vcodec_pm *pm;
> > + struct platform_device *larb_pdev;
> > struct mtk_vcodec_clk *dec_clk;
> > struct mtk_vcodec_clk_info *clk_info;
> > int i = 0, ret = 0;
> >
> > - pdev = mtkdev->plat_dev;
> > - pm = >pm;
> > - pm->mtkdev = mtkdev;
> > dec_clk = >vdec_clk;
> > node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> > if (!node) {
> > @@ -32,13 +29,12 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> > return -1;
> > }
> >
> > - pdev = of_find_device_by_node(node);
> > + larb_pdev = of_find_device_by_node(node);
> > of_node_put(node);
> > - if (WARN_ON(!pdev)) {
> > + if (WARN_ON(!larb_pdev)) {
> > return -1;
> > }
> > - pm->larbvdec = >dev;
> > - pdev = mtkdev->plat_dev;
> > + pm->larbvdec = _pdev->dev;
> > pm->dev = >dev;
> >
> > dec_clk->clk_num =
> > @@ -82,10 +78,10 @@ int mtk_vcodec_init_dec_pm(struct
> > mtk_vcodec_dev *mtkdev)
> > return ret;
> > }
> >
> > -void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
> > +void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm)
> > {
> > - pm_runtime_disable(dev->pm.dev);
> > - put_device(dev->pm.larbvdec);
> > + pm_runtime_disable(pm->dev);
> > + put_device(pm->larbvdec);
> > }
> >
> > int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> > diff --git
Re: [PATCH v7, 11/15] media: mtk-vcodec: Add core thread
Hi Ezequiel,
1. Thanks for your suggestion.
On Thu, 2021-10-14 at 09:29 -0300, Ezequiel Garcia wrote:
> Hi Yunfei,
>
> On Mon, Oct 11, 2021 at 03:02:43PM +0800, Yunfei Dong wrote:
> > Core thread:
> > 1. Gets lat_buf from core msg queue.
> > 2. Proceeds core decode.
> > 3. Puts the lat_buf back to lat msg queue.
> >
> > Both H264 and VP9 rely on the core thread.
> >
>
> Avoid the kthread API and instead go with the workqueue API.
>
> See Documentation/core-api/workqueue.rst and
> include/linux/workqueue.h.
>
Fix it in patch v8.
> Thanks!
> Ezequiel
>
Thanks
Yunfei Dong
> > Signed-off-by: Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 12 +++
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 7
> > .../platform/mtk-vcodec/vdec_msg_queue.c | 32
> > +++
> > .../platform/mtk-vcodec/vdec_msg_queue.h | 6
> > 4 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index e21e0c4bcd86..de83e3b821b4 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -364,6 +364,18 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > goto err_dec_pm;
> > }
> >
> > + if (VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) {
> > + vdec_msg_queue_init_ctx(>msg_queue_core_ctx,
> > + MTK_VDEC_CORE);
> > + dev->kthread_core =
> > kthread_run(vdec_msg_queue_core_thead, dev,
> > + "mtk-%s", "core");
> > + if (IS_ERR(dev->kthread_core)) {
> > + dev_err(>dev, "Failed to create core
> > thread");
> > + ret = PTR_ERR(dev->kthread_core);
> > + goto err_res;
> > + }
> > + }
> > +
> > for (i = 0; i < MTK_VDEC_HW_MAX; i++)
> > mutex_init(>dec_mutex[i]);
> > spin_lock_init(>irqlock);
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > index 9d072c082f73..68a9b1a2d3b3 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > @@ -27,6 +27,7 @@
> > #define MTK_VCODEC_MAX_PLANES 3
> > #define MTK_V4L2_BENCHMARK 0
> > #define WAIT_INTR_TIMEOUT_MS 1000
> > +#define VDEC_LAT_ARCH(hw_arch) ((hw_arch) >=
> > MTK_VDEC_LAT_SINGLE_CORE)
> >
> > /*
> > * enum mtk_hw_reg_idx - MTK hw register base index
> > @@ -466,6 +467,9 @@ struct mtk_vcodec_enc_pdata {
> > * @comp_dev: component hardware device
> > * @component_node: component node
> > *
> > + * @kthread_core: thread used for core hardware decode
> > + * @msg_queue_core_ctx: msg queue context used for core thread
> > + *
> > * @hardware_bitmap: used to record hardware is ready or not
> > */
> > struct mtk_vcodec_dev {
> > @@ -508,6 +512,9 @@ struct mtk_vcodec_dev {
> > void *comp_dev[MTK_VDEC_HW_MAX];
> > struct device_node *component_node[MTK_VDEC_HW_MAX];
> >
> > + struct task_struct *kthread_core;
> > + struct vdec_msg_queue_ctx msg_queue_core_ctx;
> > +
> > DECLARE_BITMAP(hardware_bitmap, MTK_VDEC_HW_MAX);
> > };
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > index d66ed98c79a9..665f571eab4b 100644
> > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > @@ -256,3 +256,35 @@ void vdec_msg_queue_deinit(
> > kfree(lat_buf->private_data);
> > }
> > }
> > +
> > +int vdec_msg_queue_core_thead(void *data)
> > +{
> > + struct mtk_vcodec_dev *dev = data;
> > + struct vdec_lat_buf *lat_buf;
> > + struct mtk_vcodec_ctx *ctx;
> > +
> > + set_freezable();
> > + for (;;) {
> > + try_to_freeze();
> > + if (kthread_should_stop())
> > + break;
> > +
> > + lat_buf = vdec_msg_queue_dqbuf(
> > >msg_queue_core_ctx);
> > + if (!lat_buf)
> > + continue;
> > +
> > + ctx = lat_buf->ctx;
> > + mtk_vcodec_set_curr_ctx(dev, ctx, MTK_VDEC_CORE);
> > +
> > + if (!lat_buf->core_decode)
> > + mtk_v4l2_err("Core decode callback func is
> > NULL");
> > + else
> > + lat_buf->core_decode(lat_buf);
> > +
> > + mtk_vcodec_set_curr_ctx(dev, NULL, MTK_VDEC_CORE);
> > + vdec_msg_queue_qbuf(>msg_queue.lat_ctx, lat_buf);
> > + }
> > +
> > + mtk_v4l2_debug(3, "Video Capture Thread End");
> > + return 0;
> > +}
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h
> > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h
> > index 1905ce713592..b5745b144140 100644
> > ---
Re: [PATCH v6, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, Thanks for your suggestion. On Thu, 2021-10-14 at 09:38 -0300, Ezequiel Garcia wrote: > Hi Yunfei, > > On Tue, 12 Oct 2021 at 22:17, yunfei.d...@mediatek.com > wrote: > > Hi Ezequiel, > > > > Thanks for your feedback, > > > > The driver can work well now according to your advice with > > of_platform_populate interface. > > > > In order to separate parent node with children node, parent node > > is > > master device, children node is component device. > > > > The master and component are registered platform device. > > > > > > Could you please help to review the patch again when you are free: > > > > https://patchwork.linuxtv.org/project/linux-media/cover/20211011070247.792-1-yunfei.d...@mediatek.com/ > > > > I'm glad you managed to simplify the driver. I tried applying the > patches > but they don't apply on media master. Please push a branch to gitlab > or github > or somewhere public. > > Keep in mind that when you need people to review your code, > it's generally good practice to try to make it easy on them. > The harder you make it, the less inclined people will be to > spend time on your work. > I will send the patch v8 base on media tree, and I already test it in my local environment. And you can get media_tree then sync patch v8. > Thanks, > Ezequiel Thanks Yunfei Dong
Re: [PATCH v7, 04/15] media: mtk-vcodec: Manage multi hardware information
Hi AngeloGioacchino,
Thanks for your suggestion.
On Thu, 2021-10-14 at 11:20 +0200, AngeloGioacchino Del Regno wrote:
> > Manage each hardware information which includes irq/power/clk.
> > The hardware includes LAT0, LAT1 and CORE.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > v7: Using of_platform_populate not component framework to manage
> > multi hardware.
> > ---
> > drivers/media/platform/mtk-vcodec/Makefile| 1 +
> > .../platform/mtk-vcodec/mtk_vcodec_dec.h | 1 +
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 151 -
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 165
> > ++
> > .../platform/mtk-vcodec/mtk_vcodec_dec_hw.h | 49 ++
> > .../mtk-vcodec/mtk_vcodec_dec_stateful.c | 1 +
> > .../mtk-vcodec/mtk_vcodec_dec_stateless.c | 1 +
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 23 +++
> > 8 files changed, 359 insertions(+), 33 deletions(-)
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/mtk_vcodec_dec_hw.c
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/mtk_vcodec_dec_hw.h
> >
>
>
>
> Hello Yunfei,
>
> thanks for this great series! However, there are a few things to
> improve...
>
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > b/drivers/media/platform/mtk-vcodec/Makefile
> > index ca8e9e7a9c4e..edeb3b66e9e9 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -15,6 +15,7 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > mtk_vcodec_dec_stateful.o \
> > mtk_vcodec_dec_stateless.o \
> > mtk_vcodec_dec_pm.o \
> > + mtk_vcodec_dec_hw.o \
> >
> > mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> > venc/venc_h264_if.o \
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> > index 9fbd24186c1a..c509224cbff4 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.h
> > @@ -66,6 +66,7 @@ extern const struct v4l2_ioctl_ops
> > mtk_vdec_ioctl_ops;
> > extern const struct v4l2_m2m_ops mtk_vdec_m2m_ops;
> > extern const struct media_device_ops mtk_vcodec_media_ops;
> >
> > +extern struct platform_driver mtk_vdec_comp_driver;
>
> This may be useless... more on that later in this review.
>
Remove in patch v8.
> >
> > /*
> >* mtk_vdec_lock/mtk_vdec_unlock are for ctx instance to
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index dd749d41c75a..17cb3e3519eb 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -18,19 +18,61 @@
> >
> > #include "mtk_vcodec_drv.h"
> > #include "mtk_vcodec_dec.h"
> > +#include "mtk_vcodec_dec_hw.h"
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_intr.h"
> > -#include "mtk_vcodec_util.h"
> > #include "mtk_vcodec_fw.h"
> >
> > -#define VDEC_HW_ACTIVE 0x10
> > -#define VDEC_IRQ_CFG 0x11
> > -#define VDEC_IRQ_CLR 0x10
> > -#define VDEC_IRQ_CFG_REG 0xa4
> > -
> > module_param(mtk_v4l2_dbg_level, int, 0644);
> > module_param(mtk_vcodec_dbg, bool, 0644);
> >
> > +static struct of_device_id mtk_vdec_drv_ids[] = {
> > + {
> > + .compatible = "mediatek,mtk-vcodec-lat",
> > + .data = (void *)MTK_VDEC_LAT0,
> > + },
> > + {
> > + .compatible = "mediatek,mtk-vcodec-core",
> > + .data = (void *)MTK_VDEC_CORE,
> > + },
> > + {},
> > +};
>
> Is this a duplicate of "mtk_vdec_comp_ids", found in
> mtk_vcodec_dec_hw.c?!
>
Will change it.
> > +
> > +static int mtk_vcodec_comp_device_check(struct mtk_vcodec_ctx
> > *ctx)
> > + {
> > + struct mtk_vcodec_dev *vdec_dev = ctx->dev;
> > + struct platform_device *pdev = vdec_dev->plat_dev;
> > + struct device_node *comp_node;
> > + enum mtk_vdec_hw_id comp_idx;
> > + const struct of_device_id *of_id;
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(mtk_vdec_drv_ids); i++) {
> > + of_id = _vdec_drv_ids[i];
> > + comp_node = of_find_compatible_node(NULL, NULL,
> > + of_id->compatible);
> > + if (!comp_node)
> > + continue;
> > +
> > + if (!of_device_is_available(comp_node)) {
> > + of_node_put(comp_node);
> > + dev_err(>dev, "Fail to get MMSYS
> > node\n");
> > + continue;
> > + }
> > +
> > + comp_idx = (enum mtk_vdec_hw_id)of_id->data;
> > + vdec_dev->component_node[comp_idx] = comp_node;
> > +
> > + if (!test_bit(comp_idx, vdec_dev->hardware_bitmap)) {
> > + dev_err(>dev, "Vdec comp_idx is not ready
> > %d.",
> > +
Re: [PATCH v7, 07/15] media: mtk-vcodec: Add irq interface for multi hardware
Hi AngeloGioacchino,
Thanks for your suggestion.
On Thu, 2021-10-14 at 12:04 +0200, AngeloGioacchino Del Regno wrote:
> > Adds irq interface for multi hardware.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 33
> > +--
> > .../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 2 +-
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 25 ++
> > .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 4 +--
> > .../platform/mtk-vcodec/mtk_vcodec_intr.c | 27 +++---
> > -
> > .../platform/mtk-vcodec/mtk_vcodec_intr.h | 4 +--
> > .../platform/mtk-vcodec/vdec/vdec_h264_if.c | 2 +-
> > .../mtk-vcodec/vdec/vdec_h264_req_if.c| 2 +-
> > .../platform/mtk-vcodec/vdec/vdec_vp8_if.c| 2 +-
> > .../platform/mtk-vcodec/vdec/vdec_vp9_if.c| 2 +-
> > .../platform/mtk-vcodec/venc/venc_h264_if.c | 2 +-
> > .../platform/mtk-vcodec/venc/venc_vp8_if.c| 2 +-
> > 12 files changed, 71 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index 17cb3e3519eb..ff70fa5b34e3 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -73,6 +73,20 @@ static int mtk_vcodec_comp_device_check(struct
> > mtk_vcodec_ctx *ctx)
> > return 0;
> > }
> >
> > +static int mtk_vcodec_get_hw_count(struct mtk_vcodec_dev *dev)
> > +{
> > + switch (dev->vdec_pdata->hw_arch) {
> > + case MTK_VDEC_PURE_SINGLE_CORE:
> > +return MTK_VDEC_ONE_CORE;
> > + case MTK_VDEC_LAT_SINGLE_CORE:
> > + return MTK_VDEC_ONE_LAT_ONE_CORE;
> > + default:
> > + mtk_v4l2_err("not support hw arch:%d",
> > + dev->vdec_pdata->hw_arch);
> > + return MTK_VDEC_NO_HW;
> > + }
> > +}
> > +
> > static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void
> > *priv)
> > {
> > struct mtk_vcodec_dev *dev = priv;
> > @@ -104,7 +118,7 @@ static irqreturn_t
> > mtk_vcodec_dec_irq_handler(int irq, void *priv)
> > writel((readl(vdec_misc_addr) & ~VDEC_IRQ_CLR),
> > dev->reg_base[VDEC_MISC] + VDEC_IRQ_CFG_REG);
> >
> > - wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED);
> > + wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED, 0);
> >
> > mtk_v4l2_debug(3,
> > "mtk_vcodec_dec_irq_handler :wake up ctx %d,
> > dec_done_status=%x",
> > @@ -176,7 +190,7 @@ static int fops_vcodec_open(struct file *file)
> > {
> > struct mtk_vcodec_dev *dev = video_drvdata(file);
> > struct mtk_vcodec_ctx *ctx = NULL;
> > - int ret = 0;
> > + int ret = 0, i, hw_count;
> > struct vb2_queue *src_vq;
> >
> > ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > @@ -190,7 +204,19 @@ static int fops_vcodec_open(struct file *file)
> > v4l2_fh_add(>fh);
> > INIT_LIST_HEAD(>list);
> > ctx->dev = dev;
> > - init_waitqueue_head(>queue);
> > +
> > + if (ctx->dev->vdec_pdata->is_comp_supported) {
>
> As pointed out in the review for patch 04/15 of this series,
> is_comp_supported
> is always false since you moved away from the component framework.
>
> That means that this code can be removed, as nothing ever hits that.
>
> Please note that other patches in this series are following
> similar/same patterns,
> so this comment applies to the entire series.
>
> > + hw_count = mtk_vcodec_get_hw_count(dev);
> > + if (!hw_count) {
> > + ret = -EINVAL;
> > + goto err_init_queue;
> > + }
> > + for (i = 0; i < hw_count; i++)
> > + init_waitqueue_head(>queue[i]);
> > + } else {
> > + init_waitqueue_head(>queue[0]);
> > + }
> > +
> > mutex_init(>lock);
> >
> > ret = mtk_vcodec_comp_device_check(ctx);
> > @@ -253,6 +279,7 @@ static int fops_vcodec_open(struct file *file)
> > err_m2m_ctx_init:
> > v4l2_ctrl_handler_free(>ctrl_hdl);
> > err_ctrls_setup:
> > +err_init_queue:
>
> ...and if that unused component code gets removed, this label can
> also be removed.
>
For add 8192 to support component, these code need to keep.
> > v4l2_fh_del(>fh);
> > v4l2_fh_exit(>fh);
> > kfree(ctx);
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > index 3752ccaea284..0997a5a08156 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > @@ -56,7 +56,7 @@ static irqreturn_t mtk_vdec_comp_irq_handler(int
> > irq, void *priv)
> > writel(dec_done_status | VDEC_IRQ_CFG, vdec_misc_addr);
> > writel(dec_done_status & ~VDEC_IRQ_CLR, vdec_misc_addr);
> >
> > - wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED);
> > + wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED, dev->comp_idx);
>
Re: [PATCH v7, 08/15] media: mtk-vcodec: Add msg queue feature for lat and core architecture
Hi AngeloGioacchino,
Thanks for your suggestion.
On Thu, 2021-10-14 at 12:35 +0200, AngeloGioacchino Del Regno wrote:
> > For lat and core architecture, lat thread will send message to core
> > thread when lat decode done. Core hardware will use the message
> > from lat to decode, then free message to lat thread when decode
> > done.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > drivers/media/platform/mtk-vcodec/Makefile| 1 +
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 5 +
> > .../platform/mtk-vcodec/vdec_msg_queue.c | 258
> > ++
> > .../platform/mtk-vcodec/vdec_msg_queue.h | 151 ++
> > 4 files changed, 415 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/vdec_msg_queue.c
> > create mode 100644 drivers/media/platform/mtk-
> > vcodec/vdec_msg_queue.h
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/Makefile
> > b/drivers/media/platform/mtk-vcodec/Makefile
> > index edeb3b66e9e9..5000e59da576 100644
> > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > @@ -11,6 +11,7 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > mtk_vcodec_dec_drv.o \
> > vdec_drv_if.o \
> > vdec_vpu_if.o \
> > + vdec_msg_queue.o \
> > mtk_vcodec_dec.o \
> > mtk_vcodec_dec_stateful.o \
> > mtk_vcodec_dec_stateless.o \
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > index f8e8b5ba408b..ab401b2db30e 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > @@ -15,7 +15,9 @@
> > #include
> > #include
> > #include
> > +
> > #include "mtk_vcodec_util.h"
> > +#include "vdec_msg_queue.h"
> >
> > #define MTK_VCODEC_DRV_NAME "mtk_vcodec_drv"
> > #define MTK_VCODEC_DEC_NAME "mtk-vcodec-dec"
> > @@ -282,6 +284,8 @@ struct vdec_pic_info {
> >* @decoded_frame_cnt: number of decoded frames
> >* @lock: protect variables accessed by V4L2 threads and worker
> > thread such as
> >* mtk_video_dec_buf.
> > + *
> > + * @msg_queue: msg queue used to store lat buffer information.
> >*/
> > struct mtk_vcodec_ctx {
> > enum mtk_instance_type type;
> > @@ -325,6 +329,7 @@ struct mtk_vcodec_ctx {
> > int decoded_frame_cnt;
> > struct mutex lock;
> >
> > + struct vdec_msg_queue msg_queue;
> > };
> >
> > enum mtk_chip {
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > new file mode 100644
> > index ..d66ed98c79a9
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > @@ -0,0 +1,258 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2021 MediaTek Inc.
> > + * Author: Yunfei Dong
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +
> > +#include "mtk_vcodec_dec_pm.h"
> > +#include "mtk_vcodec_drv.h"
> > +#include "vdec_msg_queue.h"
> > +
> > +#define VDEC_LAT_SLICE_HEADER_SZ(640 * 1024)
>
> This can be 640 * SZ_1K, or 5 * SZ_128K
> ...if applicable, please multiply as value * alignment, such that,
> for example,
> if the data needs to be 1K aligned, you should prefer writing it as
> 640 * SZ_1K.
>
> > +#define VDEC_ERR_MAP_SZ_AVC ((8192 / 16) * (4352 / 16) /
> > 8)
>
Fix in v8.
> ...and you could do the same here... except, I see some sizes here
> being divided
> and multiplied and I take that as a hint.
> In that case, when you convert it to use sizes definitions, it would
> be very nice
> if you keep that hint / better describe it in a comment.
>
Fix in v8.
> > +
> > +/* lat write decoded hardware information to trans buffer,
> > + * and core will read the trans buffer to decode again. The
> > + * trans buffer size of FHD and 4K bitstreams are different.
> > + */
> > +static int vde_msg_queue_get_trans_size(int width, int height)
> > +{
> > + if (width > 1920 || height > 1088)
> > + return (30 * 1024 * 1024);
>
> So here we're referring to buffer sizes. 1024 * 1024 is SZ_1M, as
> defined in
> linux/sizes.h.
>
> Means that this can be simply
>
> return 30 * SZ_1M;
>
> > + else
> > + return 6 * 1024 * 1024;
>
> return 6 * SZ_1M;
>
Fix in v8.
> > +}
> > +
> > +void vdec_msg_queue_init_ctx(struct vdec_msg_queue_ctx *ctx,
> > + int hardware_index)
> > +{
> > + init_waitqueue_head(>ready_to_use);
> > + INIT_LIST_HEAD(>ready_queue);
> > + spin_lock_init(>ready_lock);
> > + ctx->ready_num = 0;
> > + ctx->hardware_index = hardware_index;
> > +}
> > +
> > +int vdec_msg_queue_init(
> > + struct vdec_msg_queue *msg_queue,
> > + struct mtk_vcodec_ctx *ctx,
> > + core_decode_cb_t core_decode,
> > + int private_size)
> > +{
> > + struct vdec_lat_buf *lat_buf;
Re: [PATCH v7, 09/15] media: mtk-vcodec: Generalize power and clock on/off interfaces
Hi AngeloGioacchino,
Thanks for your suggestion.
On Thu, 2021-10-14 at 12:44 +0200, AngeloGioacchino Del Regno wrote:
> > Generalizes power and clock on/off interfaces to support different
> > hardware.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 6 +-
> > .../platform/mtk-vcodec/mtk_vcodec_dec_hw.c | 2 +-
> > .../platform/mtk-vcodec/mtk_vcodec_dec_hw.h | 4 +
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 76
> > ++--
> > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h | 8 +-
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 2 +
> > .../platform/mtk-vcodec/mtk_vcodec_util.c | 87
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_util.h | 8 +-
> > .../media/platform/mtk-vcodec/vdec_drv_if.c | 21 ++---
> > 9 files changed, 174 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index ff70fa5b34e3..3ea1e96e0ec0 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -96,7 +96,7 @@ static irqreturn_t mtk_vcodec_dec_irq_handler(int
> > irq, void *priv)
> > void __iomem *vdec_misc_addr = dev->reg_base[VDEC_MISC] +
> > VDEC_IRQ_CFG_REG;
> >
> > - ctx = mtk_vcodec_get_curr_ctx(dev);
> > + ctx = mtk_vcodec_get_curr_ctx(dev, MTK_VDEC_CORE);
> >
> > /* check if HW active or not */
> > cg_status = readl(dev->reg_base[0]);
> > @@ -245,7 +245,7 @@ static int fops_vcodec_open(struct file *file)
> > mtk_vcodec_dec_set_default_params(ctx);
> >
> > if (v4l2_fh_is_singular(>fh)) {
> > - ret = mtk_vcodec_dec_pw_on(>pm);
> > + ret = mtk_vcodec_dec_pw_on(dev, MTK_VDEC_LAT0);
> > if (ret < 0)
> > goto err_load_fw;
> > /*
> > @@ -306,7 +306,7 @@ static int fops_vcodec_release(struct file
> > *file)
> > mtk_vcodec_dec_release(ctx);
> >
> > if (v4l2_fh_is_singular(>fh))
> > - mtk_vcodec_dec_pw_off(>pm);
> > + mtk_vcodec_dec_pw_off(dev, MTK_VDEC_LAT0);
> > v4l2_fh_del(>fh);
> > v4l2_fh_exit(>fh);
> > v4l2_ctrl_handler_free(>ctrl_hdl);
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > index 0997a5a08156..78d25916395d 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c
> > @@ -37,7 +37,7 @@ static irqreturn_t mtk_vdec_comp_irq_handler(int
> > irq, void *priv)
> > void __iomem *vdec_misc_addr = dev->reg_base[VDEC_COMP_MISC] +
> > VDEC_IRQ_CFG_REG;
> >
> > - ctx = mtk_vcodec_get_curr_ctx(dev->master_dev);
> > + ctx = mtk_vcodec_get_curr_ctx(dev->master_dev, dev->comp_idx);
> >
> > /* check if HW active or not */
> > cg_status = readl(dev->reg_base[VDEC_COMP_SYS]);
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
> > index 8d6e818a3474..0a4c2e6f5df2 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h
> > @@ -32,6 +32,8 @@ enum mtk_comp_hw_reg_idx {
> >* @master_dev: master device
> >* @reg_base: Mapped address of MTK Vcodec registers.
> >*
> > + * @curr_ctx: The context that is waiting for codec hardware
> > + *
> >* @dec_irq: decoder irq resource
> >* @pm: power management control
> >* @comp_idx: component index
> > @@ -41,6 +43,8 @@ struct mtk_vdec_comp_dev {
> > struct mtk_vcodec_dev *master_dev;
> > void __iomem *reg_base[VDEC_COMP_MAX];
> >
> > + struct mtk_vcodec_ctx *curr_ctx;
> > +
> > int dec_irq;
> > struct mtk_vcodec_pm pm;
> > int comp_idx;
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > index 20bd157a855c..183d4c4e36f0 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > @@ -5,11 +5,13 @@
> >*/
> >
> > #include
> > +#include
> > #include
> > #include
> > #include
> > #include
> >
> > +#include "mtk_vcodec_dec_hw.h"
> > #include "mtk_vcodec_dec_pm.h"
> > #include "mtk_vcodec_util.h"
> >
> > @@ -84,10 +86,23 @@ void mtk_vcodec_release_dec_pm(struct
> > mtk_vcodec_pm *pm)
> > put_device(pm->larbvdec);
> > }
> >
> > -int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> > +int mtk_vcodec_dec_pw_on(struct mtk_vcodec_dev *vdec_dev, int
> > comp_idx)
> > {
> > + struct mtk_vdec_comp_dev *comp_dev;
> > + struct mtk_vcodec_pm *pm;
> > int ret;
> >
> > + if
Re: [PATCH v7, 11/15] media: mtk-vcodec: Add core thread
Hi AngeloGioacchino,
Thanks for your suggestion.
On Thu, 2021-10-14 at 12:56 +0200, AngeloGioacchino Del Regno wrote:
> > Core thread:
> > 1. Gets lat_buf from core msg queue.
> > 2. Proceeds core decode.
> > 3. Puts the lat_buf back to lat msg queue.
> >
> > Both H264 and VP9 rely on the core thread.
> >
> > Signed-off-by: Yunfei Dong
>
> I would be happier to see a better commit message, for example:
> "Introduce a core thread, responsible for... getting lat_buf from ...
> which then proceeds core decode by ... and finally, puts the lat_buf
> back to the lat message queue"
>
> > ---
> > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 12 +++
> > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 7
> > .../platform/mtk-vcodec/vdec_msg_queue.c | 32
> > +++
> > .../platform/mtk-vcodec/vdec_msg_queue.h | 6
> > 4 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > index e21e0c4bcd86..de83e3b821b4 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > @@ -364,6 +364,18 @@ static int mtk_vcodec_probe(struct
> > platform_device *pdev)
> > goto err_dec_pm;
> > }
> >
> > + if (VDEC_LAT_ARCH(dev->vdec_pdata->hw_arch)) {
> > + vdec_msg_queue_init_ctx(>msg_queue_core_ctx,
> > + MTK_VDEC_CORE);
>
> No need to break this line.
>
Fix in v8.
> > + dev->kthread_core =
> > kthread_run(vdec_msg_queue_core_thead, dev,
> > + "mtk-%s", "core");
>
> Please fix indentation, like so:
> dev->kthread_core =
> kthread_run(vdec_msg_queue_core_thead, dev,
>
> "mtk-%s", "core");
>
> > + if (IS_ERR(dev->kthread_core)) {
> > + dev_err(>dev, "Failed to create core
> > thread");
> > + ret = PTR_ERR(dev->kthread_core);
> > + goto err_res;
> > + }
> > + }
> > +
> > for (i = 0; i < MTK_VDEC_HW_MAX; i++)
> > mutex_init(>dec_mutex[i]);
> > spin_lock_init(>irqlock);
> > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > index 9d072c082f73..68a9b1a2d3b3 100644
> > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > @@ -27,6 +27,7 @@
> > #define MTK_VCODEC_MAX_PLANES 3
> > #define MTK_V4L2_BENCHMARK0
> > #define WAIT_INTR_TIMEOUT_MS 1000
> > +#define VDEC_LAT_ARCH(hw_arch) ((hw_arch) >=
> > MTK_VDEC_LAT_SINGLE_CORE)
>
> I'd propose to change this to IS_VDEC_LAT_ARCH(hw_arch): that would
> increase human
> readability when using this macro.
>
Fix in v8.
> >
> > /*
> >* enum mtk_hw_reg_idx - MTK hw register base index
> > @@ -466,6 +467,9 @@ struct mtk_vcodec_enc_pdata {
> >* @comp_dev: component hardware device
> >* @component_node: component node
> >*
> > + * @kthread_core: thread used for core hardware decode
> > + * @msg_queue_core_ctx: msg queue context used for core thread
> > + *
> >* @hardware_bitmap: used to record hardware is ready or not
> >*/
> > struct mtk_vcodec_dev {
> > @@ -508,6 +512,9 @@ struct mtk_vcodec_dev {
> > void *comp_dev[MTK_VDEC_HW_MAX];
> > struct device_node *component_node[MTK_VDEC_HW_MAX];
> >
> > + struct task_struct *kthread_core;
> > + struct vdec_msg_queue_ctx msg_queue_core_ctx;
> > +
> > DECLARE_BITMAP(hardware_bitmap, MTK_VDEC_HW_MAX);
> > };
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > index d66ed98c79a9..665f571eab4b 100644
> > --- a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.c
> > @@ -256,3 +256,35 @@ void vdec_msg_queue_deinit(
> > kfree(lat_buf->private_data);
> > }
> > }
> > +
> > +int vdec_msg_queue_core_thead(void *data)
> > +{
> > + struct mtk_vcodec_dev *dev = data;
> > + struct vdec_lat_buf *lat_buf;
> > + struct mtk_vcodec_ctx *ctx;
> > +
> > + set_freezable();
> > + for (;;) {
> > + try_to_freeze();
> > + if (kthread_should_stop())
> > + break;
> > +
> > + lat_buf = vdec_msg_queue_dqbuf(
> > >msg_queue_core_ctx);
> > + if (!lat_buf)
> > + continue;
> > +
> > + ctx = lat_buf->ctx;
> > + mtk_vcodec_set_curr_ctx(dev, ctx, MTK_VDEC_CORE);
> > +
> > + if (!lat_buf->core_decode)
> > + mtk_v4l2_err("Core decode callback func is
> > NULL");
>
> Is this supposed to really happen?
> I see that this is always initialized in function
> vdec_msg_queue_init().
Fix in v8, using work queue
Re: [PATCH v7, 12/15] media: mtk-vcodec: Support 34bits dma address for vdec
Hi AngeloGioacchino, Thanks for your suggestion. On Thu, 2021-10-14 at 13:02 +0200, AngeloGioacchino Del Regno wrote: > > Use the dma_set_mask_and_coherent helper to set vdec > > DMA bit mask to support 34bits iova space(16GB) that > > the mt8192 iommu HW support. > > > > Whole the iova range separate to 0~4G/4G~8G/8G~12G/12G~16G, > > regarding which iova range VDEC actually locate, it > > depends on the dma-ranges property of vdec dtsi node. > > > > Signed-off-by: Yunfei Dong > > --- > > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > index de83e3b821b4..da963cdac96b 100644 > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > @@ -376,6 +376,9 @@ static int mtk_vcodec_probe(struct > > platform_device *pdev) > > } > > } > > > > + if (of_get_property(pdev->dev.of_node, "dma-ranges", NULL)) > > + dma_set_mask_and_coherent(>dev, > > DMA_BIT_MASK(34)); > > + > Will fix in patch v8. > This function returns 0 for success, or negative number for failure: > please check > the return value, or this driver may not work correctly in some > corner cases. > > Regards, > - Angelo > > > for (i = 0; i < MTK_VDEC_HW_MAX; i++) > > mutex_init(>dec_mutex[i]); > > spin_lock_init(>irqlock); > >
Re: [PATCH v6, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, Thanks for your feedback, The driver can work well now according to your advice with of_platform_populate interface. In order to separate parent node with children node, parent node is master device, children node is component device. The master and component are registered platform device. Could you please help to review the patch again when you are free: https://patchwork.linuxtv.org/project/linux-media/cover/20211011070247.792-1-yunfei.d...@mediatek.com/ Best Regards, Yunfei Dong On Sun, 2021-09-26 at 11:51 -0300, Ezequiel Garcia wrote: > On Sun, 26 Sept 2021 at 05:27, yunfei.d...@mediatek.com > wrote: > > > > Hi Ezequiel, > > > > Could you please help to give some feedback when you are free for > > iommu > > limitation? > > > > How about you work on the architecture I originally suggested? > > As the saying goes, talk is cheap, show us the code. > So let's see the code and let's discuss the limitations > with the code. > > > According to google's suggestion, it's better not to use v4l2 async > > also. > > Hum? I haven't seen such objection on the mailing list. > > Thanks, > Ezequiel
Re: [PATCH v7, 00/15] Support multi hardware decode using of_platform_populate
Hi Andrzej, On Tue, 2021-10-12 at 16:27 +0200, Andrzej Pietrasiewicz wrote: > Hi Yunfei Dong, > > W dniu 11.10.2021 o 09:02, Yunfei Dong pisze: > > This series adds support for multi hardware decode into mtk-vcodec, > > by first > > adding use of_platform_populate to manage each hardware > > information: interrupt, > > clock, register bases and power. Secondly add core thread to deal > > with core > > hardware message, at the same time, add msg queue for different > > hardware > > share messages. Lastly, the architecture of different specs are not > > the same, > > using specs type to separate them. > > > > This series has been tested with both MT8183 and MT8173. Decoding > > was working > > for both chips. > > > > Patches 1~3 rewrite get register bases and power on/off interface. > > > > Patch 4 add to support multi hardware. > > > > Patch 5 separate video encoder and decoder document > > > > Patches 6-15 add interfaces to support core hardware. > > Which tree does the series apply to? I don't understand your mean clearly. Media tree? You can get the patches from this link: https://patchwork.linuxtv.org/project/linux-media/cover/20211011070247.792-1-yunfei.d...@mediatek.com/ Thanks, Yunfei Dong > > Regards, > > Andrzej
Re: [PATCH v6, 12/15] media: mtk-vcodec: Support 34bits dma address for vdec
Hi Benjiamin, On Thu, 2021-10-07 at 13:37 +0200, Benjamin Gaignard wrote: > Le 01/09/2021 à 10:32, Yunfei Dong a écrit : > > Use the dma_set_mask_and_coherent helper to set vdec > > DMA bit mask to support 34bits iova space(16GB) that > > the mt8192 iommu HW support. > > > > Whole the iova range separate to 0~4G/4G~8G/8G~12G/12G~16G, > > regarding which iova range VDEC actually locate, it > > depends on the dma-ranges property of vdec dtsi node. > > > > Signed-off-by: Yunfei Dong > > --- > > drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > index 002352fcf8de..1a8d9308327d 100644 > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c > > @@ -417,6 +417,9 @@ static int mtk_vcodec_probe(struct > > platform_device *pdev) > > } > > } > > > > + if (of_get_property(pdev->dev.of_node, "dma-ranges", NULL)) > > + dma_set_mask_and_coherent(>dev, > > DMA_BIT_MASK(34)); > > + > > Hi Yunfei, > > Does all SoC support 34bits iova space ? > If not you need to also check SoC version before setting dma mask. > Not all SoC support 34bits. Will add dma-ranges property in dtsi if the SoC support 34bits. like this: dma-ranges = <>; Thanks, Yunfei Dong > Regards, > Benjamin > > > for (i = 0; i < MTK_VDEC_HW_MAX; i++) > > mutex_init(>dec_mutex[i]); > > spin_lock_init(>irqlock);
Re: [PATCH v6, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, Could you please help to give some feedback when you are free for iommu limitation? According to google's suggestion, it's better not to use v4l2 async also. If there are no further comments, I don't have other choice for this architecture. Thanks, Yunfei Dong On Tue, 2021-09-14 at 20:16 +0800, yunfei.d...@mediatek.com wrote: > Hi Ezequiel, > > On Fri, 2021-09-03 at 11:08 +0800, yunfei.d...@mediatek.com wrote: > > Hi Ezequiel, > > > > Thanks for your suggestion. > > On Thu, 2021-09-02 at 13:30 -0300, Ezequiel Garcia wrote: > > > On Wed, 1 Sept 2021 at 05:32, Yunfei Dong < > > > yunfei.d...@mediatek.com > > > > > > > > > > wrote: > > > > > > > > This series adds support for multi hardware decode into mtk- > > > > vcodec, > > > > by first > > > > adding component framework to manage each hardware information: > > > > interrupt, > > > > clock, register bases and power. Secondly add core thread to > > > > deal > > > > with core > > > > hardware message, at the same time, add msg queue for different > > > > hardware > > > > share messages. Lastly, the architecture of different specs are > > > > not > > > > the same, > > > > using specs type to separate them. > > > > > > > > This series has been tested with both MT8183 and MT8173. > > > > Decoding > > > > was working > > > > for both chips. > > > > > > > > Patches 1~3 rewrite get register bases and power on/off > > > > interface. > > > > > > > > Patch 4 add component framework to support multi hardware. > > > > > > > > Patch 5 separate video encoder and decoder document > > > > > > > > Patches 6-15 add interfaces to support core hardware. > > > > > > > > This patch dependents on : "media: mtk-vcodec: support for > > > > MT8183 > > > > decoder"[1] and > > > > "Mediatek MT8192 clock support"[2]. > > > > > > > > 1: Multi hardware decode is based on stateless decoder, MT8183 > > > > is > > > > the first time > > > > to add stateless decoder. Otherwise it will cause conflict. > > > > This > > > > patch will be > > > > accepted in 5.15[1]. > > > > > > > > 2: The definition of decoder clocks are in mt8192-clk.h, this > > > > patch > > > > already in clk tree[2]. > > > > > > > > [1] > > > > > > https://patchwork.linuxtv.org/project/linux-media/list/?series=5826 > > > > [2] > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next=f35f1a23e0e12e3173e9e9dedbc150d139027189 > > > > > > > > Changes compared with v5: > > > > -Add decoder hardware block diagram for patch 13/15 > > > > > > > > > > > > > The discussion on v5 was still on-going, so sending this v6 > > > is not helpful. The context for v5's discussion is now harder to > > > find. > > > > > > Please avoid sending a new version without properly > > > discussing all the feedback, and without reaching consensus. > > > This is very important, please keep it in mind. > > > > > > > Thanks for your remind, I will keep this patch until get the > > solution. > > > > > Specifically, the feedback on v5 was NAK, with the request to > > > avoid > > > using any async framework, and instead try to find a simpler > > > solution. > > > > > > For instance, you can model things with a bus-like pattern, > > > which ties all the devices together, under a parent node. > > > This pattern is common in the kernel, the parent > > > node can use of_platform_populate or similar > > > (git grep of_platform_populate, you will see plenty of examples). > > > > > > You will still have to do some work to have the proper > > > regs resources, but this is doable. Each child is a device, > > > so it can have its own resources (clocks, interrupts, iommus). > > > > > > You don't need any async framework. > > > > > Thanks for your suggestion very much, and there are several actions > need to check. > > 1: The iommu register like this: > ret = bus_set_iommu(_bus_type, > _iommu_ops); > It expect the consumer is a st
Re: [PATCH v6, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel,
On Fri, 2021-09-03 at 11:08 +0800, yunfei.d...@mediatek.com wrote:
> Hi Ezequiel,
>
> Thanks for your suggestion.
> On Thu, 2021-09-02 at 13:30 -0300, Ezequiel Garcia wrote:
> > On Wed, 1 Sept 2021 at 05:32, Yunfei Dong > >
> > wrote:
> > >
> > > This series adds support for multi hardware decode into mtk-
> > > vcodec,
> > > by first
> > > adding component framework to manage each hardware information:
> > > interrupt,
> > > clock, register bases and power. Secondly add core thread to deal
> > > with core
> > > hardware message, at the same time, add msg queue for different
> > > hardware
> > > share messages. Lastly, the architecture of different specs are
> > > not
> > > the same,
> > > using specs type to separate them.
> > >
> > > This series has been tested with both MT8183 and MT8173. Decoding
> > > was working
> > > for both chips.
> > >
> > > Patches 1~3 rewrite get register bases and power on/off
> > > interface.
> > >
> > > Patch 4 add component framework to support multi hardware.
> > >
> > > Patch 5 separate video encoder and decoder document
> > >
> > > Patches 6-15 add interfaces to support core hardware.
> > >
> > > This patch dependents on : "media: mtk-vcodec: support for MT8183
> > > decoder"[1] and
> > > "Mediatek MT8192 clock support"[2].
> > >
> > > 1: Multi hardware decode is based on stateless decoder, MT8183 is
> > > the first time
> > > to add stateless decoder. Otherwise it will cause conflict. This
> > > patch will be
> > > accepted in 5.15[1].
> > >
> > > 2: The definition of decoder clocks are in mt8192-clk.h, this
> > > patch
> > > already in clk tree[2].
> > >
> > > [1]
> > >
https://patchwork.linuxtv.org/project/linux-media/list/?series=5826
> > > [2]
> > >
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next=f35f1a23e0e12e3173e9e9dedbc150d139027189
> > >
> > > Changes compared with v5:
> > > -Add decoder hardware block diagram for patch 13/15
> > >
> >
> >
> > The discussion on v5 was still on-going, so sending this v6
> > is not helpful. The context for v5's discussion is now harder to
> > find.
> >
> > Please avoid sending a new version without properly
> > discussing all the feedback, and without reaching consensus.
> > This is very important, please keep it in mind.
> >
>
> Thanks for your remind, I will keep this patch until get the
> solution.
>
> > Specifically, the feedback on v5 was NAK, with the request to avoid
> > using any async framework, and instead try to find a simpler
> > solution.
> >
> > For instance, you can model things with a bus-like pattern,
> > which ties all the devices together, under a parent node.
> > This pattern is common in the kernel, the parent
> > node can use of_platform_populate or similar
> > (git grep of_platform_populate, you will see plenty of examples).
> >
> > You will still have to do some work to have the proper
> > regs resources, but this is doable. Each child is a device,
> > so it can have its own resources (clocks, interrupts, iommus).
> >
> > You don't need any async framework.
> >
>
Thanks for your suggestion very much, and there are several actions
need to check.
1: The iommu register like this:
ret = bus_set_iommu(_bus_type,
_iommu_ops);
It expect the consumer is a standard platform device.
otherwise it
could not enter to the iommu of_xlate.)
So if putting the iommus property in the child node, all the child
device need to registered as platform device.
2: For the interrupt in each child node, but the logical processing in
parent part. Child and parent need to send message for each other. In
order to control clk/power/irq for multi instance, need send message to
child to separate different hardware; child also need send message to
parent when get interrupt.
3: About Chen-Yu's mail, do you have any advice?
Do you have any suggestion about these two scenarios?
I'm very happy to get your reply.
Thanks
Yunfei Dong
> > vcodec_dec: vcodec_dec@1600 {
> > compatible = "mediatek,mt8192-vcodec-dec";
> > reg = ;
> > mediatek,scp = <>;
> > iommus = < M4U_PORT_L4_VDEC_MC_EXT>;
> > dma-ranges = <0x1 0x0 0x0 0x4000 0x0 0xfff0>;
> >
> > vcodec_lat@0x1 {
> > compatible = "mediatek,mtk-vcodec-lat";
> > reg = <0x1 0x800>; /* VDEC_MISC */
> > interrupts = ;
> > // etc
> > };
> >
> > vcodec_core@0x25000 {
> >compatible = "mediatek,mtk-vcodec-core";
> >reg = <0x25000 0x1000>; /* VDEC_CORE_MISC */
> >interrupts = ;
> >// etc
> > };
> > };
> >
> > Thanks,
> > Ezequiel
Re: [PATCH v6, 15/15] media: mtk-vcodec: Use codec type to separate different hardware
Hi Danfa,
Thanks for your suggestion.
On Fri, 2021-09-03 at 14:42 +0200, Dafna Hirschfeld wrote:
>
> On 02.09.21 08:05, yunfei.d...@mediatek.com wrote:
> > On Wed, 2021-09-01 at 14:17 +0200, Dafna Hirschfeld wrote:
> > Hi Dafna,
> >
> > Thanks for your suggestion.
> > > Hi
> > >
> > > On 01.09.21 10:32, Yunfei Dong wrote:
> > > > There are just one core thread, in order to separeate different
> > > > hardware, using codec type to separeate it in scp driver.
> > >
> > > this code seems to relate to the vpu driver not the scp driver.
> > > Is there a corresponding code added to the vpu driver that test
> > > the
> > > codec_type?
> > >
> >
> > Vpu is video processor unit, used to connect with micro processor.
> > In mt8173: vdec_vpu_if.c -> mtk_vpu.c -> micro processor
> > In mt8192/mt8183: vdec_vpu_if.c -> mtk_scp.c ->micro processor
> >
> > This init/dec start/dec_end interfaces are the same for vpu and
> > scp.
> > > >
> > > > Signed-off-by: Yunfei Dong
> > > > ---
> > > >.../media/platform/mtk-vcodec/vdec_ipi_msg.h | 12 ---
> > > >.../media/platform/mtk-vcodec/vdec_vpu_if.c | 34
> > > > ---
> > > >.../media/platform/mtk-vcodec/vdec_vpu_if.h | 4 +++
> > > >3 files changed, 41 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > > > b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > > > index 9d8079c4f976..c488f0c40190 100644
> > > > --- a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > > > +++ b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > > > @@ -35,6 +35,8 @@ enum vdec_ipi_msgid {
> > > > * @msg_id : vdec_ipi_msgid
> > > > * @vpu_inst_addr : VPU decoder instance address. Used if
> > > > ABI
> > > > version < 2.
> > > > * @inst_id : instance ID. Used if the ABI version >= 2.
> > > > + * @codec_type : Codec fourcc
> > > > + * @reserved : reserved param
> > > > */
> > > >struct vdec_ap_ipi_cmd {
> > > > uint32_t msg_id;
> > > > @@ -42,6 +44,8 @@ struct vdec_ap_ipi_cmd {
> > > > uint32_t vpu_inst_addr;
> > > > uint32_t inst_id;
> > > > };
> > > > + uint32_t codec_type;
> > > > + uint32_t reserved;
> > > >};
> > > >
> > > >/**
> > > > @@ -59,12 +63,12 @@ struct vdec_vpu_ipi_ack {
> > > >/**
> > > > * struct vdec_ap_ipi_init - for AP_IPIMSG_DEC_INIT
> > > > * @msg_id : AP_IPIMSG_DEC_INIT
> > > > - * @reserved : Reserved field
> > > > + * @codec_type : Codec fourcc
> > > > * @ap_inst_addr : AP video decoder instance address
> > > > */
> > > >struct vdec_ap_ipi_init {
> > > > uint32_t msg_id;
> > > > - uint32_t reserved;
> > > > + uint32_t codec_type;
> > > > uint64_t ap_inst_addr;
> > > >};
> > > >
> > > > @@ -77,7 +81,7 @@ struct vdec_ap_ipi_init {
> > > > * H264 decoder [0]:buf_sz [1]:nal_start
> > > > * VP8 decoder [0]:width/height
> > > > * VP9 decoder [0]:profile, [1][2] width/height
> > > > - * @reserved : Reserved field
> > > > + * @codec_type : Codec fourcc
> > > > */
> > > >struct vdec_ap_ipi_dec_start {
> > > > uint32_t msg_id;
> > > > @@ -86,7 +90,7 @@ struct vdec_ap_ipi_dec_start {
> > > > uint32_t inst_id;
> > > > };
> > > > uint32_t data[3];
> > > > - uint32_t reserved;
> > > > + uint32_t codec_type;
> > > >};
> > > >
> > > >/**
> > > > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > > > b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > > > index bfd8e87dceff..c84fac52fe26 100644
> > > > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > > > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > > > @@ -100,18 +100,29 @@ static void vpu_dec_ipi_handler(void
> > > > *data,
> > > > unsigned int len, void *pr
Re: [PATCH v6, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel,
Thanks for your suggestion.
On Thu, 2021-09-02 at 13:30 -0300, Ezequiel Garcia wrote:
> On Wed, 1 Sept 2021 at 05:32, Yunfei Dong
> wrote:
> >
> > This series adds support for multi hardware decode into mtk-vcodec,
> > by first
> > adding component framework to manage each hardware information:
> > interrupt,
> > clock, register bases and power. Secondly add core thread to deal
> > with core
> > hardware message, at the same time, add msg queue for different
> > hardware
> > share messages. Lastly, the architecture of different specs are not
> > the same,
> > using specs type to separate them.
> >
> > This series has been tested with both MT8183 and MT8173. Decoding
> > was working
> > for both chips.
> >
> > Patches 1~3 rewrite get register bases and power on/off interface.
> >
> > Patch 4 add component framework to support multi hardware.
> >
> > Patch 5 separate video encoder and decoder document
> >
> > Patches 6-15 add interfaces to support core hardware.
> >
> > This patch dependents on : "media: mtk-vcodec: support for MT8183
> > decoder"[1] and
> > "Mediatek MT8192 clock support"[2].
> >
> > 1: Multi hardware decode is based on stateless decoder, MT8183 is
> > the first time
> > to add stateless decoder. Otherwise it will cause conflict. This
> > patch will be
> > accepted in 5.15[1].
> >
> > 2: The definition of decoder clocks are in mt8192-clk.h, this patch
> > already in clk tree[2].
> >
> > [1]
> > https://patchwork.linuxtv.org/project/linux-media/list/?series=5826
> > [2]
> > https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/commit/?h=clk-next=f35f1a23e0e12e3173e9e9dedbc150d139027189
> >
> > Changes compared with v5:
> > -Add decoder hardware block diagram for patch 13/15
> >
>
>
> The discussion on v5 was still on-going, so sending this v6
> is not helpful. The context for v5's discussion is now harder to
> find.
>
> Please avoid sending a new version without properly
> discussing all the feedback, and without reaching consensus.
> This is very important, please keep it in mind.
>
Thanks for your remind, I will keep this patch until get the solution.
> Specifically, the feedback on v5 was NAK, with the request to avoid
> using any async framework, and instead try to find a simpler
> solution.
>
> For instance, you can model things with a bus-like pattern,
> which ties all the devices together, under a parent node.
> This pattern is common in the kernel, the parent
> node can use of_platform_populate or similar
> (git grep of_platform_populate, you will see plenty of examples).
>
> You will still have to do some work to have the proper
> regs resources, but this is doable. Each child is a device,
> so it can have its own resources (clocks, interrupts, iommus).
>
> You don't need any async framework.
>
If put the lat and core node in the parent node, need to check the
below things after discussed with iommu owner.
If putting the iommus property in the child node, then is the child
device a standard platform device?
The iommu registe like this:
ret = bus_set_iommu(_bus_type, _iommu_ops);
It expect the consumer is a standard platform device. otherwise it
could not enter to the iommu of_xlate.)
Thanks
Yunfei Dong
> vcodec_dec: vcodec_dec@1600 {
> compatible = "mediatek,mt8192-vcodec-dec";
> reg = ;
> mediatek,scp = <>;
> iommus = < M4U_PORT_L4_VDEC_MC_EXT>;
> dma-ranges = <0x1 0x0 0x0 0x4000 0x0 0xfff0>;
>
> vcodec_lat@0x1 {
> compatible = "mediatek,mtk-vcodec-lat";
> reg = <0x1 0x800>; /* VDEC_MISC */
> interrupts = ;
> // etc
> };
>
> vcodec_core@0x25000 {
>compatible = "mediatek,mtk-vcodec-core";
>reg = <0x25000 0x1000>; /* VDEC_CORE_MISC */
>interrupts = ;
>// etc
> };
> };
>
> Thanks,
> Ezequiel
Re: [PATCH v6, 15/15] media: mtk-vcodec: Use codec type to separate different hardware
On Wed, 2021-09-01 at 14:17 +0200, Dafna Hirschfeld wrote:
Hi Dafna,
Thanks for your suggestion.
> Hi
>
> On 01.09.21 10:32, Yunfei Dong wrote:
> > There are just one core thread, in order to separeate different
> > hardware, using codec type to separeate it in scp driver.
>
> this code seems to relate to the vpu driver not the scp driver.
> Is there a corresponding code added to the vpu driver that test the
> codec_type?
>
Vpu is video processor unit, used to connect with micro processor.
In mt8173: vdec_vpu_if.c -> mtk_vpu.c -> micro processor
In mt8192/mt8183: vdec_vpu_if.c -> mtk_scp.c ->micro processor
This init/dec start/dec_end interfaces are the same for vpu and scp.
> >
> > Signed-off-by: Yunfei Dong
> > ---
> > .../media/platform/mtk-vcodec/vdec_ipi_msg.h | 12 ---
> > .../media/platform/mtk-vcodec/vdec_vpu_if.c | 34
> > ---
> > .../media/platform/mtk-vcodec/vdec_vpu_if.h | 4 +++
> > 3 files changed, 41 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > index 9d8079c4f976..c488f0c40190 100644
> > --- a/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > +++ b/drivers/media/platform/mtk-vcodec/vdec_ipi_msg.h
> > @@ -35,6 +35,8 @@ enum vdec_ipi_msgid {
> >* @msg_id: vdec_ipi_msgid
> >* @vpu_inst_addr : VPU decoder instance address. Used if ABI
> > version < 2.
> >* @inst_id : instance ID. Used if the ABI version >= 2.
> > + * @codec_type : Codec fourcc
> > + * @reserved : reserved param
> >*/
> > struct vdec_ap_ipi_cmd {
> > uint32_t msg_id;
> > @@ -42,6 +44,8 @@ struct vdec_ap_ipi_cmd {
> > uint32_t vpu_inst_addr;
> > uint32_t inst_id;
> > };
> > + uint32_t codec_type;
> > + uint32_t reserved;
> > };
> >
> > /**
> > @@ -59,12 +63,12 @@ struct vdec_vpu_ipi_ack {
> > /**
> >* struct vdec_ap_ipi_init - for AP_IPIMSG_DEC_INIT
> >* @msg_id: AP_IPIMSG_DEC_INIT
> > - * @reserved : Reserved field
> > + * @codec_type : Codec fourcc
> >* @ap_inst_addr : AP video decoder instance address
> >*/
> > struct vdec_ap_ipi_init {
> > uint32_t msg_id;
> > - uint32_t reserved;
> > + uint32_t codec_type;
> > uint64_t ap_inst_addr;
> > };
> >
> > @@ -77,7 +81,7 @@ struct vdec_ap_ipi_init {
> >*H264 decoder [0]:buf_sz [1]:nal_start
> >*VP8 decoder [0]:width/height
> >*VP9 decoder [0]:profile, [1][2] width/height
> > - * @reserved : Reserved field
> > + * @codec_type : Codec fourcc
> >*/
> > struct vdec_ap_ipi_dec_start {
> > uint32_t msg_id;
> > @@ -86,7 +90,7 @@ struct vdec_ap_ipi_dec_start {
> > uint32_t inst_id;
> > };
> > uint32_t data[3];
> > - uint32_t reserved;
> > + uint32_t codec_type;
> > };
> >
> > /**
> > diff --git a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > index bfd8e87dceff..c84fac52fe26 100644
> > --- a/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > +++ b/drivers/media/platform/mtk-vcodec/vdec_vpu_if.c
> > @@ -100,18 +100,29 @@ static void vpu_dec_ipi_handler(void *data,
> > unsigned int len, void *priv)
> >
> > static int vcodec_vpu_send_msg(struct vdec_vpu_inst *vpu, void
> > *msg, int len)
> > {
> > - int err;
> > + int err, id, msgid;
> >
> > - mtk_vcodec_debug(vpu, "id=%X", *(uint32_t *)msg);
> > + msgid = *(uint32_t *)msg;
> > + mtk_vcodec_debug(vpu, "id=%X", msgid);
> >
> > vpu->failure = 0;
> > vpu->signaled = 0;
> >
> > - err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, vpu-
> > >id, msg,
> > + if (vpu->ctx->dev->vdec_pdata->hw_arch ==
> > MTK_VDEC_LAT_SINGLE_CORE) {
> > + if (msgid == AP_IPIMSG_DEC_CORE ||
> > + msgid == AP_IPIMSG_DEC_CORE_END)
> > + id = vpu->core_id;
> > + else
> > + id = vpu->id;
> > + } else {
> > + id = vpu->id;
> > + }
> > +
> > + err = mtk_vcodec_fw_ipi_send(vpu->ctx->dev->fw_handler, id,
> > msg,
> > len, 2000);
>
> so
> > if (err) {
> > mtk_vcodec_err(vpu, "send fail vpu_id=%d msg_id=%X
> > status=%d",
> > - vpu->id, *(uint32_t *)msg, err);
> > + id, msgid, err);
> > return err;
> > }
> >
> > @@ -131,6 +142,7 @@ static int vcodec_send_ap_ipi(struct
> > vdec_vpu_inst *vpu, unsigned int msg_id)
> > msg.vpu_inst_addr = vpu->inst_addr;
> > else
> > msg.inst_id = vpu->inst_id;
> > + msg.codec_type = vpu->codec_type;
> >
> > err = vcodec_vpu_send_msg(vpu, , sizeof(msg));
> > mtk_vcodec_debug(vpu, "- id=%X ret=%d", msg_id, err);
> > @@ -149,14 +161,25 @@ int vpu_dec_init(struct vdec_vpu_inst *vpu)
> >
> > err =
Re: [PATCH v5, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, You can get the dtsi information from patch 13, it is decoder yaml file about component architecture: [PATCH v4, 13/15] dt-bindings: media: mtk-vcodec: Adds decoder dt- bindings for mt8192 Thanks Yunfei Dong On Tue, 2021-08-24 at 18:21 +0800, yunfei.d...@mediatek.com wrote: > Hi Ezequiel, > > Thanks for your suggestion. > > On Sun, 2021-08-22 at 11:32 -0300, Ezequiel Garcia wrote: > > On Fri, 20 Aug 2021 at 04:59, yunfei.d...@mediatek.com > > wrote: > > > > > > Hi Ezequiel, > > > > > > Thanks for your detail feedback. > > > > > > On Thu, 2021-08-19 at 11:10 -0300, Ezequiel Garcia wrote: > > > > On Thu, 19 Aug 2021 at 04:13, yunfei.d...@mediatek.com > > > > wrote: > > > > > > > > > > Hi Ezequiel, > > > > > > > > > > Thanks for your suggestion. > > > > > > > > > > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote: > > > > > > +danvet > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Tue, 10 Aug 2021 at 23:58, Yunfei Dong < > > > > > > yunfei.d...@mediatek.com> > > > > > > wrote: > > > > > > > > > > > > > > This series adds support for multi hardware decode into > > > > > > > mtk-vcodec, > > > > > > > by first > > > > > > > adding component framework to manage each hardware > > > > > > > information: > > > > > > > interrupt, > > > > > > > clock, register bases and power. Secondly add core thread > > > > > > > to deal > > > > > > > with core > > > > > > > hardware message, at the same time, add msg queue for > > > > > > > different > > > > > > > hardware > > > > > > > share messages. Lastly, the architecture of different > > > > > > > specs > > > > > > > are not > > > > > > > the same, > > > > > > > using specs type to separate them. > > > > > > > > > > > > > > > > > > > I don't think it's a good idea to introduce the component > > > > > > API > > > > > > in the > > > > > > media subsystem. It doesn't seem to be maintained, IRC > > > > > > there's not > > > > > > even > > > > > > a maintainer for it, and it has some issues that were never > > > > > > addressed. > > > > > > > > > > > > It would be really important to avoid it. Is it really > > > > > > needed > > > > > > in the > > > > > > first place? > > > > > > > > > > > > Thanks, > > > > > > Ezequiel > > > > > > > > > > For there are many hardware need to use, mt8192 is three and > > > > > mt8195 is > > > > > five. Maybe need more to be used in the feature. > > > > > > > > > > Each hardware has independent clk/power/iommu port/irq. > > > > > Use component interface in prob to get each component's > > > > > information. > > > > > Just enable the hardware when need to use it, very convenient > > > > > and > > > > > simple. > > > > > > > > > > I found that there are many modules use component to manage > > > > > hardware > > > > > information, such as iommu and drm etc. > > > > > > > > > > > > > Many drivers support multiple hardware variants, where each > > > > variant > > > > has a different number of clocks or interrupts, see for > > > > instance > > > > struct hantro_variant which allows to expose different codec > > > > cores, > > > > some having both decoder/encoder, and some having just a > > > > decoder. > > > > > > > > The component API is mostly used by DRM to aggregate > > > > independent > > > > subdevices (called components) into an aggregated driver. > > > > > > > > For instance, a DRM driver needs to glue together the HDMI, > > > > MIPI, > > > > and plany controller, or any other hardware arrangement where > > > >
Re: [PATCH v5, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, Thanks for your suggestion. On Sun, 2021-08-22 at 11:32 -0300, Ezequiel Garcia wrote: > On Fri, 20 Aug 2021 at 04:59, yunfei.d...@mediatek.com > wrote: > > > > Hi Ezequiel, > > > > Thanks for your detail feedback. > > > > On Thu, 2021-08-19 at 11:10 -0300, Ezequiel Garcia wrote: > > > On Thu, 19 Aug 2021 at 04:13, yunfei.d...@mediatek.com > > > wrote: > > > > > > > > Hi Ezequiel, > > > > > > > > Thanks for your suggestion. > > > > > > > > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote: > > > > > +danvet > > > > > > > > > > Hi, > > > > > > > > > > On Tue, 10 Aug 2021 at 23:58, Yunfei Dong < > > > > > yunfei.d...@mediatek.com> > > > > > wrote: > > > > > > > > > > > > This series adds support for multi hardware decode into > > > > > > mtk-vcodec, > > > > > > by first > > > > > > adding component framework to manage each hardware > > > > > > information: > > > > > > interrupt, > > > > > > clock, register bases and power. Secondly add core thread > > > > > > to deal > > > > > > with core > > > > > > hardware message, at the same time, add msg queue for > > > > > > different > > > > > > hardware > > > > > > share messages. Lastly, the architecture of different specs > > > > > > are not > > > > > > the same, > > > > > > using specs type to separate them. > > > > > > > > > > > > > > > > I don't think it's a good idea to introduce the component API > > > > > in the > > > > > media subsystem. It doesn't seem to be maintained, IRC > > > > > there's not > > > > > even > > > > > a maintainer for it, and it has some issues that were never > > > > > addressed. > > > > > > > > > > It would be really important to avoid it. Is it really needed > > > > > in the > > > > > first place? > > > > > > > > > > Thanks, > > > > > Ezequiel > > > > > > > > For there are many hardware need to use, mt8192 is three and > > > > mt8195 is > > > > five. Maybe need more to be used in the feature. > > > > > > > > Each hardware has independent clk/power/iommu port/irq. > > > > Use component interface in prob to get each component's > > > > information. > > > > Just enable the hardware when need to use it, very convenient > > > > and > > > > simple. > > > > > > > > I found that there are many modules use component to manage > > > > hardware > > > > information, such as iommu and drm etc. > > > > > > > > > > Many drivers support multiple hardware variants, where each > > > variant > > > has a different number of clocks or interrupts, see for instance > > > struct hantro_variant which allows to expose different codec > > > cores, > > > some having both decoder/encoder, and some having just a decoder. > > > > > > The component API is mostly used by DRM to aggregate independent > > > subdevices (called components) into an aggregated driver. > > > > > > For instance, a DRM driver needs to glue together the HDMI, MIPI, > > > and plany controller, or any other hardware arrangement where > > > devices can be described independently. > > > > > > > The usage scenario is very similar with drm and iommu, So decide to > > use > > component framework. > > Decode has three/five or more hardwares, these hardware are > > independent. > > For mt8183 just need core hardware to decode, but mt8192 has > > lat,soc and > > core hardware to decode. When lat need to use, just enable lat > > hardware, > > core is the same.And mt8195 will has two cores, each core can work > > well > > independent. > > > > For each component device just used to open their power/clk/iommu > > port/irq when master need to enable it. The main logic is in master > > device. > > > > > The component API may look simple but has some issues, it's not > > > easy > > > to debug, and can cause troubles if not use
Re: [PATCH v5, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, Thanks for your detail feedback. On Thu, 2021-08-19 at 11:10 -0300, Ezequiel Garcia wrote: > On Thu, 19 Aug 2021 at 04:13, yunfei.d...@mediatek.com > wrote: > > > > Hi Ezequiel, > > > > Thanks for your suggestion. > > > > On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote: > > > +danvet > > > > > > Hi, > > > > > > On Tue, 10 Aug 2021 at 23:58, Yunfei Dong > > > wrote: > > > > > > > > This series adds support for multi hardware decode into mtk-vcodec, > > > > by first > > > > adding component framework to manage each hardware information: > > > > interrupt, > > > > clock, register bases and power. Secondly add core thread to deal > > > > with core > > > > hardware message, at the same time, add msg queue for different > > > > hardware > > > > share messages. Lastly, the architecture of different specs are not > > > > the same, > > > > using specs type to separate them. > > > > > > > > > > I don't think it's a good idea to introduce the component API in the > > > media subsystem. It doesn't seem to be maintained, IRC there's not > > > even > > > a maintainer for it, and it has some issues that were never > > > addressed. > > > > > > It would be really important to avoid it. Is it really needed in the > > > first place? > > > > > > Thanks, > > > Ezequiel > > > > For there are many hardware need to use, mt8192 is three and mt8195 is > > five. Maybe need more to be used in the feature. > > > > Each hardware has independent clk/power/iommu port/irq. > > Use component interface in prob to get each component's information. > > Just enable the hardware when need to use it, very convenient and > > simple. > > > > I found that there are many modules use component to manage hardware > > information, such as iommu and drm etc. > > > > Many drivers support multiple hardware variants, where each variant > has a different number of clocks or interrupts, see for instance > struct hantro_variant which allows to expose different codec cores, > some having both decoder/encoder, and some having just a decoder. > > The component API is mostly used by DRM to aggregate independent > subdevices (called components) into an aggregated driver. > > For instance, a DRM driver needs to glue together the HDMI, MIPI, > and plany controller, or any other hardware arrangement where > devices can be described independently. > The usage scenario is very similar with drm and iommu, So decide to use component framework. Decode has three/five or more hardwares, these hardware are independent. For mt8183 just need core hardware to decode, but mt8192 has lat,soc and core hardware to decode. When lat need to use, just enable lat hardware, core is the same.And mt8195 will has two cores, each core can work well independent. For each component device just used to open their power/clk/iommu port/irq when master need to enable it. The main logic is in master device. > The component API may look simple but has some issues, it's not easy > to debug, and can cause troubles if not used as expected [1]. > It's worth making sure you actually need a framework > to glue different devices together. > Each hardware has its index, master can get hardware information according these index, looks not complex. What do you mean about not easy to debug? > > Do you have any other suggestion for this architecture? > > > > Looking at the different patchsets that are posted, it's not clear > to me what exactly are the different architectures that you intend > to support, can you some documentation which clarifies that? > Have five hardwares lat,soc,core0,core1 and main. Lat thread can use lat soc and main, core thread can use soc,lat, core0 and core1. Core thread can be used or not for different project. Also Need to use these hardware dynamic at the same time. So I use component framework, just need to know the used hardware index according to different project.Need not to do complex logic to manage these hardwares. > Thanks, > Ezequiel > > [1] > https://patchwork.kernel.org/project/linux-rockchip/cover/20200120170602.3832-1-ezequ...@collabora.com/ Thanks, Yunfei Dong
Re: [PATCH v5, 00/15] Using component framework to support multi hardware decode
Hi Ezequiel, Thanks for your suggestion. On Wed, 2021-08-18 at 11:11 -0300, Ezequiel Garcia wrote: > +danvet > > Hi, > > On Tue, 10 Aug 2021 at 23:58, Yunfei Dong > wrote: > > > > This series adds support for multi hardware decode into mtk-vcodec, > > by first > > adding component framework to manage each hardware information: > > interrupt, > > clock, register bases and power. Secondly add core thread to deal > > with core > > hardware message, at the same time, add msg queue for different > > hardware > > share messages. Lastly, the architecture of different specs are not > > the same, > > using specs type to separate them. > > > > I don't think it's a good idea to introduce the component API in the > media subsystem. It doesn't seem to be maintained, IRC there's not > even > a maintainer for it, and it has some issues that were never > addressed. > > It would be really important to avoid it. Is it really needed in the > first place? > > Thanks, > Ezequiel For there are many hardware need to use, mt8192 is three and mt8195 is five. Maybe need more to be used in the feature. Each hardware has independent clk/power/iommu port/irq. Use component interface in prob to get each component's information. Just enable the hardware when need to use it, very convenient and simple. I found that there are many modules use component to manage hardware information, such as iommu and drm etc. Do you have any other suggestion for this architecture? Thanks Yunfei Dong
