[PATCH v1 0/4] Refactor MTK MDP driver into core/components

[PATCH v1 0/4] 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.11.

It has been rebased on top of the series
https://patchwork.kernel.org/project/linux-mediatek/list/?series=464873

The first two patches are unchanged from the previous series submission.


Eizan Miyamoto (4):
  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: soc: mediatek: register mdp from mmsys

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 191 ++++++++++++++--
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  35 +--
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 214 +++++++++++-------
 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, 345 insertions(+), 122 deletions(-)

-- 
2.31.1.498.g6c1eba8ee3d-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/4] 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>
---

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 139 ++++++++++++--
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  26 +--
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 175 +++++++++++++-----
 drivers/media/platform/mtk-mdp/mtk_mdp_core.h |   1 +
 4 files changed, 255 insertions(+), 86 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 1e3833f1c9ae..3fbbcf05440a 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -5,13 +5,51 @@
  */
 
 #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_device.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
 #include <linux/of_platform.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);
 
 void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 {
@@ -23,8 +61,8 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
 		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);
+				"failed to enable clock, err %d. i:%d\n",
+				err, i);
 	}
 }
 
@@ -39,15 +77,40 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
 	}
 }
 
-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_dev *mdp = data;
+	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
+
+	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)
 {
 	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);
@@ -55,23 +118,71 @@ 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;
 	}
 
 	return 0;
 
-put_dev:
-	of_node_put(comp->dev_node);
+err:
+	dev_err(dev, "err %d\n", ret);
 
 	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)
+{
+	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)
 {
-	of_node_put(comp->dev_node);
+	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 36bc1b8f6222..956d20c01e34 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -7,41 +7,21 @@
 #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,
-	MTK_MDP_COMP_TYPE_MAX,
-};
-
 /**
  * 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
- * @type:	component type
  */
 struct mtk_mdp_comp {
 	struct list_head	node;
-	struct device_node	*dev_node;
 	struct clk		*clk[2];
-	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);
+
 void 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 70a8eab16863..d79bf7f0031a 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>
@@ -18,6 +19,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/workqueue.h>
 
+#include "mtk_mdp_comp.h"
 #include "mtk_mdp_core.h"
 #include "mtk_mdp_m2m.h"
 #include "mtk_vpu.h"
@@ -31,16 +33,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
 	},
 	{ },
 };
@@ -90,6 +88,64 @@ 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);
+		v4l2_err(&mdp->v4l2_dev, "Failed to init mem2mem device\n");
+		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)
 {
@@ -107,8 +163,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)
@@ -133,36 +189,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);
@@ -187,18 +250,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);
@@ -206,15 +263,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:
@@ -226,11 +293,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;
 }
@@ -238,11 +300,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);
@@ -251,10 +312,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_err(&pdev->dev, "not all components removed\n");
 
 	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
 	return 0;
@@ -309,7 +368,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 a7da14b97077..230f531400ca 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.31.1.498.g6c1eba8ee3d-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] mtk-mdp: add driver to probe mdp components

[Enric Balletbo i Serra]

Hi Eizan,

Thank kyou for your patch.

On 23/4/21 7:58, Eizan Miyamoto 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. 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>
> ---
> 
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 139 ++++++++++++--
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  26 +--
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 175 +++++++++++++-----
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.h |   1 +
>  4 files changed, 255 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index 1e3833f1c9ae..3fbbcf05440a 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -5,13 +5,51 @@
>   */
>  
>  #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_device.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
>  #include <linux/of_platform.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);
>  
>  void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
>  {
> @@ -23,8 +61,8 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
>  		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);
> +				"failed to enable clock, err %d. i:%d\n",
> +				err, i);

I think youcan take advantage of the new line lenght limit and put all in one
line here.


>  	}
>  }
>  
> @@ -39,15 +77,40 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
>  	}
>  }
>  
> -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_dev *mdp = data;
> +	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
> +
> +	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)
>  {
>  	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);
> @@ -55,23 +118,71 @@ 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;
>  	}
>  
>  	return 0;
>  
> -put_dev:
> -	of_node_put(comp->dev_node);
> +err:
> +	dev_err(dev, "err %d\n", ret);

This error is too generic. You can remove. Anyway you're printing the same error
code if mtk_mdp_comp_init fails in probe function, there makes more sense.

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

This is the thing that I'm not completely sure is doing the right thing. Having
a DT propriety to indicate to skip the probing is really weird to me. Also the
name is quite confusing. When I read this name I'd expect mediatek,vpu being a
phandle to the VPU device node. If there is a master and childs below from
hardware POV, the DT graph should reflect that, if this is only a way to
instantiate properly the devices then I think we can find other solutions. If
you know that RDMA is always the master maybe we can just find this node.

Other thoughts?


> +		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)
>  {
> -	of_node_put(comp->dev_node);
> +	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 36bc1b8f6222..956d20c01e34 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> @@ -7,41 +7,21 @@
>  #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,
> -	MTK_MDP_COMP_TYPE_MAX,
> -};
> -
>  /**
>   * 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
> - * @type:	component type
>   */
>  struct mtk_mdp_comp {
>  	struct list_head	node;
> -	struct device_node	*dev_node;
>  	struct clk		*clk[2];
> -	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);
> +
>  void 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 70a8eab16863..d79bf7f0031a 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>
> @@ -18,6 +19,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/workqueue.h>
>  
> +#include "mtk_mdp_comp.h"
>  #include "mtk_mdp_core.h"
>  #include "mtk_mdp_m2m.h"
>  #include "mtk_vpu.h"
> @@ -31,16 +33,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
>  	},
>  	{ },
>  };
> @@ -90,6 +88,64 @@ 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);
> +		v4l2_err(&mdp->v4l2_dev, "Failed to init mem2mem device\n");
> +		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)
>  {
> @@ -107,8 +163,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)
> @@ -133,36 +189,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);
> @@ -187,18 +250,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);
> @@ -206,15 +263,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:
> @@ -226,11 +293,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;
>  }
> @@ -238,11 +300,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);
> @@ -251,10 +312,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_err(&pdev->dev, "not all components removed\n");
>  
>  	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>  	return 0;
> @@ -309,7 +368,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 a7da14b97077..230f531400ca 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;
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/4] mtk-mdp: add driver to probe mdp components

[houlong wei]

Hi Eizan,

Thanks for your patch. I have inline comments below.

Houlong


On Fri, 2021-05-14 at 16:19 +0800, Enric Balletbo i Serra wrote:
> Hi Eizan,
> 
> Thank kyou for your patch.
> 
> On 23/4/21 7:58, Eizan Miyamoto 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. 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>
> > ---
> > 
> >  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 139 ++++++++++++--
> >  drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  26 +--
> >  drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 175 +++++++++++++-----
> >  drivers/media/platform/mtk-mdp/mtk_mdp_core.h |   1 +
> >  4 files changed, 255 insertions(+), 86 deletions(-)
> > 
> > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> > index 1e3833f1c9ae..3fbbcf05440a 100644
> > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> > @@ -5,13 +5,51 @@
> >   */
> >  
> >  #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_device.h>
> > +#include <linux/of.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/of_platform.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

Reszer is the typo for Resizer. Can you help to fix it, although it
already exists in the original version ?

> > + * @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);
> >  
> >  void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
> >  {
> > @@ -23,8 +61,8 @@ void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
> >  		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);
> > +				"failed to enable clock, err %d. i:%d\n",
> > +				err, i);
> 
> I think youcan take advantage of the new line lenght limit and put all in one
> line here.
> 
> 
> >  	}
> >  }
> >  
> > @@ -39,15 +77,40 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
> >  	}
> >  }
> >  
> > -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_dev *mdp = data;
> > +	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
> > +
> > +	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)
> >  {
> >  	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);
> > @@ -55,23 +118,71 @@ 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;
> >  	}
> >  
> >  	return 0;
> >  
> > -put_dev:
> > -	of_node_put(comp->dev_node);
> > +err:
> > +	dev_err(dev, "err %d\n", ret);
> 
> This error is too generic. You can remove. Anyway you're printing the same error
> code if mtk_mdp_comp_init fails in probe function, there makes more sense.
> 
> >  
> >  	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)
> > +{
> > +	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");
> 
> This is the thing that I'm not completely sure is doing the right thing. Having
> a DT propriety to indicate to skip the probing is really weird to me. Also the
> name is quite confusing. When I read this name I'd expect mediatek,vpu being a
> phandle to the VPU device node. If there is a master and childs below from
> hardware POV, the DT graph should reflect that, if this is only a way to
> instantiate properly the devices then I think we can find other solutions. If
> you know that RDMA is always the master maybe we can just find this node.
> 
> Other thoughts?
> 

One way to judge it is to compare the of_device_get_match_data(dev) with
MTK_MDP_RDMA.

> 
> > +		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)
> >  {
> > -	of_node_put(comp->dev_node);
> > +	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 36bc1b8f6222..956d20c01e34 100644
> > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> > @@ -7,41 +7,21 @@
> >  #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,
> > -	MTK_MDP_COMP_TYPE_MAX,
> > -};
> > -
> >  /**
> >   * 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
> > - * @type:	component type
> >   */
> >  struct mtk_mdp_comp {
> >  	struct list_head	node;
> > -	struct device_node	*dev_node;
> >  	struct clk		*clk[2];
> > -	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);
> > +
> >  void 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 70a8eab16863..d79bf7f0031a 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>
> > @@ -18,6 +19,7 @@
> >  #include <linux/pm_runtime.h>
> >  #include <linux/workqueue.h>
> >  
> > +#include "mtk_mdp_comp.h"
> >  #include "mtk_mdp_core.h"
> >  #include "mtk_mdp_m2m.h"
> >  #include "mtk_vpu.h"
> > @@ -31,16 +33,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
> >  	},
> >  	{ },
> >  };
> > @@ -90,6 +88,64 @@ 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);
> > +		v4l2_err(&mdp->v4l2_dev, "Failed to init mem2mem device\n");
> > +		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)
> >  {
> > @@ -107,8 +163,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)
> > @@ -133,36 +189,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;

