On Mon, 2021-08-16 at 11:00 +0800, houlong wei wrote:

On Mon, 2021-08-02 at 20:12 +0800, Eizan Miyamoto wrote:

quoted

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

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

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

Signed-off-by: Eizan Miyamoto <redacted>
---

(no changes since v1)

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

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 85ef274841a3..9527649de98e 100644

--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c

@@ -151,29 +151,50 @@ void mtk_mdp_comp_clock_off(struct

mtk_mdp_comp
*comp)
 		mtk_smi_larb_put(comp->larb_dev);
 }
 
-static int mtk_mdp_comp_bind(struct device *dev, struct device
*master, void *data)
+/*
+ * The MDP master device node is identified by the device tree
alias
+ * "mdp-rdma0".
+ */
+static bool is_mdp_master(struct device *dev)
+{
+	struct device_node *aliases, *mdp_rdma0_node;
+	const char *mdp_rdma0_path;
+
+	if (!dev->of_node)
+		return false;
+
+	aliases = of_find_node_by_path("/aliases");
+	if (!aliases) {
+		dev_err(dev, "no aliases found for mdp-rdma0");
+		return false;
+	}
+
+	mdp_rdma0_path = of_get_property(aliases, "mdp-rdma0", NULL);
+	if (!mdp_rdma0_path) {
+		dev_err(dev, "get mdp-rdma0 property of /aliases
failed");
+		return false;
+	}
+
+	mdp_rdma0_node = of_find_node_by_path(mdp_rdma0_path);
+	if (!mdp_rdma0_node) {
+		dev_err(dev, "path resolution failed for %s",
mdp_rdma0_path);
+		return false;
+	}
+

Hi Eizan,

"mdp-rdma0" may be not the only one master device node. In fact,
there
are 2 "mdp-rdma" in mt8173. You can see "mdp_rdma1" via below link.

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.13.11#n1016

If we add "mediatek,mt8173-mdp" to "mdp_rdma1" like below, we will
have
one more V4L2 video devie node.

		mdp_rdma1: rdma@14002000 {
			compatible = "mediatek,mt8173-mdp-rdma",
				     "mediatek,mt8173-mdp";
			...
		}

We should consider the case that there are more than one "MDP_RDMA"
components. 

Regards,
Houlong

quoted

+	return dev->of_node == mdp_rdma0_node;
+}
+
+static int mtk_mdp_comp_bind(struct device *dev, struct device
*master,
+			void *data)
 {
 	struct mtk_mdp_comp *comp = dev_get_drvdata(dev);
 	struct mtk_mdp_dev *mdp = data;
-	struct device_node *vpu_node;
 
 	mtk_mdp_register_component(mdp, comp);
 
-	/*
-	 * If this component has a "mediatek-vpu" property, it is
responsible for
-	 * notifying the mdp master driver about it so it can be
further initialized
-	 * later.
-	 */
-	vpu_node = of_parse_phandle(dev->of_node, "mediatek,vpu", 0);
-	if (vpu_node) {
+	if (is_mdp_master(dev)) {
 		int ret;
 
-		mdp->vpu_dev = of_find_device_by_node(vpu_node);
-		if (WARN_ON(!mdp->vpu_dev)) {
-			dev_err(dev, "vpu pdev failed\n");
-			of_node_put(vpu_node);
-		}
-
 		ret = v4l2_device_register(dev, &mdp->v4l2_dev);
 		if (ret) {
 			dev_err(dev, "Failed to register v4l2
device\n");

@@ -187,9 +208,8 @@ static int mtk_mdp_comp_bind(struct device

*dev,
struct device *master, void *da
 		}
 
 		/*
-		 * presence of the "mediatek,vpu" property in a device
node
-		 * indicates that it is the primary MDP rdma device and
MDP DMA
-		 * ops should be handled by its DMA callbacks.
+		 * MDP DMA ops will be handled by the DMA callbacks
associated with this
+		 * device;
 		 */
 		mdp->rdma_dev = dev;
 	}

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index 50eafcc9993d..6a775463399c 100644

--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c

@@ -150,8 +150,9 @@ static void release_of(struct device *dev, void

*data)
 
 static int mtk_mdp_master_bind(struct device *dev)
 {
-	int status;
 	struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);
+	struct device_node *vpu_node;
+	int status;
 
 	status = component_bind_all(dev, mdp);
 	if (status) {

@@ -159,15 +160,30 @@ static int mtk_mdp_master_bind(struct device

*dev)
 		goto err_component_bind_all;
 	}
 
-	if (mdp->vpu_dev) {
-		int ret = vpu_wdt_reg_handler(mdp->vpu_dev,
mtk_mdp_reset_handler, mdp,
-					  VPU_RST_MDP);
-		if (ret) {
-			dev_err(dev, "Failed to register reset
handler\n");
-			goto err_wdt_reg;
-		}
-	} else {
-		dev_err(dev, "no vpu_dev found\n");
+	if (mdp->rdma_dev == NULL) {
+		dev_err(dev, "Primary MDP device not found");
+		status = -ENODEV;
+		goto err_component_bind_all;
+	}
+
+	vpu_node = of_find_node_by_name(NULL, "vpu");
+	if (!vpu_node) {
+		dev_err(dev, "unable to find vpu node");
+		status = -ENODEV;
+		goto err_wdt_reg;
+	}

Hi Eizan,

Is your removing "mediatek,vpu" necessary for this series "Refactor MTK
MDP driver into core/components" ?

In some MediaTek projects (not upstream yet), the device-tree node name
"vpu" may be changed to something like "vpu0", "vpu1" or other name,
which depends on the implementation of "mtk-vpu" driver. We can specify
the different phandle to "mediatek,vpu" in .dtsi file. If we use
of_find_node_by_name() to get the vpu_node, we have to modify the name
string in different project.
If the answer of my previous questions is "No", I prefer to slow down
the modification of removing "mediatek,vpu".

Sorry for late reply.

Regards,
Houlong

quoted

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek