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 tzungbi@google.com Signed-off-by: Yunfei Dong yunfei.dong@mediatek.com

---

.../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, &dev->pm); if (ret < 0) { dev_err(&pdev->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(&dev->v4l2_dev); err_res:

• mtk_vcodec_release_dec_pm(dev);

• mtk_vcodec_release_dec_pm(&dev->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(&dev->v4l2_dev);

• mtk_vcodec_release_dec_pm(dev);

• mtk_vcodec_release_dec_pm(&dev->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 = &mtkdev->pm;
• pm->mtkdev = mtkdev; dec_clk = &pm->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 = &pdev->dev;
• pdev = mtkdev->plat_dev;

• pm->larbvdec = &larb_pdev->dev; pm->dev = &pdev->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); -void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev); +int mtk_vcodec_init_dec_pm(struct platform_device *pdev,

• struct mtk_vcodec_pm *pm);

+void mtk_vcodec_release_dec_pm(struct mtk_vcodec_pm *pm);

int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm); void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm); diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h index 3b1e5e3a450e..973b0b3649c6 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h @@ -195,7 +195,6 @@ struct mtk_vcodec_pm { struct mtk_vcodec_clk venc_clk; struct device *larbvenc; struct device *dev;

• struct mtk_vcodec_dev *mtkdev; };

  /**

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 1b2e4930ed27..0c8c8f86788c 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c @@ -26,7 +26,6 @@ int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev) pdev = mtkdev->plat_dev; pm = &mtkdev->pm; memset(pm, 0, sizeof(struct mtk_vcodec_pm));

• pm->mtkdev = mtkdev; pm->dev = &pdev->dev; dev = &pdev->dev; enc_clk = &pm->venc_clk;

Manage each hardware information which includes irq/power/clk. The hardware includes LAT0, LAT1 and CORE.

Signed-off-by: Yunfei Dong yunfei.dong@mediatek.com

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.

/*

• 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?!

+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 = &mtk_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(&pdev->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(&pdev->dev, "Vdec comp_idx is not ready %d.",
  

• 		comp_idx);
  

• 	return -EINVAL;
  

• }
  

• }
• return 0;

+}

• static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv) { struct mtk_vcodec_dev *dev = priv;

@@ -95,6 +137,41 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev) return 0; }

+static int mtk_vcodec_init_dec_params(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_comp_supported) {

component is never supported (is_comp_supported is always false), so this check is useless.

• dev->dec_irq = platform_get_irq(pdev, 0);
  

• if (dev->dec_irq < 0) {
  

• 	dev_err(&pdev->dev, "failed to get irq number");
  

• 	return dev->dec_irq;
  

• }
  

• irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
  

• ret = devm_request_irq(&pdev->dev, dev->dec_irq,
  

• 	mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
  

• if (ret) {
  

• 	dev_err(&pdev->dev, "failed to install dev->dec_irq %d (%d)",
  

• 		dev->dec_irq, ret);
  

• 	return ret;
  

• }
  

• ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);
  

• if (ret < 0) {
  

• 	dev_err(&pdev->dev, "failed to get mt vcodec clock source");
  

• 	return ret;
  

• }
  

• }
• return 0;

+}

• static int fops_vcodec_open(struct file *file) { struct mtk_vcodec_dev *dev = video_drvdata(file);

@@ -116,6 +193,12 @@ static int fops_vcodec_open(struct file *file) init_waitqueue_head(&ctx->queue); mutex_init(&ctx->lock);

• ret = mtk_vcodec_comp_device_check(ctx);

Are you checking available devices outside of a probe function? From what I can see, you require that the MMSYS device is available at this time, but that's something that should be granted if we reach this point.

I would say that this driver probe should be deferred until the MMSYS device becomes available, since it's required for this to work and since, once that device gets registered and probed successfully, it'll never go away (unless module unloaded, but you can't unload module A if module B is using it, or depends on it, unless you also unload module B): like this, you will avoid performing N useless checks, everytime the userspace opens the vdec device.

• if (ret) {

• mtk_v4l2_err("Failed to check vdec comp device.");
  

• goto err_ctrls_setup;
  

• }
• ctx->type = MTK_INST_DECODER; ret = dev->vdec_pdata->ctrls_setup(ctx); if (ret) {

@@ -220,7 +303,6 @@ 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 ret;

@@ -249,32 +331,10 @@ 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->plat_dev, &dev->pm);
• if (ret < 0) {

• dev_err(&pdev->dev, "Failed to get mt vcodec clock source");
  

• goto err_dec_pm;
  

• }
• ret = mtk_vcodec_get_reg_bases(dev);
• if (ret)

• goto err_res;
  

• res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
• if (res == NULL) {

• dev_err(&pdev->dev, "failed to get irq resource");
  

• ret = -ENOENT;
  

• goto err_res;
  

• }
• dev->dec_irq = platform_get_irq(pdev, 0);
• irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
• ret = devm_request_irq(&pdev->dev, dev->dec_irq,

• 	mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
  

• ret = mtk_vcodec_init_dec_params(dev); if (ret) {

• dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
  

• 	dev->dec_irq,
  

• 	ret);
  

• goto err_res;
  

• dev_err(&pdev->dev, "Failed to init pm and registers");
  

• goto err_dec_pm;
  

  }

  mutex_init(&dev->dec_mutex);

@@ -329,6 +389,13 @@ static int mtk_vcodec_probe(struct platform_device *pdev) goto err_event_workq; }

• ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,

• &pdev->dev);
  

• if (ret) {

• mtk_v4l2_err("Master device of_platform_populate failed.");
  

• goto err_event_workq;
  

• }
• if (dev->vdec_pdata->uses_stateless_api) { dev->mdev_dec.dev = &pdev->dev; strscpy(dev->mdev_dec.model, MTK_VCODEC_DEC_NAME,

@@ -436,7 +503,25 @@ static struct platform_driver mtk_vcodec_dec_driver = { }, };

-module_platform_driver(mtk_vcodec_dec_driver); +static struct platform_driver * const mtk_vdec_drivers[] = {

• &mtk_vdec_comp_driver,
• &mtk_vcodec_dec_driver,

+};

I would say that this is not a good idea, since you can simply register your mtk_vdec_comp_driver in mtk_vcodec_dec_hw.c.

+static int __init mtk_vdec_init(void) +{

• return platform_register_drivers(mtk_vdec_drivers,

• 			 ARRAY_SIZE(mtk_vdec_drivers));
  

+}

+static void __exit mtk_vdec_exit(void) +{

• platform_unregister_drivers(mtk_vdec_drivers,

• 		    ARRAY_SIZE(mtk_vdec_drivers));
  

+}

+module_init(mtk_vdec_init); +module_exit(mtk_vdec_exit);

MODULE_LICENSE("GPL v2"); MODULE_DESCRIPTION("Mediatek video codec V4L2 decoder driver"); diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c new file mode 100644 index 000000000000..3752ccaea284 --- /dev/null +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2021 MediaTek Inc.
• • Author: Yunfei Dong yunfei.dong@mediatek.com
• */

+#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/slab.h>

+#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"

+static const struct of_device_id mtk_vdec_comp_ids[] = {

• {

• .compatible = "mediatek,mtk-vcodec-lat",
  

• },
• {

• .compatible = "mediatek,mtk-vcodec-core",
  

• },
• {},

+};