'pdev' is used above, so we'd better to use another variable name here
to avoid confusion.

> >  
> > -		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);
> > @@ -187,18 +250,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);
> > @@ -206,15 +263,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:
> > @@ -226,11 +293,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;
> >  }
> > @@ -238,11 +300,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);
> > @@ -251,10 +312,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_err(&pdev->dev, "not all components removed\n");
> >  
> >  	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
> >  	return 0;
> > @@ -309,7 +368,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 a7da14b97077..230f531400ca 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;
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/4] 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>
---

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 19 ++++++++++++++++---
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  6 ++++--
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c |  6 ++----
 3 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 3fbbcf05440a..84f9c529d74a 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -13,6 +13,8 @@
 #include <linux/of.h>
 #include <linux/of_irq.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"
@@ -51,22 +53,28 @@ static const struct of_device_id mtk_mdp_comp_driver_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_mdp_comp_driver_dt_match);
 
-void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
+void mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
 {
 	int i, err;
 
+	err = pm_runtime_get_sync(comp->dev);
+	if (err < 0)
+		dev_err(comp->dev,
+			"failed to runtime get, err %d.\n",
+			err);
+
 	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
 		if (IS_ERR(comp->clk[i]))
 			continue;
 		err = clk_prepare_enable(comp->clk[i]);
 		if (err)
-			dev_err(dev,
+			dev_err(comp->dev,
 				"failed to enable clock, err %d. i:%d\n",
 				err, i);
 	}
 }
 
-void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
+void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
 {
 	int i;
 
@@ -75,6 +83,8 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
 			continue;
 		clk_disable_unprepare(comp->clk[i]);
 	}
+
+	pm_runtime_put_sync(comp->dev);
 }
 
 static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
@@ -84,6 +94,7 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
 	struct mtk_mdp_dev *mdp = data;
 
 	mtk_mdp_register_component(mdp, comp);
+	pm_runtime_enable(dev);
 
 	return 0;
 }
@@ -94,6 +105,7 @@ static void mtk_mdp_comp_unbind(struct device *dev, struct device *master,
 	struct mtk_mdp_dev *mdp = data;
 	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
 
+	pm_runtime_disable(dev);
 	mtk_mdp_unregister_component(mdp, comp);
 }
 
@@ -111,6 +123,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 956d20c01e34..355e226d74fe 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -11,16 +11,18 @@
  * struct mtk_mdp_comp - the MDP's function component data
  * @node:	list node to track sibing MDP components
  * @clk:	clocks required for component
+ * @dev:	component's device
  */
 struct mtk_mdp_comp {
 	struct list_head	node;
 	struct clk		*clk[2];
+	struct device		*dev;
 };
 
 int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev);
 
-void 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);
+void mtk_mdp_comp_clock_on(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 d79bf7f0031a..c55bcfe4cbb7 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -51,20 +51,18 @@ MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids);
 
 static void mtk_mdp_clock_on(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_on(dev, comp_node);
+		mtk_mdp_comp_clock_on(comp_node);
 }
 
 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.31.1.498.g6c1eba8ee3d-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/4] mtk-mdp: use pm_runtime in MDP component driver

[Enric Balletbo i Serra]

Hi Eizan,

Thank you for your patch.

