[PATCH v7 0/7] Refactor MTK MDP driver into core/components

[PATCH v7 0/7] Refactor MTK MDP driver into core/components

[Eizan Miyamoto]

This is an update to
https://patchwork.kernel.org/project/linux-mediatek/list/?series=283075
To address some comments and fixes.

This series has been verified to work on 5.13.


Changes in v7:
- Drop last two changes from this series related to removing the vpu
  property from the mdp_rdma0 device node

Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
  actually loading it from mmsys into two changs to make review easier.
- Update devicetree bindings to reflect no longer needing the
  mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.

Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6

Changes in v4:
- rebase and test on 5.13
- don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873

Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name

Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
  mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
  correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
  mediatek,vpu property inside the mdp_rdma0 device node.

Eizan Miyamoto (7):
  mtk-mdp: propagate errors from clock_on
  mtk-mdp: add driver to probe mdp components
  mtk-mdp: use pm_runtime in MDP component driver
  media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on
  mtk-mdp: make mdp driver to be loadable by platform_device_register*()
  soc: mediatek: mmsys: instantiate mdp virtual device from mmsys
  media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 268 +++++++++++++++--
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  34 +--
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 282 ++++++++++++------
 drivers/media/platform/mtk-mdp/mtk_mdp_core.h |   3 +
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |   4 +-
 drivers/soc/mediatek/mtk-mmsys.c              |  20 +-
 6 files changed, 470 insertions(+), 141 deletions(-)

-- 
2.33.0.rc2.250.ged5fa647cd-goog

[PATCH v7 1/7] mtk-mdp: propagate errors from clock_on

[Eizan Miyamoto]

Up until this change, many errors were logged but ignored when powering
on clocks inside mtk_mdp_core. This change tries to do a better job at
propagating errors back to the power management framework.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---

(no changes since v1)

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 25 ++++++++++++-----
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  2 +-
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 27 ++++++++++++++-----
 3 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index b3426a551bea..76e295c8d9bc 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -13,10 +13,9 @@
 
 #include "mtk_mdp_comp.h"
 
-
-void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
+int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 {
-	int i, err;
+	int i, err, status;
 
 	if (comp->larb_dev) {
 		err = mtk_smi_larb_get(comp->larb_dev);
@@ -30,11 +29,23 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 		if (IS_ERR(comp->clk[i]))
 			continue;
 		err = clk_prepare_enable(comp->clk[i]);
-		if (err)
-			dev_err(dev,
-			"failed to enable clock, err %d. type:%d i:%d\n",
-				err, comp->type, i);
+		if (err) {
+			status = err;
+			dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i);
+			goto err_clk_prepare_enable;
+		}
+	}
+
+	return 0;
+
+err_clk_prepare_enable:
+	for (--i; i >= 0; i--) {
+		if (IS_ERR(comp->clk[i]))
+			continue;
+		clk_disable_unprepare(comp->clk[i]);
 	}
+
+	return status;
 }
 
 void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
index 7897766c96bb..92ab5249bcad 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -41,7 +41,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
 		      struct mtk_mdp_comp *comp,
 		      enum mtk_mdp_comp_type comp_type);
 void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp);
-void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
+int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
 void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp);
 
 
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index 976aa1f4829b..412bbec0f735 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -52,13 +52,28 @@ static const struct of_device_id mtk_mdp_of_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids);
 
-static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
+static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
 {
-	struct device *dev = &mdp->pdev->dev;
 	struct mtk_mdp_comp *comp_node;
+	int status;
+	struct device *dev = &mdp->pdev->dev;
+	int err;
 
-	list_for_each_entry(comp_node, &mdp->comp_list, node)
-		mtk_mdp_comp_clock_on(dev, comp_node);
+	list_for_each_entry(comp_node, &mdp->comp_list, node) {
+		err = mtk_mdp_comp_clock_on(dev, comp_node);
+		if (err) {
+			status = err;
+			goto err_mtk_mdp_comp_clock_on;
+		}
+	}
+
+	return 0;
+
+err_mtk_mdp_comp_clock_on:
+	list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node)
+		mtk_mdp_comp_clock_off(dev, comp_node);
+
+	return status;
 }
 
 static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
@@ -274,9 +289,7 @@ static int __maybe_unused mtk_mdp_pm_resume(struct device *dev)
 {
 	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
 
-	mtk_mdp_clock_on(mdp);
-
-	return 0;
+	return mtk_mdp_clock_on(mdp);
 }
 
 static int __maybe_unused mtk_mdp_suspend(struct device *dev)
-- 
2.33.0.rc2.250.ged5fa647cd-goog

[PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[Eizan Miyamoto]

Broadly, this patch (1) adds a driver for various MTK MDP components to
go alongside the main MTK MDP driver, and (2) hooks them all together
using the component framework.

(1) Up until now, the MTK MDP driver controls 8 devices in the device
tree on its own. When running tests for the hardware video decoder, we
found that the iommus and LARBs were not being properly configured. To
configure them, a driver for each be added to mtk_mdp_comp so that
mtk_iommu_add_device() can (eventually) be called from dma_configure()
inside really_probe().

(2) The integration into the component framework allows us to defer the
registration with the v4l2 subsystem until all the MDP-related devices
have been probed, so that the relevant device node does not become
available until initialization of all the components is complete.

Some notes about how the component framework has been integrated:

- The driver for the rdma0 component serves double duty as the "master"
  (aggregate) driver as well as a component driver. This is a non-ideal
  compromise until a better solution is developed. This device is
  differentiated from the rest by checking for a "mediatek,vpu" property
  in the device node.

- The list of mdp components remains hard-coded as mtk_mdp_comp_dt_ids[]
  in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
  mtk_mdp_comp.c. This unfortunate duplication of information is
  addressed in a following patch in this series.

- The component driver calls component_add() for each device that is
  probed.

- In mtk_mdp_probe (the "master" device), we scan the device tree for
  any matching nodes against mtk_mdp_comp_dt_ids, and add component
  matches for them. The match criteria is a matching device node
  pointer.

- When the set of components devices that have been probed corresponds
  with the list that is generated by the "master", the callback to
  mtk_mdp_master_bind() is made, which then calls the component bind
  functions.

- Inside mtk_mdp_master_bind(), once all the component bind functions
  have been called, we can then register our device to the v4l2
  subsystem.

- The call to pm_runtime_enable() in the master device is called after
  all the components have been registered by their bind() functions
  called by mtk_mtp_master_bind(). As a result, the list of components
  will not change while power management callbacks mtk_mdp_suspend()/
  resume() are accessing the list of components.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---

(no changes since v1)

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 143 ++++++++++++--
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  25 +--
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 174 +++++++++++++-----
 drivers/media/platform/mtk-mdp/mtk_mdp_core.h |   1 +
 4 files changed, 252 insertions(+), 91 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 76e295c8d9bc..7a0e3acffab9 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -5,13 +5,50 @@
  */
 
 #include <linux/clk.h>
+#include <linux/component.h>
 #include <linux/device.h>
 #include <linux/of.h>
+#include <linux/module.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <soc/mediatek/smi.h>
 
 #include "mtk_mdp_comp.h"
+#include "mtk_mdp_core.h"
+
+/**
+ * enum mtk_mdp_comp_type - the MDP component
+ * @MTK_MDP_RDMA:		Read DMA
+ * @MTK_MDP_RSZ:		Reszer
+ * @MTK_MDP_WDMA:		Write DMA
+ * @MTK_MDP_WROT:		Write DMA with rotation
+ * @MTK_MDP_COMP_TYPE_MAX:	Placeholder for num elems in this enum
+ */
+enum mtk_mdp_comp_type {
+	MTK_MDP_RDMA,
+	MTK_MDP_RSZ,
+	MTK_MDP_WDMA,
+	MTK_MDP_WROT,
+	MTK_MDP_COMP_TYPE_MAX,
+};
+
+static const struct of_device_id mtk_mdp_comp_driver_dt_match[] = {
+	{
+		.compatible = "mediatek,mt8173-mdp-rdma",
+		.data = (void *)MTK_MDP_RDMA
+	}, {
+		.compatible = "mediatek,mt8173-mdp-rsz",
+		.data = (void *)MTK_MDP_RSZ
+	}, {
+		.compatible = "mediatek,mt8173-mdp-wdma",
+		.data = (void *)MTK_MDP_WDMA
+	}, {
+		.compatible = "mediatek,mt8173-mdp-wrot",
+		.data = (void *)MTK_MDP_WROT
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mtk_mdp_comp_driver_dt_match);
 
 int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 {
@@ -20,9 +57,7 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 	if (comp->larb_dev) {
 		err = mtk_smi_larb_get(comp->larb_dev);
 		if (err)
-			dev_err(dev,
-				"failed to get larb, err %d. type:%d\n",
-				err, comp->type);
+			dev_err(dev, "failed to get larb, err %d.\n", err);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
@@ -62,17 +97,41 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
 		mtk_smi_larb_put(comp->larb_dev);
 }
 
-int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
-		      struct mtk_mdp_comp *comp,
-		      enum mtk_mdp_comp_type comp_type)
+static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
+{
+	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
+	struct mtk_mdp_dev *mdp = data;
+
+	mtk_mdp_register_component(mdp, comp);
+
+	return 0;
+}
+
+static void mtk_mdp_comp_unbind(struct device *dev, struct device *master,
+			   void *data)
+{
+	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
+	struct mtk_mdp_dev *mdp = data;
+
+	mtk_mdp_unregister_component(mdp, comp);
+}
+
+static const struct component_ops mtk_mdp_component_ops = {
+	.bind   = mtk_mdp_comp_bind,
+	.unbind = mtk_mdp_comp_unbind,
+};
+
+int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev)
 {
 	struct device_node *larb_node;
 	struct platform_device *larb_pdev;
 	int ret;
 	int i;
+	struct device_node *node = dev->of_node;
+	enum mtk_mdp_comp_type comp_type =
+		 (enum mtk_mdp_comp_type)of_device_get_match_data(dev);
 
-	comp->dev_node = of_node_get(node);
-	comp->type = comp_type;
+	INIT_LIST_HEAD(&comp->node);
 
 	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
 		comp->clk[i] = of_clk_get(node, i);
@@ -80,19 +139,17 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
 			if (PTR_ERR(comp->clk[i]) != -EPROBE_DEFER)
 				dev_err(dev, "Failed to get clock\n");
 			ret = PTR_ERR(comp->clk[i]);
-			goto put_dev;
+			goto err;
 		}
 
 		/* Only RDMA needs two clocks */
-		if (comp->type != MTK_MDP_RDMA)
+		if (comp_type != MTK_MDP_RDMA)
 			break;
 	}
 
 	/* Only DMA capable components need the LARB property */
 	comp->larb_dev = NULL;
-	if (comp->type != MTK_MDP_RDMA &&
-	    comp->type != MTK_MDP_WDMA &&
-	    comp->type != MTK_MDP_WROT)
+	if (comp_type != MTK_MDP_RDMA && comp_type != MTK_MDP_WDMA && comp_type != MTK_MDP_WROT)
 		return 0;
 
 	larb_node = of_parse_phandle(node, "mediatek,larb", 0);
@@ -100,7 +157,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
 		dev_err(dev,
 			"Missing mediadek,larb phandle in %pOF node\n", node);
 		ret = -EINVAL;
-		goto put_dev;
+		goto err;
 	}
 
 	larb_pdev = of_find_device_by_node(larb_node);
@@ -108,7 +165,7 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
 		dev_warn(dev, "Waiting for larb device %pOF\n", larb_node);
 		of_node_put(larb_node);
 		ret = -EPROBE_DEFER;
-		goto put_dev;
+		goto err;
 	}
 	of_node_put(larb_node);
 
@@ -116,13 +173,59 @@ int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
 
 	return 0;
 
-put_dev:
-	of_node_put(comp->dev_node);
-
+err:
 	return ret;
 }
 
-void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp)
+static int mtk_mdp_comp_probe(struct platform_device *pdev)
 {
-	of_node_put(comp->dev_node);
+	struct device *dev = &pdev->dev;
+	struct device_node *vpu_node;
+	int status;
+	struct mtk_mdp_comp *comp;
+
+	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
+	if (vpu_node) {
+		of_node_put(vpu_node);
+		/*
+		 * The device tree node with a mediatek,vpu property is deemed
+		 * the MDP "master" device, we don't want to add a component
+		 * for it in this function because the initialization for the
+		 * master is done elsewhere.
+		 */
+		dev_info(dev, "vpu node found, not probing\n");
+		return -ENODEV;
+	}
+
+	comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
+	if (!comp)
+		return -ENOMEM;
+
+	status = mtk_mdp_comp_init(comp, dev);
+	if (status) {
+		dev_err(dev, "Failed to initialize component: %d\n", status);
+		return status;
+	}
+
+	dev_set_drvdata(dev, comp);
+
+	return component_add(dev, &mtk_mdp_component_ops);
 }
+
+static int mtk_mdp_comp_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+
+	component_del(dev, &mtk_mdp_component_ops);
+	return 0;
+}
+
+struct platform_driver mtk_mdp_component_driver = {
+	.probe          = mtk_mdp_comp_probe,
+	.remove         = mtk_mdp_comp_remove,
+	.driver         = {
+		.name   = "mediatek-mdp-comp",
+		.owner  = THIS_MODULE,
+		.of_match_table = mtk_mdp_comp_driver_dt_match,
+	},
+};
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
index 92ab5249bcad..df5fc4c94f90 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -7,42 +7,23 @@
 #ifndef __MTK_MDP_COMP_H__
 #define __MTK_MDP_COMP_H__
 
-/**
- * enum mtk_mdp_comp_type - the MDP component
- * @MTK_MDP_RDMA:	Read DMA
- * @MTK_MDP_RSZ:	Riszer
- * @MTK_MDP_WDMA:	Write DMA
- * @MTK_MDP_WROT:	Write DMA with rotation
- */
-enum mtk_mdp_comp_type {
-	MTK_MDP_RDMA,
-	MTK_MDP_RSZ,
-	MTK_MDP_WDMA,
-	MTK_MDP_WROT,
-};
-
 /**
  * struct mtk_mdp_comp - the MDP's function component data
  * @node:	list node to track sibing MDP components
- * @dev_node:	component device node
  * @clk:	clocks required for component
  * @larb_dev:	SMI device required for component
- * @type:	component type
  */
 struct mtk_mdp_comp {
 	struct list_head	node;
-	struct device_node	*dev_node;
 	struct clk		*clk[2];
 	struct device		*larb_dev;
-	enum mtk_mdp_comp_type	type;
 };
 
-int mtk_mdp_comp_init(struct device *dev, struct device_node *node,
-		      struct mtk_mdp_comp *comp,
-		      enum mtk_mdp_comp_type comp_type);
-void mtk_mdp_comp_deinit(struct device *dev, struct mtk_mdp_comp *comp);
+int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev);
+
 int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
 void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp);
 
+extern struct platform_driver mtk_mdp_component_driver;
 
 #endif /* __MTK_MDP_COMP_H__ */
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index 412bbec0f735..b813a822439a 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/component.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/interrupt.h>
@@ -19,6 +20,7 @@
 #include <linux/workqueue.h>
 #include <soc/mediatek/smi.h>
 
+#include "mtk_mdp_comp.h"
 #include "mtk_mdp_core.h"
 #include "mtk_mdp_m2m.h"
 #include "mtk_vpu.h"
@@ -32,16 +34,12 @@ module_param(mtk_mdp_dbg_level, int, 0644);
 static const struct of_device_id mtk_mdp_comp_dt_ids[] = {
 	{
 		.compatible = "mediatek,mt8173-mdp-rdma",
-		.data = (void *)MTK_MDP_RDMA
 	}, {
 		.compatible = "mediatek,mt8173-mdp-rsz",
-		.data = (void *)MTK_MDP_RSZ
 	}, {
 		.compatible = "mediatek,mt8173-mdp-wdma",
-		.data = (void *)MTK_MDP_WDMA
 	}, {
 		.compatible = "mediatek,mt8173-mdp-wrot",
-		.data = (void *)MTK_MDP_WROT
 	},
 	{ },
 };
@@ -106,6 +104,63 @@ static void mtk_mdp_reset_handler(void *priv)
 	queue_work(mdp->wdt_wq, &mdp->wdt_work);
 }
 
+static int compare_of(struct device *dev, void *data)
+{
+	return dev->of_node == data;
+}
+
+static void release_of(struct device *dev, void *data)
+{
+	of_node_put(data);
+}
+
+static int mtk_mdp_master_bind(struct device *dev)
+{
+	int status;
+	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
+
+	mtk_mdp_register_component(mdp, &mdp->comp_self);
+
+	status = component_bind_all(dev, mdp);
+	if (status) {
+		dev_err(dev, "Failed to bind all components: %d\n", status);
+		goto err_component_bind_all;
+	}
+
+	status = mtk_mdp_register_m2m_device(mdp);
+	if (status) {
+		dev_err(dev, "Failed to register m2m device: %d\n", status);
+		goto err_mtk_mdp_register_m2m_device;
+	}
+
+	pm_runtime_enable(dev);
+
+	return 0;
+
+err_mtk_mdp_register_m2m_device:
+	component_unbind_all(dev, mdp);
+
+err_component_bind_all:
+	mtk_mdp_unregister_component(mdp, &mdp->comp_self);
+
+	return status;
+}
+
+static void mtk_mdp_master_unbind(struct device *dev)
+{
+	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
+
+	pm_runtime_disable(dev);
+	mtk_mdp_unregister_m2m_device(mdp);
+	component_unbind_all(dev, mdp);
+	mtk_mdp_unregister_component(mdp, &mdp->comp_self);
+}
+
+static const struct component_master_ops mtk_mdp_com_ops = {
+	.bind		= mtk_mdp_master_bind,
+	.unbind		= mtk_mdp_master_unbind,
+};
+
 void mtk_mdp_register_component(struct mtk_mdp_dev *mdp,
 				struct mtk_mdp_comp *comp)
 {
@@ -123,8 +178,8 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 	struct mtk_mdp_dev *mdp;
 	struct device *dev = &pdev->dev;
 	struct device_node *node, *parent;
-	struct mtk_mdp_comp *comp, *comp_temp;
-	int ret = 0;
+	int i, ret = 0;
+	struct component_match *match = NULL;
 
 	mdp = devm_kzalloc(dev, sizeof(*mdp), GFP_KERNEL);
 	if (!mdp)
@@ -149,36 +204,43 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 	}
 
 	/* Iterate over sibling MDP function blocks */
+	i = 0;
 	for_each_child_of_node(parent, node) {
-		const struct of_device_id *of_id;
-		enum mtk_mdp_comp_type comp_type;
+		struct platform_device *pdev;
 
-		of_id = of_match_node(mtk_mdp_comp_dt_ids, node);
-		if (!of_id)
+		if (!of_match_node(mtk_mdp_comp_dt_ids, node))
 			continue;
 
-		if (!of_device_is_available(node)) {
-			dev_err(dev, "Skipping disabled component %pOF\n",
-				node);
+		if (!of_device_is_available(node))
 			continue;
-		}
-
-		comp_type = (enum mtk_mdp_comp_type)of_id->data;
 
-		comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
-		if (!comp) {
-			ret = -ENOMEM;
-			of_node_put(node);
-			goto err_comp;
+		pdev = of_find_device_by_node(node);
+		if (!pdev) {
+			dev_warn(dev, "Unable to find comp device %s\n",
+				 node->full_name);
+			continue;
 		}
 
-		ret = mtk_mdp_comp_init(dev, node, comp, comp_type);
-		if (ret) {
-			of_node_put(node);
-			goto err_comp;
+		/*
+		 * Do not add a match for my own (rdma0) device node.
+		 * I will be managing it directly instead using comp_self.
+		 */
+		if (&pdev->dev != dev) {
+			dev_dbg(dev, "adding match %d for: %pOF\n", i++, node);
+			component_match_add_release(dev, &match, release_of,
+						    compare_of,
+						    of_node_get(node));
 		}
+	}
 
-		mtk_mdp_register_component(mdp, comp);
+	/*
+	 * Create a component for myself so that clocks can be toggled in
+	 * clock_on().
+	 */
+	ret = mtk_mdp_comp_init(&mdp->comp_self, dev);
+	if (ret) {
+		dev_err(dev, "Failed to initialize component\n");
+		goto err_comp;
 	}
 
 	mdp->job_wq = create_singlethread_workqueue(MTK_MDP_MODULE_NAME);
@@ -203,18 +265,12 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 		goto err_dev_register;
 	}
 
-	ret = mtk_mdp_register_m2m_device(mdp);
-	if (ret) {
-		v4l2_err(&mdp->v4l2_dev, "Failed to init mem2mem device\n");
-		goto err_m2m_register;
-	}
-
 	mdp->vpu_dev = vpu_get_plat_device(pdev);
 	ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
 				  VPU_RST_MDP);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to register reset handler\n");
-		goto err_m2m_register;
+		goto err_wdt_reg;
 	}
 
 	platform_set_drvdata(pdev, mdp);
@@ -222,15 +278,25 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 	ret = vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to set vb2 dma mag seg size\n");
-		goto err_m2m_register;
+		goto err_set_max_seg_size;
+	}
+
+	ret = component_master_add_with_match(dev, &mtk_mdp_com_ops, match);
+	if (ret) {
+		dev_err(dev, "Component master add failed\n");
+		goto err_component_master_add;
 	}
 
-	pm_runtime_enable(dev);
 	dev_dbg(dev, "mdp-%d registered successfully\n", mdp->id);
 
 	return 0;
 
-err_m2m_register:
+err_component_master_add:
+	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
+
+err_set_max_seg_size:
+
+err_wdt_reg:
 	v4l2_device_unregister(&mdp->v4l2_dev);
 
 err_dev_register:
@@ -242,11 +308,6 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 err_alloc_job_wq:
 
 err_comp:
-	list_for_each_entry_safe(comp, comp_temp, &mdp->comp_list, node) {
-		mtk_mdp_unregister_component(mdp, comp);
-		mtk_mdp_comp_deinit(dev, comp);
-	}
-
 	dev_dbg(dev, "err %d\n", ret);
 	return ret;
 }
@@ -254,11 +315,10 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 static int mtk_mdp_remove(struct platform_device *pdev)
 {
 	struct mtk_mdp_dev *mdp = platform_get_drvdata(pdev);
-	struct mtk_mdp_comp *comp, *comp_temp;
 
-	pm_runtime_disable(&pdev->dev);
+	component_master_del(&pdev->dev, &mtk_mdp_com_ops);
+
 	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
-	mtk_mdp_unregister_m2m_device(mdp);
 	v4l2_device_unregister(&mdp->v4l2_dev);
 
 	flush_workqueue(mdp->wdt_wq);
@@ -267,10 +327,8 @@ static int mtk_mdp_remove(struct platform_device *pdev)
 	flush_workqueue(mdp->job_wq);
 	destroy_workqueue(mdp->job_wq);
 
-	list_for_each_entry_safe(comp, comp_temp, &mdp->comp_list, node) {
-		mtk_mdp_unregister_component(mdp, comp);
-		mtk_mdp_comp_deinit(&pdev->dev, comp);
-	}
+	if (!list_empty(&mdp->comp_list))
+		dev_warn(&pdev->dev, "not all components removed\n");
 
 	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
 	return 0;
@@ -323,7 +381,25 @@ static struct platform_driver mtk_mdp_driver = {
 	}
 };
 
