Re: [PATCH v3 01/16] media: mtk-vcodec: abstract firmware interface
From: Alexandre Courbot
Date: Fri Aug 21 2020 - 06:35:37 EST
On Mon, Jul 27, 2020 at 11:25 PM Ezequiel Garcia
<ezequiel@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, 27 Jul 2020 at 11:24, Ezequiel Garcia
> <ezequiel@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, 27 Jul 2020 at 06:06, Alexandre Courbot <acourbot@xxxxxxxxxxxx> wrote:
> > >
> > > On Thu, Jul 23, 2020 at 6:23 AM Ezequiel Garcia
> > > <ezequiel@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Mon, 13 Jul 2020 at 03:09, Alexandre Courbot <acourbot@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > From: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > > > >
> > > > > MT8183's codec firwmare is run by a different remote processor from
> > > > > MT8173. While the firmware interface is basically the same, the way to
> > > > > invoke it differs. Abstract all firmware calls under a layer that will
> > > > > allow us to handle both firmware types transparently.
> > > > >
> > > > > Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > > > > [acourbot: refactor, cleanup and split]
> > > > > Co-developed-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
> > > > > Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxxxx>
> > > > > [pihsun: fix error path and add mtk_vcodec_fw_release]
> > > > > Signed-off-by: Pi-Hsun Shih <pihsun@xxxxxxxxxxxx>
> > > > > Reviewed-by: Tiffany Lin <tiffany.lin@xxxxxxxxxxxx>
> > > > > Acked-by: Tiffany Lin <tiffany.lin@xxxxxxxxxxxx>
> > > > > ---
> > > > > drivers/media/platform/mtk-vcodec/Makefile | 4 +-
> > > > > .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 50 ++---
> > > > > .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c | 1 -
> > > > > .../platform/mtk-vcodec/mtk_vcodec_drv.h | 5 +-
> > > > > .../platform/mtk-vcodec/mtk_vcodec_enc_drv.c | 47 ++---
> > > > > .../platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 2 -
> > > > > .../media/platform/mtk-vcodec/mtk_vcodec_fw.c | 172 ++++++++++++++++++
> > > > > .../media/platform/mtk-vcodec/mtk_vcodec_fw.h | 36 ++++
> > > > > .../platform/mtk-vcodec/mtk_vcodec_util.c | 1 -
> > > > > .../platform/mtk-vcodec/vdec/vdec_h264_if.c | 1 -
> > > > > .../platform/mtk-vcodec/vdec/vdec_vp8_if.c | 1 -
> > > > > .../platform/mtk-vcodec/vdec/vdec_vp9_if.c | 1 -
> > > > > .../media/platform/mtk-vcodec/vdec_drv_base.h | 2 -
> > > > > .../media/platform/mtk-vcodec/vdec_drv_if.c | 1 -
> > > > > .../media/platform/mtk-vcodec/vdec_vpu_if.c | 12 +-
> > > > > .../media/platform/mtk-vcodec/vdec_vpu_if.h | 11 +-
> > > > > .../platform/mtk-vcodec/venc/venc_h264_if.c | 15 +-
> > > > > .../platform/mtk-vcodec/venc/venc_vp8_if.c | 8 +-
> > > > > .../media/platform/mtk-vcodec/venc_drv_if.c | 1 -
> > > > > .../media/platform/mtk-vcodec/venc_vpu_if.c | 17 +-
> > > > > .../media/platform/mtk-vcodec/venc_vpu_if.h | 5 +-
> > > > > 21 files changed, 290 insertions(+), 103 deletions(-)
> > > > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> > > > > create mode 100644 drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.h
> > > > >
> > > > > diff --git a/drivers/media/platform/mtk-vcodec/Makefile b/drivers/media/platform/mtk-vcodec/Makefile
> > > > > index 37b94b555fa1..b8636119ed0a 100644
> > > > > --- a/drivers/media/platform/mtk-vcodec/Makefile
> > > > > +++ b/drivers/media/platform/mtk-vcodec/Makefile
> > > > > @@ -12,7 +12,7 @@ mtk-vcodec-dec-y := vdec/vdec_h264_if.o \
> > > > > vdec_vpu_if.o \
> > > > > mtk_vcodec_dec.o \
> > > > > mtk_vcodec_dec_pm.o \
> > > > > -
> > > > > + mtk_vcodec_fw.o
> > > > >
> > > > > mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> > > > > venc/venc_h264_if.o \
> > > > > @@ -25,5 +25,3 @@ mtk-vcodec-enc-y := venc/venc_vp8_if.o \
> > > > >
> > > > > mtk-vcodec-common-y := mtk_vcodec_intr.o \
> > > > > mtk_vcodec_util.o\
> > > > > -
> > > > > -ccflags-y += -I$(srctree)/drivers/media/platform/mtk-vpu
> > > > > 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 97a1b6664c20..4f07a5fcce7f 100644
> > > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> > > > > @@ -20,7 +20,7 @@
> > > > > #include "mtk_vcodec_dec_pm.h"
> > > > > #include "mtk_vcodec_intr.h"
> > > > > #include "mtk_vcodec_util.h"
> > > > > -#include "mtk_vpu.h"
> > > > > +#include "mtk_vcodec_fw.h"
> > > > >
> > > > > #define VDEC_HW_ACTIVE 0x10
> > > > > #define VDEC_IRQ_CFG 0x11
> > > > > @@ -77,22 +77,6 @@ static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv)
> > > > > return IRQ_HANDLED;
> > > > > }
> > > > >
> > > > > -static void mtk_vcodec_dec_reset_handler(void *priv)
> > > > > -{
> > > > > - struct mtk_vcodec_dev *dev = priv;
> > > > > - struct mtk_vcodec_ctx *ctx;
> > > > > -
> > > > > - mtk_v4l2_err("Watchdog timeout!!");
> > > > > -
> > > > > - mutex_lock(&dev->dev_mutex);
> > > > > - list_for_each_entry(ctx, &dev->ctx_list, list) {
> > > > > - ctx->state = MTK_STATE_ABORT;
> > > > > - mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ERROR",
> > > > > - ctx->id);
> > > > > - }
> > > > > - mutex_unlock(&dev->dev_mutex);
> > > > > -}
> > > > > -
> > > > > static int fops_vcodec_open(struct file *file)
> > > > > {
> > > > > struct mtk_vcodec_dev *dev = video_drvdata(file);
> > > > > @@ -144,21 +128,20 @@ static int fops_vcodec_open(struct file *file)
> > > > > if (v4l2_fh_is_singular(&ctx->fh)) {
> > > > > mtk_vcodec_dec_pw_on(&dev->pm);
> > > > > /*
> > > > > - * vpu_load_firmware checks if it was loaded already and
> > > > > - * does nothing in that case
> > > > > + * Does nothing if firmware was already loaded.
> > > > > */
> > > > > - ret = vpu_load_firmware(dev->vpu_plat_dev);
> > > > > + ret = mtk_vcodec_fw_load_firmware(dev->fw_handler);
> > > > > if (ret < 0) {
> > > > > /*
> > > > > * Return 0 if downloading firmware successfully,
> > > > > * otherwise it is failed
> > > > > */
> > > > > - mtk_v4l2_err("vpu_load_firmware failed!");
> > > > > + mtk_v4l2_err("failed to load firmware!");
> > > > > goto err_load_fw;
> > > > > }
> > > > >
> > > > > dev->dec_capability =
> > > > > - vpu_get_vdec_hw_capa(dev->vpu_plat_dev);
> > > > > + mtk_vcodec_fw_get_vdec_capa(dev->fw_handler);
> > > > > mtk_v4l2_debug(0, "decoder capability %x", dev->dec_capability);
> > > > > }
> > > > >
> > > > > @@ -228,6 +211,8 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > > > > struct mtk_vcodec_dev *dev;
> > > > > struct video_device *vfd_dec;
> > > > > struct resource *res;
> > > > > + phandle rproc_phandle;
> > > > > + enum mtk_vcodec_fw_type fw_type;
> > > > > int i, ret;
> > > > >
> > > > > dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> > > > > @@ -237,19 +222,21 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > > > > INIT_LIST_HEAD(&dev->ctx_list);
> > > > > dev->plat_dev = pdev;
> > > > >
> > > > > - dev->vpu_plat_dev = vpu_get_plat_device(dev->plat_dev);
> > > > > - if (dev->vpu_plat_dev == NULL) {
> > > > > - mtk_v4l2_err("[VPU] vpu device in not ready");
> > > > > - return -EPROBE_DEFER;
> > > > > + if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
> > > > > + &rproc_phandle)) {
> > > > > + fw_type = VPU;
> > > > > + } else {
> > > > > + mtk_v4l2_err("Could not get vdec IPI device");
> > > > > + return -ENODEV;
> > > > > }
> > > > > -
> > > > > - vpu_wdt_reg_handler(dev->vpu_plat_dev, mtk_vcodec_dec_reset_handler,
> > > > > - dev, VPU_RST_DEC);
> > > > > + dev->fw_handler = mtk_vcodec_fw_select(dev, fw_type, VPU_RST_DEC);
> > > > > + if (IS_ERR(dev->fw_handler))
> > > > > + return PTR_ERR(dev->fw_handler);
> > > > >
> > > > > ret = mtk_vcodec_init_dec_pm(dev);
> > > > > if (ret < 0) {
> > > > > dev_err(&pdev->dev, "Failed to get mt vcodec clock source");
> > > > > - return ret;
> > > > > + goto err_dec_pm;
> > > > > }
> > > > >
> > > > > for (i = 0; i < NUM_MAX_VDEC_REG_BASE; i++) {
> > > > > @@ -352,6 +339,8 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > > > > v4l2_device_unregister(&dev->v4l2_dev);
> > > > > err_res:
> > > > > mtk_vcodec_release_dec_pm(dev);
> > > > > +err_dec_pm:
> > > > > + mtk_vcodec_fw_release(dev->fw_handler);
> > > > > return ret;
> > > > > }
> > > > >
> > > > > @@ -376,6 +365,7 @@ static int mtk_vcodec_dec_remove(struct platform_device *pdev)
> > > > >
> > > > > v4l2_device_unregister(&dev->v4l2_dev);
> > > > > mtk_vcodec_release_dec_pm(dev);
> > > > > + 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 5a6ec8fb52da..36dfe3fc056a 100644
> > > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> > > > > @@ -12,7 +12,6 @@
> > > > >
> > > > > #include "mtk_vcodec_dec_pm.h"
> > > > > #include "mtk_vcodec_util.h"
> > > > > -#include "mtk_vpu.h"
> > > > >
> > > > > int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *mtkdev)
> > > > > {
> > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > > > > index 9fd56dee7fd1..e132c4ec463a 100644
> > > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h
> > > > > @@ -309,13 +309,13 @@ struct mtk_vcodec_ctx {
> > > > > * @m2m_dev_dec: m2m device for decoder
> > > > > * @m2m_dev_enc: m2m device for encoder.
> > > > > * @plat_dev: platform device
> > > > > - * @vpu_plat_dev: mtk vpu platform device
> > > > > * @ctx_list: list of struct mtk_vcodec_ctx
> > > > > * @irqlock: protect data access by irq handler and work thread
> > > > > * @curr_ctx: The context that is waiting for codec hardware
> > > > > *
> > > > > * @reg_base: Mapped address of MTK Vcodec registers.
> > > > > *
> > > > > + * @fw_handler: used to communicate with the firmware.
> > > > > * @id_counter: used to identify current opened instance
> > > > > *
> > > > > * @encode_workqueue: encode work queue
> > > > > @@ -344,12 +344,13 @@ struct mtk_vcodec_dev {
> > > > > struct v4l2_m2m_dev *m2m_dev_dec;
> > > > > struct v4l2_m2m_dev *m2m_dev_enc;
> > > > > struct platform_device *plat_dev;
> > > > > - struct platform_device *vpu_plat_dev;
> > > > > struct list_head ctx_list;
> > > > > spinlock_t irqlock;
> > > > > struct mtk_vcodec_ctx *curr_ctx;
> > > > > void __iomem *reg_base[NUM_MAX_VCODEC_REG_BASE];
> > > > >
> > > > > + struct mtk_vcodec_fw *fw_handler;
> > > > > +
> > > > > unsigned long id_counter;
> > > > >
> > > > > struct workqueue_struct *decode_workqueue;
> > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
> > > > > index 4d31f1ed113f..4340ea10afd0 100644
> > > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
> > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c
> > > > > @@ -21,7 +21,7 @@
> > > > > #include "mtk_vcodec_enc_pm.h"
> > > > > #include "mtk_vcodec_intr.h"
> > > > > #include "mtk_vcodec_util.h"
> > > > > -#include "mtk_vpu.h"
> > > > > +#include "mtk_vcodec_fw.h"
> > > > >
> > > > > module_param(mtk_v4l2_dbg_level, int, S_IRUGO | S_IWUSR);
> > > > > module_param(mtk_vcodec_dbg, bool, S_IRUGO | S_IWUSR);
> > > > > @@ -101,22 +101,6 @@ static irqreturn_t mtk_vcodec_enc_lt_irq_handler(int irq, void *priv)
> > > > > return IRQ_HANDLED;
> > > > > }
> > > > >
> > > > > -static void mtk_vcodec_enc_reset_handler(void *priv)
> > > > > -{
> > > > > - struct mtk_vcodec_dev *dev = priv;
> > > > > - struct mtk_vcodec_ctx *ctx;
> > > > > -
> > > > > - mtk_v4l2_debug(0, "Watchdog timeout!!");
> > > > > -
> > > > > - mutex_lock(&dev->dev_mutex);
> > > > > - list_for_each_entry(ctx, &dev->ctx_list, list) {
> > > > > - ctx->state = MTK_STATE_ABORT;
> > > > > - mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> > > > > - ctx->id);
> > > > > - }
> > > > > - mutex_unlock(&dev->dev_mutex);
> > > > > -}
> > > > > -
> > > > > static int fops_vcodec_open(struct file *file)
> > > > > {
> > > > > struct mtk_vcodec_dev *dev = video_drvdata(file);
> > > > > @@ -159,10 +143,10 @@ static int fops_vcodec_open(struct file *file)
> > > > >
> > > > > if (v4l2_fh_is_singular(&ctx->fh)) {
> > > > > /*
> > > > > - * vpu_load_firmware checks if it was loaded already and
> > > > > + * load fireware to checks if it was loaded already and
> > > > > * does nothing in that case
> > > > > */
> > > > > - ret = vpu_load_firmware(dev->vpu_plat_dev);
> > > > > + ret = mtk_vcodec_fw_load_firmware(dev->fw_handler);
> > > > > if (ret < 0) {
> > > > > /*
> > > > > * Return 0 if downloading firmware successfully,
> > > > > @@ -173,7 +157,7 @@ static int fops_vcodec_open(struct file *file)
> > > > > }
> > > > >
> > > > > dev->enc_capability =
> > > > > - vpu_get_venc_hw_capa(dev->vpu_plat_dev);
> > > > > + mtk_vcodec_fw_get_venc_capa(dev->fw_handler);
> > > > > mtk_v4l2_debug(0, "encoder capability %x", dev->enc_capability);
> > > > > }
> > > > >
> > > > > @@ -235,6 +219,8 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > > > > struct mtk_vcodec_dev *dev;
> > > > > struct video_device *vfd_enc;
> > > > > struct resource *res;
> > > > > + phandle rproc_phandle;
> > > > > + enum mtk_vcodec_fw_type fw_type;
> > > > > int i, j, ret;
> > > > >
> > > > > dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> > > > > @@ -244,19 +230,21 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > > > > INIT_LIST_HEAD(&dev->ctx_list);
> > > > > dev->plat_dev = pdev;
> > > > >
> > > > > - dev->vpu_plat_dev = vpu_get_plat_device(dev->plat_dev);
> > > > > - if (dev->vpu_plat_dev == NULL) {
> > > > > - mtk_v4l2_err("[VPU] vpu device in not ready");
> > > > > - return -EPROBE_DEFER;
> > > > > + if (!of_property_read_u32(pdev->dev.of_node, "mediatek,vpu",
> > > > > + &rproc_phandle)) {
> > > > > + fw_type = VPU;
> > > > > + } else {
> > > > > + mtk_v4l2_err("Could not get venc IPI device");
> > > > > + return -ENODEV;
> > > > > }
> > > > > -
> > > > > - vpu_wdt_reg_handler(dev->vpu_plat_dev, mtk_vcodec_enc_reset_handler,
> > > > > - dev, VPU_RST_ENC);
> > > > > + dev->fw_handler = mtk_vcodec_fw_select(dev, fw_type, VPU_RST_ENC);
> > > > > + if (IS_ERR(dev->fw_handler))
> > > > > + return PTR_ERR(dev->fw_handler);
> > > > >
> > > > > ret = mtk_vcodec_init_enc_pm(dev);
> > > > > if (ret < 0) {
> > > > > dev_err(&pdev->dev, "Failed to get mt vcodec clock source!");
> > > > > - return ret;
> > > > > + goto err_enc_pm;
> > > > > }
> > > > >
> > > > > for (i = VENC_SYS, j = 0; i < NUM_MAX_VCODEC_REG_BASE; i++, j++) {
> > > > > @@ -377,6 +365,8 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
> > > > > v4l2_device_unregister(&dev->v4l2_dev);
> > > > > err_res:
> > > > > mtk_vcodec_release_enc_pm(dev);
> > > > > +err_enc_pm:
> > > > > + mtk_vcodec_fw_release(dev->fw_handler);
> > > > > return ret;
> > > > > }
> > > > >
> > > > > @@ -401,6 +391,7 @@ static int mtk_vcodec_enc_remove(struct platform_device *pdev)
> > > > >
> > > > > v4l2_device_unregister(&dev->v4l2_dev);
> > > > > mtk_vcodec_release_enc_pm(dev);
> > > > > + mtk_vcodec_fw_release(dev->fw_handler);
> > > > > return 0;
> > > > > }
> > > > >
> > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
> > > > > index 3e2bfded79a6..ee22902aaa71 100644
> > > > > --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
> > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
> > > > > @@ -12,8 +12,6 @@
> > > > >
> > > > > #include "mtk_vcodec_enc_pm.h"
> > > > > #include "mtk_vcodec_util.h"
> > > > > -#include "mtk_vpu.h"
> > > > > -
> > > > >
> > > > > int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
> > > > > {
> > > > > diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> > > > > new file mode 100644
> > > > > index 000000000000..967bb100a990
> > > > > --- /dev/null
> > > > > +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_fw.c
> > > > > @@ -0,0 +1,172 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +
> > > > > +#include "mtk_vcodec_fw.h"
> > > > > +#include "mtk_vcodec_util.h"
> > > > > +#include "mtk_vcodec_drv.h"
> > > > > +
> > > > > +struct mtk_vcodec_fw_ops {
> > > > > + int (*load_firmware)(struct mtk_vcodec_fw *fw);
> > > > > + unsigned int (*get_vdec_capa)(struct mtk_vcodec_fw *fw);
> > > > > + unsigned int (*get_venc_capa)(struct mtk_vcodec_fw *fw);
> > > > > + void * (*map_dm_addr)(struct mtk_vcodec_fw *fw, u32 dtcm_dmem_addr);
> > > > > + int (*ipi_register)(struct mtk_vcodec_fw *fw, int id,
> > > > > + mtk_vcodec_ipi_handler handler, const char *name, void *priv);
> > > > > + int (*ipi_send)(struct mtk_vcodec_fw *fw, int id, void *buf,
> > > > > + unsigned int len, unsigned int wait);
> > > > > +};
> > > > > +
> > > > > +struct mtk_vcodec_fw {
> > > > > + enum mtk_vcodec_fw_type type;
> > > > > + const struct mtk_vcodec_fw_ops *ops;
> > > > > + struct platform_device *pdev;
> > > > > +};
> > > > > +
> > > > > +static int mtk_vcodec_vpu_load_firmware(struct mtk_vcodec_fw *fw)
> > > > > +{
> > > > > + return vpu_load_firmware(fw->pdev);
> > > > > +}
> > > > > +
> > > > > +static unsigned int mtk_vcodec_vpu_get_vdec_capa(struct mtk_vcodec_fw *fw)
> > > > > +{
> > > > > + return vpu_get_vdec_hw_capa(fw->pdev);
> > > > > +}
> > > > > +
> > > > > +static unsigned int mtk_vcodec_vpu_get_venc_capa(struct mtk_vcodec_fw *fw)
> > > > > +{
> > > > > + return vpu_get_venc_hw_capa(fw->pdev);
> > > > > +}
> > > > > +
> > > > > +static void *mtk_vcodec_vpu_map_dm_addr(struct mtk_vcodec_fw *fw,
> > > > > + u32 dtcm_dmem_addr)
> > > > > +{
> > > > > + return vpu_mapping_dm_addr(fw->pdev, dtcm_dmem_addr);
> > > > > +}
> > > > > +
> > > > > +static int mtk_vcodec_vpu_set_ipi_register(struct mtk_vcodec_fw *fw, int id,
> > > > > + mtk_vcodec_ipi_handler handler, const char *name, void *priv)
> > > > > +{
> > > > > + /*
> > > > > + * The handler we receive takes a void * as its first argument. We
> > > > > + * cannot change this because it needs to be passed down to the rproc
> > > > > + * subsystem when SCP is used. VPU takes a const argument, which is
> > > > > + * more constrained, so the conversion below is safe.
> > > > > + */
> > > > > + ipi_handler_t handler_const = (ipi_handler_t)handler;
> > > > > +
> > > > > + return vpu_ipi_register(fw->pdev, id, handler_const, name, priv);
> > > > > +}
> > > > > +
> > > > > +static int mtk_vcodec_vpu_ipi_send(struct mtk_vcodec_fw *fw, int id, void *buf,
> > > > > + unsigned int len, unsigned int wait)
> > > > > +{
> > > > > + return vpu_ipi_send(fw->pdev, id, buf, len);
> > > > > +}
> > > > > +
> > > > > +static const struct mtk_vcodec_fw_ops mtk_vcodec_vpu_msg = {
> > > > > + .load_firmware = mtk_vcodec_vpu_load_firmware,
> > > > > + .get_vdec_capa = mtk_vcodec_vpu_get_vdec_capa,
> > > > > + .get_venc_capa = mtk_vcodec_vpu_get_venc_capa,
> > > > > + .map_dm_addr = mtk_vcodec_vpu_map_dm_addr,
> > > > > + .ipi_register = mtk_vcodec_vpu_set_ipi_register,
> > > > > + .ipi_send = mtk_vcodec_vpu_ipi_send,
> > > > > +};
> > > > > +
> > > > > +static void mtk_vcodec_reset_handler(void *priv)
> > > > > +{
> > > > > + struct mtk_vcodec_dev *dev = priv;
> > > > > + struct mtk_vcodec_ctx *ctx;
> > > > > +
> > > > > + mtk_v4l2_err("Watchdog timeout!!");
> > > > > +
> > > > > + mutex_lock(&dev->dev_mutex);
> > > > > + list_for_each_entry(ctx, &dev->ctx_list, list) {
> > > > > + ctx->state = MTK_STATE_ABORT;
> > > > > + mtk_v4l2_debug(0, "[%d] Change to state MTK_STATE_ABORT",
> > > > > + ctx->id);
> > > > > + }
> > > > > + mutex_unlock(&dev->dev_mutex);
> > > > > +}
> > > > > +
> > > > > +struct mtk_vcodec_fw *mtk_vcodec_fw_select(struct mtk_vcodec_dev *dev,
> > > > > + enum mtk_vcodec_fw_type type,
> > > > > + enum rst_id rst_id)
> > > > > +{
> > > > > + const struct mtk_vcodec_fw_ops *ops;
> > > > > + struct mtk_vcodec_fw *fw;
> > > > > + struct platform_device *fw_pdev = NULL;
> > > > > +
> > > > > + switch (type) {
> > > > > + case VPU:
> > > > > + ops = &mtk_vcodec_vpu_msg;
> > > > > + fw_pdev = vpu_get_plat_device(dev->plat_dev);
> > > > > + if (!fw_pdev) {
> > > > > + mtk_v4l2_err("firmware device is not ready");
> > > > > + return ERR_PTR(-EINVAL);
> > > > > + }
> > > > > + vpu_wdt_reg_handler(fw_pdev, mtk_vcodec_reset_handler,
> > > > > + dev, rst_id);
> > > > > + break;
> > > > > + default:
> > > > > + mtk_v4l2_err("invalid vcodec fw type");
> > > > > + return ERR_PTR(-EINVAL);
> > > > > + }
> > > > > +
> > > > > + fw = devm_kzalloc(&dev->plat_dev->dev, sizeof(*fw), GFP_KERNEL);
> > > > > + if (!fw)
> > > > > + return ERR_PTR(-EINVAL);
> > > > > +
> > > > > + fw->type = type;
> > > > > + fw->ops = ops;
> > > > > + fw->pdev = fw_pdev;
> > > > > +
> > > > > + return fw;
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mtk_vcodec_fw_select);
> > > > > +
> > > > > +void mtk_vcodec_fw_release(struct mtk_vcodec_fw *fw)
> > > > > +{
> > > > > + switch (fw->type) {
> > > > > + case VPU:
> > > > > + put_device(&fw->pdev->dev);
> > > > > + break;
> > > > > + }
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(mtk_vcodec_fw_release);
> > > > > +
> > > >
> > > > What are these symbols exported for?
> > >
> > > This driver is made of three modules: mtk_vcodec_common,
> > > mtk_vcodec_dec and mtk_vcodec_enc. These functions are used by both
> > > the encoder and decoder module, so they need to be exported.
> > >
> > > There is mistake in the patch though that I noticed while
> > > investigating your comment: the file defining these functions
> > > (mtk_vcodec_fw.c) is linked against the mtk_vcodec_dec module while it
> > > should be part of mtk_vcodec_common, so thanks for asking. :)
> >
> > Oh, I missed that. Out of curiosity, what's the design decision motivating
> > this split?
> >
> > FWIW, I've had a somewhat painful experience with splitted
> > modules, so I try to minimize that if possible.
> >
> > If you decide to keep the split, how about you consider
> > also splitting the user option. Something around two visible
> > ones: VIDEO_MEDIATEK_VCODEC_ENC/DEC
> > and one hidden for the _COMMON one.
> >
> > That would make the design more obvious, I think.
> >
> > Minor nitpick, currently the Kconfig help states:
> >
> > """
> > To compile this driver as a module, choose M here: the
> > module will be called mtk-vcodec
> > """
> >
>
> Ah, one more thing.
>
> In order to build this with COMPILE_TEST, I believe you need
> something along these lines:
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index c4d1731295eb..ee83f407b9a3 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -34,7 +34,7 @@ config INGENIC_VPU_RPROC
>
> config MTK_SCP
> tristate "Mediatek SCP support"
> - depends on ARCH_MEDIATEK
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> select RPMSG_MTK_SCP
> help
> Say y here to support Mediatek's System Companion Processor (SCP) via
Sure, but the file you pointed at is for the SCP which is untouched by
this patch. The VCODEC entry has COMPILE_TEST enabled.
You are right nonetheless, so I'll submit another patch for that.
>
> Thanks,
> Ezequiel