I think that you forgot some ".data = " here...

+static irqreturn_t mtk_vdec_comp_irq_handler(int irq, void *priv) +{

• struct mtk_vdec_comp_dev *dev = priv;
• struct mtk_vcodec_ctx *ctx;
• u32 cg_status;
• unsigned int dec_done_status;
• void __iomem *vdec_misc_addr = dev->reg_base[VDEC_COMP_MISC] +

• 			VDEC_IRQ_CFG_REG;
  

• ctx = mtk_vcodec_get_curr_ctx(dev->master_dev);
• /* check if HW active or not */
• cg_status = readl(dev->reg_base[VDEC_COMP_SYS]);
• if ((cg_status & VDEC_HW_ACTIVE) != 0) {

The "!= 0" can be dropped here, since cg_status is unsigned.

• mtk_v4l2_err("vdec active is not 0x0 (0x%08x)",
  

• 	cg_status);
  

• return IRQ_HANDLED;
  

• }
• dec_done_status = readl(vdec_misc_addr);
• if ((dec_done_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS) !=

• MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)
  

I suggest to fix the indentation here to increase human readability, like so:

if ((dec_done_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS) !=

MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)

• return IRQ_HANDLED;
  

• /* clear interrupt */
• 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);
• mtk_v4l2_debug(3, "wake up ctx %d, dec_done_status=%x",

• ctx->id, dec_done_status);
  

• return IRQ_HANDLED;

+}

+static int mtk_vdec_comp_init_irq(struct mtk_vdec_comp_dev *dev) +{

• struct platform_device *pdev = dev->plat_dev;
• int ret;
• dev->dec_irq = platform_get_irq(pdev, 0);
• if (dev->dec_irq < 0) {

• dev_err(&pdev->dev, "Failed to get irq resource");
  

• return dev->dec_irq;
  

• }
• irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
• ret = devm_request_irq(&pdev->dev, dev->dec_irq,

• 		mtk_vdec_comp_irq_handler, 0, pdev->name, dev);
  

• if (ret) {

• dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
  

• 	dev->dec_irq, ret);
  

• return ret;
  

• }
• return 0;

+}

+static int mtk_vdec_comp_probe(struct platform_device *pdev) +{

• struct device *dev = &pdev->dev;
• struct mtk_vdec_comp_dev *comp_dev;
• struct mtk_vcodec_dev *master_dev;
• const struct of_device_id *of_id;
• int comp_idx;
• int ret;
• if (!dev->parent) {

• dev_err(dev, "Vdec parent prob is not ready.");
  

This is not an error, since it's not a "critical" error, until it becomes one (so, after max deferral count has been reached). I would remove this message entirely, however, if you really want this message to be here, you should use dev_dbg.

• return -EPROBE_DEFER;
  

• }
• master_dev = dev_get_drvdata(dev->parent);
• if (!master_dev) {

• dev_err(dev, "Failed to get master device.");
  

Same here.

• return -EPROBE_DEFER;
  

• }
• comp_dev = devm_kzalloc(dev, sizeof(*comp_dev), GFP_KERNEL);
• if (!comp_dev)

• return -ENOMEM;
  

• comp_dev->plat_dev = pdev;
• ret = mtk_vcodec_init_dec_pm(pdev, &comp_dev->pm);
• if (ret) {

• dev_err(dev, "Failed to get mt vcodec clock source.");
  

As far as I can see, you have failure prints all over in function mtk_vcodec_init_dec_pm, so there's no need to add another print here: you can simply return the error value here.

• return ret;
  

• }
• comp_dev->reg_base[VDEC_COMP_MISC] =

• devm_platform_ioremap_resource_byname(pdev, "reg-misc");
  

• if (IS_ERR((__force void *)comp_dev->reg_base[VDEC_COMP_MISC])) {

• ret = PTR_ERR((__force void *)comp_dev->reg_base[VDEC_COMP_MISC]);
  

• goto err;
  

• }
• ret = mtk_vdec_comp_init_irq(comp_dev);
• if (ret) {

• dev_err(dev, "Failed to register irq handler.\n");
  

• goto err;
  

• }

...and the same applies here: mtk_vdec_comp_init_irq already prints on its own.

• of_id = of_match_device(mtk_vdec_comp_ids, dev);
• if (!of_id) {

• dev_err(dev, "Can't get vdec comp device id.\n");
  

• ret = -EINVAL;
  

• goto err;
  

• }
• comp_idx = (enum mtk_vdec_hw_id)of_id->data;

Are you sure that this one is working as expected? The only driver that registers compatibles mediatek,mtk-vcodec-lat and mediatek,mtk-vcodec-core is exactly this one, but there's no .data assigned to any of the of_device_id entries... and this means that your of_id->data will not contain the expected value of type enum mtk_vdec_hw_id.

• if (comp_idx < MTK_VDEC_HW_MAX) {

• master_dev->comp_dev[comp_idx] = comp_dev;
  

• comp_dev->comp_idx = comp_idx;
  

• comp_dev->master_dev = master_dev;
  

• comp_dev->reg_base[VDEC_COMP_SYS] =
  

• 	master_dev->reg_base[VDEC_COMP_SYS];
  

• set_bit(comp_dev->comp_idx, master_dev->hardware_bitmap);
  

• }
• platform_set_drvdata(pdev, comp_dev);
• return 0;

+err:

• mtk_vcodec_release_dec_pm(&comp_dev->pm);
• return ret;

+}

+MODULE_DEVICE_TABLE(of, mtk_vdec_comp_ids); +struct platform_driver mtk_vdec_comp_driver = {

The `_comp` here is a name that's making a logical reference to that: in this case,

that may confuse developers around. Can you please drop that `_comp`?

That would become `mtk_vdec_driver` instead.

Logically, the same applies for the rest - always assuming that it's referencing to the Linux component API.

• .probe = mtk_vdec_comp_probe,
• .driver = {

• .name	= "mtk-vdec-comp",
  

• .of_match_table = mtk_vdec_comp_ids,
  

• },

+};

Since you're using of_platform_populate, there is no need to register this driver externally: you can simply register it here (module_platform_driver) and it will be probed at of_platform_populate time in your parent.

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h new file mode 100644 index 000000000000..8d6e818a3474 --- /dev/null +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2021 MediaTek Inc.
• • Author: Yunfei Dong yunfei.dong@mediatek.com
• */

+#ifndef _MTK_VCODEC_DEC_HW_H_ +#define _MTK_VCODEC_DEC_HW_H_

+#include <linux/io.h> +#include <linux/platform_device.h>

+#include "mtk_vcodec_drv.h"

+#define VDEC_HW_ACTIVE 0x10 +#define VDEC_IRQ_CFG 0x11 +#define VDEC_IRQ_CLR 0x10 +#define VDEC_IRQ_CFG_REG 0xa4

Cosmetic nitpick: either all values in the same column, or simply use a space instead of tabulations.

+/**

• • enum mtk_comp_hw_reg_idx - component register base index
• */

+enum mtk_comp_hw_reg_idx {

• VDEC_COMP_SYS,
• VDEC_COMP_MISC,
• VDEC_COMP_MAX

+};

+/**

• • struct mtk_vdec_comp_dev - component framwork driver data

You've switched away from the component framework... so this structure's description seems to be a bit misleading now...

• • @plat_dev: platform device
• • @master_dev: master device
• • @reg_base: Mapped address of MTK Vcodec registers.
• • @dec_irq: decoder irq resource
• • @pm: power management control
• • @comp_idx: component index
• */