-module_platform_driver(mtk_mdp_driver);
+static struct platform_driver * const mtk_mdp_drivers[] = {
+	&mtk_mdp_driver,
+	&mtk_mdp_component_driver,
+};
+
+static int __init mtk_mdp_init(void)
+{
+	return platform_register_drivers(mtk_mdp_drivers,
+					 ARRAY_SIZE(mtk_mdp_drivers));
+}
+
+static void __exit mtk_mdp_exit(void)
+{
+	platform_unregister_drivers(mtk_mdp_drivers,
+				    ARRAY_SIZE(mtk_mdp_drivers));
+}
+
+module_init(mtk_mdp_init);
+module_exit(mtk_mdp_exit);
 
 MODULE_AUTHOR("Houlong Wei <houlong.wei@mediatek.com>");
 MODULE_DESCRIPTION("Mediatek image processor driver");
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
index a6e6dc36307b..8a52539b15d4 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
@@ -155,6 +155,7 @@ struct mtk_mdp_dev {
 	struct mtk_mdp_variant		*variant;
 	u16				id;
 	struct list_head		comp_list;
+	struct mtk_mdp_comp		comp_self;
 	struct v4l2_m2m_dev		*m2m_dev;
 	struct list_head		ctx_list;
 	struct video_device		*vdev;
-- 
2.33.0.rc2.250.ged5fa647cd-goog

Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[Ezequiel Garcia]

Hi Eizan,

Sorry for seeing this series so late.

On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org> wrote:
>
> Broadly, this patch (1) adds a driver for various MTK MDP components to
> go alongside the main MTK MDP driver, and (2) hooks them all together
> using the component framework.
>
> (1) Up until now, the MTK MDP driver controls 8 devices in the device
> tree on its own. When running tests for the hardware video decoder, we
> found that the iommus and LARBs were not being properly configured.

Why were not being properly configured? What was the problem?
Why not fixing that instead?

Does this mean the driver is currently broken and unusable?

> To
> configure them, a driver for each be added to mtk_mdp_comp so that
> mtk_iommu_add_device() can (eventually) be called from dma_configure()
> inside really_probe().
>
> (2) The integration into the component framework allows us to defer the
> registration with the v4l2 subsystem until all the MDP-related devices
> have been probed, so that the relevant device node does not become
> available until initialization of all the components is complete.
>
> Some notes about how the component framework has been integrated:
>
> - The driver for the rdma0 component serves double duty as the "master"
>   (aggregate) driver as well as a component driver. This is a non-ideal
>   compromise until a better solution is developed. This device is
>   differentiated from the rest by checking for a "mediatek,vpu" property
>   in the device node.
>

As I have stated in Yunfei, I am not convinced you need an async framework
at all. It seems all these devices could have been linked together
in the device tree, and then have a master device to tie them.

I.e. something like

mdp {
  mdp_rdma0 {
  }
  mdp_rsz0 {
  }
  mdp_rsz1 {
  }
}

All this async games seem like making the driver really obfuscated,
which will mean harder to debug and maintain.
I am not sure we want that burden.

Even if we are all fully convinced that you absolutely need
an async framework, then what's wrong with v4l2-async?

I would start by addressing what is wrong with the IOMMUs
in the current design.

Thanks,
Ezequiel

> - The list of mdp components remains hard-coded as mtk_mdp_comp_dt_ids[]
>   in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
>   mtk_mdp_comp.c. This unfortunate duplication of information is
>   addressed in a following patch in this series.
>
> - The component driver calls component_add() for each device that is
>   probed.
>
> - In mtk_mdp_probe (the "master" device), we scan the device tree for
>   any matching nodes against mtk_mdp_comp_dt_ids, and add component
>   matches for them. The match criteria is a matching device node
>   pointer.
>
> - When the set of components devices that have been probed corresponds
>   with the list that is generated by the "master", the callback to
>   mtk_mdp_master_bind() is made, which then calls the component bind
>   functions.
>
> - Inside mtk_mdp_master_bind(), once all the component bind functions
>   have been called, we can then register our device to the v4l2
>   subsystem.
>
> - The call to pm_runtime_enable() in the master device is called after
>   all the components have been registered by their bind() functions
>   called by mtk_mtp_master_bind(). As a result, the list of components
>   will not change while power management callbacks mtk_mdp_suspend()/
>   resume() are accessing the list of components.
>
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
> ---
>

Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[houlong wei]

Hi Ezequiel,

Thank you for your attention to this series of patches. I answer partial of your questions below.
Regards,
Houlong

On Sat, 2021-09-04 at 20:34 +0800, Ezequiel Garcia wrote:
> Hi Eizan,
> 
> Sorry for seeing this series so late.
> 
> On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org>
> wrote:
> > 
> > Broadly, this patch (1) adds a driver for various MTK MDP
> > components to
> > go alongside the main MTK MDP driver, and (2) hooks them all
> > together
> > using the component framework.
> > 
> > (1) Up until now, the MTK MDP driver controls 8 devices in the
> > device
> > tree on its own. When running tests for the hardware video decoder,
> > we
> > found that the iommus and LARBs were not being properly configured.
> 
> Why were not being properly configured? What was the problem?
> Why not fixing that instead?
> 
> Does this mean the driver is currently broken and unusable?

This series of patches are supplements to another series, please refer
to  
https://patchwork.kernel.org/project/linux-mediatek/list/?series=515129c
, which add device link between the mtk-iommu consumer and the mtk-larb 
devices. Without that series of patches, the mtk-mdp driver can work
well so far.
But with that series, it seems the device link only can be established
for the device which is registered as a platform driver. That's why
Eizan adds this series of patches to make all mdp components to be
registered as platform drivers.

> 
> > To
> > configure them, a driver for each be added to mtk_mdp_comp so that
> > mtk_iommu_add_device() can (eventually) be called from
> > dma_configure()
> > inside really_probe().
> > 
> > (2) The integration into the component framework allows us to defer
> > the
> > registration with the v4l2 subsystem until all the MDP-related
> > devices
> > have been probed, so that the relevant device node does not become
> > available until initialization of all the components is complete.
> > 
> > Some notes about how the component framework has been integrated:
> > 
> > - The driver for the rdma0 component serves double duty as the
> > "master"
> >   (aggregate) driver as well as a component driver. This is a non-
> > ideal
> >   compromise until a better solution is developed. This device is
> >   differentiated from the rest by checking for a "mediatek,vpu"
> > property
> >   in the device node.
> > 
> 
> As I have stated in Yunfei, I am not convinced you need an async
> framework
> at all. It seems all these devices could have been linked together
> in the device tree, and then have a master device to tie them.
> 
> I.e. something like
> 
> mdp {
>   mdp_rdma0 {
>   }
>   mdp_rsz0 {
>   }
>   mdp_rsz1 {
>   }
> }
> 

The commit message of the patch below explains that " If the mdp_*
nodes are under an mdp sub-node, their corresponding platform device
does not automatically get its iommu assigned properly."
Please refer to 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.14.1&id=8127881f741dbbf9a1da9e9bc59133820160b217

> All this async games seem like making the driver really obfuscated,
> which will mean harder to debug and maintain.
> I am not sure we want that burden.
> 
> Even if we are all fully convinced that you absolutely need
> an async framework, then what's wrong with v4l2-async?
> 
> I would start by addressing what is wrong with the IOMMUs
> in the current design.
> 
> Thanks,
> Ezequiel
> 
> > - The list of mdp components remains hard-coded as
> > mtk_mdp_comp_dt_ids[]
> >   in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
> >   mtk_mdp_comp.c. This unfortunate duplication of information is
> >   addressed in a following patch in this series.
> > 
> > - The component driver calls component_add() for each device that
> > is
> >   probed.
> > 
> > - In mtk_mdp_probe (the "master" device), we scan the device tree
> > for
> >   any matching nodes against mtk_mdp_comp_dt_ids, and add component
> >   matches for them. The match criteria is a matching device node
> >   pointer.
> > 
> > - When the set of components devices that have been probed
> > corresponds
> >   with the list that is generated by the "master", the callback to
> >   mtk_mdp_master_bind() is made, which then calls the component
> > bind
> >   functions.
> > 
> > - Inside mtk_mdp_master_bind(), once all the component bind
> > functions
> >   have been called, we can then register our device to the v4l2
> >   subsystem.
> > 
> > - The call to pm_runtime_enable() in the master device is called
> > after
> >   all the components have been registered by their bind() functions
> >   called by mtk_mtp_master_bind(). As a result, the list of
> > components
> >   will not change while power management callbacks
> > mtk_mdp_suspend()/
> >   resume() are accessing the list of components.
> > 
> > Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> > Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
> > ---
> > 

Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[houlong wei]

Sorry, I fix a link error below.

On Mon, 2021-09-06 at 00:23 +0800, houlong wei wrote:
> Hi Ezequiel,
> 
> Thank you for your attention to this series of patches. I answer
> partial of your questions below.
> Regards,
> Houlong
> 
> On Sat, 2021-09-04 at 20:34 +0800, Ezequiel Garcia wrote:
> > Hi Eizan,
> > 
> > Sorry for seeing this series so late.
> > 
> > On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org>
> > wrote:
> > > 
> > > Broadly, this patch (1) adds a driver for various MTK MDP
> > > components to
> > > go alongside the main MTK MDP driver, and (2) hooks them all
> > > together
> > > using the component framework.
> > > 
> > > (1) Up until now, the MTK MDP driver controls 8 devices in the
> > > device
> > > tree on its own. When running tests for the hardware video
> > > decoder,
> > > we
> > > found that the iommus and LARBs were not being properly
> > > configured.
> > 
> > Why were not being properly configured? What was the problem?
> > Why not fixing that instead?
> > 
> > Does this mean the driver is currently broken and unusable?
> 
> This series of patches are supplements to another series, please
> refer
> to  
> 
https://patchwork.kernel.org/project/linux-mediatek/list/?series=515129c

Fix the link address: 
https://patchwork.kernel.org/project/linux-mediatek/list/?series=523771

> , which add device link between the mtk-iommu consumer and the mtk-
> larb 
> devices. Without that series of patches, the mtk-mdp driver can work
> well so far.
> But with that series, it seems the device link only can be
> established
> for the device which is registered as a platform driver. That's why
> Eizan adds this series of patches to make all mdp components to be
> registered as platform drivers.
> 
> > 
> > > To
> > > configure them, a driver for each be added to mtk_mdp_comp so
> > > that
> > > mtk_iommu_add_device() can (eventually) be called from
> > > dma_configure()
> > > inside really_probe().
> > > 
> > > (2) The integration into the component framework allows us to
> > > defer
> > > the
> > > registration with the v4l2 subsystem until all the MDP-related
> > > devices
> > > have been probed, so that the relevant device node does not
> > > become
> > > available until initialization of all the components is complete.
> > > 
> > > Some notes about how the component framework has been integrated:
> > > 
> > > - The driver for the rdma0 component serves double duty as the
> > > "master"
> > >   (aggregate) driver as well as a component driver. This is a
> > > non-
> > > ideal
> > >   compromise until a better solution is developed. This device is
> > >   differentiated from the rest by checking for a "mediatek,vpu"
> > > property
> > >   in the device node.
> > > 
> > 
> > As I have stated in Yunfei, I am not convinced you need an async
> > framework
> > at all. It seems all these devices could have been linked together
> > in the device tree, and then have a master device to tie them.
> > 
> > I.e. something like
> > 
> > mdp {
> >   mdp_rdma0 {
> >   }
> >   mdp_rsz0 {
> >   }
> >   mdp_rsz1 {
> >   }
> > }
> > 
> 
> The commit message of the patch below explains that " If the mdp_*
> nodes are under an mdp sub-node, their corresponding platform device
> does not automatically get its iommu assigned properly."
> Please refer to 
> 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.14.1&id=8127881f741dbbf9a1da9e9bc59133820160b217
> 
> > All this async games seem like making the driver really obfuscated,
> > which will mean harder to debug and maintain.
> > I am not sure we want that burden.
> > 
> > Even if we are all fully convinced that you absolutely need
> > an async framework, then what's wrong with v4l2-async?
> > 
> > I would start by addressing what is wrong with the IOMMUs
> > in the current design.
> > 
> > Thanks,
> > Ezequiel
> > 
> > > - The list of mdp components remains hard-coded as
> > > mtk_mdp_comp_dt_ids[]
> > >   in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
> > >   mtk_mdp_comp.c. This unfortunate duplication of information is
> > >   addressed in a following patch in this series.
> > > 
> > > - The component driver calls component_add() for each device that
> > > is
> > >   probed.
> > > 
> > > - In mtk_mdp_probe (the "master" device), we scan the device tree
> > > for
> > >   any matching nodes against mtk_mdp_comp_dt_ids, and add
> > > component
> > >   matches for them. The match criteria is a matching device node
> > >   pointer.
> > > 
> > > - When the set of components devices that have been probed
> > > corresponds
> > >   with the list that is generated by the "master", the callback
> > > to
> > >   mtk_mdp_master_bind() is made, which then calls the component
> > > bind
> > >   functions.
> > > 
> > > - Inside mtk_mdp_master_bind(), once all the component bind
> > > functions
> > >   have been called, we can then register our device to the v4l2
> > >   subsystem.
> > > 
> > > - The call to pm_runtime_enable() in the master device is called
> > > after
> > >   all the components have been registered by their bind()
> > > functions
> > >   called by mtk_mtp_master_bind(). As a result, the list of
> > > components
> > >   will not change while power management callbacks
> > > mtk_mdp_suspend()/
> > >   resume() are accessing the list of components.
> > > 
> > > Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> > > Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
> > > >
> > > Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
> > > ---
> > > 
> 
> 

Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[Hans Verkuil]

On 9/5/21 6:23 PM, houlong wei wrote:
> Hi Ezequiel,
> 
> Thank you for your attention to this series of patches. I answer partial of your questions below.
> Regards,
> Houlong
> 
> On Sat, 2021-09-04 at 20:34 +0800, Ezequiel Garcia wrote:
>> Hi Eizan,
>>
>> Sorry for seeing this series so late.
>>
>> On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org>
>> wrote:
>>>
>>> Broadly, this patch (1) adds a driver for various MTK MDP
>>> components to
>>> go alongside the main MTK MDP driver, and (2) hooks them all
>>> together
>>> using the component framework.
>>>
>>> (1) Up until now, the MTK MDP driver controls 8 devices in the
>>> device
>>> tree on its own. When running tests for the hardware video decoder,
>>> we
>>> found that the iommus and LARBs were not being properly configured.
>>
>> Why were not being properly configured? What was the problem?
>> Why not fixing that instead?
>>
>> Does this mean the driver is currently broken and unusable?
> 
> This series of patches are supplements to another series, please refer
> to  
> https://patchwork.kernel.org/project/linux-mediatek/list/?series=515129c
> , which add device link between the mtk-iommu consumer and the mtk-larb 
> devices. Without that series of patches, the mtk-mdp driver can work
> well so far.
> But with that series, it seems the device link only can be established
> for the device which is registered as a platform driver. That's why
> Eizan adds this series of patches to make all mdp components to be
> registered as platform drivers.

Hold on, so this means that if that iommu device-link series is merged,
then the mtk-mdp driver breaks? I posted a PR for that iommu series, but
I've just withdrawn that PR until this issue is clarified.

Is it only mtk-mdp that is affected by this iommu device-link series, or
others as well?

Regards,

	Hans

> 
>>
>>> To
>>> configure them, a driver for each be added to mtk_mdp_comp so that
>>> mtk_iommu_add_device() can (eventually) be called from
>>> dma_configure()
>>> inside really_probe().
>>>
>>> (2) The integration into the component framework allows us to defer
>>> the
>>> registration with the v4l2 subsystem until all the MDP-related
>>> devices
>>> have been probed, so that the relevant device node does not become
>>> available until initialization of all the components is complete.
>>>
>>> Some notes about how the component framework has been integrated:
>>>
>>> - The driver for the rdma0 component serves double duty as the
>>> "master"
>>>   (aggregate) driver as well as a component driver. This is a non-
>>> ideal
>>>   compromise until a better solution is developed. This device is
>>>   differentiated from the rest by checking for a "mediatek,vpu"
>>> property
>>>   in the device node.
>>>
>>
>> As I have stated in Yunfei, I am not convinced you need an async
>> framework
>> at all. It seems all these devices could have been linked together
>> in the device tree, and then have a master device to tie them.
>>
>> I.e. something like
>>
>> mdp {
>>   mdp_rdma0 {
>>   }
>>   mdp_rsz0 {
>>   }
>>   mdp_rsz1 {
>>   }
>> }
>>
> 
> The commit message of the patch below explains that " If the mdp_*
> nodes are under an mdp sub-node, their corresponding platform device
> does not automatically get its iommu assigned properly."
> Please refer to 
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.14.1&id=8127881f741dbbf9a1da9e9bc59133820160b217
> 
>> All this async games seem like making the driver really obfuscated,
>> which will mean harder to debug and maintain.
>> I am not sure we want that burden.
>>
>> Even if we are all fully convinced that you absolutely need
>> an async framework, then what's wrong with v4l2-async?
>>
>> I would start by addressing what is wrong with the IOMMUs
>> in the current design.
>>
>> Thanks,
>> Ezequiel
>>
>>> - The list of mdp components remains hard-coded as
>>> mtk_mdp_comp_dt_ids[]
>>>   in mtk_mdp_core.c, and as mtk_mdp_comp_driver_dt_match[] in
>>>   mtk_mdp_comp.c. This unfortunate duplication of information is
>>>   addressed in a following patch in this series.
>>>
>>> - The component driver calls component_add() for each device that
>>> is
>>>   probed.
>>>
>>> - In mtk_mdp_probe (the "master" device), we scan the device tree
>>> for
>>>   any matching nodes against mtk_mdp_comp_dt_ids, and add component
>>>   matches for them. The match criteria is a matching device node
>>>   pointer.
>>>
>>> - When the set of components devices that have been probed
>>> corresponds
>>>   with the list that is generated by the "master", the callback to
>>>   mtk_mdp_master_bind() is made, which then calls the component
>>> bind
>>>   functions.
>>>
>>> - Inside mtk_mdp_master_bind(), once all the component bind
>>> functions
>>>   have been called, we can then register our device to the v4l2
>>>   subsystem.
>>>
>>> - The call to pm_runtime_enable() in the master device is called
>>> after
>>>   all the components have been registered by their bind() functions
>>>   called by mtk_mtp_master_bind(). As a result, the list of
>>> components
>>>   will not change while power management callbacks
>>> mtk_mdp_suspend()/
>>>   resume() are accessing the list of components.
>>>
>>> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
>>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>> Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
>>> ---
>>>
> 

Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[Ezequiel Garcia]

Hi everyone,

On Tue, 7 Dec 2021 at 09:37, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 9/5/21 6:23 PM, houlong wei wrote:
> > Hi Ezequiel,
> >
> > Thank you for your attention to this series of patches. I answer partial of your questions below.
> > Regards,
> > Houlong
> >
> > On Sat, 2021-09-04 at 20:34 +0800, Ezequiel Garcia wrote:
> >> Hi Eizan,
> >>
> >> Sorry for seeing this series so late.
> >>
> >> On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org>
> >> wrote:
> >>>
> >>> Broadly, this patch (1) adds a driver for various MTK MDP
> >>> components to
> >>> go alongside the main MTK MDP driver, and (2) hooks them all
> >>> together
> >>> using the component framework.
> >>>
> >>> (1) Up until now, the MTK MDP driver controls 8 devices in the
> >>> device
> >>> tree on its own. When running tests for the hardware video decoder,
> >>> we
> >>> found that the iommus and LARBs were not being properly configured.
> >>
> >> Why were not being properly configured? What was the problem?
> >> Why not fixing that instead?
> >>
> >> Does this mean the driver is currently broken and unusable?
> >
> > This series of patches are supplements to another series, please refer
> > to
> > https://patchwork.kernel.org/project/linux-mediatek/list/?series=515129c
> > , which add device link between the mtk-iommu consumer and the mtk-larb
> > devices. Without that series of patches, the mtk-mdp driver can work
> > well so far.
> > But with that series, it seems the device link only can be established
> > for the device which is registered as a platform driver. That's why
> > Eizan adds this series of patches to make all mdp components to be
> > registered as platform drivers.
>
> Hold on, so this means that if that iommu device-link series is merged,
> then the mtk-mdp driver breaks? I posted a PR for that iommu series, but
> I've just withdrawn that PR until this issue is clarified.
>
> Is it only mtk-mdp that is affected by this iommu device-link series, or
> others as well?
>

Like I said before, I think it's a mistake to introduce the component
framework in V4L2. This whole idea looks like a hack to me.

If we merge this, then we validate using the component framework
as a way to avoid fixing things properly.

Regards,
Ezequiel

Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[Hans Verkuil]

On 12/7/21 1:46 PM, Ezequiel Garcia wrote:
> Hi everyone,
> 
> On Tue, 7 Dec 2021 at 09:37, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 9/5/21 6:23 PM, houlong wei wrote:
>>> Hi Ezequiel,
>>>
>>> Thank you for your attention to this series of patches. I answer partial of your questions below.
>>> Regards,
>>> Houlong
>>>
>>> On Sat, 2021-09-04 at 20:34 +0800, Ezequiel Garcia wrote:
>>>> Hi Eizan,
>>>>
>>>> Sorry for seeing this series so late.
>>>>
>>>> On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org>
>>>> wrote:
>>>>>
>>>>> Broadly, this patch (1) adds a driver for various MTK MDP
>>>>> components to
>>>>> go alongside the main MTK MDP driver, and (2) hooks them all
>>>>> together
>>>>> using the component framework.
>>>>>
>>>>> (1) Up until now, the MTK MDP driver controls 8 devices in the
>>>>> device
>>>>> tree on its own. When running tests for the hardware video decoder,
>>>>> we
>>>>> found that the iommus and LARBs were not being properly configured.
>>>>
>>>> Why were not being properly configured? What was the problem?
>>>> Why not fixing that instead?
>>>>
>>>> Does this mean the driver is currently broken and unusable?
>>>
>>> This series of patches are supplements to another series, please refer
>>> to
>>> https://patchwork.kernel.org/project/linux-mediatek/list/?series=515129c
>>> , which add device link between the mtk-iommu consumer and the mtk-larb
>>> devices. Without that series of patches, the mtk-mdp driver can work
>>> well so far.
>>> But with that series, it seems the device link only can be established
>>> for the device which is registered as a platform driver. That's why
>>> Eizan adds this series of patches to make all mdp components to be
>>> registered as platform drivers.
>>
>> Hold on, so this means that if that iommu device-link series is merged,
>> then the mtk-mdp driver breaks? I posted a PR for that iommu series, but
>> I've just withdrawn that PR until this issue is clarified.
>>
>> Is it only mtk-mdp that is affected by this iommu device-link series, or
>> others as well?
>>
> 
> Like I said before, I think it's a mistake to introduce the component
> framework in V4L2. This whole idea looks like a hack to me.
> 
> If we merge this, then we validate using the component framework
> as a way to avoid fixing things properly.

I agree with Ezequiel.

	Hans

Re: [PATCH v7 2/7] mtk-mdp: add driver to probe mdp components

[Yong Wu]

On Mon, 2021-09-06 at 00:23 +0800, houlong wei wrote:
> Hi Ezequiel,
> 
> Thank you for your attention to this series of patches. I answer
> partial of your questions below.
> Regards,
> Houlong
> 
> On Sat, 2021-09-04 at 20:34 +0800, Ezequiel Garcia wrote:
> > Hi Eizan,
> > 
> > Sorry for seeing this series so late.
> > 
> > On Wed, 25 Aug 2021 at 03:35, Eizan Miyamoto <eizan@chromium.org>
> > wrote:
> > > 
> > > Broadly, this patch (1) adds a driver for various MTK MDP
> > > components to
> > > go alongside the main MTK MDP driver, and (2) hooks them all
> > > together
> > > using the component framework.
> > > 
> > > (1) Up until now, the MTK MDP driver controls 8 devices in the
> > > device
> > > tree on its own. When running tests for the hardware video
> > > decoder,
> > > we
> > > found that the iommus and LARBs were not being properly
> > > configured.
> > 
> > Why were not being properly configured? What was the problem?
> > Why not fixing that instead?
> > 
> > Does this mean the driver is currently broken and unusable?
> 
> This series of patches are supplements to another series, please
> refer
> to  
> 
https://patchwork.kernel.org/project/linux-mediatek/list/?series=515129c
> , which add device link between the mtk-iommu consumer and the mtk-
> larb 
> devices. Without that series of patches, the mtk-mdp driver can work
> well so far.
> But with that series, it seems the device link only can be
> established
> for the device which is registered as a platform driver. That's why
> Eizan adds this series of patches to make all mdp components to be
> registered as platform drivers.

The mt8173 mdp has several devices:
   mediatek,mt8173-mdp-rdma, mediatek,mt8173-mdp  
   mediatek,mt8173-mdp-rsz
   mediatek,mt8173-mdp-wdma
   mediatek,mt8173-mdp-wrot

Except the first one, the last three devices are not the standard
platform devices. Thus, they should not be the iommu consumer devices.

Question 1: The last three device don't work actually in mt8173 chrome,
right? or they access continuous buffers?

Question 2: The IOMMU device-link patchset just replaces the pm runtime
interfaces. It don't improve the mdp flow, also should not introduce
regression. thus, my v8 don't rebase this mdp patches. Does the iommu
patchset introduce regression for mdp?

@Eizan, @houlong, Could you help confirm this?
Thanks.

> 
> > 
> > > To
> > > configure them, a driver for each be added to mtk_mdp_comp so
> > > that
> > > mtk_iommu_add_device() can (eventually) be called from
> > > dma_configure()
> > > inside really_probe().
> > > 
> > > (2) The integration into the component framework allows us to
> > > defer
> > > the
> > > registration with the v4l2 subsystem until all the MDP-related
> > > devices
> > > have been probed, so that the relevant device node does not
> > > become
> > > available until initialization of all the components is complete.
> > > 
> > > Some notes about how the component framework has been integrated:
> > > 
> > > - The driver for the rdma0 component serves double duty as the
> > > "master"
> > >   (aggregate) driver as well as a component driver. This is a
> > > non-
> > > ideal
> > >   compromise until a better solution is developed. This device is
> > >   differentiated from the rest by checking for a "mediatek,vpu"
> > > property
> > >   in the device node.
> > > 
> > 
> > As I have stated in Yunfei, I am not convinced you need an async
> > framework
> > at all. It seems all these devices could have been linked together
> > in the device tree, and then have a master device to tie them.
> > 
> > I.e. something like
> > 
> > mdp {
> >   mdp_rdma0 {
> >   }
> >   mdp_rsz0 {
> >   }
> >   mdp_rsz1 {
> >   }
> > }
> > 
> 
> The commit message of the patch below explains that " If the mdp_*
> nodes are under an mdp sub-node, their corresponding platform device
> does not automatically get its iommu assigned properly."
> Please refer to 
> 
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.14.1&id=8127881f741dbbf9a1da9e9bc59133820160b217
> 
[snip]

[PATCH v7 3/7] mtk-mdp: use pm_runtime in MDP component driver

[Eizan Miyamoto]

Without this change, the MDP components are not fully integrated into
the runtime power management subsystem, and the MDP driver does not
work.

For each of the component device drivers to be able to call
pm_runtime_get/put_sync() a pointer to the component's device struct
had to be added to struct mtk_mdp_comp, set by mtk_mdp_comp_init().

Note that the dev argument to mtk_mdp_comp_clock_on/off() has been
removed. Those functions used to be called from the "master" mdp driver
in mtk_mdp_core.c, but the component's device pointer no longer
corresponds to the mdp master device pointer, which is not the right
device to pass to pm_runtime_put/get_sync() which we had to add to get
the driver to work properly.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---

(no changes since v1)

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 24 +++++++++++++++----
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  6 +++--
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c |  7 +++---
 3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 7a0e3acffab9..472c261b01e8 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -12,6 +12,7 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <soc/mediatek/smi.h>
+#include <linux/pm_runtime.h>
 
 #include "mtk_mdp_comp.h"
 #include "mtk_mdp_core.h"
@@ -50,14 +51,22 @@ static const struct of_device_id mtk_mdp_comp_driver_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_mdp_comp_driver_dt_match);
 