On 23/4/21 7:58, Eizan Miyamoto wrote:
> 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>
> ---
> 
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 19 ++++++++++++++++---
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  6 ++++--
>  drivers/media/platform/mtk-mdp/mtk_mdp_core.c |  6 ++----
>  3 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index 3fbbcf05440a..84f9c529d74a 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -13,6 +13,8 @@
>  #include <linux/of.h>
>  #include <linux/of_irq.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"
> @@ -51,22 +53,28 @@ static const struct of_device_id mtk_mdp_comp_driver_dt_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, mtk_mdp_comp_driver_dt_match);
>  
> -void mtk_mdp_comp_clock_on(struct device *dev, struct mtk_mdp_comp *comp)
> +void mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
>  {
>  	int i, err;
>  
> +	err = pm_runtime_get_sync(comp->dev);
> +	if (err < 0)
> +		dev_err(comp->dev,
> +			"failed to runtime get, err %d.\n",
> +			err);
> +

Personally, in the subsystem I take care I don't allow printing errors that are
ignored. Shouldn't you propagate the error?

>  	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
>  		if (IS_ERR(comp->clk[i]))
>  			continue;
>  		err = clk_prepare_enable(comp->clk[i]);
>  		if (err)
> -			dev_err(dev,
> +			dev_err(comp->dev,
>  				"failed to enable clock, err %d. i:%d\n",
>  				err, i);
>  	}
>  }
>  
> -void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
> +void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
>  {
>  	int i;
>  
> @@ -75,6 +83,8 @@ void mtk_mdp_comp_clock_off(struct device *dev, struct mtk_mdp_comp *comp)
>  			continue;
>  		clk_disable_unprepare(comp->clk[i]);
>  	}
> +
> +	pm_runtime_put_sync(comp->dev);
>  }
>  
>  static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
> @@ -84,6 +94,7 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
>  	struct mtk_mdp_dev *mdp = data;
>  
>  	mtk_mdp_register_component(mdp, comp);
> +	pm_runtime_enable(dev);
>  
>  	return 0;
>  }
> @@ -94,6 +105,7 @@ static void mtk_mdp_comp_unbind(struct device *dev, struct device *master,
>  	struct mtk_mdp_dev *mdp = data;
>  	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
>  
> +	pm_runtime_disable(dev);
>  	mtk_mdp_unregister_component(mdp, comp);
>  }
>  
> @@ -111,6 +123,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 956d20c01e34..355e226d74fe 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
> @@ -11,16 +11,18 @@
>   * struct mtk_mdp_comp - the MDP's function component data
>   * @node:	list node to track sibing MDP components
>   * @clk:	clocks required for component
> + * @dev:	component's device
>   */
>  struct mtk_mdp_comp {
>  	struct list_head	node;
>  	struct clk		*clk[2];
> +	struct device		*dev;
>  };
>  
>  int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev);
>  
> -void 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);
> +void mtk_mdp_comp_clock_on(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 d79bf7f0031a..c55bcfe4cbb7 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -51,20 +51,18 @@ MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids);
>  
>  static void mtk_mdp_clock_on(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_on(dev, comp_node);
> +		mtk_mdp_comp_clock_on(comp_node);
>  }
>  
>  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)
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/4] 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>
---

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 28 ++++++++++++++-----
 drivers/media/platform/mtk-mdp/mtk_mdp_comp.h |  3 ++
 drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 23 ++++++++++-----
 3 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 84f9c529d74a..d447bfaadef4 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -53,15 +53,31 @@ static const struct of_device_id mtk_mdp_comp_driver_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_mdp_comp_driver_dt_match);
 
-void mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
+void mtk_mdp_comp_power_on(struct mtk_mdp_comp *comp)
 {
-	int i, err;
+	int err;
 
 	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);
+
+	mtk_mdp_comp_clock_on(comp);
+}
+
+void mtk_mdp_comp_power_off(struct mtk_mdp_comp *comp)
+{
+	int 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);
+}
+
+void mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
+{
+	int i, err;
 
 	for (i = 0; i < ARRAY_SIZE(comp->clk); i++) {
 		if (IS_ERR(comp->clk[i]))
@@ -83,8 +99,6 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
 			continue;
 		clk_disable_unprepare(comp->clk[i]);
 	}
-
-	pm_runtime_put_sync(comp->dev);
 }
 
 static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
index 355e226d74fe..7ad9b06bb11b 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -21,6 +21,9 @@ struct mtk_mdp_comp {
 
 int mtk_mdp_comp_init(struct mtk_mdp_comp *comp, struct device *dev);
 
+void mtk_mdp_comp_power_on(struct mtk_mdp_comp *comp);
+void mtk_mdp_comp_power_off(struct mtk_mdp_comp *comp);
+
 void mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp);
 void mtk_mdp_comp_clock_off(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 c55bcfe4cbb7..5e71496e2517 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -53,8 +53,15 @@ static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
 {
 	struct mtk_mdp_comp *comp_node;
 
+	/*
+	 * 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.
+	 */
+	mtk_mdp_comp_clock_on(&mdp->comp_self);
+
 	list_for_each_entry(comp_node, &mdp->comp_list, node)
-		mtk_mdp_comp_clock_on(comp_node);
+		mtk_mdp_comp_power_on(comp_node);
 }
 
 static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
@@ -62,7 +69,14 @@ static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
 	struct mtk_mdp_comp *comp_node;
 
 	list_for_each_entry(comp_node, &mdp->comp_list, node)
-		mtk_mdp_comp_clock_off(comp_node);
+		mtk_mdp_comp_power_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);
 }
 
 static void mtk_mdp_wdt_worker(struct work_struct *work)
@@ -101,8 +115,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);
@@ -124,8 +136,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;
 }
 
@@ -136,7 +146,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 = {
-- 
2.31.1.498.g6c1eba8ee3d-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 4/4] mtk-mdp: soc: mediatek: register mdp from mmsys

