2021-06-18 09:16:46

by Eizan Miyamoto

[permalink] [raw]
Subject: [PATCH v3 0/8] Refactor MTK MDP driver into core/components


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

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


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

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

Eizan Miyamoto (8):
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
media: mtk-mdp: search for vpu node instead of linking it to a
property
media: mtk-mdp: propagate errors better in pm_suspend/resume
media: mtk-mdp: use mdp-rdma0 alias to point to MDP master
dts: mtk-mdp: remove mediatek,vpu property from primary MDP device

arch/arm64/boot/dts/mediatek/mt8173.dtsi | 1 -
drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 258 ++++++++++++++--
drivers/media/platform/mtk-mdp/mtk_mdp_comp.h | 35 +--
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 281 ++++++++++++------
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 +-
7 files changed, 466 insertions(+), 136 deletions(-)

--
2.32.0.288.g62a8d224e6-goog


2021-06-18 09:16:51

by Eizan Miyamoto

[permalink] [raw]
Subject: [PATCH v3 1/8] mtk-mdp: add driver to probe mdp components

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 <[email protected]>
---

(no changes since v1)

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, 254 insertions(+), 87 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..aced0029b7cd 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,69 @@ 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:
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 <[email protected]>");
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.32.0.288.g62a8d224e6-goog

2021-06-18 09:16:57

by Eizan Miyamoto

[permalink] [raw]
Subject: [PATCH v3 2/8] mtk-mdp: use pm_runtime in MDP component driver

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 <[email protected]>
---

(no changes since v1)

drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 21 ++++++++++++++-----
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(+), 11 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index aced0029b7cd..8cebc3565826 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,26 @@ 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,
- "failed to enable clock, err %d. i:%d\n",
- err, i);
+ dev_err(comp->dev, "failed to enable clock, err %d. i:%d\n", err, i);
}
}

-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 +81,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 +92,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 +103,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 +121,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.32.0.288.g62a8d224e6-goog

2021-06-18 10:44:17

by Eizan Miyamoto

[permalink] [raw]
Subject: [PATCH v3 5/8] media: mtk-mdp: search for vpu node instead of linking it to a property

Since there is only one vpu node, it suffices to search for it instead
of having a link coded into the primary mdp device node.

Signed-off-by: Eizan Miyamoto <[email protected]>
---

(no changes since v1)

drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 6 ----
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 28 +++++++++++++------
2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index fdbb46a5330a..3d9bb7265159 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -117,12 +117,6 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
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");
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
index 267e4cb4bf32..48e9a557c446 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -113,6 +113,7 @@ static void release_of(struct device *dev, void *data)
static int mtk_mdp_master_bind(struct device *dev)
{
int status;
+ struct device_node *vpu_node;
struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);

status = component_bind_all(dev, mdp);
@@ -121,15 +122,24 @@ 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");
+ vpu_node = of_find_node_by_name(NULL, "vpu");
+ if (!vpu_node) {
+ dev_err(dev, "unable to find vpu node");
+ status = -ENODEV;
+ goto err_wdt_reg;
+ }
+
+ mdp->vpu_dev = of_find_device_by_node(vpu_node);
+ if (!mdp->vpu_dev) {
+ dev_err(dev, "unable to find vpu device");
+ status = -ENODEV;
+ goto err_wdt_reg;
+ }
+
+ status = vpu_wdt_reg_handler(mdp->vpu_dev, mtk_mdp_reset_handler, mdp, VPU_RST_MDP);
+ if (status) {
+ dev_err(dev, "Failed to register reset handler\n");
+ goto err_wdt_reg;
}

status = mtk_mdp_register_m2m_device(mdp);
--
2.32.0.288.g62a8d224e6-goog

2021-06-18 10:45:19

by Eizan Miyamoto

[permalink] [raw]
Subject: [PATCH v3 7/8] media: mtk-mdp: use mdp-rdma0 alias to point to MDP master

... 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 <[email protected]>
---

(no changes since v1)

drivers/media/platform/mtk-mdp/mtk_mdp_comp.c | 47 ++++++++++++++-----
drivers/media/platform/mtk-mdp/mtk_mdp_core.c | 6 +++
2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 59911ee063c1..2a71c582d8b1 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -141,22 +141,48 @@ void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
}
}

+/*
+ * The MDP master device node is identified by the device tree alias
+ * "mdp-rdma0".
+ */
+static int is_mdp_master(struct device *dev)
+{
+ struct device_node *aliases, *mdp_rdma0_node;
+ const char *mdp_rdma0_path;
+
+ if (!dev->of_node)
+ return 0;
+
+ aliases = of_find_node_by_path("/aliases");
+ if (!aliases) {
+ dev_err(dev, "no aliases found for mdp-rdma0");
+ return 0;
+ }
+
+ 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 0;
+ }
+
+ 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 0;
+ }
+
+ 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;

ret = v4l2_device_register(dev, &mdp->v4l2_dev);
@@ -172,9 +198,8 @@ static int mtk_mdp_comp_bind(struct device *dev, struct device *master,
}

/*
- * 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 523e3c2798e1..be7d35b3e3ff 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -157,6 +157,12 @@ static int mtk_mdp_master_bind(struct device *dev)
goto err_component_bind_all;
}

+ 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");
--
2.32.0.288.g62a8d224e6-goog

2021-06-18 11:26:56

by Eizan Miyamoto

[permalink] [raw]
Subject: [PATCH v3 6/8] media: mtk-mdp: propagate errors better in pm_suspend/resume

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

Signed-off-by: Eizan Miyamoto <[email protected]>
---

(no changes since v1)

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

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
index 3d9bb7265159..59911ee063c1 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c
@@ -53,39 +53,81 @@ 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_power_on(struct mtk_mdp_comp *comp)
+int mtk_mdp_comp_power_on(struct mtk_mdp_comp *comp)
{
- int err;
+ int status, err;

err = pm_runtime_get_sync(comp->dev);
- if (err < 0)
+ if (err < 0) {
dev_err(comp->dev, "failed to runtime get, err %d.\n", err);
+ return err;
+ }
+
+ err = mtk_mdp_comp_clock_on(comp);
+ if (err) {
+ dev_err(comp->dev, "failed to turn on clock. err=%d", err);
+ status = err;
+ goto err_mtk_mdp_comp_clock_on;
+ }
+
+ return err;
+
+err_mtk_mdp_comp_clock_on:
+ err = pm_runtime_put_sync(comp->dev);
+ if (err)
+ dev_err(comp->dev, "failed to runtime put in cleanup. err=%d", err);

- mtk_mdp_comp_clock_on(comp);
+ return status;
}

-void mtk_mdp_comp_power_off(struct mtk_mdp_comp *comp)
+int mtk_mdp_comp_power_off(struct mtk_mdp_comp *comp)
{
- int err;
+ int status, err;

mtk_mdp_comp_clock_off(comp);

err = pm_runtime_put_sync(comp->dev);
- if (err < 0)
+ if (err < 0) {
dev_err(comp->dev, "failed to runtime put, err %d.\n", err);
+ status = err;
+ goto err_pm_runtime_put_sync;
+ }
+
+ return 0;
+
+err_pm_runtime_put_sync:
+ err = mtk_mdp_comp_clock_on(comp);
+ if (err)
+ dev_err(comp->dev, "failed to turn on clock in cleanup. err=%d", err);
+
+ return status;
}

-void mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
+int mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp)
{
- int i, err;
+ int i, err, status;

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)
+ if (err) {
+ status = err;
dev_err(comp->dev, "failed to enable clock, err %d. i:%d\n", err, i);
+ goto err_clk_prepare_enable;
+ }
+ }
+
+ return 0;
+
+err_clk_prepare_enable:
+ for (--i; i >= 0; i--) {
+ if (IS_ERR(comp->clk[i]))
+ continue;
+ clk_disable_unprepare(comp->clk[i]);
}
+
+ return status;
}

void mtk_mdp_comp_clock_off(struct mtk_mdp_comp *comp)
diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
index 7ad9b06bb11b..2bd229cc7eae 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_comp.h
@@ -21,10 +21,10 @@ 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);
+int mtk_mdp_comp_power_on(struct mtk_mdp_comp *comp);
+int mtk_mdp_comp_power_off(struct mtk_mdp_comp *comp);

-void mtk_mdp_comp_clock_on(struct mtk_mdp_comp *comp);
+int 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 48e9a557c446..523e3c2798e1 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.c
@@ -49,8 +49,9 @@ static const struct of_device_id mtk_mdp_of_ids[] = {
};
MODULE_DEVICE_TABLE(of, mtk_mdp_of_ids);

-static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
+static int mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
{
+ int err, status;
struct mtk_mdp_comp *comp_node;

/*
@@ -58,18 +59,41 @@ static void mtk_mdp_clock_on(struct mtk_mdp_dev *mdp)
* 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);
+ err = mtk_mdp_comp_clock_on(&mdp->comp_self);
+ if (err)
+ return err;
+
+ list_for_each_entry(comp_node, &mdp->comp_list, node) {
+ err = mtk_mdp_comp_power_on(comp_node);
+ if (err) {
+ status = err;
+ goto err_mtk_mdp_comp_power_on;
+ }
+ }

- list_for_each_entry(comp_node, &mdp->comp_list, node)
- mtk_mdp_comp_power_on(comp_node);
+ return 0;
+
+err_mtk_mdp_comp_power_on:
+ list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node) {
+ err = mtk_mdp_comp_power_off(comp_node);
+ if (err)
+ dev_err(&mdp->pdev->dev, "failed to power off after error. err=%d", err);
+ }
+ return status;
}

-static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
+static int mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
{
+ int status, err;
struct mtk_mdp_comp *comp_node;

- list_for_each_entry(comp_node, &mdp->comp_list, node)
- mtk_mdp_comp_power_off(comp_node);
+ list_for_each_entry(comp_node, &mdp->comp_list, node) {
+ err = mtk_mdp_comp_power_off(comp_node);
+ if (err) {
+ status = err;
+ goto err_mtk_mdp_comp_power_off;
+ }
+ }

/*
* The master / rdma0 component will have pm_runtime_put called
@@ -77,6 +101,17 @@ static void mtk_mdp_clock_off(struct mtk_mdp_dev *mdp)
* have mtk_mdp_comp_power_off called on it.
*/
mtk_mdp_comp_clock_off(&mdp->comp_self);
+
+ return 0;
+
+err_mtk_mdp_comp_power_off:
+ list_for_each_entry_continue_reverse(comp_node, &mdp->comp_list, node) {
+ err = mtk_mdp_comp_power_on(comp_node);
+ if (err)
+ dev_err(&mdp->pdev->dev, "failed to power on after error. err=%d", err);
+ }
+
+ return status;
}

static void mtk_mdp_wdt_worker(struct work_struct *work)
@@ -317,18 +352,14 @@ static int __maybe_unused mtk_mdp_pm_suspend(struct device *dev)
{
struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);

- mtk_mdp_clock_off(mdp);
-
- return 0;
+ return mtk_mdp_clock_off(mdp);
}

static int __maybe_unused mtk_mdp_pm_resume(struct device *dev)
{
struct mtk_mdp_dev *mdp = dev_get_drvdata(dev);

- mtk_mdp_clock_on(mdp);
-
- return 0;
+ return mtk_mdp_clock_on(mdp);
}

static int __maybe_unused mtk_mdp_suspend(struct device *dev)
--
2.32.0.288.g62a8d224e6-goog