+struct mtk_vdec_comp_dev {

• struct platform_device *plat_dev;
• struct mtk_vcodec_dev *master_dev;
• void __iomem *reg_base[VDEC_COMP_MAX];
• int dec_irq;
• struct mtk_vcodec_pm pm;
• int comp_idx;

+};

+#endif /* _MTK_VCODEC_DEC_HW_H_ */ diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c index 59c24b22ab6d..4f9a80ce15d8 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c @@ -623,4 +623,5 @@ const struct mtk_vcodec_dec_pdata mtk_vdec_8173_pdata = { .num_framesizes = NUM_SUPPORTED_FRAMESIZE, .worker = mtk_vdec_worker, .flush_decoder = mtk_vdec_flush_decoder,

• .is_comp_supported = false, };

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c index 8f4a1f0a0769..762633572b49 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c @@ -357,4 +357,5 @@ const struct mtk_vcodec_dec_pdata mtk_vdec_8183_pdata = { .uses_stateless_api = true, .worker = mtk_vdec_worker, .flush_decoder = mtk_vdec_flush_decoder,

• .is_comp_supported = false,

component is not supported anywhere, since you've switched away from it?!

}; diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h index 973b0b3649c6..ec3850b4c638 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h @@ -93,6 +93,17 @@ enum mtk_fmt_type { MTK_FMT_FRAME = 2, };

+/**

• • struct mtk_vdec_hw_id - Hardware index used to separate

• •                     different hardware
    

• */

+enum mtk_vdec_hw_id {

• MTK_VDEC_CORE,
• MTK_VDEC_LAT0,
• MTK_VDEC_LAT1,
• MTK_VDEC_HW_MAX,

+};

• /*
  • struct mtk_video_fmt - Structure used to store information about pixelformats
  */

@@ -331,6 +342,7 @@ enum mtk_chip {

• @chip: chip this decoder is compatible with

• • @is_comp_supported: true: using compoent framework, false: not support
  • @uses_stateless_api: whether the decoder uses the stateless API with requests
  */

@@ -352,6 +364,7 @@ struct mtk_vcodec_dec_pdata {

enum mtk_chip chip;

• bool is_comp_supported;

Adding this member looks useless: there's nothing declaring is_comp_supported as true... not here, not in any other patch of this entire series!

Regards, - Angelo

Manage each hardware information which includes irq/power/clk.

The hardware includes LAT0, LAT1 and CORE.

Signed-off-by: Yunfei Dong yunfei.dong@mediatek.com

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.

/*

• 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?!

+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 = &mtk_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(&pdev->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(&pdev->dev, "Vdec comp_idx is not ready %d.",
  

•            comp_idx);
  

•        return -EINVAL;
  

•    }
  

• }

• return 0;

+}

static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv)

{

  struct mtk_vcodec_dev *dev = priv;

@@ -95,6 +137,41 @@ static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)

  return 0;

}

+static int mtk_vcodec_init_dec_params(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_comp_supported) {

component is never supported (is_comp_supported is always false), so this check

is useless.

•    dev->dec_irq = platform_get_irq(pdev, 0);
  

•    if (dev->dec_irq < 0) {
  

•        dev_err(&pdev->dev, "failed to get irq number");
  

•        return dev->dec_irq;
  

•    }
  

•    irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
  

•    ret = devm_request_irq(&pdev->dev, dev->dec_irq,
  

•        mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
  

•    if (ret) {
  

•        dev_err(&pdev->dev, "failed to install dev->dec_irq %d (%d)",
  

•            dev->dec_irq, ret);
  

•        return ret;
  

•    }
  

•    ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);
  

•    if (ret < 0) {
  

•        dev_err(&pdev->dev, "failed to get mt vcodec clock source");
  

•        return ret;
  

•    }
  

• }

• return 0;

+}

static int fops_vcodec_open(struct file *file)

{

  struct mtk_vcodec_dev *dev = video_drvdata(file);

@@ -116,6 +193,12 @@ static int fops_vcodec_open(struct file *file)

  init_waitqueue_head(&ctx->queue);

  mutex_init(&ctx->lock);

• ret = mtk_vcodec_comp_device_check(ctx);

Are you checking available devices outside of a probe function?

From what I can see, you require that the MMSYS device is available at this

time, but that's something that should be granted if we reach this point.

I would say that this driver probe should be deferred until the MMSYS device

becomes available, since it's required for this to work and since, once that

device gets registered and probed successfully, it'll never go away (unless

module unloaded, but you can't unload module A if module B is using it, or

depends on it, unless you also unload module B): like this, you will avoid

performing N useless checks, everytime the userspace opens the vdec device.

• if (ret) {

•    mtk_v4l2_err("Failed to check vdec comp device.");
  

•    goto err_ctrls_setup;
  

• }

  ctx->type = MTK_INST_DECODER;

  ret = dev->vdec_pdata->ctrls_setup(ctx);

  if (ret) {

@@ -220,7 +303,6 @@ 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 ret;

@@ -249,32 +331,10 @@ 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->plat_dev, &dev->pm);

• if (ret < 0) {

•    dev_err(&pdev->dev, "Failed to get mt vcodec clock source");
  

•    goto err_dec_pm;
  

• }

• ret = mtk_vcodec_get_reg_bases(dev);

• if (ret)

•    goto err_res;
  

• res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

• if (res == NULL) {

•    dev_err(&pdev->dev, "failed to get irq resource");
  

•    ret = -ENOENT;
  

•    goto err_res;
  

• }

• dev->dec_irq = platform_get_irq(pdev, 0);

• irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);

• ret = devm_request_irq(&pdev->dev, dev->dec_irq,

•        mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
  

• ret = mtk_vcodec_init_dec_params(dev);

  if (ret) {

•    dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
  

•        dev->dec_irq,
  

•        ret);
  

•    goto err_res;
  

•    dev_err(&pdev->dev, "Failed to init pm and registers");
  

•    goto err_dec_pm;
  

  }

    mutex_init(&dev->dec_mutex);

@@ -329,6 +389,13 @@ static int mtk_vcodec_probe(struct platform_device *pdev)

      goto err_event_workq;

  }

• ret = of_platform_populate(pdev->dev.of_node, NULL, NULL,

•    &pdev->dev);
  

• if (ret) {

•    mtk_v4l2_err("Master device of_platform_populate failed.");
  

•    goto err_event_workq;
  

• }

  if (dev->vdec_pdata->uses_stateless_api) {

      dev->mdev_dec.dev = &pdev->dev;

      strscpy(dev->mdev_dec.model, MTK_VCODEC_DEC_NAME,

@@ -436,7 +503,25 @@ static struct platform_driver mtk_vcodec_dec_driver = {

  },

};

-module_platform_driver(mtk_vcodec_dec_driver);

+static struct platform_driver * const mtk_vdec_drivers[] = {

• &mtk_vdec_comp_driver,

• &mtk_vcodec_dec_driver,

+};

I would say that this is not a good idea, since you can simply register

your mtk_vdec_comp_driver in mtk_vcodec_dec_hw.c.

+static int __init mtk_vdec_init(void)

+{

• return platform_register_drivers(mtk_vdec_drivers,

•                 ARRAY_SIZE(mtk_vdec_drivers));
  

+}

+static void __exit mtk_vdec_exit(void)

+{

• platform_unregister_drivers(mtk_vdec_drivers,

•                ARRAY_SIZE(mtk_vdec_drivers));
  

+}

+module_init(mtk_vdec_init);

+module_exit(mtk_vdec_exit);

MODULE_LICENSE("GPL v2");

MODULE_DESCRIPTION("Mediatek video codec V4L2 decoder driver");

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c

b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c

new file mode 100644

index 000000000000..3752ccaea284

--- /dev/null

+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.c

@@ -0,0 +1,165 @@

+// SPDX-License-Identifier: GPL-2.0

+/*

• • Copyright (c) 2021 MediaTek Inc.

• • Author: Yunfei Dong yunfei.dong@mediatek.com

• */

+#include <linux/interrupt.h>

+#include <linux/irq.h>

+#include <linux/module.h>

+#include <linux/of.h>

+#include <linux/of_device.h>

+#include <linux/slab.h>

+#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"

+static const struct of_device_id mtk_vdec_comp_ids[] = {

• {

•    .compatible = "mediatek,mtk-vcodec-lat",
  

• },

• {

•    .compatible = "mediatek,mtk-vcodec-core",
  

• },

• {},

+};

I think that you forgot some ".data = " here...

+static irqreturn_t mtk_vdec_comp_irq_handler(int irq, void *priv)

+{

• struct mtk_vdec_comp_dev *dev = priv;

• struct mtk_vcodec_ctx *ctx;

• u32 cg_status;

• unsigned int dec_done_status;

• void __iomem *vdec_misc_addr = dev->reg_base[VDEC_COMP_MISC] +

•                VDEC_IRQ_CFG_REG;
  

• ctx = mtk_vcodec_get_curr_ctx(dev->master_dev);

• /* check if HW active or not */

• cg_status = readl(dev->reg_base[VDEC_COMP_SYS]);

• if ((cg_status & VDEC_HW_ACTIVE) != 0) {

The "!= 0" can be dropped here, since cg_status is unsigned.

•    mtk_v4l2_err("vdec active is not 0x0 (0x%08x)",
  

•        cg_status);
  

•    return IRQ_HANDLED;
  

• }

• dec_done_status = readl(vdec_misc_addr);

• if ((dec_done_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS) !=

•    MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)
  

I suggest to fix the indentation here to increase human readability, like so:

if ((dec_done_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS) !=

MTK_VDEC_IRQ_STATUS_DEC_SUCCESS)

•    return IRQ_HANDLED;
  

• /* clear interrupt */

• 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);

• mtk_v4l2_debug(3, "wake up ctx %d, dec_done_status=%x",

•    ctx->id, dec_done_status);
  

• return IRQ_HANDLED;

+}

+static int mtk_vdec_comp_init_irq(struct mtk_vdec_comp_dev *dev)

+{

• struct platform_device *pdev = dev->plat_dev;

• int ret;

• dev->dec_irq = platform_get_irq(pdev, 0);

• if (dev->dec_irq < 0) {

•    dev_err(&pdev->dev, "Failed to get irq resource");
  

•    return dev->dec_irq;
  

• }

• irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);

• ret = devm_request_irq(&pdev->dev, dev->dec_irq,

•            mtk_vdec_comp_irq_handler, 0, pdev->name, dev);
  

• if (ret) {

•    dev_err(&pdev->dev, "Failed to install dev->dec_irq %d (%d)",
  

•        dev->dec_irq, ret);
  

•    return ret;
  

• }

• return 0;

+}

+static int mtk_vdec_comp_probe(struct platform_device *pdev)

+{

• struct device *dev = &pdev->dev;

• struct mtk_vdec_comp_dev *comp_dev;

• struct mtk_vcodec_dev *master_dev;

• const struct of_device_id *of_id;

• int comp_idx;

• int ret;

• if (!dev->parent) {

•    dev_err(dev, "Vdec parent prob is not ready.");
  

This is not an error, since it's not a "critical" error, until it becomes one (so,

after max deferral count has been reached).

I would remove this message entirely, however, if you really want this message to

be here, you should use dev_dbg.

•    return -EPROBE_DEFER;
  

• }

• master_dev = dev_get_drvdata(dev->parent);

• if (!master_dev) {

•    dev_err(dev, "Failed to get master device.");
  

Same here.

•    return -EPROBE_DEFER;
  

• }

• comp_dev = devm_kzalloc(dev, sizeof(*comp_dev), GFP_KERNEL);

• if (!comp_dev)

•    return -ENOMEM;
  

• comp_dev->plat_dev = pdev;

• ret = mtk_vcodec_init_dec_pm(pdev, &comp_dev->pm);

• if (ret) {

•    dev_err(dev, "Failed to get mt vcodec clock source.");
  

As far as I can see, you have failure prints all over in function mtk_vcodec_init_dec_pm, so there's no need to add another print here:

you can simply return the error value here.

•    return ret;
  

• }

• comp_dev->reg_base[VDEC_COMP_MISC] =

•    devm_platform_ioremap_resource_byname(pdev, "reg-misc");
  

• if (IS_ERR((__force void *)comp_dev->reg_base[VDEC_COMP_MISC])) {

•    ret = PTR_ERR((__force void *)comp_dev->reg_base[VDEC_COMP_MISC]);
  

•    goto err;
  

• }

• ret = mtk_vdec_comp_init_irq(comp_dev);

• if (ret) {

•    dev_err(dev, "Failed to register irq handler.\n");
  

•    goto err;
  

• }

...and the same applies here: mtk_vdec_comp_init_irq already prints on its own.

• of_id = of_match_device(mtk_vdec_comp_ids, dev);

• if (!of_id) {

•    dev_err(dev, "Can't get vdec comp device id.\n");
  

•    ret = -EINVAL;
  

•    goto err;
  

• }

• comp_idx = (enum mtk_vdec_hw_id)of_id->data;

Are you sure that this one is working as expected?

The only driver that registers compatibles mediatek,mtk-vcodec-lat and

mediatek,mtk-vcodec-core is exactly this one, but there's no .data assigned

to any of the of_device_id entries... and this means that your of_id->data

will not contain the expected value of type enum mtk_vdec_hw_id.

• if (comp_idx < MTK_VDEC_HW_MAX) {

•    master_dev->comp_dev[comp_idx] = comp_dev;
  

•    comp_dev->comp_idx = comp_idx;
  

•    comp_dev->master_dev = master_dev;
  

•    comp_dev->reg_base[VDEC_COMP_SYS] =
  

•        master_dev->reg_base[VDEC_COMP_SYS];
  

•    set_bit(comp_dev->comp_idx, master_dev->hardware_bitmap);
  

• }

• platform_set_drvdata(pdev, comp_dev);

• return 0;

+err:

• mtk_vcodec_release_dec_pm(&comp_dev->pm);

• return ret;

+}

+MODULE_DEVICE_TABLE(of, mtk_vdec_comp_ids);

+struct platform_driver mtk_vdec_comp_driver = {

The `_comp` here is a name that's making a logical reference to that: in this case,

that may confuse developers around. Can you please drop that `_comp`?

That would become `mtk_vdec_driver` instead.

Logically, the same applies for the rest - always assuming that it's referencing to

the Linux component API.

• .probe = mtk_vdec_comp_probe,

• .driver = {

•    .name    = "mtk-vdec-comp",
  

•    .of_match_table = mtk_vdec_comp_ids,
  

• },

+};

Since you're using of_platform_populate, there is no need to register this driver

externally: you can simply register it here (module_platform_driver) and it will

be probed at of_platform_populate time in your parent.

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h

b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h

new file mode 100644

index 000000000000..8d6e818a3474

--- /dev/null

+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_hw.h

@@ -0,0 +1,49 @@

+// SPDX-License-Identifier: GPL-2.0

+/*

• • Copyright (c) 2021 MediaTek Inc.

• • Author: Yunfei Dong yunfei.dong@mediatek.com

• */

+#ifndef _MTK_VCODEC_DEC_HW_H_

+#define _MTK_VCODEC_DEC_HW_H_

+#include <linux/io.h>

+#include <linux/platform_device.h>

+#include "mtk_vcodec_drv.h"

+#define VDEC_HW_ACTIVE 0x10

+#define VDEC_IRQ_CFG 0x11

+#define VDEC_IRQ_CLR 0x10

+#define VDEC_IRQ_CFG_REG 0xa4

Cosmetic nitpick: either all values in the same column, or simply use a space

instead of tabulations.

+/**

• • enum mtk_comp_hw_reg_idx - component register base index

• */

+enum mtk_comp_hw_reg_idx {

• VDEC_COMP_SYS,

• VDEC_COMP_MISC,

• VDEC_COMP_MAX

+};

+/**

• • struct mtk_vdec_comp_dev - component framwork driver data

You've switched away from the component framework... so this structure's

description seems to be a bit misleading now...

• • @plat_dev: platform device

• • @master_dev: master device

• • @reg_base: Mapped address of MTK Vcodec registers.

• • @dec_irq: decoder irq resource

• • @pm: power management control

• • @comp_idx: component index

• */

+struct mtk_vdec_comp_dev {

• struct platform_device *plat_dev;

• struct mtk_vcodec_dev *master_dev;

• void __iomem *reg_base[VDEC_COMP_MAX];

• int dec_irq;

• struct mtk_vcodec_pm pm;

• int comp_idx;

+};

+#endif /* _MTK_VCODEC_DEC_HW_H_ */

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c

b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c

index 59c24b22ab6d..4f9a80ce15d8 100644

--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c

+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateful.c

@@ -623,4 +623,5 @@ const struct mtk_vcodec_dec_pdata mtk_vdec_8173_pdata = {

  .num_framesizes = NUM_SUPPORTED_FRAMESIZE,

  .worker = mtk_vdec_worker,

  .flush_decoder = mtk_vdec_flush_decoder,

• .is_comp_supported = false,

};

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c

b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c

index 8f4a1f0a0769..762633572b49 100644

--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c

+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_stateless.c

@@ -357,4 +357,5 @@ const struct mtk_vcodec_dec_pdata mtk_vdec_8183_pdata = {

  .uses_stateless_api = true,

  .worker = mtk_vdec_worker,

  .flush_decoder = mtk_vdec_flush_decoder,

• .is_comp_supported = false,

component is not supported anywhere, since you've switched away from it?!

};

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h

b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h

index 973b0b3649c6..ec3850b4c638 100644

--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h

+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h

@@ -93,6 +93,17 @@ enum mtk_fmt_type {

  MTK_FMT_FRAME = 2,

};

+/**

• • struct mtk_vdec_hw_id - Hardware index used to separate

• •                     different hardware
    

• */

+enum mtk_vdec_hw_id {

• MTK_VDEC_CORE,

• MTK_VDEC_LAT0,

• MTK_VDEC_LAT1,

• MTK_VDEC_HW_MAX,

+};

/*

• struct mtk_video_fmt - Structure used to store information about pixelformats

*/