[Eizan Miyamoto]

Rather than hanging the MDP master component driver off of the rdma0
device, create a "virtual" device by the mmsys driver instead which is
probed by the mtk_mdp_core driver.

Broadly, four interdependent things are done by this change:
- A virtual device that is probed by the mtk_mdp_core driver is
  instantiated by the mtk_mmsys driver.
- Presence of a mediatek,vpu property in a child node to the mmsys
  device node is used to determine what device to use when dispatching
  dma ops from the relevant ioctl.
- v4l-related setup is moved into from the mtk_mdp_core driver to the
  mtk_mdp_comp driver.

Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
---

 drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 47 +++++++++-----
 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 +-
 drivers/soc/mediatek/mtk-mmsys.c              | 20 +++++-
 5 files changed, 75 insertions(+), 60 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index d447bfaadef4..dc5231a1fcfd 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -106,8 +106,41 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
 {
 	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;
@@ -164,23 +197,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 5e71496e2517..4d7aa4e26be6 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -121,6 +121,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);
@@ -133,6 +144,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:
@@ -191,8 +204,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 */
@@ -225,16 +243,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");
@@ -250,29 +258,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");
@@ -284,22 +271,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;
 }
@@ -371,7 +348,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 230f531400ca..78c3c77cd226 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 d351e5a44768..c80ad8299c5e 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
@@ -932,7 +932,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);
@@ -947,7 +947,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);
diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
index 18f93979e14a..6f9cf7725529 100644
--- a/drivers/soc/mediatek/mtk-mmsys.c
+++ b/drivers/soc/mediatek/mtk-mmsys.c
@@ -305,6 +305,7 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct platform_device *clks;
 	struct platform_device *drm;
+	struct platform_device *mdp;
 	void __iomem *config_regs;
 	int ret;
 
@@ -328,10 +329,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.31.1.498.g6c1eba8ee3d-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/4] mtk-mdp: soc: mediatek: register mdp from mmsys

[Enric Balletbo i Serra]

Hi Eizan,

Thank you for your patch.

On 23/4/21 7:58, Eizan Miyamoto wrote:
> Rather than hanging the MDP master component driver off of the rdma0
> device, create a "virtual" device by the mmsys driver instead which is
> probed by the mtk_mdp_core driver.
> 
> Broadly, four interdependent things are done by this change:
> - A virtual device that is probed by the mtk_mdp_core driver is
>   instantiated by the mtk_mmsys driver.
> - Presence of a mediatek,vpu property in a child node to the mmsys
>   device node is used to determine what device to use when dispatching
>   dma ops from the relevant ioctl.
> - v4l-related setup is moved into from the mtk_mdp_core driver to the
>   mtk_mdp_comp driver.
> 
> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> ---
> 
>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 47 +++++++++-----
>  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 +-
>  drivers/soc/mediatek/mtk-mmsys.c              | 20 +++++-
>  5 files changed, 75 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> index d447bfaadef4..dc5231a1fcfd 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> @@ -106,8 +106,41 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
>  {
>  	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.

Please use c-style comments here.

> +	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);

That's a bit confusing to me, please correct me if I am wrong, so, the
mediatek,vpu property is used to tell the code that this component should be the
'vpu master', not to point a vpu node in the DT? I understood correctly?


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

Isn't rdma0 always the primary MDP device? or there are SoCs or configurations
where this is different? At least I think it is for MT8173 and MT8183.

> +		mdp->rdma_dev = dev;
> +	}
> +
>  	pm_runtime_enable(dev);
>  
>  	return 0;
> @@ -164,23 +197,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 5e71496e2517..4d7aa4e26be6 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> @@ -121,6 +121,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);
> @@ -133,6 +144,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:
> @@ -191,8 +204,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 */
> @@ -225,16 +243,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");
> @@ -250,29 +258,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");
> @@ -284,22 +271,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;
>  }
> @@ -371,7 +348,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 230f531400ca..78c3c77cd226 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 d351e5a44768..c80ad8299c5e 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -932,7 +932,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);
> @@ -947,7 +947,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);
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index 18f93979e14a..6f9cf7725529 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -305,6 +305,7 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct platform_device *clks;
>  	struct platform_device *drm;
> +	struct platform_device *mdp;
>  	void __iomem *config_regs;
>  	int ret;
>  
> @@ -328,10 +329,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[] = {
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/4] mtk-mdp: soc: mediatek: register mdp from mmsys

[Eizan Miyamoto]

On Fri, Apr 30, 2021 at 1:46 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Eizan,
>
> Thank you for your patch.
>
> On 23/4/21 7:58, Eizan Miyamoto wrote:
> > Rather than hanging the MDP master component driver off of the rdma0
> > device, create a "virtual" device by the mmsys driver instead which is
> > probed by the mtk_mdp_core driver.
> >
> > Broadly, four interdependent things are done by this change:
> > - A virtual device that is probed by the mtk_mdp_core driver is
> >   instantiated by the mtk_mmsys driver.
> > - Presence of a mediatek,vpu property in a child node to the mmsys
> >   device node is used to determine what device to use when dispatching
> >   dma ops from the relevant ioctl.
> > - v4l-related setup is moved into from the mtk_mdp_core driver to the
> >   mtk_mdp_comp driver.
> >
> > Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> > ---
> >
> >  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 47 +++++++++-----
> >  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 +-
> >  drivers/soc/mediatek/mtk-mmsys.c              | 20 +++++-
> >  5 files changed, 75 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> > index d447bfaadef4..dc5231a1fcfd 100644
> > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> > @@ -106,8 +106,41 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
> >  {
> >       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.
>
> Please use c-style comments here.

Thank you for the reminder, I'll update these in the next version of
this patch series.

>
> > +     vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
>
> That's a bit confusing to me, please correct me if I am wrong, so, the
> mediatek,vpu property is used to tell the code that this component should be the
> 'vpu master', not to point a vpu node in the DT? I understood correctly?

Is what you mean by 'vpu master' is that it is the device whose driver
implements the wdt reset function? In that case, the mtk_mdp_core
driver is still the 'vpu master' because mtk_mdp_reset_handler
(contained in mtk_mdp_core) is passed to vpu_wdt_reg_handler(). The
presence of the property in any MDP component device node can do the
job of passing the vpu device (obtained from the node being pointed
to) back mtk_mdp_core's mtk_mdp_master_bind() function.

*However*, I'm using the presence of that property to indicate another
thing: this is the device that the mdp filesystem device node in /dev
should be registered against for v4l2. We will need to save this
device for later (in mdp->rdma_dev) to be used to find the DMA
callbacks when a call to mtk_mdp_m2m_queue_init is made from the file
open() callback (mtk_mdp_m2m_open) attached to the filesystem device
node.

Before this change, the mtk_mdp_core driver was serving triple duty as
the driver for the device that provided DMA op callbacks, the vpu
master, and the MDP component master. Now it is the vpu master and the
MDP component master, but not the driver for the device that provides
DMA op callbacks.

>
>
> > +     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.
>
> Isn't rdma0 always the primary MDP device? or there are SoCs or configurations
> where this is different? At least I think it is for MT8173 and MT8183.

I suppose you're right, though now it seems to be called mdp_rdma0 in
the device tree?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?id=9ccce092fc64d19504fa54de4fd659e279cc92e7#n1004

Maybe somebody from MediaTek can confirm this?

>
> > +             mdp->rdma_dev = dev;
> > +     }
> > +
> >       pm_runtime_enable(dev);
> >
> >       return 0;
> > @@ -164,23 +197,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 5e71496e2517..4d7aa4e26be6 100644
> > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> > @@ -121,6 +121,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);
> > @@ -133,6 +144,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:
> > @@ -191,8 +204,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 */
> > @@ -225,16 +243,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");
> > @@ -250,29 +258,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");
> > @@ -284,22 +271,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;
> >  }
> > @@ -371,7 +348,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 230f531400ca..78c3c77cd226 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 d351e5a44768..c80ad8299c5e 100644
> > --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> > +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> > @@ -932,7 +932,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);
> > @@ -947,7 +947,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);
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> > index 18f93979e14a..6f9cf7725529 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -305,6 +305,7 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
> >       struct device *dev = &pdev->dev;
> >       struct platform_device *clks;
> >       struct platform_device *drm;
> > +     struct platform_device *mdp;
> >       void __iomem *config_regs;
> >       int ret;
> >
> > @@ -328,10 +329,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[] = {
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/4] mtk-mdp: soc: mediatek: register mdp from mmsys

[Enric Balletbo i Serra]

Hi Eizan,


