On Wed, 2021-08-25 at 17:07 +0800, Enric Balletbo i Serra wrote:

Hi Houlong,

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

On 25/8/21 10:43, houlong wei wrote:

quoted

Hi Eizan,

Thanks for you patch. I have inline comment below.

Regards,
Houlong

On Wed, 2021-08-25 at 14:33 +0800, Eizan Miyamoto wrote:

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>
Reviewed-by: Enric Balletbo i Serra <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;
+	}
+
+	return dev->of_node == mdp_rdma0_node;

About how to determine the master mdp driver, we also can
judge the component type. The component type can be gotten by calling
of_device_get_match_data(dev). If the component is MTK_MDP_RDMA, it is
the master driver. No matter it is mdp_rdma0 or mdp_rdma1.

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

Hi Enric,

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

Regards,
Houlong

quoted

http://lists.infradead.org/pipermail/linux-mediatek/2021-August/028533.html

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

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

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

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

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

What about MT8183 and other SoCs?

Thanks,
  Enric

Hi Enric,

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

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

Regards,
Houlong

quoted

quoted

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

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

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

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

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

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

quoted

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

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