@@ -331,6 +342,7 @@ enum mtk_chip {

• @chip: chip this decoder is compatible with

• • @is_comp_supported: true: using compoent framework, false: not support

• @uses_stateless_api: whether the decoder uses the stateless API with requests

*/

@@ -352,6 +364,7 @@ struct mtk_vcodec_dec_pdata {

    enum mtk_chip chip;

• bool is_comp_supported;

Adding this member looks useless: there's nothing declaring is_comp_supported as

true... not here, not in any other patch of this entire series!

Regards,

- Angelo

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 yunfei.dong@mediatek.com

.../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(&ctx->fh); INIT_LIST_HEAD(&ctx->list); ctx->dev = dev;

• init_waitqueue_head(&ctx->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(&ctx->queue[i]);
  

• } else {

• 	init_waitqueue_head(&ctx->queue[0]);
  

• }

• mutex_init(&ctx->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(&ctx->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(&ctx->fh); v4l2_fh_exit(&ctx->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);

  mtk_v4l2_debug(3, "wake up ctx %d, dec_done_status=%x", ctx->id, dec_done_status);

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h index 140f1a761942..f8e8b5ba408b 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_drv.h @@ -104,6 +104,16 @@ enum mtk_vdec_hw_id { MTK_VDEC_HW_MAX, };

+/**

• • struct mtk_vdec_hw_count - Supported hardware count
• */

+enum mtk_vdec_hw_count {

• MTK_VDEC_NO_HW = 0,
• MTK_VDEC_ONE_CORE,
• MTK_VDEC_ONE_LAT_ONE_CORE,
• MTK_VDEC_MAX_HW_COUNT,

+};

• /*
  • struct mtk_video_fmt - Structure used to store information

about pixelformats */ @@ -293,9 +303,9 @@ struct mtk_vcodec_ctx { struct vdec_pic_info picinfo; int dpb_size;

• int int_cond;
• int int_type;
• wait_queue_head_t queue;

• int int_cond[MTK_VDEC_HW_MAX];

• int int_type[MTK_VDEC_HW_MAX];

• wait_queue_head_t queue[MTK_VDEC_HW_MAX]; unsigned int irq_status;

  struct v4l2_ctrl_handler ctrl_hdl;

@@ -505,11 +515,12 @@ static inline struct mtk_vcodec_ctx *ctrl_to_ctx(struct v4l2_ctrl *ctrl) }

/* Wake up context wait_queue */ -static inline void wake_up_ctx(struct mtk_vcodec_ctx *ctx, unsigned int reason) +static inline void wake_up_ctx(struct mtk_vcodec_ctx *ctx, unsigned int reason,

• unsigned int hw_id) {

• ctx->int_cond = 1;
• ctx->int_type = reason;
• wake_up_interruptible(&ctx->queue);

• ctx->int_cond[hw_id] = 1;

• ctx->int_type[hw_id] = reason;

• wake_up_interruptible(&ctx->queue[hw_id]); }

  #endif /* _MTK_VCODEC_DRV_H_ */

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 2828df77020c..c7540c0cdcea 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_drv.c @@ -114,7 +114,7 @@ static irqreturn_t mtk_vcodec_enc_irq_handler(int irq, void *priv)

clean_irq_status(ctx->irq_status, addr);

• wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED);

• wake_up_ctx(ctx, MTK_INST_IRQ_RECEIVED, 0); return IRQ_HANDLED; }

@@ -140,7 +140,7 @@ static int fops_vcodec_open(struct file *file) v4l2_fh_add(&ctx->fh); INIT_LIST_HEAD(&ctx->list); ctx->dev = dev;

• init_waitqueue_head(&ctx->queue);

• init_waitqueue_head(&ctx->queue[0]);

  ctx->type = MTK_INST_ENCODER; ret = mtk_vcodec_enc_ctrls_setup(ctx);

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c index 70580c2525ba..8147533da98b 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.c @@ -11,34 +11,31 @@ #include "mtk_vcodec_intr.h" #include "mtk_vcodec_util.h"

-int mtk_vcodec_wait_for_done_ctx(struct mtk_vcodec_ctx *ctx, int command,

• 		 unsigned int timeout_ms)
  

+int mtk_vcodec_wait_for_done_ctx(struct mtk_vcodec_ctx *ctx,

• int command, unsigned int timeout_ms, unsigned hw_id) {

• wait_queue_head_t *waitqueue; long timeout_jiff, ret; int status = 0;

• waitqueue = (wait_queue_head_t *)&ctx->queue; timeout_jiff = msecs_to_jiffies(timeout_ms);

• ret = wait_event_interruptible_timeout(*waitqueue,

• 		ctx->int_cond,
  

• ret = wait_event_interruptible_timeout(ctx->queue[hw_id],

• 		ctx->int_cond[hw_id],
  	timeout_jiff);
  
  

  if (!ret) { status = -1; /* timeout */

• mtk_v4l2_err("[%d] ctx->type=%d, cmd=%d,
  

wait_event_interruptible_timeout time=%ums out %d %d!",

• 	     ctx->id, ctx->type, command, timeout_ms,
  

• 	     ctx->int_cond, ctx->int_type);
  

• mtk_v4l2_err("[%d] cmd=%d, type=%d, dec timeout=%ums
  

(%d %d)",

• 		ctx->id, command, ctx->type,
  

timeout_ms,

• 		ctx->int_cond[hw_id], ctx-
  

...

int_type[hw_id]);

} else if (-ERESTARTSYS == ret) {

• mtk_v4l2_err("[%d] ctx->type=%d, cmd=%d,
  

wait_event_interruptible_timeout interrupted by a signal %d %d",

• 	     ctx->id, ctx->type, command, ctx-
  

...

int_cond,

• 	     ctx->int_type);
  

  status = -1;

• mtk_v4l2_err("[%d] cmd=%d, type=%d, dec inter fail (%d
  

%d)",

• 		ctx->id, command, ctx->type,
  

• 		ctx->int_cond[hw_id], ctx-
  

...

int_type[hw_id]);

}

• ctx->int_cond = 0;
• ctx->int_type = 0;

• ctx->int_cond[hw_id] = 0;

• ctx->int_type[hw_id] = 0;

  return status; }

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h index 638cd1f3526a..2bf4cabfaa43 100644 --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_intr.h @@ -12,7 +12,7 @@ struct mtk_vcodec_ctx;

/* timeout is ms */ -int mtk_vcodec_wait_for_done_ctx(struct mtk_vcodec_ctx *data, int command,

• 		unsigned int timeout_ms);
  

+int mtk_vcodec_wait_for_done_ctx(struct mtk_vcodec_ctx *ctx,

• 		int command, unsigned int timeout_ms,
  

unsigned int hw_id);

#endif /* _MTK_VCODEC_INTR_H_ */ diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c index 40d6e6c5ac7a..481655bb6016 100644 --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_h264_if.c @@ -413,7 +413,7 @@ static int vdec_h264_decode(void *h_vdec, struct mtk_vcodec_mem *bs, /* wait decoder done interrupt */ err = mtk_vcodec_wait_for_done_ctx(inst->ctx, MTK_INST_IRQ_RECEIVE D,

• 				   WAIT_INTR_TIMEOUT_MS
  

);

• 				   WAIT_INTR_TIMEOUT_MS
  

, 0); if (err) goto err_free_fb_out;

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 4dde9ee786b4..d23baa364246 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 @@ -724,7 +724,7 @@ static int vdec_h264_slice_decode(void *h_vdec, struct mtk_vcodec_mem *bs, /* wait decoder done interrupt */ err = mtk_vcodec_wait_for_done_ctx(inst->ctx, MTK_INST_IRQ_RECEIVED,

• 			   WAIT_INTR_TIMEOUT_MS);
  

• 			   WAIT_INTR_TIMEOUT_MS, 0);
  

  if (err) goto err_free_fb_out; vpu_dec_end(vpu);

diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c index e5393f841080..88c046731754 100644 --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp8_if.c @@ -488,7 +488,7 @@ static int vdec_vp8_decode(void *h_vdec, struct mtk_vcodec_mem *bs,

/* wait decoder done interrupt */ mtk_vcodec_wait_for_done_ctx(inst->ctx, MTK_INST_IRQ_RECEIVED,

• 		     WAIT_INTR_TIMEOUT_MS);
  

• 		     WAIT_INTR_TIMEOUT_MS, 0);
  
  

  if (inst->vsi->load_data) load_dec_table(inst);

diff --git a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c index 71cdc3ddafcb..70b8383f7c8e 100644 --- a/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c +++ b/drivers/media/platform/mtk-vcodec/vdec/vdec_vp9_if.c @@ -539,7 +539,7 @@ static bool vp9_wait_dec_end(struct vdec_vp9_inst *inst)

mtk_vcodec_wait_for_done_ctx(inst->ctx, MTK_INST_IRQ_RECEIVED,

• 	WAIT_INTR_TIMEOUT_MS);
  

• 	WAIT_INTR_TIMEOUT_MS, 0);
  
  

  if (ctx->irq_status & MTK_VDEC_IRQ_STATUS_DEC_SUCCESS) return true;

diff --git a/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c b/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c index b6a4f2074fa5..9ae1bd8dbc32 100644 --- a/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c +++ b/drivers/media/platform/mtk-vcodec/venc/venc_h264_if.c @@ -335,7 +335,7 @@ static unsigned int h264_enc_wait_venc_done(struct venc_h264_inst *inst) struct mtk_vcodec_ctx *ctx = (struct mtk_vcodec_ctx *)inst-

...

ctx;

if (!mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IRQ_RECEIVED,

• 			  WAIT_INTR_TIMEOUT_MS)) {
  

• 			  WAIT_INTR_TIMEOUT_MS, 0)) {
  

  irq_status = ctx->irq_status; mtk_vcodec_debug(inst, "irq_status %x <-", irq_status); }

diff --git a/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c b/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c index 8267a9c4fd25..f96564277577 100644 --- a/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c +++ b/drivers/media/platform/mtk-vcodec/venc/venc_vp8_if.c @@ -222,7 +222,7 @@ static unsigned int vp8_enc_wait_venc_done(struct venc_vp8_inst *inst) struct mtk_vcodec_ctx *ctx = (struct mtk_vcodec_ctx *)inst-

...

ctx;

if (!mtk_vcodec_wait_for_done_ctx(ctx, MTK_INST_IRQ_RECEIVED,

• 			  WAIT_INTR_TIMEOUT_MS)) {
  

• 			  WAIT_INTR_TIMEOUT_MS, 0)) {
  

  irq_status = ctx->irq_status; mtk_vcodec_debug(inst, "isr return %x", irq_status); }

Thanks, Yunfei Dong

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 yunfei.dong@mediatek.com

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 <media/v4l2-ioctl.h> #include <media/v4l2-mem2mem.h> #include <media/videobuf2-core.h>

• #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 000000000000..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 yunfei.dong@mediatek.com
• */

+#include <linux/freezer.h> +#include <linux/interrupt.h> +#include <linux/kthread.h>

+#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)

...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.

+/* 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;

+}

+void vdec_msg_queue_init_ctx(struct vdec_msg_queue_ctx *ctx,

• int hardware_index)

+{

• init_waitqueue_head(&ctx->ready_to_use);
• INIT_LIST_HEAD(&ctx->ready_queue);
• spin_lock_init(&ctx->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;
• int i, err;
• /* already init msg queue */
• if (msg_queue->wdma_addr.size)

• return 0;
  

• vdec_msg_queue_init_ctx(&msg_queue->lat_ctx, MTK_VDEC_LAT0);
• msg_queue->wdma_addr.size = vde_msg_queue_get_trans_size(

• ctx->picinfo.buf_w, ctx->picinfo.buf_h);
  

• err = mtk_vcodec_mem_alloc(ctx, &msg_queue->wdma_addr);
• if (err) {

• mtk_v4l2_err("failed to allocate wdma_addr buf");
  

• return -ENOMEM;
  

• }
• msg_queue->wdma_rptr_addr = msg_queue->wdma_addr.dma_addr;
• msg_queue->wdma_wptr_addr = msg_queue->wdma_addr.dma_addr;
• for (i = 0; i < NUM_BUFFER_COUNT; i++) {

• lat_buf = &msg_queue->lat_buf[i];
  

• lat_buf->wdma_err_addr.size = VDEC_ERR_MAP_SZ_AVC;
  

• err = mtk_vcodec_mem_alloc(ctx, &lat_buf->wdma_err_addr);
  

• if (err) {
  

• 	mtk_v4l2_err("failed to allocate wdma_err_addr buf[%d]", i);
  

• 	goto mem_alloc_err;
  

• }
  

• lat_buf->slice_bc_addr.size = VDEC_LAT_SLICE_HEADER_SZ;
  

• err = mtk_vcodec_mem_alloc(ctx, &lat_buf->slice_bc_addr);
  

• if (err) {
  

• 	mtk_v4l2_err("failed to allocate wdma_addr buf[%d]", i);
  

• 	goto mem_alloc_err;
  

• }
  

• lat_buf->private_data = kzalloc(private_size, GFP_KERNEL);
  

• if (!lat_buf->private_data) {
  

• 	mtk_v4l2_err("failed to allocate private_data[%d]", i);
  

• 	goto mem_alloc_err;
  

• }
  

• lat_buf->ctx = ctx;
  

• lat_buf->core_decode = core_decode;
  

• vdec_msg_queue_qbuf(&msg_queue->lat_ctx, lat_buf);
  

• }
• return 0;

+mem_alloc_err:

• vdec_msg_queue_deinit(msg_queue, ctx);
• return -ENOMEM;

+}

+static struct list_head *vdec_get_buf_list(int hardware_index,

• struct vdec_lat_buf *buf)

+{

• switch (hardware_index) {
• case MTK_VDEC_CORE:

• return &buf->core_list;
  

• case MTK_VDEC_LAT0:

• return &buf->lat_list;
  

• default:

• return NULL;
  

• }

+}

+void vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx *ctx,

• struct vdec_lat_buf *buf)

+{

• struct list_head *head;
• head = vdec_get_buf_list(ctx->hardware_index, buf);
• if (!head) {

• mtk_v4l2_err("fail to qbuf: %d",ctx->hardware_index);
  

• return;
  

• }
• spin_lock(&ctx->ready_lock);
• list_add_tail(head, &ctx->ready_queue);
• ctx->ready_num++;
• wake_up_all(&ctx->ready_to_use);
• mtk_v4l2_debug(3, "enqueue buf type: %d addr: 0x%p num: %d",

• ctx->hardware_index, buf, ctx->ready_num);
  

• spin_unlock(&ctx->ready_lock);

+}

+static bool vdec_msg_queue_wait_event(struct vdec_msg_queue_ctx *ctx) +{

• long timeout_jiff;
• int ret;
• if (ctx->hardware_index == MTK_VDEC_CORE) {

• ret = wait_event_freezable(ctx->ready_to_use,
  

• 	!list_empty(&ctx->ready_queue));
  

• if (ret)
  

• 	return false;
  

• } else {

• timeout_jiff = msecs_to_jiffies(1500);
  

• ret = wait_event_timeout(ctx->ready_to_use,
  

• 	!list_empty(&ctx->ready_queue), timeout_jiff);
  

Suggestion (feel free to not follow this one): since you're using that msecs_to_jiffies(1500) return value only for this wait_event_timeout call, you may as well remove the timeout_jiff variable and simply do

ret = wait_event_timeout(ctx->ready_to_use,

!list_empty(&ctx->ready_queue), msecs_to_jiffies(1500));

• if (!ret)
  

• 	return false;
  

• }
• return true;

+}

+struct vdec_lat_buf *vdec_msg_queue_dqbuf(struct vdec_msg_queue_ctx *ctx) +{

• struct vdec_lat_buf *buf;
• struct list_head *head;
• int ret;
• spin_lock(&ctx->ready_lock);
• if (list_empty(&ctx->ready_queue)) {

• mtk_v4l2_debug(3, "queue is NULL, type:%d num: %d",
  

• 	ctx->hardware_index, ctx->ready_num);
  

• spin_unlock(&ctx->ready_lock);
  

• ret = vdec_msg_queue_wait_event(ctx);
  

• if (!ret)
  

• 	return NULL;
  

• spin_lock(&ctx->ready_lock);
  

• }
• if (ctx->hardware_index == MTK_VDEC_CORE)

• buf = list_first_entry(&ctx->ready_queue,
  

• 	struct vdec_lat_buf, core_list);
  

• else

• buf = list_first_entry(&ctx->ready_queue,
  

• 	struct vdec_lat_buf, lat_list);
  

• head = vdec_get_buf_list(ctx->hardware_index, buf);
• if (!head) {

• mtk_v4l2_err("fail to dqbuf: %d",ctx->hardware_index);
  

• return NULL;
  

• }
• list_del(head);
• ctx->ready_num--;
• mtk_v4l2_debug(3, "dqueue buf type:%d addr: 0x%p num: %d",

• ctx->hardware_index, buf, ctx->ready_num);
  

• spin_unlock(&ctx->ready_lock);
• return buf;

+}

+void vdec_msg_queue_update_ube_rptr(struct vdec_msg_queue *msg_queue,

• uint64_t ube_rptr)

+{

• spin_lock(&msg_queue->lat_ctx.ready_lock);
• msg_queue->wdma_rptr_addr = ube_rptr;
• mtk_v4l2_debug(3, "update ube rprt (0x%llx)", ube_rptr);
• spin_unlock(&msg_queue->lat_ctx.ready_lock);

+}

+void vdec_msg_queue_update_ube_wptr(struct vdec_msg_queue *msg_queue,

• uint64_t ube_wptr)

+{

• spin_lock(&msg_queue->lat_ctx.ready_lock);
• msg_queue->wdma_wptr_addr = ube_wptr;
• mtk_v4l2_debug(3, "update ube wprt: (0x%llx 0x%llx) offset: 0x%llx",

• msg_queue->wdma_rptr_addr, msg_queue->wdma_wptr_addr, ube_wptr);
  

• spin_unlock(&msg_queue->lat_ctx.ready_lock);

+}

+bool vdec_msg_queue_wait_lat_buf_full(struct vdec_msg_queue *msg_queue) +{

• long timeout_jiff;
• int ret;
• timeout_jiff = msecs_to_jiffies(1000 * (NUM_BUFFER_COUNT + 2));
• ret = wait_event_timeout(msg_queue->lat_ctx.ready_to_use,

• msg_queue->lat_ctx.ready_num == NUM_BUFFER_COUNT,
  

• timeout_jiff);
  

• if (ret) {

• mtk_v4l2_debug(3, "success to get lat buf: %d",
  

• 	msg_queue->lat_ctx.ready_num);
  

• return true;
  

• }
• mtk_v4l2_err("failed with lat buf isn't full: %d",

• msg_queue->lat_ctx.ready_num);
  

• return false;

+}

+void vdec_msg_queue_deinit(

• struct vdec_msg_queue *msg_queue,
• struct mtk_vcodec_ctx *ctx)

+{

• struct vdec_lat_buf *lat_buf;
• struct mtk_vcodec_mem *mem;
• int i;
• mem = &msg_queue->wdma_addr;
• if (mem->va)

• mtk_vcodec_mem_free(ctx, mem);
  

• for (i = 0; i < NUM_BUFFER_COUNT; i++) {

• lat_buf = &msg_queue->lat_buf[i];
  

• mem = &lat_buf->wdma_err_addr;
  

• if (mem->va)
  

• 	mtk_vcodec_mem_free(ctx, mem);
  

• mem = &lat_buf->slice_bc_addr;
  

• if (mem->va)
  

• 	mtk_vcodec_mem_free(ctx, mem);
  

• if (lat_buf->private_data)
  

• 	kfree(lat_buf->private_data);
  

• }

+} diff --git a/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h new file mode 100644 index 000000000000..1905ce713592 --- /dev/null +++ b/drivers/media/platform/mtk-vcodec/vdec_msg_queue.h @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2021 MediaTek Inc.
• • Author: Yunfei Dong yunfei.dong@mediatek.com
• */

+#ifndef _VDEC_MSG_QUEUE_H_ +#define _VDEC_MSG_QUEUE_H_

+#include <linux/sched.h> +#include <linux/semaphore.h> +#include <linux/slab.h> +#include <media/videobuf2-v4l2.h>

+#include "mtk_vcodec_util.h"

+#define NUM_BUFFER_COUNT 3

+struct vdec_lat_buf; +struct mtk_vcodec_ctx; +struct mtk_vcodec_dev; +typedef int (*core_decode_cb_t)(struct vdec_lat_buf *lat_buf);

+/**

• • struct vdec_msg_queue_ctx - represents a queue for buffers ready to be

• •                           processed
    

I would prefer if you don't break this line, 84 columns is fine to have.

• • @ready_used: ready used queue used to signalize when get a job queue
• • @ready_queue: list of ready lat buffer queues
• • @ready_lock: spin lock to protect the lat buffer usage
• • @ready_num: number of buffers ready to be processed
• • @hardware_index: hardware id that this queue is used for
• */

+struct vdec_msg_queue_ctx {

• wait_queue_head_t ready_to_use;
• struct list_head ready_queue;
• spinlock_t ready_lock;
• int ready_num;
• int hardware_index;

+};

+/**

• • struct vdec_lat_buf - lat buffer message used to store lat

• •                   info for core decode
    

• • @wdma_err_addr: wdma error address used for lat hardware
• • @slice_bc_addr: slice bc address used for lat hardware
• • @ts_info: need to set timestamp from output to capture
• • @private_data: shared information used to lat and core hardware
• • @ctx: mtk vcodec context information
• • @core_decode: different codec use different decode callback function
• • @lat_list: add lat buffer to lat head list
• • @core_list: add lat buffer to core head list
• */

+struct vdec_lat_buf {

• struct mtk_vcodec_mem wdma_err_addr;
• struct mtk_vcodec_mem slice_bc_addr;
• struct vb2_v4l2_buffer ts_info;
• void *private_data;
• struct mtk_vcodec_ctx *ctx;
• core_decode_cb_t core_decode;
• struct list_head lat_list;
• struct list_head core_list;

+};

+/**

• • struct vdec_msg_queue - used to store lat buffer message
• • @lat_buf: lat buffer used to store lat buffer information
• • @wdma_addr: wdma address used for ube
• • @wdma_rptr_addr: ube read point
• • @wdma_wptr_addr: ube write point
• • @lat_ctx: used to store lat buffer list
• */

+struct vdec_msg_queue {

• struct vdec_lat_buf lat_buf[NUM_BUFFER_COUNT];
• struct mtk_vcodec_mem wdma_addr;
• uint64_t wdma_rptr_addr;
• uint64_t wdma_wptr_addr;
• struct vdec_msg_queue_ctx lat_ctx;

+};

+/**

• • vdec_msg_queue_init - init lat buffer information.
• • @msg_queue: used to store the lat buffer information
• • @ctx: v4l2 ctx
• • @core_decode: core decode callback for each codec
• • @private_size: the private data size used to share with core
• */

+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);

+/**

• • vdec_msg_queue_init_ctx - used to init msg queue context information.
• • @ctx: message queue context
• • @hardware_index: hardware index
• */

+void vdec_msg_queue_init_ctx(struct vdec_msg_queue_ctx *ctx,

• int hardware_index);

+/**

• • vdec_msg_queue_qbuf - enqueue lat buffer to queue list.
• • @ctx: message queue context
• • @buf: current lat buffer
• */

+void vdec_msg_queue_qbuf(struct vdec_msg_queue_ctx *ctx,

• struct vdec_lat_buf *buf);

+/**

• • vdec_msg_queue_dqbuf - dequeue lat buffer from queue list.
• • @ctx: message queue context
• */

+struct vdec_lat_buf *vdec_msg_queue_dqbuf(struct vdec_msg_queue_ctx *ctx);

+/**

• • vdec_msg_queue_update_ube_rptr - used to updata the ube read point.
• • @msg_queue: used to store the lat buffer information
• • @ube_rptr: current ube read point
• */

+void vdec_msg_queue_update_ube_rptr(struct vdec_msg_queue *msg_queue,

• uint64_t ube_rptr);

+/**

• • vdec_msg_queue_update_ube_wptr - used to updata the ube write point.
• • @msg_queue: used to store the lat buffer information
• • @ube_wptr: current ube write point
• */

+void vdec_msg_queue_update_ube_wptr(struct vdec_msg_queue *msg_queue,

• uint64_t ube_wptr);

+/**

• • vdec_msg_queue_wait_lat_buf_full - used to check whether all lat buffer

• •                                in lat list.
    

• • @msg_queue: used to store the lat buffer information
• */

+bool vdec_msg_queue_wait_lat_buf_full(struct vdec_msg_queue *msg_queue);

+/**

• • vdec_msg_queue_deinit - deinit lat buffer information.
• • @msg_queue: used to store the lat buffer information
• • @ctx: v4l2 ctx
• */

+void vdec_msg_queue_deinit(

• struct vdec_msg_queue *msg_queue,
• struct mtk_vcodec_ctx *ctx);

void vdec_msg_queue_deinit(struct vdec_msg_queue *msg_queue, struct mtk_vcodec_ctx *ctx);

+#endif

Regards, - Angelo