On 3/5/21 8:42, Eizan Miyamoto wrote:
> On Fri, Apr 30, 2021 at 1:46 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>>
>> Hi Eizan,
>>
>> Thank you for your patch.
>>
>> On 23/4/21 7:58, Eizan Miyamoto wrote:
>>> Rather than hanging the MDP master component driver off of the rdma0
>>> device, create a "virtual" device by the mmsys driver instead which is
>>> probed by the mtk_mdp_core driver.
>>>
>>> Broadly, four interdependent things are done by this change:
>>> - A virtual device that is probed by the mtk_mdp_core driver is
>>>   instantiated by the mtk_mmsys driver.
>>> - Presence of a mediatek,vpu property in a child node to the mmsys
>>>   device node is used to determine what device to use when dispatching
>>>   dma ops from the relevant ioctl.
>>> - v4l-related setup is moved into from the mtk_mdp_core driver to the
>>>   mtk_mdp_comp driver.
>>>
>>> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
>>> ---
>>>
>>>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 47 +++++++++-----
>>>  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 +-
>>>  drivers/soc/mediatek/mtk-mmsys.c              | 20 +++++-
>>>  5 files changed, 75 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>>> index d447bfaadef4..dc5231a1fcfd 100644
>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
>>> @@ -106,8 +106,41 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
>>>  {
>>>       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.
>>
>> Please use c-style comments here.
> 
> Thank you for the reminder, I'll update these in the next version of
> this patch series.
> 
>>
>>> +     vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
>>
>> That's a bit confusing to me, please correct me if I am wrong, so, the
>> mediatek,vpu property is used to tell the code that this component should be the
>> 'vpu master', not to point a vpu node in the DT? I understood correctly?
> 
> Is what you mean by 'vpu master' is that it is the device whose driver
> implements the wdt reset function? In that case, the mtk_mdp_core
> driver is still the 'vpu master' because mtk_mdp_reset_handler
> (contained in mtk_mdp_core) is passed to vpu_wdt_reg_handler(). The
> presence of the property in any MDP component device node can do the
> job of passing the vpu device (obtained from the node being pointed
> to) back mtk_mdp_core's mtk_mdp_master_bind() function.
> 
> *However*, I'm using the presence of that property to indicate another
> thing: this is the device that the mdp filesystem device node in /dev
> should be registered against for v4l2. We will need to save this
> device for later (in mdp->rdma_dev) to be used to find the DMA
> callbacks when a call to mtk_mdp_m2m_queue_init is made from the file
> open() callback (mtk_mdp_m2m_open) attached to the filesystem device
> node.
> 
> Before this change, the mtk_mdp_core driver was serving triple duty as
> the driver for the device that provided DMA op callbacks, the vpu
> master, and the MDP component master. Now it is the vpu master and the
> MDP component master, but not the driver for the device that provides
> DMA op callbacks.
> 
>>
>>
>>> +     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.
>>
>> Isn't rdma0 always the primary MDP device? or there are SoCs or configurations
>> where this is different? At least I think it is for MT8173 and MT8183.
> 
> I suppose you're right, though now it seems to be called mdp_rdma0 in
> the device tree?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?id=9ccce092fc64d19504fa54de4fd659e279cc92e7#n1004
> 
> Maybe somebody from MediaTek can confirm this?

That's the case on all the devices that are upstream, so maybe we can just
assume that for now if nobody from MediaTek confirms. I am in the opinion that
we should avoid use the mediatek,vpu property if is possible.

> 
>>
>>> +             mdp->rdma_dev = dev;
>>> +     }
>>> +
>>>       pm_runtime_enable(dev);
>>>
>>>       return 0;
>>> @@ -164,23 +197,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 5e71496e2517..4d7aa4e26be6 100644
>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
>>> @@ -121,6 +121,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);
>>> @@ -133,6 +144,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:
>>> @@ -191,8 +204,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 */
>>> @@ -225,16 +243,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");
>>> @@ -250,29 +258,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");
>>> @@ -284,22 +271,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;
>>>  }
>>> @@ -371,7 +348,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 230f531400ca..78c3c77cd226 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 d351e5a44768..c80ad8299c5e 100644
>>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
>>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
>>> @@ -932,7 +932,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);
>>> @@ -947,7 +947,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);
>>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
>>> index 18f93979e14a..6f9cf7725529 100644
>>> --- a/drivers/soc/mediatek/mtk-mmsys.c
>>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
>>> @@ -305,6 +305,7 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
>>>       struct device *dev = &pdev->dev;
>>>       struct platform_device *clks;
>>>       struct platform_device *drm;
>>> +     struct platform_device *mdp;
>>>       void __iomem *config_regs;
>>>       int ret;
>>>
>>> @@ -328,10 +329,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[] = {
>>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 4/4] mtk-mdp: soc: mediatek: register mdp from mmsys

[houlong wei]