-int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
+int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
 {
 	int i, err, status;
 
 	if (comp->larb_dev) {
 		err = mtk_smi_larb_get(comp->larb_dev);
 		if (err)
-			dev_err(dev, "failed to get larb, err %d.\n", err);
+			dev_err(comp->dev, "failed to get larb, err %d.\n", err);
+	}
+
+	err = pm_runtime_get_sync(comp->dev);
+	if (err < 0) {
+		dev_err(comp->dev,
+			"failed to runtime get, err %d.\n",
+			err);
+		return err;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
@@ -66,7 +75,7 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 		err = clk_prepare_enable(comp->clk[i]);
 		if (err) {
 			status = err;
-			dev_err(dev, "failed to enable clock, err %d. i:%d\n", err, i);
+			dev_err(comp->dev, "failed to enable clock, err %d. i:%d\n", err, i);
 			goto err_clk_prepare_enable;
 		}
 	}
@@ -80,10 +89,12 @@ int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 		clk_disable_unprepare(comp->clk[i]);
 	}
 
+	pm_runtime_put_sync(comp->dev);
+
 	return status;
 }
 
-void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
+int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
 {
 	int i;
 
@@ -95,6 +106,8 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
 
 	if (comp->larb_dev)
 		mtk_smi_larb_put(comp->larb_dev);
+
+	return pm_runtime_put_sync(comp->dev);
 }
 
 static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
@@ -103,6 +116,7 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
 	struct mtk_mdp_dev *mdp = data;
 
 	mtk_mdp_register_component(mdp, comp);
+	pm_runtime_enable(dev);
 
 	return 0;
 }
@@ -113,6 +127,7 @@ static void mtk_mdp_comp_unbind(struct device *dev, struct device *master,
 	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
 	struct mtk_mdp_dev *mdp = data;
 
+	pm_runtime_disable(dev);
 	mtk_mdp_unregister_component(mdp, comp);
 }
 
@@ -132,6 +147,7 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev)
 		 (enum mtk_mdp_comp_type)of_device_get_match_data(dev);
 
 	INIT_LIST_HEAD(&comp->node);
+	comp->dev = dev;
 
 	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
 		comp->clk[i] = of_clk_get(node, i);
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
index df5fc4c94f90..f2e22e7e7c45 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -12,17 +12,19 @@
  * @node:	list node to track sibing MDP components
  * @clk:	clocks required for component
  * @larb_dev:	SMI device required for component
+ * @dev:	component's device
  */
 struct mtk_mdp_comp {
 	struct list_head	node;
 	struct clk		*clk[2];
+	struct device		*dev;
 	struct device		*larb_dev;
 };
 
 int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev);
 
-int mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp);
-void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp);
+int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp);
+int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp);
 
 extern struct platform_driver mtk_mdp_component_driver;
 
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index b813a822439a..714154450981 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -58,7 +58,7 @@ static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
 	int err;
 
 	list_for_each_entry(comp_node, &mdp->comp_list, node) {
-		err = mtk_mdp_comp_clock_on(dev, comp_node);
+		err = mtk_mdp_comp_clock_on(comp_node);
 		if (err) {
 			status = err;
 			goto err_mtk_mdp_comp_clock_on;
@@ -69,18 +69,17 @@ static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
 
 err_mtk_mdp_comp_clock_on:
 	list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node)
-		mtk_mdp_comp_clock_off(dev, comp_node);
+		mtk_mdp_comp_clock_off(comp_node);
 
 	return status;
 }
 
 static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
 {
-	struct device *dev = &mdp->pdev->dev;
 	struct mtk_mdp_comp *comp_node;
 
 	list_for_each_entry(comp_node, &mdp->comp_list, node)
-		mtk_mdp_comp_clock_off(dev, comp_node);
+		mtk_mdp_comp_clock_off(comp_node);
 }
 
 static void mtk_mdp_wdt_worker(struct work_struct *work)
-- 
2.33.0.rc2.250.ged5fa647cd-goog

[PATCH v7 4/7] media: mtk-mdp: don't pm_run_time_get/put for master comp in clock_on

[Eizan Miyamoto]

The original intent of commit 86698b9505bbc ("media: mtk-mdp: convert
mtk_mdp_dev.comp array to list") was to create a list to track all the
MDP components that needed to have their clocks enabled/disabled when
calling mtk_mdp_comp_clock_on/off. However, there was a bug inside
mtk_mdp_register_component where the args to a call to list_add were
swapped. The result is that only one component was added to
mtk_mdp_dev.comp_list because comp_list was instead being
repeatedly added to the single element lists headed by each
mtk_mdp_comp.

The order of the args to list_add in mtk_mdp_register_component was
fixed in https://patchwork.kernel.org/patch/11742895/ (Fix Null pointer
dereference when calling list_add).

Then, as a result of https://patchwork.kernel.org/patch/11530769/
(mtk-mdp: use pm_runtime in MDP component driver) if all the components
are added to the component list, the mdp "master" / rdma0 component
ends up having pm_runtime_get_sync() called on it twice recursively:

    rpm_resume+0x694/0x8f8
    __pm_runtime_resume+0x7c/0xa0 ***NOTE***
    mtk_mdp_comp_clock_on+0x48/0x104 [mtk_mdp]
    mtk_mdp_pm_resume+0x2c/0x44 [mtk_mdp]
    pm_generic_runtime_resume+0x34/0x48
    __genpd_runtime_resume+0x6c/0x80
    genpd_runtime_resume+0x104/0x1ac
    __rpm_callback+0x120/0x238
    rpm_callback+0x34/0x8c
    rpm_resume+0x7a0/0x8f8
    __pm_runtime_resume+0x7c/0xa0 ***NOTE***
    mtk_mdp_m2m_start_streaming+0x2c/0x3c [mtk_mdp]

(The calls to pm_runtime_get_sync are inlined and correspond to the
calls to __pm_runtime_resume). It is not correct to have
pm_runtime_get_sync called recursively and the second call will block
indefinitely.

As a result of all that, this change factors mtk_mdp_comp_clock_on/off
into mtk_mdp_comp_power_on/off and moves the calls to
pm_runtime_get/put into the power_on/off functions.

This change then special-cases the master/rdma0 MDP component and does
these things:
- the master/rdma0 component is not added to mtk_mdp_dev.comp_list
- the master/rdma0 component has its clocks (*but not power*) toggled
  by mtk_mpd_comp_clock_on/off inside mtk_mdp_clock_on/off.
- the other components have their clocks *and* power toggled with
  mtk_mdp_comp_power_on/off.

This change introduces the assumption that mtk_mdp_pm_resume will
always be called though a callback from pm_runtime_get_sync made on the
master / rdma0 component.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---

(no changes since v1)

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 57 ++++++++++++++---
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  5 +-
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++++++++++-----
 3 files changed, 98 insertions(+), 26 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 472c261b01e8..7b6c8a3f3455 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -51,9 +51,9 @@ static const struct of_device_id mtk_mdp_comp_driver_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_mdp_comp_driver_dt_match);
 
-int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
+int mtk_mdp_comp_power_on(struct mtk_mdp_comp *comp)
 {
-	int i, err, status;
+	int status, err;
 
 	if (comp->larb_dev) {
 		err = mtk_smi_larb_get(comp->larb_dev);
@@ -63,12 +63,54 @@ int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
 
 	err = pm_runtime_get_sync(comp->dev);
 	if (err < 0) {
-		dev_err(comp->dev,
-			"failed to runtime get, err %d.\n",
-			err);
+		dev_err(comp->dev, "failed to runtime get, err %d.\n", err);
 		return err;
 	}
 
+	err = mtk_mdp_comp_clock_on(comp);
+	if (err) {
+		dev_err(comp->dev, "failed to turn on clock. err=%d", err);
+		status = err;
+		goto err_mtk_mdp_comp_clock_on;
+	}
+
+	return 0;
+
+err_mtk_mdp_comp_clock_on:
+	err = pm_runtime_put_sync(comp->dev);
+	if (err)
+		dev_err(comp->dev, "failed to runtime put in cleanup. err=%d", err);
+
+	return status;
+}
+
+int mtk_mdp_comp_power_off(struct mtk_mdp_comp *comp)
+{
+	int status, err;
+
+	mtk_mdp_comp_clock_off(comp);
+
+	err = pm_runtime_put_sync(comp->dev);
+	if (err < 0) {
+		dev_err(comp->dev, "failed to runtime put, err %d.\n", err);
+		status = err;
+		goto err_pm_runtime_put_sync;
+	}
+
+	return 0;
+
+err_pm_runtime_put_sync:
+	err = mtk_mdp_comp_clock_on(comp);
+	if (err)
+		dev_err(comp->dev, "failed to turn on clock in cleanup. err=%d", err);
+
+	return status;
+}
+
+int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
+{
+	int i, err, status;
+
 	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
 		if (IS_ERR(comp->clk[i]))
 			continue;
@@ -94,7 +136,8 @@ int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
 	return status;
 }
 
-int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
+
+void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
 {
 	int i;
 
@@ -106,8 +149,6 @@ int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
 
 	if (comp->larb_dev)
 		mtk_smi_larb_put(comp->larb_dev);
-
-	return pm_runtime_put_sync(comp->dev);
 }
 
 static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
index f2e22e7e7c45..e3d6aef52869 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -23,8 +23,11 @@ struct mtk_mdp_comp {
 
 int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev);
 
+int mtk_mdp_comp_power_on(struct mtk_mdp_comp *comp);
+int mtk_mdp_comp_power_off(struct mtk_mdp_comp *comp);
+
 int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp);
-int mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp);
+void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp);
 
 extern struct platform_driver mtk_mdp_component_driver;
 
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index 714154450981..a72a9ba41ea6 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -57,29 +57,64 @@ static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
 	struct device *dev = &mdp->pdev->dev;
 	int err;
 
+	/*
+	 * The master / rdma0 component will have pm_runtime_get_sync called
+	 * on it through mtk_mdp_m2m_start_streaming, making it unnecessary to
+	 * have mtk_mdp_comp_power_on called on it.
+	 */
+	err = mtk_mdp_comp_clock_on(&mdp->comp_self);
+	if (err)
+		return err;
+
 	list_for_each_entry(comp_node, &mdp->comp_list, node) {
-		err = mtk_mdp_comp_clock_on(comp_node);
+		err = mtk_mdp_comp_power_on(comp_node);
 		if (err) {
 			status = err;
-			goto err_mtk_mdp_comp_clock_on;
+			goto err_mtk_mdp_comp_power_on;
 		}
 	}
 
 	return 0;
 
-err_mtk_mdp_comp_clock_on:
-	list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node)
-		mtk_mdp_comp_clock_off(comp_node);
-
+err_mtk_mdp_comp_power_on:
+	list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node) {
+		err = mtk_mdp_comp_power_off(comp_node);
+		if (err)
+			dev_err(&mdp->pdev->dev, "failed to power off after error. err=%d", err);
+	}
 	return status;
 }
 
-static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
+static int mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
 {
 	struct mtk_mdp_comp *comp_node;
+	int status, err;
+
+	list_for_each_entry(comp_node, &mdp->comp_list, node) {
+		err = mtk_mdp_comp_power_off(comp_node);
+		if (err) {
+			status = err;
+			goto err_mtk_mdp_comp_power_off;
+		}
+	}
 
-	list_for_each_entry(comp_node, &mdp->comp_list, node)
-		mtk_mdp_comp_clock_off(comp_node);
+	/*
+	 * The master / rdma0 component will have pm_runtime_put called
+	 * on it through mtk_mdp_m2m_stop_streaming, making it unnecessary to
+	 * have mtk_mdp_comp_power_off called on it.
+	 */
+	mtk_mdp_comp_clock_off(&mdp->comp_self);
+
+	return 0;
+
+err_mtk_mdp_comp_power_off:
+	list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node) {
+		err = mtk_mdp_comp_power_on(comp_node);
+		if (err)
+			dev_err(&mdp->pdev->dev, "failed to power on after error. err=%d", err);
+	}
+
+	return status;
 }
 
 static void mtk_mdp_wdt_worker(struct work_struct *work)
@@ -118,8 +153,6 @@ static int mtk_mdp_master_bind(struct device *dev)
 	int status;
 	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
 
-	mtk_mdp_register_component(mdp, &mdp->comp_self);
-
 	status = component_bind_all(dev, mdp);
 	if (status) {
 		dev_err(dev, "Failed to bind all components: %d\n", status);
@@ -140,8 +173,6 @@ static int mtk_mdp_master_bind(struct device *dev)
 	component_unbind_all(dev, mdp);
 
 err_component_bind_all:
-	mtk_mdp_unregister_component(mdp, &mdp->comp_self);
-
 	return status;
 }
 
@@ -152,7 +183,6 @@ static void mtk_mdp_master_unbind(struct device *dev)
 	pm_runtime_disable(dev);
 	mtk_mdp_unregister_m2m_device(mdp);
 	component_unbind_all(dev, mdp);
-	mtk_mdp_unregister_component(mdp, &mdp->comp_self);
 }
 
 static const struct component_master_ops mtk_mdp_com_ops = {
@@ -337,9 +367,7 @@ static int __maybe_unused mtk_mdp_pm_suspend(struct device *dev)
 {
 	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
 
-	mtk_mdp_clock_off(mdp);
-
-	return 0;
+	return mtk_mdp_clock_off(mdp);
 }
 
 static int __maybe_unused mtk_mdp_pm_resume(struct device *dev)
-- 
2.33.0.rc2.250.ged5fa647cd-goog

[PATCH v7 5/7] mtk-mdp: make mdp driver to be loadable by platform_device_register*()

[Eizan Miyamoto]

Rather than hanging the MDP master component driver off of the rdma0
device, make it possible too create a "virtual" device by registering
it with platform_device_register_*() to be probed by the mtk_mdp_core
driver.

Broadly, three interdependent things are done by this change:
- Make it is possible to search for MDP devices in the device tree
  through the grandparent device's of_node.
- v4l-related setup is moved into from the mtk_mdp_core driver to the
  mtk_mdp_comp driver.
- Presence of a mediatek,vpu property in an MDP component device node
  is used to determine what device to use when dispatching DMA ops from
  the relevant ioctl, and also do V4L2 initialization in this case.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---

Changes in v7:
- Drop last two changes from this series related to removing the vpu
  property from the mdp_rdma0 device node

Changes in v6:
- Don't propagate errors from clock_on/off as an afterthought.
- Split apart modifying mdp driver to be loadable from mmsys from
  actually loading it from mmsys into two changs to make review easier.
- Update devicetree bindings to reflect no longer needing the
  mediatek,vpu property in the mdp_rdma0 device node.
- Some stylistic cleanups.

Changes in v5:
- rebase and test on 5.13-next @ e2f74b13dbe6

Changes in v4:
- rebase and test on 5.13
- don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873

Changes in v3:
- get mdp master from aliases instead of strcmp against of_node->name

Changes in v2:
- rebased onto Linux 5.12
- 100 char line length allowance was utilized in a few places
- Removal of a redundant dev_err() print at the end of
  mtk_mdp_comp_init()
- Instead of printing errors and ignoring them, I've added a patch to
  correctly propagate them.
- Use of C style comments.
- Three additional patches were added to eliminate dependency on the
  mediatek,vpu property inside the mdp_rdma0 device node.

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 51 ++++++++++-----
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++-------------
 drivers/media/platform/mtk-mdp/mtk_mdp_core.h |  2 +
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  4 +-
 4 files changed, 60 insertions(+), 59 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 7b6c8a3f3455..85ef274841a3 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -155,8 +155,45 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
 {
 	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
 	struct mtk_mdp_dev *mdp = data;
+	struct device_node *vpu_node;
 
 	mtk_mdp_register_component(mdp, comp);
+
+	/*
+	 * If this component has a "mediatek-vpu" property, it is responsible for
+	 * notifying the mdp master driver about it so it can be further initialized
+	 * later.
+	 */
+	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
+	if (vpu_node) {
+		int ret;
+
+		mdp->vpu_dev = of_find_device_by_node(vpu_node);
+		if (WARN_ON(!mdp->vpu_dev)) {
+			dev_err(dev, "vpu pdev failed\n");
+			of_node_put(vpu_node);
+		}
+
+		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
+		if (ret) {
+			dev_err(dev, "Failed to register v4l2 device\n");
+			return -EINVAL;
+		}
+
+		ret = vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
+		if (ret) {
+			dev_err(dev, "Failed to set vb2 dma mag seg size\n");
+			return -EINVAL;
+		}
+
+		/*
+		 * presence of the "mediatek,vpu" property in a device node
+		 * indicates that it is the primary MDP rdma device and MDP DMA
+		 * ops should be handled by its DMA callbacks.
+		 */
+		mdp->rdma_dev = dev;
+	}
+
 	pm_runtime_enable(dev);
 
 	return 0;
@@ -237,23 +274,9 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev)
 static int mtk_mdp_comp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *vpu_node;
 	int status;
 	struct mtk_mdp_comp *comp;
 
-	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
-	if (vpu_node) {
-		of_node_put(vpu_node);
-		/*
-		 * The device tree node with a mediatek,vpu property is deemed
-		 * the MDP "master" device, we don't want to add a component
-		 * for it in this function because the initialization for the
-		 * master is done elsewhere.
-		 */
-		dev_info(dev, "vpu node found, not probing\n");
-		return -ENODEV;
-	}
-
 	comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
 	if (!comp)
 		return -ENOMEM;
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index a72a9ba41ea6..50eafcc9993d 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -159,6 +159,17 @@ static int mtk_mdp_master_bind(struct device *dev)
 		goto err_component_bind_all;
 	}
 
+	if (mdp->vpu_dev) {
+		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
+					  VPU_RST_MDP);
+		if (ret) {
+			dev_err(dev, "Failed to register reset handler\n");
+			goto err_wdt_reg;
+		}
+	} else {
+		dev_err(dev, "no vpu_dev found\n");
+	}
+
 	status = mtk_mdp_register_m2m_device(mdp);
 	if (status) {
 		dev_err(dev, "Failed to register m2m device: %d\n", status);
@@ -170,6 +181,8 @@ static int mtk_mdp_master_bind(struct device *dev)
 	return 0;
 
 err_mtk_mdp_register_m2m_device:
+
+err_wdt_reg:
 	component_unbind_all(dev, mdp);
 
 err_component_bind_all:
@@ -228,8 +241,13 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 		of_node_put(node);
 		parent = dev->of_node;
 		dev_warn(dev, "device tree is out of date\n");
-	} else {
+	} else if (dev->of_node) {
 		parent = dev->of_node->parent;
+	} else if (dev->parent) {
+		/* maybe we were created from a call to platform_device_register_data() */
+		parent = dev->parent->parent->of_node;
+	} else {
+		return -ENODEV;
 	}
 
 	/* Iterate over sibling MDP function blocks */
@@ -262,16 +280,6 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 		}
 	}
 
-	/*
-	 * Create a component for myself so that clocks can be toggled in
-	 * clock_on().
-	 */
-	ret = mtk_mdp_comp_init(&mdp->comp_self, dev);
-	if (ret) {
-		dev_err(dev, "Failed to initialize component\n");
-		goto err_comp;
-	}
-
 	mdp->job_wq = create_singlethread_workqueue(MTK_MDP_MODULE_NAME);
 	if (!mdp->job_wq) {
 		dev_err(&pdev->dev, "unable to alloc job workqueue\n");
@@ -287,29 +295,8 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 	}
 	INIT_WORK(&mdp->wdt_work, mtk_mdp_wdt_worker);
 
-	ret = v4l2_device_register(dev, &mdp->v4l2_dev);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register v4l2 device\n");
-		ret = -EINVAL;
-		goto err_dev_register;
-	}
-
-	mdp->vpu_dev = vpu_get_plat_device(pdev);
-	ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
-				  VPU_RST_MDP);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to register reset handler\n");
-		goto err_wdt_reg;
-	}
-
 	platform_set_drvdata(pdev, mdp);
 
-	ret = vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to set vb2 dma mag seg size\n");
-		goto err_set_max_seg_size;
-	}
-
 	ret = component_master_add_with_match(dev, &mtk_mdp_com_ops, match);
 	if (ret) {
 		dev_err(dev, "Component master add failed\n");
@@ -321,22 +308,12 @@ static int mtk_mdp_probe(struct platform_device *pdev)
 	return 0;
 
 err_component_master_add:
-	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
-
-err_set_max_seg_size:
-
-err_wdt_reg:
-	v4l2_device_unregister(&mdp->v4l2_dev);
-
-err_dev_register:
 	destroy_workqueue(mdp->wdt_wq);
 
 err_alloc_wdt_wq:
 	destroy_workqueue(mdp->job_wq);
 
 err_alloc_job_wq:
-
-err_comp:
 	dev_dbg(dev, "err %d\n", ret);
 	return ret;
 }
@@ -404,7 +381,6 @@ static struct platform_driver mtk_mdp_driver = {
 	.driver = {
 		.name	= MTK_MDP_MODULE_NAME,
 		.pm	= &mtk_mdp_pm_ops,
-		.of_match_table = mtk_mdp_of_ids,
 	}
 };
 
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
index 8a52539b15d4..9fcd8b8e7c25 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
@@ -133,6 +133,7 @@ struct mtk_mdp_variant {
  * struct mtk_mdp_dev - abstraction for image processor entity
  * @lock:	the mutex protecting this data structure
  * @vpulock:	the mutex protecting the communication with VPU
+ * @rdma_dev:  device pointer to rdma device for MDP
  * @pdev:	pointer to the image processor platform device
  * @variant:	the IP variant information
  * @id:		image processor device index (0..MTK_MDP_MAX_DEVS)
@@ -151,6 +152,7 @@ struct mtk_mdp_variant {
 struct mtk_mdp_dev {
 	struct mutex			lock;
 	struct mutex			vpulock;
+	struct device			*rdma_dev;
 	struct platform_device		*pdev;
 	struct mtk_mdp_variant		*variant;
 	u16				id;
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
index f14779e7596e..9834d3bbe851 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
@@ -929,7 +929,7 @@ static int mtk_mdp_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->mem_ops = &vb2_dma_contig_memops;
 	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
-	src_vq->dev = &ctx->mdp_dev->pdev->dev;
+	src_vq->dev = ctx->mdp_dev->rdma_dev;
 	src_vq->lock = &ctx->mdp_dev->lock;
 
 	ret = vb2_queue_init(src_vq);
@@ -944,7 +944,7 @@ static int mtk_mdp_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->mem_ops = &vb2_dma_contig_memops;
 	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
 	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
-	dst_vq->dev = &ctx->mdp_dev->pdev->dev;
+	dst_vq->dev = ctx->mdp_dev->rdma_dev;
 	dst_vq->lock = &ctx->mdp_dev->lock;
 
 	return vb2_queue_init(dst_vq);
-- 
2.33.0.rc2.250.ged5fa647cd-goog

Re: [PATCH v7 5/7] mtk-mdp: make mdp driver to be loadable by platform_device_register*()

[houlong wei]

Hi Eizan,
I have inline comment below.

Regards,
Houlong

On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote:
> Rather than hanging the MDP master component driver off of the rdma0
> device, make it possible too create a "virtual" device by registering
> it with platform_device_register_*() to be probed by the mtk_mdp_core
> driver.
> 
> Broadly, three interdependent things are done by this change:
> - Make it is possible to search for MDP devices in the device tree
>   through the grandparent device's of_node.
> - v4l-related setup is moved into from the mtk_mdp_core driver to the
>   mtk_mdp_comp driver.
> - Presence of a mediatek,vpu property in an MDP component device node
>   is used to determine what device to use when dispatching DMA ops from
>   the relevant ioctl, and also do V4L2 initialization in this case.
> 
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
> ---
> 
> Changes in v7:
> - Drop last two changes from this series related to removing the vpu
>   property from the mdp_rdma0 device node
> 
> Changes in v6:
> - Don't propagate errors from clock_on/off as an afterthought.
> - Split apart modifying mdp driver to be loadable from mmsys from
>   actually loading it from mmsys into two changs to make review easier.
> - Update devicetree bindings to reflect no longer needing the
>   mediatek,vpu property in the mdp_rdma0 device node.
> - Some stylistic cleanups.
> 
> Changes in v5:
> - rebase and test on 5.13-next @ e2f74b13dbe6
> 
> Changes in v4:
> - rebase and test on 5.13
> - don't depend on https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873
> 
> Changes in v3:
> - get mdp master from aliases instead of strcmp against of_node->name
> 
> Changes in v2:
> - rebased onto Linux 5.12
> - 100 char line length allowance was utilized in a few places
> - Removal of a redundant dev_err() print at the end of
>   mtk_mdp_comp_init()
> - Instead of printing errors and ignoring them, I've added a patch to
>   correctly propagate them.
> - Use of C style comments.
> - Three additional patches were added to eliminate dependency on the
>   mediatek,vpu property inside the mdp_rdma0 device node.
> 
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 51 ++++++++++-----
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 62 ++++++-------------
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.h |  2 +
>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  4 +-
>  4 files changed, 60 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index 7b6c8a3f3455..85ef274841a3 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -155,8 +155,45 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
>  {
>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
>  	struct mtk_mdp_dev *mdp = data;
> +	struct device_node *vpu_node;
>  
>  	mtk_mdp_register_component(mdp, comp);
> +
> +	/*
> +	 * If this component has a "mediatek-vpu" property, it is responsible for
> +	 * notifying the mdp master driver about it so it can be further initialized
> +	 * later.
> +	 */
> +	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
> +	if (vpu_node) {
> +		int ret;
> +
> +		mdp->vpu_dev = of_find_device_by_node(vpu_node);
> +		if (WARN_ON(!mdp->vpu_dev)) {
> +			dev_err(dev, "vpu pdev failed\n");
> +			of_node_put(vpu_node);
> +		}

The 'vpu_node' should be put when 'mdp->vpu_dev' is valid.

		mdp->vpu_dev = of_find_device_by_node(vpu_node);
		of_node_put(vpu_node);
		if (WARN_ON(!mdp->vpu_dev))
			dev_err(dev, "vpu pdev failed\n");

I commented it before, refer to
http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028370.html

> +		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> +		if (ret) {
> +			dev_err(dev, "Failed to register v4l2 device\n");
> +			return -EINVAL;
> +		}
> +
> +		ret = vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
> +		if (ret) {
> +			dev_err(dev, "Failed to set vb2 dma mag seg size\n");
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * presence of the "mediatek,vpu" property in a device node
> +		 * indicates that it is the primary MDP rdma device and MDP DMA
> +		 * ops should be handled by its DMA callbacks.
> +		 */
> +		mdp->rdma_dev = dev;
> +	}
> +
>  	pm_runtime_enable(dev);
>  
>  	return 0;
> @@ -237,23 +274,9 @@ int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev)
>  static int mtk_mdp_comp_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> -	struct device_node *vpu_node;
>  	int status;
>  	struct mtk_mdp_comp *comp;
>  
> -	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
> -	if (vpu_node) {
> -		of_node_put(vpu_node);
> -		/*
> -		 * The device tree node with a mediatek,vpu property is deemed
> -		 * the MDP "master" device, we don't want to add a component
> -		 * for it in this function because the initialization for the
> -		 * master is done elsewhere.
> -		 */
> -		dev_info(dev, "vpu node found, not probing\n");
> -		return -ENODEV;
> -	}
> -
>  	comp = devm_kzalloc(dev, sizeof(*comp), GFP_KERNEL);
>  	if (!comp)
>  		return -ENOMEM;
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> index a72a9ba41ea6..50eafcc9993d 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -159,6 +159,17 @@ static int mtk_mdp_master_bind(struct device *dev)
>  		goto err_component_bind_all;
>  	}
>  
> +	if (mdp->vpu_dev) {
> +		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
> +					  VPU_RST_MDP);
> +		if (ret) {
> +			dev_err(dev, "Failed to register reset handler\n");
> +			goto err_wdt_reg;
> +		}
> +	} else {
> +		dev_err(dev, "no vpu_dev found\n");
> +	}
> +
>  	status = mtk_mdp_register_m2m_device(mdp);
>  	if (status) {
>  		dev_err(dev, "Failed to register m2m device: %d\n", status);
> @@ -170,6 +181,8 @@ static int mtk_mdp_master_bind(struct device *dev)
>  	return 0;
>  
>  err_mtk_mdp_register_m2m_device:
> +
> +err_wdt_reg:
>  	component_unbind_all(dev, mdp);
>  
>  err_component_bind_all:
> @@ -228,8 +241,13 @@ static int mtk_mdp_probe(struct platform_device *pdev)
>  		of_node_put(node);
>  		parent = dev->of_node;
>  		dev_warn(dev, "device tree is out of date\n");
> -	} else {
> +	} else if (dev->of_node) {
>  		parent = dev->of_node->parent;
> +	} else if (dev->parent) {
> +		/* maybe we were created from a call to platform_device_register_data() */
> +		parent = dev->parent->parent->of_node;
> +	} else {
> +		return -ENODEV;
>  	}
>  
>  	/* Iterate over sibling MDP function blocks */
> @@ -262,16 +280,6 @@ static int mtk_mdp_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	/*
> -	 * Create a component for myself so that clocks can be toggled in
> -	 * clock_on().
> -	 */
> -	ret = mtk_mdp_comp_init(&mdp->comp_self, dev);
> -	if (ret) {
> -		dev_err(dev, "Failed to initialize component\n");
> -		goto err_comp;
> -	}
> -
>  	mdp->job_wq = create_singlethread_workqueue(MTK_MDP_MODULE_NAME);
>  	if (!mdp->job_wq) {
>  		dev_err(&pdev->dev, "unable to alloc job workqueue\n");
> @@ -287,29 +295,8 @@ static int mtk_mdp_probe(struct platform_device *pdev)
>  	}
>  	INIT_WORK(&mdp->wdt_work, mtk_mdp_wdt_worker);
>  
> -	ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to register v4l2 device\n");
> -		ret = -EINVAL;
> -		goto err_dev_register;
> -	}
> -
> -	mdp->vpu_dev = vpu_get_plat_device(pdev);
> -	ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
> -				  VPU_RST_MDP);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to register reset handler\n");
> -		goto err_wdt_reg;
> -	}
> -
>  	platform_set_drvdata(pdev, mdp);
>  
> -	ret = vb2_dma_contig_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to set vb2 dma mag seg size\n");
> -		goto err_set_max_seg_size;
> -	}
> -
>  	ret = component_master_add_with_match(dev, &mtk_mdp_com_ops, match);
>  	if (ret) {
>  		dev_err(dev, "Component master add failed\n");
> @@ -321,22 +308,12 @@ static int mtk_mdp_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_component_master_add:
> -	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> -
> -err_set_max_seg_size:
> -
> -err_wdt_reg:
> -	v4l2_device_unregister(&mdp->v4l2_dev);
> -
> -err_dev_register:
>  	destroy_workqueue(mdp->wdt_wq);
>  
>  err_alloc_wdt_wq:
>  	destroy_workqueue(mdp->job_wq);
>  
>  err_alloc_job_wq:
> -
> -err_comp:
>  	dev_dbg(dev, "err %d\n", ret);
>  	return ret;
>  }
> @@ -404,7 +381,6 @@ static struct platform_driver mtk_mdp_driver = {
>  	.driver = {
>  		.name	= MTK_MDP_MODULE_NAME,
>  		.pm	= &mtk_mdp_pm_ops,
> -		.of_match_table = mtk_mdp_of_ids,
>  	}
>  };
>  
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> index 8a52539b15d4..9fcd8b8e7c25 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> @@ -133,6 +133,7 @@ struct mtk_mdp_variant {
>   * struct mtk_mdp_dev - abstraction for image processor entity
>   * @lock:	the mutex protecting this data structure
>   * @vpulock:	the mutex protecting the communication with VPU
> + * @rdma_dev:  device pointer to rdma device for MDP
>   * @pdev:	pointer to the image processor platform device
>   * @variant:	the IP variant information
>   * @id:		image processor device index (0..MTK_MDP_MAX_DEVS)
> @@ -151,6 +152,7 @@ struct mtk_mdp_variant {
>  struct mtk_mdp_dev {
>  	struct mutex			lock;
>  	struct mutex			vpulock;
> +	struct device			*rdma_dev;
>  	struct platform_device		*pdev;
>  	struct mtk_mdp_variant		*variant;
>  	u16				id;
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index f14779e7596e..9834d3bbe851 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -929,7 +929,7 @@ static int mtk_mdp_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
>  	src_vq->mem_ops = &vb2_dma_contig_memops;
>  	src_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
>  	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> -	src_vq->dev = &ctx->mdp_dev->pdev->dev;
> +	src_vq->dev = ctx->mdp_dev->rdma_dev;
>  	src_vq->lock = &ctx->mdp_dev->lock;
>  
>  	ret = vb2_queue_init(src_vq);
> @@ -944,7 +944,7 @@ static int mtk_mdp_m2m_queue_init(void *priv, struct vb2_queue *src_vq,
>  	dst_vq->mem_ops = &vb2_dma_contig_memops;
>  	dst_vq->buf_struct_size = sizeof(struct v4l2_m2m_buffer);
>  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> -	dst_vq->dev = &ctx->mdp_dev->pdev->dev;
> +	dst_vq->dev = ctx->mdp_dev->rdma_dev;
>  	dst_vq->lock = &ctx->mdp_dev->lock;
>  
>  	return vb2_queue_init(dst_vq);

[PATCH v7 6/7] soc: mediatek: mmsys: instantiate mdp virtual device from mmsys

[Eizan Miyamoto]

A virtual device that is probed by the mtk_mdp_core driver is
instantiated by the mtk_mmsys driver.

This better reflects the logical organization of the hardware and
driver: there are a number of hardware blocks that are used by the MDP
that have no strict hierarchy, and the software driver is responsible
for driving them properly.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Reviewed-by: Houlong Wei <houlong.wei@mediatek.com>
---

(no changes since v1)

 drivers/soc/mediatek/mtk-mmsys.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
index 080660ef11bf..e681029fe804 100644
--- a/drivers/soc/mediatek/mtk-mmsys.c
+++ b/drivers/soc/mediatek/mtk-mmsys.c
@@ -97,6 +97,7 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
 	struct platform_device *clks;
 	struct platform_device *drm;
 	struct mtk_mmsys *mmsys;
+	struct platform_device *mdp;
 	int ret;
 
 	mmsys = devm_kzalloc(dev, sizeof(*mmsys), GFP_KERNEL);
@@ -122,10 +123,27 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
 					    PLATFORM_DEVID_AUTO, NULL, 0);
 	if (IS_ERR(drm)) {
 		platform_device_unregister(clks);
-		return PTR_ERR(drm);
+		ret = PTR_ERR(drm);
+		goto err_drm;
+	}
+
+	mdp = platform_device_register_data(&pdev->dev, "mtk-mdp",
+					    PLATFORM_DEVID_AUTO, NULL, 0);
+	if (IS_ERR(mdp)) {
+		ret = PTR_ERR(mdp);
+		dev_err(dev, "Failed to register mdp: %d\n", ret);
+		goto err_mdp;
 	}
 
 	return 0;
+
+err_mdp:
+	platform_device_unregister(drm);
+
+err_drm:
+	platform_device_unregister(clks);
+
+	return ret;
 }
 
 static const struct of_device_id of_match_mtk_mmsys[] = {
-- 
2.33.0.rc2.250.ged5fa647cd-goog

[PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

[Eizan Miyamoto]

... Instead of depending on the presence of a mediatek,vpu property in
the device node.

That property was originally added to link to the vpu node so that the
mtk_mdp_core driver could pass the right device to
vpu_wdt_reg_handler(). However in a previous patch in this series,
the driver has been modified to search the device tree for that node
instead.

That property was also used to indicate the primary MDP device, so that
it can be passed to the V4L2 subsystem as well as register it to be
used when setting up queues in the open() callback for the filesystem
device node that is created. In this case, assuming that the primary
MDP device is the one with a specific alias seems useable because the
alternative is to add a property to the device tree which doesn't
actually represent any facet of hardware (i.e., this being the primary
MDP device is a software decision). In other words, this solution is
equally as arbitrary, but at least it doesn't add a property to a
device node where said property is unrelated to the hardware present.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

(no changes since v1)

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
 2 files changed, 64 insertions(+), 28 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 85ef274841a3..9527649de98e 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
 		mtk_smi_larb_put(comp->larb_dev);
 }
 
-static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
+/*
+ * The MDP master device node is identified by the device tree alias
+ * "mdp-rdma0".
+ */
+static bool is_mdp_master(struct device *dev)
+{
+	struct device_node *aliases, *mdp_rdma0_node;
+	const char *mdp_rdma0_path;
+
+	if (!dev->of_node)
+		return false;
+
+	aliases = of_find_node_by_path("/aliases");
+	if (!aliases) {
+		dev_err(dev, "no aliases found for mdp-rdma0");
+		return false;
+	}
+
+	mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
+	if (!mdp_rdma0_path) {
+		dev_err(dev, "get mdp-rdma0 property of /aliases failed");
+		return false;
+	}
+
+	mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
+	if (!mdp_rdma0_node) {
+		dev_err(dev, "path resolution failed for %s", mdp_rdma0_path);
+		return false;
+	}
+
+	return dev->of_node == mdp_rdma0_node;
+}
+
+static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
+			void *data)
 {
 	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
 	struct mtk_mdp_dev *mdp = data;
-	struct device_node *vpu_node;
 
 	mtk_mdp_register_component(mdp, comp);
 
-	/*
-	 * If this component has a "mediatek-vpu" property, it is responsible for
-	 * notifying the mdp master driver about it so it can be further initialized
-	 * later.
-	 */
-	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
-	if (vpu_node) {
+	if (is_mdp_master(dev)) {
 		int ret;
 
-		mdp->vpu_dev = of_find_device_by_node(vpu_node);
-		if (WARN_ON(!mdp->vpu_dev)) {
-			dev_err(dev, "vpu pdev failed\n");
-			of_node_put(vpu_node);
-		}
-
 		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
 		if (ret) {
 			dev_err(dev, "Failed to register v4l2 device\n");
@@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
 		}
 
 		/*
-		 * presence of the "mediatek,vpu" property in a device node
-		 * indicates that it is the primary MDP rdma device and MDP DMA
-		 * ops should be handled by its DMA callbacks.
+		 * MDP DMA ops will be handled by the DMA callbacks associated with this
+		 * device;
 		 */
 		mdp->rdma_dev = dev;
 	}
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index 50eafcc9993d..6a775463399c 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -150,8 +150,9 @@ static void release_of(struct device *dev, void *data)
 
 static int mtk_mdp_master_bind(struct device *dev)
 {
-	int status;
 	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
+	struct device_node *vpu_node;
+	int status;
 
 	status = component_bind_all(dev, mdp);
 	if (status) {
@@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device *dev)
 		goto err_component_bind_all;
 	}
 
-	if (mdp->vpu_dev) {
-		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
-					  VPU_RST_MDP);
-		if (ret) {
-			dev_err(dev, "Failed to register reset handler\n");
-			goto err_wdt_reg;
-		}
-	} else {
-		dev_err(dev, "no vpu_dev found\n");
+	if (mdp->rdma_dev == NULL) {
+		dev_err(dev, "Primary MDP device not found");
+		status = -ENODEV;
+		goto err_component_bind_all;
+	}
+
+	vpu_node = of_find_node_by_name(NULL, "vpu");
+	if (!vpu_node) {
+		dev_err(dev, "unable to find vpu node");
+		status = -ENODEV;
+		goto err_wdt_reg;
+	}
+
+	mdp->vpu_dev = of_find_device_by_node(vpu_node);
+	if (!mdp->vpu_dev) {
+		dev_err(dev, "unable to find vpu device");
+		status = -ENODEV;
+		goto err_wdt_reg;
+	}
+
+	status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
+	if (status) {
+		dev_err(dev, "Failed to register reset handler\n");
+		goto err_wdt_reg;
 	}
 
 	status = mtk_mdp_register_m2m_device(mdp);
-- 
2.33.0.rc2.250.ged5fa647cd-goog

Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

[houlong wei]

Hi Eizan,

Thanks for you patch. I have inline comment below.

Regards,
Houlong

On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote:
> ... Instead of depending on the presence of a mediatek,vpu property in
> the device node.
> 
> That property was originally added to link to the vpu node so that the
> mtk_mdp_core driver could pass the right device to
> vpu_wdt_reg_handler(). However in a previous patch in this series,
> the driver has been modified to search the device tree for that node
> instead.
> 
> That property was also used to indicate the primary MDP device, so that
> it can be passed to the V4L2 subsystem as well as register it to be
> used when setting up queues in the open() callback for the filesystem
> device node that is created. In this case, assuming that the primary
> MDP device is the one with a specific alias seems useable because the
> alternative is to add a property to the device tree which doesn't
> actually represent any facet of hardware (i.e., this being the primary
> MDP device is a software decision). In other words, this solution is
> equally as arbitrary, but at least it doesn't add a property to a
> device node where said property is unrelated to the hardware present.
> 
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> 
> (no changes since v1)
> 
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
>  2 files changed, 64 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index 85ef274841a3..9527649de98e 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
>  		mtk_smi_larb_put(comp->larb_dev);
>  }
>  
> -static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
> +/*
> + * The MDP master device node is identified by the device tree alias
> + * "mdp-rdma0".
> + */
> +static bool is_mdp_master(struct device *dev)
> +{
> +	struct device_node *aliases, *mdp_rdma0_node;
> +	const char *mdp_rdma0_path;
> +
> +	if (!dev->of_node)
> +		return false;
> +
> +	aliases = of_find_node_by_path("/aliases");
> +	if (!aliases) {
> +		dev_err(dev, "no aliases found for mdp-rdma0");
> +		return false;
> +	}
> +
> +	mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
> +	if (!mdp_rdma0_path) {
> +		dev_err(dev, "get mdp-rdma0 property of /aliases failed");
> +		return false;
> +	}
> +
> +	mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
> +	if (!mdp_rdma0_node) {
> +		dev_err(dev, "path resolution failed for %s", mdp_rdma0_path);
> +		return false;
> +	}
> +
> +	return dev->of_node == mdp_rdma0_node;


About how to determine the master mdp driver, we also can
judge the component type. The component type can be gotten by calling
of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is
the master driver. No matter it is mdp_rdma0 or mdp_rdma1.
http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028533.html

IMO, judging it by component type is more flexible because it does not
limit to 'mdp_rdma0'.

> +}
> +
> +static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
> +			void *data)
>  {
>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
>  	struct mtk_mdp_dev *mdp = data;
> -	struct device_node *vpu_node;
>  
>  	mtk_mdp_register_component(mdp, comp);
>  
> -	/*
> -	 * If this component has a "mediatek-vpu" property, it is responsible for
> -	 * notifying the mdp master driver about it so it can be further initialized
> -	 * later.
> -	 */
> -	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
> -	if (vpu_node) {
> +	if (is_mdp_master(dev)) {
>  		int ret;
>  
> -		mdp->vpu_dev = of_find_device_by_node(vpu_node);
> -		if (WARN_ON(!mdp->vpu_dev)) {
> -			dev_err(dev, "vpu pdev failed\n");
> -			of_node_put(vpu_node);
> -		}
> -
>  		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
>  		if (ret) {
>  			dev_err(dev, "Failed to register v4l2 device\n");
> @@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
>  		}
>  
>  		/*
> -		 * presence of the "mediatek,vpu" property in a device node
> -		 * indicates that it is the primary MDP rdma device and MDP DMA
> -		 * ops should be handled by its DMA callbacks.
> +		 * MDP DMA ops will be handled by the DMA callbacks associated with this
> +		 * device;
>  		 */
>  		mdp->rdma_dev = dev;
>  	}
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> index 50eafcc9993d..6a775463399c 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -150,8 +150,9 @@ static void release_of(struct device *dev, void *data)
>  
>  static int mtk_mdp_master_bind(struct device *dev)
>  {
> -	int status;
>  	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
> +	struct device_node *vpu_node;
> +	int status;
>  
>  	status = component_bind_all(dev, mdp);
>  	if (status) {
> @@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device *dev)
>  		goto err_component_bind_all;
>  	}
>  
> -	if (mdp->vpu_dev) {
> -		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
> -					  VPU_RST_MDP);
> -		if (ret) {
> -			dev_err(dev, "Failed to register reset handler\n");
> -			goto err_wdt_reg;
> -		}
> -	} else {
> -		dev_err(dev, "no vpu_dev found\n");
> +	if (mdp->rdma_dev == NULL) {
> +		dev_err(dev, "Primary MDP device not found");
> +		status = -ENODEV;
> +		goto err_component_bind_all;
> +	}
> +
> +	vpu_node = of_find_node_by_name(NULL, "vpu");
> +	if (!vpu_node) {
> +		dev_err(dev, "unable to find vpu node");
> +		status = -ENODEV;
> +		goto err_wdt_reg;
> +	}
> +
> +	mdp->vpu_dev = of_find_device_by_node(vpu_node);

The 'vpu_node' should be put by calling 'of_node_put(vpu_node)' when it
is not used.

> +	if (!mdp->vpu_dev) {
> +		dev_err(dev, "unable to find vpu device");
> +		status = -ENODEV;
> +		goto err_wdt_reg;
> +	}
> +
> +	status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
> +	if (status) {
> +		dev_err(dev, "Failed to register reset handler\n");
> +		goto err_wdt_reg;
>  	}
>  
>  	status = mtk_mdp_register_m2m_device(mdp);

Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

[Enric Balletbo i Serra]

Hi Houlong,

Thank you for following up this patchset. I have some questions to try to
understand better the hardware though.

On 25/8/21 10:43, houlong wei wrote:
> Hi Eizan,
> 
> Thanks for you patch. I have inline comment below.
> 
> Regards,
> Houlong
> 
> On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote:
>> ... Instead of depending on the presence of a mediatek,vpu property in
>> the device node.
>>
>> That property was originally added to link to the vpu node so that the
>> mtk_mdp_core driver could pass the right device to
>> vpu_wdt_reg_handler(). However in a previous patch in this series,
>> the driver has been modified to search the device tree for that node
>> instead.
>>
>> That property was also used to indicate the primary MDP device, so that
>> it can be passed to the V4L2 subsystem as well as register it to be
>> used when setting up queues in the open() callback for the filesystem
>> device node that is created. In this case, assuming that the primary
>> MDP device is the one with a specific alias seems useable because the
>> alternative is to add a property to the device tree which doesn't
>> actually represent any facet of hardware (i.e., this being the primary
>> MDP device is a software decision). In other words, this solution is
>> equally as arbitrary, but at least it doesn't add a property to a
>> device node where said property is unrelated to the hardware present.
>>
>> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> (no changes since v1)
>>
>>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
>>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
>>  2 files changed, 64 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>> index 85ef274841a3..9527649de98e 100644
>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>> @@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
>>  		mtk_smi_larb_put(comp->larb_dev);
>>  }
>>  
>> -static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
>> +/*
>> + * The MDP master device node is identified by the device tree alias
>> + * "mdp-rdma0".
>> + */
>> +static bool is_mdp_master(struct device *dev)
>> +{
>> +	struct device_node *aliases, *mdp_rdma0_node;
>> +	const char *mdp_rdma0_path;
>> +
>> +	if (!dev->of_node)
>> +		return false;
>> +
>> +	aliases = of_find_node_by_path("/aliases");
>> +	if (!aliases) {
>> +		dev_err(dev, "no aliases found for mdp-rdma0");
>> +		return false;
>> +	}
>> +
>> +	mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
>> +	if (!mdp_rdma0_path) {
>> +		dev_err(dev, "get mdp-rdma0 property of /aliases failed");
>> +		return false;
>> +	}
>> +
>> +	mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
>> +	if (!mdp_rdma0_node) {
>> +		dev_err(dev, "path resolution failed for %s", mdp_rdma0_path);
>> +		return false;
>> +	}
>> +
>> +	return dev->of_node == mdp_rdma0_node;
> 
> 
> About how to determine the master mdp driver, we also can
> judge the component type. The component type can be gotten by calling
> of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is
> the master driver. No matter it is mdp_rdma0 or mdp_rdma1.


I'm confused, you mean that the component type, aka MTK_MDP_RDMA is only set for
mtk_mdp_rdma0? isn't mtk_dmp_rdma1 also a MTK_MDP_RDMA type? Because looks weird
to me that two rdma have diffent types.


> http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028533.html
> 
> IMO, judging it by component type is more flexible because it does not
> limit to 'mdp_rdma0'.
> 

Using an alias like Eizan did is also flexible, you only need to set mdp-rdma0
to point the mdp_rdma1 node.

What I am really interested to know is the differences between the platforms to
understand if this is really a platform hardware property or a software
configurable thing, so would help if you can give use the different use cases
for different SoCs.

For MT8173 which is the master device? is _always_ rdma0 or can also be rdma1?

Or maybe what is not clear here is what exactly means be a master device?

What about MT8183 and other SoCs?

Thanks,
  Enric


>> +}
>> +
>> +static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
>> +			void *data)
>>  {
>>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
>>  	struct mtk_mdp_dev *mdp = data;
>> -	struct device_node *vpu_node;
>>  
>>  	mtk_mdp_register_component(mdp, comp);
>>  
>> -	/*
>> -	 * If this component has a "mediatek-vpu" property, it is responsible for
>> -	 * notifying the mdp master driver about it so it can be further initialized
>> -	 * later.
>> -	 */
>> -	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
>> -	if (vpu_node) {
>> +	if (is_mdp_master(dev)) {
>>  		int ret;
>>  
>> -		mdp->vpu_dev = of_find_device_by_node(vpu_node);
>> -		if (WARN_ON(!mdp->vpu_dev)) {
>> -			dev_err(dev, "vpu pdev failed\n");
>> -			of_node_put(vpu_node);
>> -		}
>> -
>>  		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
>>  		if (ret) {
>>  			dev_err(dev, "Failed to register v4l2 device\n");
>> @@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
>>  		}
>>  
>>  		/*
>> -		 * presence of the "mediatek,vpu" property in a device node
>> -		 * indicates that it is the primary MDP rdma device and MDP DMA
>> -		 * ops should be handled by its DMA callbacks.
>> +		 * MDP DMA ops will be handled by the DMA callbacks associated with this
>> +		 * device;
>>  		 */
>>  		mdp->rdma_dev = dev;
>>  	}
>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>> index 50eafcc9993d..6a775463399c 100644
>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>> @@ -150,8 +150,9 @@ static void release_of(struct device *dev, void *data)
>>  
>>  static int mtk_mdp_master_bind(struct device *dev)
>>  {
>> -	int status;
>>  	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
>> +	struct device_node *vpu_node;
>> +	int status;
>>  
>>  	status = component_bind_all(dev, mdp);
>>  	if (status) {
>> @@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device *dev)
>>  		goto err_component_bind_all;
>>  	}
>>  
>> -	if (mdp->vpu_dev) {
>> -		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
>> -					  VPU_RST_MDP);
>> -		if (ret) {
>> -			dev_err(dev, "Failed to register reset handler\n");
>> -			goto err_wdt_reg;
>> -		}
>> -	} else {
>> -		dev_err(dev, "no vpu_dev found\n");
>> +	if (mdp->rdma_dev == NULL) {
>> +		dev_err(dev, "Primary MDP device not found");
>> +		status = -ENODEV;
>> +		goto err_component_bind_all;
>> +	}
>> +
>> +	vpu_node = of_find_node_by_name(NULL, "vpu");
>> +	if (!vpu_node) {
>> +		dev_err(dev, "unable to find vpu node");
>> +		status = -ENODEV;
>> +		goto err_wdt_reg;
>> +	}
>> +
>> +	mdp->vpu_dev = of_find_device_by_node(vpu_node);
> 
> The 'vpu_node' should be put by calling 'of_node_put(vpu_node)' when it
> is not used.
> 
>> +	if (!mdp->vpu_dev) {
>> +		dev_err(dev, "unable to find vpu device");
>> +		status = -ENODEV;
>> +		goto err_wdt_reg;
>> +	}
>> +
>> +	status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
>> +	if (status) {
>> +		dev_err(dev, "Failed to register reset handler\n");
>> +		goto err_wdt_reg;
>>  	}
>>  
>>  	status = mtk_mdp_register_m2m_device(mdp);
> 

Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

[houlong wei]

On Wed, 2021-08-25 at 17:07 +0800, Enric Balletbo i Serra wrote:
> Hi Houlong,
> 
> Thank you for following up this patchset. I have some questions to try to
> understand better the hardware though.
> 
> On 25/8/21 10:43, houlong wei wrote:
> > Hi Eizan,
> > 
> > Thanks for you patch. I have inline comment below.
> > 
> > Regards,
> > Houlong
> > 
> > On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote:
> >> ... Instead of depending on the presence of a mediatek,vpu property in
> >> the device node.
> >>
> >> That property was originally added to link to the vpu node so that the
> >> mtk_mdp_core driver could pass the right device to
> >> vpu_wdt_reg_handler(). However in a previous patch in this series,
> >> the driver has been modified to search the device tree for that node
> >> instead.
> >>
> >> That property was also used to indicate the primary MDP device, so that
> >> it can be passed to the V4L2 subsystem as well as register it to be
> >> used when setting up queues in the open() callback for the filesystem
> >> device node that is created. In this case, assuming that the primary
> >> MDP device is the one with a specific alias seems useable because the
> >> alternative is to add a property to the device tree which doesn't
> >> actually represent any facet of hardware (i.e., this being the primary
> >> MDP device is a software decision). In other words, this solution is
> >> equally as arbitrary, but at least it doesn't add a property to a
> >> device node where said property is unrelated to the hardware present.
> >>
> >> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> >> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >> ---
> >>
> >> (no changes since v1)
> >>
> >>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
> >>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
> >>  2 files changed, 64 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >> index 85ef274841a3..9527649de98e 100644
> >> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >> @@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
> >>  		mtk_smi_larb_put(comp->larb_dev);
> >>  }
> >>  
> >> -static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
> >> +/*
> >> + * The MDP master device node is identified by the device tree alias
> >> + * "mdp-rdma0".
> >> + */
> >> +static bool is_mdp_master(struct device *dev)
> >> +{
> >> +	struct device_node *aliases, *mdp_rdma0_node;
> >> +	const char *mdp_rdma0_path;
> >> +
> >> +	if (!dev->of_node)
> >> +		return false;
> >> +
> >> +	aliases = of_find_node_by_path("/aliases");
> >> +	if (!aliases) {
> >> +		dev_err(dev, "no aliases found for mdp-rdma0");
> >> +		return false;
> >> +	}
> >> +
> >> +	mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
> >> +	if (!mdp_rdma0_path) {
> >> +		dev_err(dev, "get mdp-rdma0 property of /aliases failed");
> >> +		return false;
> >> +	}
> >> +
> >> +	mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
> >> +	if (!mdp_rdma0_node) {
> >> +		dev_err(dev, "path resolution failed for %s", mdp_rdma0_path);
> >> +		return false;
> >> +	}
> >> +
> >> +	return dev->of_node == mdp_rdma0_node;
> > 
> > 
> > About how to determine the master mdp driver, we also can
> > judge the component type. The component type can be gotten by calling
> > of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is
> > the master driver. No matter it is mdp_rdma0 or mdp_rdma1.
> 
> 
> I'm confused, you mean that the component type, aka MTK_MDP_RDMA is only set for
> mtk_mdp_rdma0? isn't mtk_dmp_rdma1 also a MTK_MDP_RDMA type? Because looks weird
> to me that two rdma have diffent types.
> 
Hi Enric,

I can understand your doubts. For mtk-mdp dirver, it is really difficult
to understand the relationship among the components. Because the MDP
hardware path is configured and connected in the VPU firmware.
For MT8173, the component type of mtk_mdp_rdma0 and mtk_mdp_rdma1 are
both MTK_MDP_RDMA. Even though the mtk-mdp driver's component list
contains both of them, only one corresponding hardware component takes
effect during the processing of a frame, the VPU decides it.

Regards,
Houlong
> 
> > http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028533.html
> > 
> > IMO, judging it by component type is more flexible because it does not
> > limit to 'mdp_rdma0'.
> > 
> 
> Using an alias like Eizan did is also flexible, you only need to set mdp-rdma0
> to point the mdp_rdma1 node.
> 
> What I am really interested to know is the differences between the platforms to
> understand if this is really a platform hardware property or a software
> configurable thing, so would help if you can give use the different use cases
> for different SoCs.
> 
> For MT8173 which is the master device? is _always_ rdma0 or can also be rdma1?
> 
> Or maybe what is not clear here is what exactly means be a master device?
> 
> What about MT8183 and other SoCs?
> 
> Thanks,
>   Enric
> 
Hi Enric,

Before answer your questions, let's unify the concept of the master
device.If an MDP device-tree node can not only generate a /dev/video
device node, but also can generate a mtk component device node, then
this component device is the master device.
From the perspective of the device tree node, the compatible of a MDP
device tree node contains both "mediatek,mt8173-mdp-rdma" and
"mediatek,mt8173-mdp", then its corresponding mtk component device is
the master device.
Since this concept comes from Eizan's patches, if my understanding is
different from yours, please let me know, thanks.

If there is only one master device, I fully agree with you. For MT8173,
both mdp_rdma0 and mdp_rdma1 can be master device at the same time.
But now only mdp_rdma0 takes this role, perhaps because one MDP hardware
path could meet the project's the requirement six years ago.
In some project, we have two or more MDP hardware paths, we can
configure all the mdp_rdma components as master devices, and there will
be several /dev/video device nodes. The user can control them
concurrently.

Regards,
Houlong

> 
> >> +}
> >> +
> >> +static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
> >> +			void *data)
> >>  {
> >>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
> >>  	struct mtk_mdp_dev *mdp = data;
> >> -	struct device_node *vpu_node;
> >>  
> >>  	mtk_mdp_register_component(mdp, comp);
> >>  
> >> -	/*
> >> -	 * If this component has a "mediatek-vpu" property, it is responsible for
> >> -	 * notifying the mdp master driver about it so it can be further initialized
> >> -	 * later.
> >> -	 */
> >> -	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
> >> -	if (vpu_node) {
> >> +	if (is_mdp_master(dev)) {
> >>  		int ret;
> >>  
> >> -		mdp->vpu_dev = of_find_device_by_node(vpu_node);
> >> -		if (WARN_ON(!mdp->vpu_dev)) {
> >> -			dev_err(dev, "vpu pdev failed\n");
> >> -			of_node_put(vpu_node);
> >> -		}
> >> -
> >>  		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> >>  		if (ret) {
> >>  			dev_err(dev, "Failed to register v4l2 device\n");
> >> @@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
> >>  		}
> >>  
> >>  		/*
> >> -		 * presence of the "mediatek,vpu" property in a device node
> >> -		 * indicates that it is the primary MDP rdma device and MDP DMA
> >> -		 * ops should be handled by its DMA callbacks.
> >> +		 * MDP DMA ops will be handled by the DMA callbacks associated with this
> >> +		 * device;
> >>  		 */
> >>  		mdp->rdma_dev = dev;
> >>  	}
> >> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >> index 50eafcc9993d..6a775463399c 100644
> >> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >> @@ -150,8 +150,9 @@ static void release_of(struct device *dev, void *data)
> >>  
> >>  static int mtk_mdp_master_bind(struct device *dev)
> >>  {
> >> -	int status;
> >>  	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
> >> +	struct device_node *vpu_node;
> >> +	int status;
> >>  
> >>  	status = component_bind_all(dev, mdp);
> >>  	if (status) {
> >> @@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device *dev)
> >>  		goto err_component_bind_all;
> >>  	}
> >>  
> >> -	if (mdp->vpu_dev) {
> >> -		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
> >> -					  VPU_RST_MDP);
> >> -		if (ret) {
> >> -			dev_err(dev, "Failed to register reset handler\n");
> >> -			goto err_wdt_reg;
> >> -		}
> >> -	} else {
> >> -		dev_err(dev, "no vpu_dev found\n");
> >> +	if (mdp->rdma_dev == NULL) {
> >> +		dev_err(dev, "Primary MDP device not found");
> >> +		status = -ENODEV;
> >> +		goto err_component_bind_all;
> >> +	}
> >> +
> >> +	vpu_node = of_find_node_by_name(NULL, "vpu");
> >> +	if (!vpu_node) {
> >> +		dev_err(dev, "unable to find vpu node");
> >> +		status = -ENODEV;
> >> +		goto err_wdt_reg;
> >> +	}
> >> +
> >> +	mdp->vpu_dev = of_find_device_by_node(vpu_node);
> > 
> > The 'vpu_node' should be put by calling 'of_node_put(vpu_node)' when it
> > is not used.
> > 
> >> +	if (!mdp->vpu_dev) {
> >> +		dev_err(dev, "unable to find vpu device");
> >> +		status = -ENODEV;
> >> +		goto err_wdt_reg;
> >> +	}
> >> +
> >> +	status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
> >> +	if (status) {
> >> +		dev_err(dev, "Failed to register reset handler\n");
> >> +		goto err_wdt_reg;
> >>  	}
> >>  
> >>  	status = mtk_mdp_register_m2m_device(mdp);
> > 

Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

[Enric Balletbo i Serra]

Hi Houlong,

Thank you for your answer, some more questions below :-)

On 25/8/21 17:18, houlong wei wrote:
> On Wed, 2021-08-25 at 17:07 +0800, Enric Balletbo i Serra wrote:
>> Hi Houlong,
>>
>> Thank you for following up this patchset. I have some questions to try to
>> understand better the hardware though.
>>
>> On 25/8/21 10:43, houlong wei wrote:
>>> Hi Eizan,
>>>
>>> Thanks for you patch. I have inline comment below.
>>>
>>> Regards,
>>> Houlong
>>>
>>> On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote:
>>>> ... Instead of depending on the presence of a mediatek,vpu property in
>>>> the device node.
>>>>
>>>> That property was originally added to link to the vpu node so that the
>>>> mtk_mdp_core driver could pass the right device to
>>>> vpu_wdt_reg_handler(). However in a previous patch in this series,
>>>> the driver has been modified to search the device tree for that node
>>>> instead.
>>>>
>>>> That property was also used to indicate the primary MDP device, so that
>>>> it can be passed to the V4L2 subsystem as well as register it to be
>>>> used when setting up queues in the open() callback for the filesystem
>>>> device node that is created. In this case, assuming that the primary
>>>> MDP device is the one with a specific alias seems useable because the
>>>> alternative is to add a property to the device tree which doesn't
>>>> actually represent any facet of hardware (i.e., this being the primary
>>>> MDP device is a software decision). In other words, this solution is
>>>> equally as arbitrary, but at least it doesn't add a property to a
>>>> device node where said property is unrelated to the hardware present.
>>>>
>>>> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
>>>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>>>> ---
>>>>
>>>> (no changes since v1)
>>>>
>>>>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
>>>>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
>>>>  2 files changed, 64 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>>>> index 85ef274841a3..9527649de98e 100644
>>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>>>> @@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
>>>>  		mtk_smi_larb_put(comp->larb_dev);
>>>>  }
>>>>  
>>>> -static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
>>>> +/*
>>>> + * The MDP master device node is identified by the device tree alias
>>>> + * "mdp-rdma0".
>>>> + */
>>>> +static bool is_mdp_master(struct device *dev)
>>>> +{
>>>> +	struct device_node *aliases, *mdp_rdma0_node;
>>>> +	const char *mdp_rdma0_path;
>>>> +
>>>> +	if (!dev->of_node)
>>>> +		return false;
>>>> +
>>>> +	aliases = of_find_node_by_path("/aliases");
>>>> +	if (!aliases) {
>>>> +		dev_err(dev, "no aliases found for mdp-rdma0");
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
>>>> +	if (!mdp_rdma0_path) {
>>>> +		dev_err(dev, "get mdp-rdma0 property of /aliases failed");
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
>>>> +	if (!mdp_rdma0_node) {
>>>> +		dev_err(dev, "path resolution failed for %s", mdp_rdma0_path);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return dev->of_node == mdp_rdma0_node;
>>>
>>>
>>> About how to determine the master mdp driver, we also can
>>> judge the component type. The component type can be gotten by calling
>>> of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is
>>> the master driver. No matter it is mdp_rdma0 or mdp_rdma1.
>>
>>
>> I'm confused, you mean that the component type, aka MTK_MDP_RDMA is only set for
>> mtk_mdp_rdma0? isn't mtk_dmp_rdma1 also a MTK_MDP_RDMA type? Because looks weird
>> to me that two rdma have diffent types.
>>
> Hi Enric,
> 
> I can understand your doubts. For mtk-mdp dirver, it is really difficult
> to understand the relationship among the components. Because the MDP
> hardware path is configured and connected in the VPU firmware.

So is fixed by the VPU firmware and can't be changed via, i.e writing some
registers? Can't be platform data then?

If that's the case, can you draw the mdp hardware path for i.e MT8173? (so I
have a better understanding)


> For MT8173, the component type of mtk_mdp_rdma0 and mtk_mdp_rdma1 are
> both MTK_MDP_RDMA. Even though the mtk-mdp driver's component list
> contains both of them, only one corresponding hardware component takes
> effect during the processing of a frame, the VPU decides it.
> 

So IIUC the VPU firmware decides which rdma uses on every frame, right? From hw
point of view, why one of them need to be master?

> Regards,
> Houlong
>>
>>> http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028533.html
>>>
>>> IMO, judging it by component type is more flexible because it does not
>>> limit to 'mdp_rdma0'.
>>>
>>
>> Using an alias like Eizan did is also flexible, you only need to set mdp-rdma0
>> to point the mdp_rdma1 node.
>>
>> What I am really interested to know is the differences between the platforms to
>> understand if this is really a platform hardware property or a software
>> configurable thing, so would help if you can give use the different use cases
>> for different SoCs.
>>
>> For MT8173 which is the master device? is _always_ rdma0 or can also be rdma1?
>>
>> Or maybe what is not clear here is what exactly means be a master device?
>>
>> What about MT8183 and other SoCs?
>>
>> Thanks,
>>   Enric
>>
> Hi Enric,
> 
> Before answer your questions, let's unify the concept of the master
> device.If an MDP device-tree node can not only generate a /dev/video
> device node, but also can generate a mtk component device node, then
> this component device is the master device.

Sorry, what you mean with a mtk component device node?

> From the perspective of the device tree node, the compatible of a MDP
> device tree node contains both "mediatek,mt8173-mdp-rdma" and
> "mediatek,mt8173-mdp", then its corresponding mtk component device is
> the master device.

So that's what's wrong, and Eizan patches get rid of this. In this case you
can't use device-tree to specify such master device. Device-tree is only about
hardware description, can't be used, in this case, as entry point to instantiate
the mdp driver. Hence Eizan patches change this, and mdp is instantiated by the
mmsys driver. Similar to what we do for the drm driver.

For the two hw rdma blocks the compatible should be just
"mediatek,mt8173-mdp-rdma" in this case.

Thanks,
  Enric

> Since this concept comes from Eizan's patches, if my understanding is
> different from yours, please let me know, thanks.
> 
> If there is only one master device, I fully agree with you. For MT8173,
> both mdp_rdma0 and mdp_rdma1 can be master device at the same time.
> But now only mdp_rdma0 takes this role, perhaps because one MDP hardware
> path could meet the project's the requirement six years ago.
> In some project, we have two or more MDP hardware paths, we can
> configure all the mdp_rdma components as master devices, and there will
> be several /dev/video device nodes. The user can control them
> concurrently.
> 
> Regards,
> Houlong
> 
>>
>>>> +}
>>>> +
>>>> +static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
>>>> +			void *data)
>>>>  {
>>>>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
>>>>  	struct mtk_mdp_dev *mdp = data;
>>>> -	struct device_node *vpu_node;
>>>>  
>>>>  	mtk_mdp_register_component(mdp, comp);
>>>>  
>>>> -	/*
>>>> -	 * If this component has a "mediatek-vpu" property, it is responsible for
>>>> -	 * notifying the mdp master driver about it so it can be further initialized
>>>> -	 * later.
>>>> -	 */
>>>> -	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
>>>> -	if (vpu_node) {
>>>> +	if (is_mdp_master(dev)) {
>>>>  		int ret;
>>>>  
>>>> -		mdp->vpu_dev = of_find_device_by_node(vpu_node);
>>>> -		if (WARN_ON(!mdp->vpu_dev)) {
>>>> -			dev_err(dev, "vpu pdev failed\n");
>>>> -			of_node_put(vpu_node);
>>>> -		}
>>>> -
>>>>  		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
>>>>  		if (ret) {
>>>>  			dev_err(dev, "Failed to register v4l2 device\n");
>>>> @@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
>>>>  		}
>>>>  
>>>>  		/*
>>>> -		 * presence of the "mediatek,vpu" property in a device node
>>>> -		 * indicates that it is the primary MDP rdma device and MDP DMA
>>>> -		 * ops should be handled by its DMA callbacks.
>>>> +		 * MDP DMA ops will be handled by the DMA callbacks associated with this
>>>> +		 * device;
>>>>  		 */
>>>>  		mdp->rdma_dev = dev;
>>>>  	}
>>>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>>>> index 50eafcc9993d..6a775463399c 100644
>>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>>>> @@ -150,8 +150,9 @@ static void release_of(struct device *dev, void *data)
>>>>  
>>>>  static int mtk_mdp_master_bind(struct device *dev)
>>>>  {
>>>> -	int status;
>>>>  	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
>>>> +	struct device_node *vpu_node;
>>>> +	int status;
>>>>  
>>>>  	status = component_bind_all(dev, mdp);
>>>>  	if (status) {
>>>> @@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device *dev)
>>>>  		goto err_component_bind_all;
>>>>  	}
>>>>  
>>>> -	if (mdp->vpu_dev) {
>>>> -		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
>>>> -					  VPU_RST_MDP);
>>>> -		if (ret) {
>>>> -			dev_err(dev, "Failed to register reset handler\n");
>>>> -			goto err_wdt_reg;
>>>> -		}
>>>> -	} else {
>>>> -		dev_err(dev, "no vpu_dev found\n");
>>>> +	if (mdp->rdma_dev == NULL) {
>>>> +		dev_err(dev, "Primary MDP device not found");
>>>> +		status = -ENODEV;
>>>> +		goto err_component_bind_all;
>>>> +	}
>>>> +
>>>> +	vpu_node = of_find_node_by_name(NULL, "vpu");
>>>> +	if (!vpu_node) {
>>>> +		dev_err(dev, "unable to find vpu node");
>>>> +		status = -ENODEV;
>>>> +		goto err_wdt_reg;
>>>> +	}
>>>> +
>>>> +	mdp->vpu_dev = of_find_device_by_node(vpu_node);
>>>
>>> The 'vpu_node' should be put by calling 'of_node_put(vpu_node)' when it
>>> is not used.
>>>
>>>> +	if (!mdp->vpu_dev) {
>>>> +		dev_err(dev, "unable to find vpu device");
>>>> +		status = -ENODEV;
>>>> +		goto err_wdt_reg;
>>>> +	}
>>>> +
>>>> +	status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
>>>> +	if (status) {
>>>> +		dev_err(dev, "Failed to register reset handler\n");
>>>> +		goto err_wdt_reg;
>>>>  	}
>>>>  
>>>>  	status = mtk_mdp_register_m2m_device(mdp);
>>>
> 

Re: [PATCH v7 7/7] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

[houlong wei]

Hi Enric,

My answers are as follows.Thanks for deep thinking about mtk-mdp.

Regards,
Houlong

On Thu, 2021-08-26 at 00:54 +0800, Enric Balletbo i Serra wrote:
> Hi Houlong,
> 
> Thank you for your answer, some more questions below :-)
> 
> On 25/8/21 17:18, houlong wei wrote:
> > On Wed, 2021-08-25 at 17:07 +0800, Enric Balletbo i Serra wrote:
> >> Hi Houlong,
> >>
> >> Thank you for following up this patchset. I have some questions to try to
> >> understand better the hardware though.
> >>
> >> On 25/8/21 10:43, houlong wei wrote:
> >>> Hi Eizan,
> >>>
> >>> Thanks for you patch. I have inline comment below.
> >>>
> >>> Regards,
> >>> Houlong
> >>>
> >>> On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote:
> >>>> ... Instead of depending on the presence of a mediatek,vpu property in
> >>>> the device node.
> >>>>
> >>>> That property was originally added to link to the vpu node so that the
> >>>> mtk_mdp_core driver could pass the right device to
> >>>> vpu_wdt_reg_handler(). However in a previous patch in this series,
> >>>> the driver has been modified to search the device tree for that node
> >>>> instead.
> >>>>
> >>>> That property was also used to indicate the primary MDP device, so that
> >>>> it can be passed to the V4L2 subsystem as well as register it to be
> >>>> used when setting up queues in the open() callback for the filesystem
> >>>> device node that is created. In this case, assuming that the primary
> >>>> MDP device is the one with a specific alias seems useable because the
> >>>> alternative is to add a property to the device tree which doesn't
> >>>> actually represent any facet of hardware (i.e., this being the primary
> >>>> MDP device is a software decision). In other words, this solution is
> >>>> equally as arbitrary, but at least it doesn't add a property to a
> >>>> device node where said property is unrelated to the hardware present.
> >>>>
> >>>> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> >>>> Reviewed-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> >>>> ---
> >>>>
> >>>> (no changes since v1)
> >>>>
> >>>>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 56 +++++++++++++------
> >>>>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 36 ++++++++----
> >>>>  2 files changed, 64 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >>>> index 85ef274841a3..9527649de98e 100644
> >>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >>>> @@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
> >>>>  		mtk_smi_larb_put(comp->larb_dev);
> >>>>  }
> >>>>  
> >>>> -static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *data)
> >>>> +/*
> >>>> + * The MDP master device node is identified by the device tree alias
> >>>> + * "mdp-rdma0".
> >>>> + */
> >>>> +static bool is_mdp_master(struct device *dev)
> >>>> +{
> >>>> +	struct device_node *aliases, *mdp_rdma0_node;
> >>>> +	const char *mdp_rdma0_path;
> >>>> +
> >>>> +	if (!dev->of_node)
> >>>> +		return false;
> >>>> +
> >>>> +	aliases = of_find_node_by_path("/aliases");
> >>>> +	if (!aliases) {
> >>>> +		dev_err(dev, "no aliases found for mdp-rdma0");
> >>>> +		return false;
> >>>> +	}
> >>>> +
> >>>> +	mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
> >>>> +	if (!mdp_rdma0_path) {
> >>>> +		dev_err(dev, "get mdp-rdma0 property of /aliases failed");
> >>>> +		return false;
> >>>> +	}
> >>>> +
> >>>> +	mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
> >>>> +	if (!mdp_rdma0_node) {
> >>>> +		dev_err(dev, "path resolution failed for %s", mdp_rdma0_path);
> >>>> +		return false;
> >>>> +	}
> >>>> +
> >>>> +	return dev->of_node == mdp_rdma0_node;
> >>>
> >>>
> >>> About how to determine the master mdp driver, we also can
> >>> judge the component type. The component type can be gotten by calling
> >>> of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is
> >>> the master driver. No matter it is mdp_rdma0 or mdp_rdma1.
> >>
> >>
> >> I'm confused, you mean that the component type, aka MTK_MDP_RDMA is only set for
> >> mtk_mdp_rdma0? isn't mtk_dmp_rdma1 also a MTK_MDP_RDMA type? Because looks weird
> >> to me that two rdma have diffent types.
> >>
> > Hi Enric,
> > 
> > I can understand your doubts. For mtk-mdp dirver, it is really difficult
> > to understand the relationship among the components. Because the MDP
> > hardware path is configured and connected in the VPU firmware.
> 
> So is fixed by the VPU firmware and can't be changed via, i.e writing some
> registers? Can't be platform data then?
> 
> If that's the case, can you draw the mdp hardware path for i.e MT8173? (so I
> have a better understanding)
> 

I reviewed the mt8173 VPU firmware code and found it only use 1 MDP
path.Perhaps one MDP path could meet the project team's requirement when
the VPU firmware was implemented.
A MDP path is a data path which consists of several MDP hardware
engines, e.g. mdp_rdma1->mdp_rsz1->mdp_wrot1,
mdp_rdma2->mdp_rsz2->mdp_wrot2. Ideally,if there are multiple MDP paths,
the users(e.g. applications) can use them concurrently.

> 
> > For MT8173, the component type of mtk_mdp_rdma0 and mtk_mdp_rdma1 are
> > both MTK_MDP_RDMA. Even though the mtk-mdp driver's component list
> > contains both of them, only one corresponding hardware component takes
> > effect during the processing of a frame, the VPU decides it.
> > 
> 
> So IIUC the VPU firmware decides which rdma uses on every frame, right? From hw
> point of view, why one of them need to be master?
> 

For mt8173, the VPU firmware only use one rdma because another MDP path
is not implemented.If we implement multiple MDP paths in certain
project, each rdma can be master.


> > Regards,
> > Houlong
> >>
> >>> http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028533.html
> >>>
> >>> IMO, judging it by component type is more flexible because it does not
> >>> limit to 'mdp_rdma0'.
> >>>
> >>
> >> Using an alias like Eizan did is also flexible, you only need to set mdp-rdma0
> >> to point the mdp_rdma1 node.
> >>
> >> What I am really interested to know is the differences between the platforms to
> >> understand if this is really a platform hardware property or a software
> >> configurable thing, so would help if you can give use the different use cases
> >> for different SoCs.
> >>
> >> For MT8173 which is the master device? is _always_ rdma0 or can also be rdma1?
> >>
> >> Or maybe what is not clear here is what exactly means be a master device?
> >>
> >> What about MT8183 and other SoCs?
> >>
> >> Thanks,
> >>   Enric
> >>
> > Hi Enric,
> > 
> > Before answer your questions, let's unify the concept of the master
> > device.If an MDP device-tree node can not only generate a /dev/video
> > device node, but also can generate a mtk component device node, then
> > this component device is the master device.
> 
> Sorry, what you mean with a mtk component device node?

My meaning is that the device-tree node can generate a platform device,
it is what the mtk_mdp_comp.c does.

> 
> > From the perspective of the device tree node, the compatible of a MDP
> > device tree node contains both "mediatek,mt8173-mdp-rdma" and
> > "mediatek,mt8173-mdp", then its corresponding mtk component device is
> > the master device.
> 
> So that's what's wrong, and Eizan patches get rid of this. In this case you
> can't use device-tree to specify such master device. Device-tree is only about
> hardware description, can't be used, in this case, as entry point to instantiate
> the mdp driver. Hence Eizan patches change this, and mdp is instantiated by the
> mmsys driver. Similar to what we do for the drm driver.
> 
> For the two hw rdma blocks the compatible should be just
> "mediatek,mt8173-mdp-rdma" in this case.
> 
> Thanks,
>   Enric

My consideration is how the driver supports it when there are multiple
MDP paths. Assume there are 2 MDP paths, as a V4L2 driver, mtk-mdp can
generates two device nodes, e.g. /dev/video0 supports one of MDP
path, /dev/video1 supports another MDP path.

> > Since this concept comes from Eizan's patches, if my understanding is
> > different from yours, please let me know, thanks.
> > 
> > If there is only one master device, I fully agree with you. For MT8173,
> > both mdp_rdma0 and mdp_rdma1 can be master device at the same time.
> > But now only mdp_rdma0 takes this role, perhaps because one MDP hardware
> > path could meet the project's the requirement six years ago.
> > In some project, we have two or more MDP hardware paths, we can
> > configure all the mdp_rdma components as master devices, and there will
> > be several /dev/video device nodes. The user can control them
> > concurrently.
> > 
> > Regards,
> > Houlong
> > 
> >>
> >>>> +}
> >>>> +
> >>>> +static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
> >>>> +			void *data)
> >>>>  {
> >>>>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
> >>>>  	struct mtk_mdp_dev *mdp = data;
> >>>> -	struct device_node *vpu_node;
> >>>>  
> >>>>  	mtk_mdp_register_component(mdp, comp);
> >>>>  
> >>>> -	/*
> >>>> -	 * If this component has a "mediatek-vpu" property, it is responsible for
> >>>> -	 * notifying the mdp master driver about it so it can be further initialized
> >>>> -	 * later.
> >>>> -	 */
> >>>> -	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
> >>>> -	if (vpu_node) {
> >>>> +	if (is_mdp_master(dev)) {
> >>>>  		int ret;
> >>>>  
> >>>> -		mdp->vpu_dev = of_find_device_by_node(vpu_node);
> >>>> -		if (WARN_ON(!mdp->vpu_dev)) {
> >>>> -			dev_err(dev, "vpu pdev failed\n");
> >>>> -			of_node_put(vpu_node);
> >>>> -		}
> >>>> -
> >>>>  		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
> >>>>  		if (ret) {
> >>>>  			dev_err(dev, "Failed to register v4l2 device\n");
> >>>> @@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master, void *da
> >>>>  		}
> >>>>  
> >>>>  		/*
> >>>> -		 * presence of the "mediatek,vpu" property in a device node
> >>>> -		 * indicates that it is the primary MDP rdma device and MDP DMA
> >>>> -		 * ops should be handled by its DMA callbacks.
> >>>> +		 * MDP DMA ops will be handled by the DMA callbacks associated with this
> >>>> +		 * device;
> >>>>  		 */
> >>>>  		mdp->rdma_dev = dev;
> >>>>  	}
> >>>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >>>> index 50eafcc9993d..6a775463399c 100644
> >>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >>>> @@ -150,8 +150,9 @@ static void release_of(struct device *dev, void *data)
> >>>>  
> >>>>  static int mtk_mdp_master_bind(struct device *dev)
> >>>>  {
> >>>> -	int status;
> >>>>  	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
> >>>> +	struct device_node *vpu_node;
> >>>> +	int status;
> >>>>  
> >>>>  	status = component_bind_all(dev, mdp);
> >>>>  	if (status) {
> >>>> @@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device *dev)
> >>>>  		goto err_component_bind_all;
> >>>>  	}
> >>>>  
> >>>> -	if (mdp->vpu_dev) {
> >>>> -		int ret = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp,
> >>>> -					  VPU_RST_MDP);
> >>>> -		if (ret) {
> >>>> -			dev_err(dev, "Failed to register reset handler\n");
> >>>> -			goto err_wdt_reg;
> >>>> -		}
> >>>> -	} else {
> >>>> -		dev_err(dev, "no vpu_dev found\n");
> >>>> +	if (mdp->rdma_dev == NULL) {
> >>>> +		dev_err(dev, "Primary MDP device not found");
> >>>> +		status = -ENODEV;
> >>>> +		goto err_component_bind_all;
> >>>> +	}
> >>>> +
> >>>> +	vpu_node = of_find_node_by_name(NULL, "vpu");
> >>>> +	if (!vpu_node) {
> >>>> +		dev_err(dev, "unable to find vpu node");
> >>>> +		status = -ENODEV;
> >>>> +		goto err_wdt_reg;
> >>>> +	}
> >>>> +
> >>>> +	mdp->vpu_dev = of_find_device_by_node(vpu_node);
> >>>
> >>> The 'vpu_node' should be put by calling 'of_node_put(vpu_node)' when it
> >>> is not used.
> >>>
> >>>> +	if (!mdp->vpu_dev) {
> >>>> +		dev_err(dev, "unable to find vpu device");
> >>>> +		status = -ENODEV;
> >>>> +		goto err_wdt_reg;
> >>>> +	}
> >>>> +
> >>>> +	status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
> >>>> +	if (status) {
> >>>> +		dev_err(dev, "Failed to register reset handler\n");
> >>>> +		goto err_wdt_reg;
> >>>>  	}
> >>>>  
> >>>>  	status = mtk_mdp_register_m2m_device(mdp);
> >>>
> >