On Fri, 2021-05-14 at 18:14 +0800, Enric Balletbo i Serra wrote:
> Hi Eizan,
> 
> 
> On 3/5/21 8:42, Eizan Miyamoto wrote:
> > On Fri, Apr 30, 2021 at 1:46 AM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> wrote:
> >>
> >> Hi Eizan,
> >>
> >> Thank you for your patch.
> >>
> >> On 23/4/21 7:58, Eizan Miyamoto wrote:
> >>> Rather than hanging the MDP master component driver off of the rdma0
> >>> device, create a "virtual" device by the mmsys driver instead which is
> >>> probed by the mtk_mdp_core driver.
> >>>
> >>> Broadly, four interdependent things are done by this change:
> >>> - A virtual device that is probed by the mtk_mdp_core driver is
> >>>   instantiated by the mtk_mmsys driver.
> >>> - Presence of a mediatek,vpu property in a child node to the mmsys
> >>>   device node is used to determine what device to use when dispatching
> >>>   dma ops from the relevant ioctl.
> >>> - v4l-related setup is moved into from the mtk_mdp_core driver to the
> >>>   mtk_mdp_comp driver.
> >>>
> >>> Signed-off-by: Eizan Miyamoto <eizan@chromium.org>
> >>> ---
> >>>
> >>>  drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 47 +++++++++-----
> >>>  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 +-
> >>>  drivers/soc/mediatek/mtk-mmsys.c              | 20 +++++-
> >>>  5 files changed, 75 insertions(+), 60 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >>> index d447bfaadef4..dc5231a1fcfd 100644
> >>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
> >>> @@ -106,8 +106,41 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
> >>>  {
> >>>       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.
> >>
> >> Please use c-style comments here.
> > 
> > Thank you for the reminder, I'll update these in the next version of
> > this patch series.
> > 
> >>
> >>> +     vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
> >>
> >> That's a bit confusing to me, please correct me if I am wrong, so, the
> >> mediatek,vpu property is used to tell the code that this component should be the
> >> 'vpu master', not to point a vpu node in the DT? I understood correctly?
> > 
> > Is what you mean by 'vpu master' is that it is the device whose driver
> > implements the wdt reset function? In that case, the mtk_mdp_core
> > driver is still the 'vpu master' because mtk_mdp_reset_handler
> > (contained in mtk_mdp_core) is passed to vpu_wdt_reg_handler(). The
> > presence of the property in any MDP component device node can do the
> > job of passing the vpu device (obtained from the node being pointed
> > to) back mtk_mdp_core's mtk_mdp_master_bind() function.
> > 
> > *However*, I'm using the presence of that property to indicate another
> > thing: this is the device that the mdp filesystem device node in /dev
> > should be registered against for v4l2. We will need to save this
> > device for later (in mdp->rdma_dev) to be used to find the DMA
> > callbacks when a call to mtk_mdp_m2m_queue_init is made from the file
> > open() callback (mtk_mdp_m2m_open) attached to the filesystem device
> > node.
> > 
> > Before this change, the mtk_mdp_core driver was serving triple duty as
> > the driver for the device that provided DMA op callbacks, the vpu
> > master, and the MDP component master. Now it is the vpu master and the
> > MDP component master, but not the driver for the device that provides
> > DMA op callbacks.
> > 
> >>
> >>
> >>> +     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.
> >>
> >> Isn't rdma0 always the primary MDP device? or there are SoCs or configurations
> >> where this is different? At least I think it is for MT8173 and MT8183.
> > 
> > I suppose you're right, though now it seems to be called mdp_rdma0 in
> > the device tree?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?id=9ccce092fc64d19504fa54de4fd659e279cc92e7#n1004
> > 
> > Maybe somebody from MediaTek can confirm this?
> 
> That's the case on all the devices that are upstream, so maybe we can just
> assume that for now if nobody from MediaTek confirms. I am in the opinion that
> we should avoid use the mediatek,vpu property if is possible.
> 

Hi Enric,
You are right! Currently, for the upstream MDP driver, the mdp_rdma
device node always stands for the primary MDP device. In some projects,
there may be more than one mdp_rdma device nodes, so there may be more
than one MDP device.
In original code, the 'vpu_dev' is retrieved from the return value of
vpu_get_plat_device(). Maybe we don't have to implement the duplicated
code to parse it again.But the 'vpu_dev' is necessary for
vpu_wdt_reg_handler(), vpu_ipi_register() and vpi_ipi_send().

Regards,
Houlong

> > 
> >>
> >>> +             mdp->rdma_dev = dev;
> >>> +     }
> >>> +
> >>>       pm_runtime_enable(dev);
> >>>
> >>>       return 0;
> >>> @@ -164,23 +197,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 5e71496e2517..4d7aa4e26be6 100644
> >>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
> >>> @@ -121,6 +121,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);
> >>> @@ -133,6 +144,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:
> >>> @@ -191,8 +204,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 */
> >>> @@ -225,16 +243,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");
> >>> @@ -250,29 +258,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");
> >>> @@ -284,22 +271,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;
> >>>  }
> >>> @@ -371,7 +348,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 230f531400ca..78c3c77cd226 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 d351e5a44768..c80ad8299c5e 100644
> >>> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> >>> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> >>> @@ -932,7 +932,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);
> >>> @@ -947,7 +947,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);
> >>> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> >>> index 18f93979e14a..6f9cf7725529 100644
> >>> --- a/drivers/soc/mediatek/mtk-mmsys.c
> >>> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> >>> @@ -305,6 +305,7 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
> >>>       struct device *dev = &pdev->dev;
> >>>       struct platform_device *clks;
> >>>       struct platform_device *drm;
> >>> +     struct platform_device *mdp;
> >>>       void __iomem *config_regs;
> >>>       int ret;
> >>>
> >>> @@ -328,10 +329,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[] = {
> >>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel