All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time
@ 2018-11-10 11:16 Laurent Pinchart
  2018-11-10 11:16 ` [PATCH v3 1/4] drm/omap: Populate DSS children in omapdss driver Laurent Pinchart
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Laurent Pinchart @ 2018-11-10 11:16 UTC (permalink / raw)
  To: dri-devel; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen

Hello,

This series fixes crashes in the omapdss driver at both load and unload
time, due to runtime PM problems related to probe deferral. The bugs got
introduced in v4.20-rc, this should thus be considered as v4.20 fixes.

At the core of the problem comes commit 27d624527d99 ("drm/omap: dss:
Acquire next dssdev at probe time") which can cause probe deferral for
the DSS when the encoder and panel modules are not loaded yet. As the
omapdss module contains drivers for the DSS and all its children, this
results in the internal encoders being probed before the DSS. Missing
runtime PM handling around register access then caused imprecise
external aborts.

Patch 1/4 moves population of the DSS children from arch code to the
omapdss driver. This prevents the DSS children from being probed before
the DSS. The change can be considered as a workaround in the sense that
runtime PM of the DSS children should operate correctly even when the
DSS probe fail. However, given that reducing the amount of arch code is
an improvement in itself, I believe the solution to be acceptable.

Patches 2/4 and 3/4 then ensure that the HDMI4 and DSI devices are
active at bind and probe time respectively, in order to access hardware
registers there.

Patch 4/4 finally fixes the internal encoders crash in their runtime PM
handlers by moving DISPC runtime PM state management to omapdrm. This is
the biggest change compared to v3 that went for a less intrusive (in my
opinion at least) workaround. As Tomi and Tony were in favour of a real
fix instead of a workaround, I gave the RFC fix more testing on
Beagleboard, Pandaboard and AM57xx EVM and concluded it should be safe.

Patch 1/4 touches both the mach-omap2 and omapdss, and has been acked by
Tony to be merged through the DRM tree.

Laurent Pinchart (4):
  drm/omap: Populate DSS children in omapdss driver
  drm/omap: hdmi4: Ensure the device is active during bind
  drm/omap: dsi: Ensure the device is active during probe
  drm/omap: Move DISPC runtime PM handling to omapdrm

 arch/arm/mach-omap2/display.c       | 111 +++++++++++++---------------
 drivers/gpu/drm/omapdrm/dss/dsi.c   |  14 ++--
 drivers/gpu/drm/omapdrm/dss/dss.c   |  11 ++-
 drivers/gpu/drm/omapdrm/dss/hdmi4.c |  37 +++-------
 drivers/gpu/drm/omapdrm/dss/hdmi5.c |  27 -------
 drivers/gpu/drm/omapdrm/dss/venc.c  |   7 --
 drivers/gpu/drm/omapdrm/omap_crtc.c |   6 ++
 7 files changed, 83 insertions(+), 130 deletions(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/4] drm/omap: Populate DSS children in omapdss driver
  2018-11-10 11:16 [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Laurent Pinchart
@ 2018-11-10 11:16 ` Laurent Pinchart
  2018-11-11  0:45   ` Sebastian Reichel
  2018-11-10 11:16 ` [PATCH v3 2/4] drm/omap: hdmi4: Ensure the device is active during bind Laurent Pinchart
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2018-11-10 11:16 UTC (permalink / raw)
  To: dri-devel; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen

The DSS DT node contains children that describe the DSS components
(DISPC and internal encoders). Each of those components is handled by a
platform driver, and thus needs to be backed by a platform device.

The corresponding platform devices are created in mach-omap2 code by a
call to of_platform_populate(). While this approach has worked so far,
it doesn't model the hardware architecture very well, as it creates
child devices before the parent is ready to handle them. This would be
akin to creating I2C slaves before the I2C master is available.

The task can be easily performed in the omapdss driver code instead,
simplifying mach-omap2 code. We however can't remove the mach-omap2 code
completely as the omap2fb driver still depends on it, but we can move it
to the omap2fb-specific section, where it can stay until the omap2fb
driver gets removed.

This has the added benefit of not allowing DSS components to probe
before the DSS itself, which led to runtime PM issues when the DSS probe
is deferred.

Fixes: 27d624527d99 ("drm/omap: dss: Acquire next dssdev at probe time")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/mach-omap2/display.c     | 111 ++++++++++++++----------------
 drivers/gpu/drm/omapdrm/dss/dss.c |  11 ++-
 2 files changed, 63 insertions(+), 59 deletions(-)

diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index 9500b6e27380..f86b72d1d59e 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -209,11 +209,61 @@ static int __init omapdss_init_fbdev(void)
 
 	return 0;
 }
-#else
-static inline int omapdss_init_fbdev(void)
+
+static const char * const omapdss_compat_names[] __initconst = {
+	"ti,omap2-dss",
+	"ti,omap3-dss",
+	"ti,omap4-dss",
+	"ti,omap5-dss",
+	"ti,dra7-dss",
+};
+
+static struct device_node * __init omapdss_find_dss_of_node(void)
 {
-	return 0;
+	struct device_node *node;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(omapdss_compat_names); ++i) {
+		node = of_find_compatible_node(NULL, NULL,
+			omapdss_compat_names[i]);
+		if (node)
+			return node;
+	}
+
+	return NULL;
 }
+
+static int __init omapdss_init_of(void)
+{
+	int r;
+	struct device_node *node;
+	struct platform_device *pdev;
+
+	/* only create dss helper devices if dss is enabled in the .dts */
+
+	node = omapdss_find_dss_of_node();
+	if (!node)
+		return 0;
+
+	if (!of_device_is_available(node))
+		return 0;
+
+	pdev = of_find_device_by_node(node);
+
+	if (!pdev) {
+		pr_err("Unable to find DSS platform device\n");
+		return -ENODEV;
+	}
+
+	r = of_platform_populate(node, NULL, NULL, &pdev->dev);
+	if (r) {
+		pr_err("Unable to populate DSS submodule devices\n");
+		return r;
+	}
+
+	return omapdss_init_fbdev();
+}
+omap_device_initcall(omapdss_init_of);
 #endif /* CONFIG_FB_OMAP2 */
 
 static void dispc_disable_outputs(void)
@@ -361,58 +411,3 @@ int omap_dss_reset(struct omap_hwmod *oh)
 
 	return r;
 }
-
-static const char * const omapdss_compat_names[] __initconst = {
-	"ti,omap2-dss",
-	"ti,omap3-dss",
-	"ti,omap4-dss",
-	"ti,omap5-dss",
-	"ti,dra7-dss",
-};
-
-static struct device_node * __init omapdss_find_dss_of_node(void)
-{
-	struct device_node *node;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(omapdss_compat_names); ++i) {
-		node = of_find_compatible_node(NULL, NULL,
-			omapdss_compat_names[i]);
-		if (node)
-			return node;
-	}
-
-	return NULL;
-}
-
-static int __init omapdss_init_of(void)
-{
-	int r;
-	struct device_node *node;
-	struct platform_device *pdev;
-
-	/* only create dss helper devices if dss is enabled in the .dts */
-
-	node = omapdss_find_dss_of_node();
-	if (!node)
-		return 0;
-
-	if (!of_device_is_available(node))
-		return 0;
-
-	pdev = of_find_device_by_node(node);
-
-	if (!pdev) {
-		pr_err("Unable to find DSS platform device\n");
-		return -ENODEV;
-	}
-
-	r = of_platform_populate(node, NULL, NULL, &pdev->dev);
-	if (r) {
-		pr_err("Unable to populate DSS submodule devices\n");
-		return r;
-	}
-
-	return omapdss_init_fbdev();
-}
-omap_device_initcall(omapdss_init_of);
diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
index 1aaf260aa9b8..7553c7fc1c45 100644
--- a/drivers/gpu/drm/omapdrm/dss/dss.c
+++ b/drivers/gpu/drm/omapdrm/dss/dss.c
@@ -1484,16 +1484,23 @@ static int dss_probe(struct platform_device *pdev)
 						   dss);
 
 	/* Add all the child devices as components. */
+	r = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+	if (r)
+		goto err_uninit_debugfs;
+
 	omapdss_gather_components(&pdev->dev);
 
 	device_for_each_child(&pdev->dev, &match, dss_add_child_component);
 
 	r = component_master_add_with_match(&pdev->dev, &dss_component_ops, match);
 	if (r)
-		goto err_uninit_debugfs;
+		goto err_of_depopulate;
 
 	return 0;
 
+err_of_depopulate:
+	of_platform_depopulate(&pdev->dev);
+
 err_uninit_debugfs:
 	dss_debugfs_remove_file(dss->debugfs.clk);
 	dss_debugfs_remove_file(dss->debugfs.dss);
@@ -1522,6 +1529,8 @@ static int dss_remove(struct platform_device *pdev)
 {
 	struct dss_device *dss = platform_get_drvdata(pdev);
 
+	of_platform_depopulate(&pdev->dev);
+
 	component_master_del(&pdev->dev, &dss_component_ops);
 
 	dss_debugfs_remove_file(dss->debugfs.clk);
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/4] drm/omap: hdmi4: Ensure the device is active during bind
  2018-11-10 11:16 [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Laurent Pinchart
  2018-11-10 11:16 ` [PATCH v3 1/4] drm/omap: Populate DSS children in omapdss driver Laurent Pinchart
@ 2018-11-10 11:16 ` Laurent Pinchart
  2018-11-11  0:48   ` Sebastian Reichel
  2018-11-10 11:16 ` [PATCH v3 3/4] drm/omap: dsi: Ensure the device is active during probe Laurent Pinchart
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2018-11-10 11:16 UTC (permalink / raw)
  To: dri-devel; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen

The bind function performs hardware access (in hdmi4_cec_init()) and
thus requires the device to be active. Ensure this by surrounding the
bind function by hdmi_runtime_get() and hdmi_runtime_put() calls.

Fixes: 27d624527d99 ("drm/omap: dss: Acquire next dssdev at probe time")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v2:

- Call hdmi_runtime_put() instead of hdmi_runtime_get() in error path
---
 drivers/gpu/drm/omapdrm/dss/hdmi4.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index cf6230eac31a..073fa462930a 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -635,10 +635,14 @@ static int hdmi4_bind(struct device *dev, struct device *master, void *data)
 
 	hdmi->dss = dss;
 
-	r = hdmi_pll_init(dss, hdmi->pdev, &hdmi->pll, &hdmi->wp);
+	r = hdmi_runtime_get(hdmi);
 	if (r)
 		return r;
 
+	r = hdmi_pll_init(dss, hdmi->pdev, &hdmi->pll, &hdmi->wp);
+	if (r)
+		goto err_runtime_put;
+
 	r = hdmi4_cec_init(hdmi->pdev, &hdmi->core, &hdmi->wp);
 	if (r)
 		goto err_pll_uninit;
@@ -652,12 +656,16 @@ static int hdmi4_bind(struct device *dev, struct device *master, void *data)
 	hdmi->debugfs = dss_debugfs_create_file(dss, "hdmi", hdmi_dump_regs,
 					       hdmi);
 
+	hdmi_runtime_put(hdmi);
+
 	return 0;
 
 err_cec_uninit:
 	hdmi4_cec_uninit(&hdmi->core);
 err_pll_uninit:
 	hdmi_pll_uninit(&hdmi->pll);
+err_runtime_put:
+	hdmi_runtime_put(hdmi);
 	return r;
 }
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 3/4] drm/omap: dsi: Ensure the device is active during probe
  2018-11-10 11:16 [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Laurent Pinchart
  2018-11-10 11:16 ` [PATCH v3 1/4] drm/omap: Populate DSS children in omapdss driver Laurent Pinchart
  2018-11-10 11:16 ` [PATCH v3 2/4] drm/omap: hdmi4: Ensure the device is active during bind Laurent Pinchart
@ 2018-11-10 11:16 ` Laurent Pinchart
  2018-11-11  0:48   ` Sebastian Reichel
  2018-11-10 11:16 ` [PATCH v3 4/4] drm/omap: Move DISPC runtime PM handling to omapdrm Laurent Pinchart
  2018-11-12 10:05 ` [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Tomi Valkeinen
  4 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2018-11-10 11:16 UTC (permalink / raw)
  To: dri-devel; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen

The probe function performs hardware access to read the number of
supported data lanes from a configuration register and thus requires the
device to be active. Ensure this by surrounding the access with
dsi_runtime_get() and dsi_runtime_put() calls.

Fixes: edb715dffdee ("drm/omap: dss: dsi: Move initialization code from bind to probe")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpu/drm/omapdrm/dss/dsi.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 394c129cfb3b..b9d5ad7e67d8 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -5409,11 +5409,14 @@ static int dsi_probe(struct platform_device *pdev)
 
 	/* DSI on OMAP3 doesn't have register DSI_GNQ, set number
 	 * of data to 3 by default */
-	if (dsi->data->quirks & DSI_QUIRK_GNQ)
+	if (dsi->data->quirks & DSI_QUIRK_GNQ) {
+		dsi_runtime_get(dsi);
 		/* NB_DATA_LANES */
 		dsi->num_lanes_supported = 1 + REG_GET(dsi, DSI_GNQ, 11, 9);
-	else
+		dsi_runtime_put(dsi);
+	} else {
 		dsi->num_lanes_supported = 3;
+	}
 
 	r = dsi_init_output(dsi);
 	if (r)
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 4/4] drm/omap: Move DISPC runtime PM handling to omapdrm
  2018-11-10 11:16 [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Laurent Pinchart
                   ` (2 preceding siblings ...)
  2018-11-10 11:16 ` [PATCH v3 3/4] drm/omap: dsi: Ensure the device is active during probe Laurent Pinchart
@ 2018-11-10 11:16 ` Laurent Pinchart
  2018-11-11  1:45   ` Sebastian Reichel
  2018-11-12 10:05 ` [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Tomi Valkeinen
  4 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2018-11-10 11:16 UTC (permalink / raw)
  To: dri-devel; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen

The internal encoders (DSI, HDMI4, HDMI5 and VENC) runtime PM handlers
attempt to manage the runtime PM state of the connected DISPC, based on
the rationale that the DISPC providing data to the encoders requires
ensuring that the display is active whenever the encoders are active.

While the DISPC provides data to the encoders, it doesn't as such
constitute a resource that encoders require in order to be taken out
of suspend, contrary to for instance a functional clock or a power
supply. Encoders registers can be accessed without the DISPC being
active, and while the encoders will not output any video stream without
being fed by the DISPC, the DISPC PM state doesn't influence the
encoders PM state.

For this reason the DISPC PM state is better managed from the omapdrm
driver, in the CRTC enable and disable operations. This allows the
encoders PM state to be handled separately from the DISPC, and in
particular at times when the DISPC may not be available (for instance at
probe due to the DSS probe being deferred, or at remove time du to the
DISPC being already removed).

Fixes: edb715dffdee ("drm/omap: dss: dsi: Move initialization code from bind to probe")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/dss/dsi.c   |  7 -------
 drivers/gpu/drm/omapdrm/dss/hdmi4.c | 27 ---------------------------
 drivers/gpu/drm/omapdrm/dss/hdmi5.c | 27 ---------------------------
 drivers/gpu/drm/omapdrm/dss/venc.c  |  7 -------
 drivers/gpu/drm/omapdrm/omap_crtc.c |  6 ++++++
 5 files changed, 6 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index b9d5ad7e67d8..36123c086d97 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -5473,19 +5473,12 @@ static int dsi_runtime_suspend(struct device *dev)
 	/* wait for current handler to finish before turning the DSI off */
 	synchronize_irq(dsi->irq);
 
-	dispc_runtime_put(dsi->dss->dispc);
-
 	return 0;
 }
 
 static int dsi_runtime_resume(struct device *dev)
 {
 	struct dsi_data *dsi = dev_get_drvdata(dev);
-	int r;
-
-	r = dispc_runtime_get(dsi->dss->dispc);
-	if (r)
-		return r;
 
 	dsi->is_enabled = true;
 	/* ensure the irq handler sees the is_enabled value */
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
index 073fa462930a..aabdda394c9c 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
@@ -841,32 +841,6 @@ static int hdmi4_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int hdmi_runtime_suspend(struct device *dev)
-{
-	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
-
-	dispc_runtime_put(hdmi->dss->dispc);
-
-	return 0;
-}
-
-static int hdmi_runtime_resume(struct device *dev)
-{
-	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
-	int r;
-
-	r = dispc_runtime_get(hdmi->dss->dispc);
-	if (r < 0)
-		return r;
-
-	return 0;
-}
-
-static const struct dev_pm_ops hdmi_pm_ops = {
-	.runtime_suspend = hdmi_runtime_suspend,
-	.runtime_resume = hdmi_runtime_resume,
-};
-
 static const struct of_device_id hdmi_of_match[] = {
 	{ .compatible = "ti,omap4-hdmi", },
 	{},
@@ -877,7 +851,6 @@ struct platform_driver omapdss_hdmi4hw_driver = {
 	.remove		= hdmi4_remove,
 	.driver         = {
 		.name   = "omapdss_hdmi",
-		.pm	= &hdmi_pm_ops,
 		.of_match_table = hdmi_of_match,
 		.suppress_bind_attrs = true,
 	},
diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
index b0e4a7463f8c..9e8556f67a29 100644
--- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
+++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
@@ -825,32 +825,6 @@ static int hdmi5_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int hdmi_runtime_suspend(struct device *dev)
-{
-	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
-
-	dispc_runtime_put(hdmi->dss->dispc);
-
-	return 0;
-}
-
-static int hdmi_runtime_resume(struct device *dev)
-{
-	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
-	int r;
-
-	r = dispc_runtime_get(hdmi->dss->dispc);
-	if (r < 0)
-		return r;
-
-	return 0;
-}
-
-static const struct dev_pm_ops hdmi_pm_ops = {
-	.runtime_suspend = hdmi_runtime_suspend,
-	.runtime_resume = hdmi_runtime_resume,
-};
-
 static const struct of_device_id hdmi_of_match[] = {
 	{ .compatible = "ti,omap5-hdmi", },
 	{ .compatible = "ti,dra7-hdmi", },
@@ -862,7 +836,6 @@ struct platform_driver omapdss_hdmi5hw_driver = {
 	.remove		= hdmi5_remove,
 	.driver         = {
 		.name   = "omapdss_hdmi5",
-		.pm	= &hdmi_pm_ops,
 		.of_match_table = hdmi_of_match,
 		.suppress_bind_attrs = true,
 	},
diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c
index ff0b18c8e4ac..b5f52727f8b1 100644
--- a/drivers/gpu/drm/omapdrm/dss/venc.c
+++ b/drivers/gpu/drm/omapdrm/dss/venc.c
@@ -946,19 +946,12 @@ static int venc_runtime_suspend(struct device *dev)
 	if (venc->tv_dac_clk)
 		clk_disable_unprepare(venc->tv_dac_clk);
 
-	dispc_runtime_put(venc->dss->dispc);
-
 	return 0;
 }
 
 static int venc_runtime_resume(struct device *dev)
 {
 	struct venc_device *venc = dev_get_drvdata(dev);
-	int r;
-
-	r = dispc_runtime_get(venc->dss->dispc);
-	if (r < 0)
-		return r;
 
 	if (venc->tv_dac_clk)
 		clk_prepare_enable(venc->tv_dac_clk);
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 62928ec0e7db..caffc547ef97 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -350,11 +350,14 @@ static void omap_crtc_arm_event(struct drm_crtc *crtc)
 static void omap_crtc_atomic_enable(struct drm_crtc *crtc,
 				    struct drm_crtc_state *old_state)
 {
+	struct omap_drm_private *priv = crtc->dev->dev_private;
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 	int ret;
 
 	DBG("%s", omap_crtc->name);
 
+	priv->dispc_ops->runtime_get(priv->dispc);
+
 	spin_lock_irq(&crtc->dev->event_lock);
 	drm_crtc_vblank_on(crtc);
 	ret = drm_crtc_vblank_get(crtc);
@@ -367,6 +370,7 @@ static void omap_crtc_atomic_enable(struct drm_crtc *crtc,
 static void omap_crtc_atomic_disable(struct drm_crtc *crtc,
 				     struct drm_crtc_state *old_state)
 {
+	struct omap_drm_private *priv = crtc->dev->dev_private;
 	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
 
 	DBG("%s", omap_crtc->name);
@@ -379,6 +383,8 @@ static void omap_crtc_atomic_disable(struct drm_crtc *crtc,
 	spin_unlock_irq(&crtc->dev->event_lock);
 
 	drm_crtc_vblank_off(crtc);
+
+	priv->dispc_ops->runtime_put(priv->dispc);
 }
 
 static enum drm_mode_status omap_crtc_mode_valid(struct drm_crtc *crtc,
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 1/4] drm/omap: Populate DSS children in omapdss driver
  2018-11-10 11:16 ` [PATCH v3 1/4] drm/omap: Populate DSS children in omapdss driver Laurent Pinchart
@ 2018-11-11  0:45   ` Sebastian Reichel
  0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2018-11-11  0:45 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen, dri-devel


[-- Attachment #1.1: Type: text/plain, Size: 6055 bytes --]

Hi,

On Sat, Nov 10, 2018 at 01:16:51PM +0200, Laurent Pinchart wrote:
> The DSS DT node contains children that describe the DSS components
> (DISPC and internal encoders). Each of those components is handled by a
> platform driver, and thus needs to be backed by a platform device.
> 
> The corresponding platform devices are created in mach-omap2 code by a
> call to of_platform_populate(). While this approach has worked so far,
> it doesn't model the hardware architecture very well, as it creates
> child devices before the parent is ready to handle them. This would be
> akin to creating I2C slaves before the I2C master is available.
> 
> The task can be easily performed in the omapdss driver code instead,
> simplifying mach-omap2 code. We however can't remove the mach-omap2 code
> completely as the omap2fb driver still depends on it, but we can move it
> to the omap2fb-specific section, where it can stay until the omap2fb
> driver gets removed.
> 
> This has the added benefit of not allowing DSS components to probe
> before the DSS itself, which led to runtime PM issues when the DSS probe
> is deferred.
> 
> Fixes: 27d624527d99 ("drm/omap: dss: Acquire next dssdev at probe time")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Tony Lindgren <tony@atomide.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  arch/arm/mach-omap2/display.c     | 111 ++++++++++++++----------------
>  drivers/gpu/drm/omapdrm/dss/dss.c |  11 ++-
>  2 files changed, 63 insertions(+), 59 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
> index 9500b6e27380..f86b72d1d59e 100644
> --- a/arch/arm/mach-omap2/display.c
> +++ b/arch/arm/mach-omap2/display.c
> @@ -209,11 +209,61 @@ static int __init omapdss_init_fbdev(void)
>  
>  	return 0;
>  }
> -#else
> -static inline int omapdss_init_fbdev(void)
> +
> +static const char * const omapdss_compat_names[] __initconst = {
> +	"ti,omap2-dss",
> +	"ti,omap3-dss",
> +	"ti,omap4-dss",
> +	"ti,omap5-dss",
> +	"ti,dra7-dss",
> +};
> +
> +static struct device_node * __init omapdss_find_dss_of_node(void)
>  {
> -	return 0;
> +	struct device_node *node;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(omapdss_compat_names); ++i) {
> +		node = of_find_compatible_node(NULL, NULL,
> +			omapdss_compat_names[i]);
> +		if (node)
> +			return node;
> +	}
> +
> +	return NULL;
>  }
> +
> +static int __init omapdss_init_of(void)
> +{
> +	int r;
> +	struct device_node *node;
> +	struct platform_device *pdev;
> +
> +	/* only create dss helper devices if dss is enabled in the .dts */
> +
> +	node = omapdss_find_dss_of_node();
> +	if (!node)
> +		return 0;
> +
> +	if (!of_device_is_available(node))
> +		return 0;
> +
> +	pdev = of_find_device_by_node(node);
> +
> +	if (!pdev) {
> +		pr_err("Unable to find DSS platform device\n");
> +		return -ENODEV;
> +	}
> +
> +	r = of_platform_populate(node, NULL, NULL, &pdev->dev);
> +	if (r) {
> +		pr_err("Unable to populate DSS submodule devices\n");
> +		return r;
> +	}
> +
> +	return omapdss_init_fbdev();
> +}
> +omap_device_initcall(omapdss_init_of);
>  #endif /* CONFIG_FB_OMAP2 */
>  
>  static void dispc_disable_outputs(void)
> @@ -361,58 +411,3 @@ int omap_dss_reset(struct omap_hwmod *oh)
>  
>  	return r;
>  }
> -
> -static const char * const omapdss_compat_names[] __initconst = {
> -	"ti,omap2-dss",
> -	"ti,omap3-dss",
> -	"ti,omap4-dss",
> -	"ti,omap5-dss",
> -	"ti,dra7-dss",
> -};
> -
> -static struct device_node * __init omapdss_find_dss_of_node(void)
> -{
> -	struct device_node *node;
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(omapdss_compat_names); ++i) {
> -		node = of_find_compatible_node(NULL, NULL,
> -			omapdss_compat_names[i]);
> -		if (node)
> -			return node;
> -	}
> -
> -	return NULL;
> -}
> -
> -static int __init omapdss_init_of(void)
> -{
> -	int r;
> -	struct device_node *node;
> -	struct platform_device *pdev;
> -
> -	/* only create dss helper devices if dss is enabled in the .dts */
> -
> -	node = omapdss_find_dss_of_node();
> -	if (!node)
> -		return 0;
> -
> -	if (!of_device_is_available(node))
> -		return 0;
> -
> -	pdev = of_find_device_by_node(node);
> -
> -	if (!pdev) {
> -		pr_err("Unable to find DSS platform device\n");
> -		return -ENODEV;
> -	}
> -
> -	r = of_platform_populate(node, NULL, NULL, &pdev->dev);
> -	if (r) {
> -		pr_err("Unable to populate DSS submodule devices\n");
> -		return r;
> -	}
> -
> -	return omapdss_init_fbdev();
> -}
> -omap_device_initcall(omapdss_init_of);
> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
> index 1aaf260aa9b8..7553c7fc1c45 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dss.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dss.c
> @@ -1484,16 +1484,23 @@ static int dss_probe(struct platform_device *pdev)
>  						   dss);
>  
>  	/* Add all the child devices as components. */
> +	r = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +	if (r)
> +		goto err_uninit_debugfs;
> +
>  	omapdss_gather_components(&pdev->dev);
>  
>  	device_for_each_child(&pdev->dev, &match, dss_add_child_component);
>  
>  	r = component_master_add_with_match(&pdev->dev, &dss_component_ops, match);
>  	if (r)
> -		goto err_uninit_debugfs;
> +		goto err_of_depopulate;
>  
>  	return 0;
>  
> +err_of_depopulate:
> +	of_platform_depopulate(&pdev->dev);
> +
>  err_uninit_debugfs:
>  	dss_debugfs_remove_file(dss->debugfs.clk);
>  	dss_debugfs_remove_file(dss->debugfs.dss);
> @@ -1522,6 +1529,8 @@ static int dss_remove(struct platform_device *pdev)
>  {
>  	struct dss_device *dss = platform_get_drvdata(pdev);
>  
> +	of_platform_depopulate(&pdev->dev);
> +
>  	component_master_del(&pdev->dev, &dss_component_ops);
>  
>  	dss_debugfs_remove_file(dss->debugfs.clk);
> -- 
> Regards,
> 
> Laurent Pinchart
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/4] drm/omap: hdmi4: Ensure the device is active during bind
  2018-11-10 11:16 ` [PATCH v3 2/4] drm/omap: hdmi4: Ensure the device is active during bind Laurent Pinchart
@ 2018-11-11  0:48   ` Sebastian Reichel
  0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2018-11-11  0:48 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen, dri-devel


[-- Attachment #1.1: Type: text/plain, Size: 1935 bytes --]

Hi,

On Sat, Nov 10, 2018 at 01:16:52PM +0200, Laurent Pinchart wrote:
> The bind function performs hardware access (in hdmi4_cec_init()) and
> thus requires the device to be active. Ensure this by surrounding the
> bind function by hdmi_runtime_get() and hdmi_runtime_put() calls.
> 
> Fixes: 27d624527d99 ("drm/omap: dss: Acquire next dssdev at probe time")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> Changes since v2:
> 
> - Call hdmi_runtime_put() instead of hdmi_runtime_get() in error path
> ---
>  drivers/gpu/drm/omapdrm/dss/hdmi4.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> index cf6230eac31a..073fa462930a 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> @@ -635,10 +635,14 @@ static int hdmi4_bind(struct device *dev, struct device *master, void *data)
>  
>  	hdmi->dss = dss;
>  
> -	r = hdmi_pll_init(dss, hdmi->pdev, &hdmi->pll, &hdmi->wp);
> +	r = hdmi_runtime_get(hdmi);
>  	if (r)
>  		return r;
>  
> +	r = hdmi_pll_init(dss, hdmi->pdev, &hdmi->pll, &hdmi->wp);
> +	if (r)
> +		goto err_runtime_put;
> +
>  	r = hdmi4_cec_init(hdmi->pdev, &hdmi->core, &hdmi->wp);
>  	if (r)
>  		goto err_pll_uninit;
> @@ -652,12 +656,16 @@ static int hdmi4_bind(struct device *dev, struct device *master, void *data)
>  	hdmi->debugfs = dss_debugfs_create_file(dss, "hdmi", hdmi_dump_regs,
>  					       hdmi);
>  
> +	hdmi_runtime_put(hdmi);
> +
>  	return 0;
>  
>  err_cec_uninit:
>  	hdmi4_cec_uninit(&hdmi->core);
>  err_pll_uninit:
>  	hdmi_pll_uninit(&hdmi->pll);
> +err_runtime_put:
> +	hdmi_runtime_put(hdmi);
>  	return r;
>  }
>  
> -- 
> Regards,
> 
> Laurent Pinchart
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 3/4] drm/omap: dsi: Ensure the device is active during probe
  2018-11-10 11:16 ` [PATCH v3 3/4] drm/omap: dsi: Ensure the device is active during probe Laurent Pinchart
@ 2018-11-11  0:48   ` Sebastian Reichel
  0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2018-11-11  0:48 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen, dri-devel


[-- Attachment #1.1: Type: text/plain, Size: 1544 bytes --]

Hi,

On Sat, Nov 10, 2018 at 01:16:53PM +0200, Laurent Pinchart wrote:
> The probe function performs hardware access to read the number of
> supported data lanes from a configuration register and thus requires the
> device to be active. Ensure this by surrounding the access with
> dsi_runtime_get() and dsi_runtime_put() calls.
> 
> Fixes: edb715dffdee ("drm/omap: dss: dsi: Move initialization code from bind to probe")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Tony Lindgren <tony@atomide.com>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/gpu/drm/omapdrm/dss/dsi.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 394c129cfb3b..b9d5ad7e67d8 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -5409,11 +5409,14 @@ static int dsi_probe(struct platform_device *pdev)
>  
>  	/* DSI on OMAP3 doesn't have register DSI_GNQ, set number
>  	 * of data to 3 by default */
> -	if (dsi->data->quirks & DSI_QUIRK_GNQ)
> +	if (dsi->data->quirks & DSI_QUIRK_GNQ) {
> +		dsi_runtime_get(dsi);
>  		/* NB_DATA_LANES */
>  		dsi->num_lanes_supported = 1 + REG_GET(dsi, DSI_GNQ, 11, 9);
> -	else
> +		dsi_runtime_put(dsi);
> +	} else {
>  		dsi->num_lanes_supported = 3;
> +	}
>  
>  	r = dsi_init_output(dsi);
>  	if (r)
> -- 
> Regards,
> 
> Laurent Pinchart
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 4/4] drm/omap: Move DISPC runtime PM handling to omapdrm
  2018-11-10 11:16 ` [PATCH v3 4/4] drm/omap: Move DISPC runtime PM handling to omapdrm Laurent Pinchart
@ 2018-11-11  1:45   ` Sebastian Reichel
  0 siblings, 0 replies; 10+ messages in thread
From: Sebastian Reichel @ 2018-11-11  1:45 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Tony Lindgren, linux-omap, Tomi Valkeinen, dri-devel


[-- Attachment #1.1: Type: text/plain, Size: 7538 bytes --]

Hi,

On Sat, Nov 10, 2018 at 01:16:54PM +0200, Laurent Pinchart wrote:
> The internal encoders (DSI, HDMI4, HDMI5 and VENC) runtime PM handlers
> attempt to manage the runtime PM state of the connected DISPC, based on
> the rationale that the DISPC providing data to the encoders requires
> ensuring that the display is active whenever the encoders are active.
> 
> While the DISPC provides data to the encoders, it doesn't as such
> constitute a resource that encoders require in order to be taken out
> of suspend, contrary to for instance a functional clock or a power
> supply. Encoders registers can be accessed without the DISPC being
> active, and while the encoders will not output any video stream without
> being fed by the DISPC, the DISPC PM state doesn't influence the
> encoders PM state.
> 
> For this reason the DISPC PM state is better managed from the omapdrm
> driver, in the CRTC enable and disable operations. This allows the
> encoders PM state to be handled separately from the DISPC, and in
> particular at times when the DISPC may not be available (for instance at
> probe due to the DSS probe being deferred, or at remove time du to the
> DISPC being already removed).
> 
> Fixes: edb715dffdee ("drm/omap: dss: dsi: Move initialization code from bind to probe")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---

+1 for writing fixes, that cleanup the code at the same time :)
Decoupling DISPC from encoders also helps to fix DSI support.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

>  drivers/gpu/drm/omapdrm/dss/dsi.c   |  7 -------
>  drivers/gpu/drm/omapdrm/dss/hdmi4.c | 27 ---------------------------
>  drivers/gpu/drm/omapdrm/dss/hdmi5.c | 27 ---------------------------
>  drivers/gpu/drm/omapdrm/dss/venc.c  |  7 -------
>  drivers/gpu/drm/omapdrm/omap_crtc.c |  6 ++++++
>  5 files changed, 6 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index b9d5ad7e67d8..36123c086d97 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -5473,19 +5473,12 @@ static int dsi_runtime_suspend(struct device *dev)
>  	/* wait for current handler to finish before turning the DSI off */
>  	synchronize_irq(dsi->irq);
>  
> -	dispc_runtime_put(dsi->dss->dispc);
> -
>  	return 0;
>  }
>  
>  static int dsi_runtime_resume(struct device *dev)
>  {
>  	struct dsi_data *dsi = dev_get_drvdata(dev);
> -	int r;
> -
> -	r = dispc_runtime_get(dsi->dss->dispc);
> -	if (r)
> -		return r;
>  
>  	dsi->is_enabled = true;
>  	/* ensure the irq handler sees the is_enabled value */
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> index 073fa462930a..aabdda394c9c 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
> @@ -841,32 +841,6 @@ static int hdmi4_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int hdmi_runtime_suspend(struct device *dev)
> -{
> -	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> -
> -	dispc_runtime_put(hdmi->dss->dispc);
> -
> -	return 0;
> -}
> -
> -static int hdmi_runtime_resume(struct device *dev)
> -{
> -	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> -	int r;
> -
> -	r = dispc_runtime_get(hdmi->dss->dispc);
> -	if (r < 0)
> -		return r;
> -
> -	return 0;
> -}
> -
> -static const struct dev_pm_ops hdmi_pm_ops = {
> -	.runtime_suspend = hdmi_runtime_suspend,
> -	.runtime_resume = hdmi_runtime_resume,
> -};
> -
>  static const struct of_device_id hdmi_of_match[] = {
>  	{ .compatible = "ti,omap4-hdmi", },
>  	{},
> @@ -877,7 +851,6 @@ struct platform_driver omapdss_hdmi4hw_driver = {
>  	.remove		= hdmi4_remove,
>  	.driver         = {
>  		.name   = "omapdss_hdmi",
> -		.pm	= &hdmi_pm_ops,
>  		.of_match_table = hdmi_of_match,
>  		.suppress_bind_attrs = true,
>  	},
> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> index b0e4a7463f8c..9e8556f67a29 100644
> --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c
> @@ -825,32 +825,6 @@ static int hdmi5_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int hdmi_runtime_suspend(struct device *dev)
> -{
> -	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> -
> -	dispc_runtime_put(hdmi->dss->dispc);
> -
> -	return 0;
> -}
> -
> -static int hdmi_runtime_resume(struct device *dev)
> -{
> -	struct omap_hdmi *hdmi = dev_get_drvdata(dev);
> -	int r;
> -
> -	r = dispc_runtime_get(hdmi->dss->dispc);
> -	if (r < 0)
> -		return r;
> -
> -	return 0;
> -}
> -
> -static const struct dev_pm_ops hdmi_pm_ops = {
> -	.runtime_suspend = hdmi_runtime_suspend,
> -	.runtime_resume = hdmi_runtime_resume,
> -};
> -
>  static const struct of_device_id hdmi_of_match[] = {
>  	{ .compatible = "ti,omap5-hdmi", },
>  	{ .compatible = "ti,dra7-hdmi", },
> @@ -862,7 +836,6 @@ struct platform_driver omapdss_hdmi5hw_driver = {
>  	.remove		= hdmi5_remove,
>  	.driver         = {
>  		.name   = "omapdss_hdmi5",
> -		.pm	= &hdmi_pm_ops,
>  		.of_match_table = hdmi_of_match,
>  		.suppress_bind_attrs = true,
>  	},
> diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c
> index ff0b18c8e4ac..b5f52727f8b1 100644
> --- a/drivers/gpu/drm/omapdrm/dss/venc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/venc.c
> @@ -946,19 +946,12 @@ static int venc_runtime_suspend(struct device *dev)
>  	if (venc->tv_dac_clk)
>  		clk_disable_unprepare(venc->tv_dac_clk);
>  
> -	dispc_runtime_put(venc->dss->dispc);
> -
>  	return 0;
>  }
>  
>  static int venc_runtime_resume(struct device *dev)
>  {
>  	struct venc_device *venc = dev_get_drvdata(dev);
> -	int r;
> -
> -	r = dispc_runtime_get(venc->dss->dispc);
> -	if (r < 0)
> -		return r;
>  
>  	if (venc->tv_dac_clk)
>  		clk_prepare_enable(venc->tv_dac_clk);
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 62928ec0e7db..caffc547ef97 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -350,11 +350,14 @@ static void omap_crtc_arm_event(struct drm_crtc *crtc)
>  static void omap_crtc_atomic_enable(struct drm_crtc *crtc,
>  				    struct drm_crtc_state *old_state)
>  {
> +	struct omap_drm_private *priv = crtc->dev->dev_private;
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>  	int ret;
>  
>  	DBG("%s", omap_crtc->name);
>  
> +	priv->dispc_ops->runtime_get(priv->dispc);
> +
>  	spin_lock_irq(&crtc->dev->event_lock);
>  	drm_crtc_vblank_on(crtc);
>  	ret = drm_crtc_vblank_get(crtc);
> @@ -367,6 +370,7 @@ static void omap_crtc_atomic_enable(struct drm_crtc *crtc,
>  static void omap_crtc_atomic_disable(struct drm_crtc *crtc,
>  				     struct drm_crtc_state *old_state)
>  {
> +	struct omap_drm_private *priv = crtc->dev->dev_private;
>  	struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>  
>  	DBG("%s", omap_crtc->name);
> @@ -379,6 +383,8 @@ static void omap_crtc_atomic_disable(struct drm_crtc *crtc,
>  	spin_unlock_irq(&crtc->dev->event_lock);
>  
>  	drm_crtc_vblank_off(crtc);
> +
> +	priv->dispc_ops->runtime_put(priv->dispc);
>  }
>  
>  static enum drm_mode_status omap_crtc_mode_valid(struct drm_crtc *crtc,
> -- 
> Regards,
> 
> Laurent Pinchart
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time
  2018-11-10 11:16 [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Laurent Pinchart
                   ` (3 preceding siblings ...)
  2018-11-10 11:16 ` [PATCH v3 4/4] drm/omap: Move DISPC runtime PM handling to omapdrm Laurent Pinchart
@ 2018-11-12 10:05 ` Tomi Valkeinen
  4 siblings, 0 replies; 10+ messages in thread
From: Tomi Valkeinen @ 2018-11-12 10:05 UTC (permalink / raw)
  To: Laurent Pinchart, dri-devel; +Cc: Tony Lindgren, linux-omap

On 10/11/18 13:16, Laurent Pinchart wrote:
> Hello,
> 
> This series fixes crashes in the omapdss driver at both load and unload
> time, due to runtime PM problems related to probe deferral. The bugs got
> introduced in v4.20-rc, this should thus be considered as v4.20 fixes.
> 
> At the core of the problem comes commit 27d624527d99 ("drm/omap: dss:
> Acquire next dssdev at probe time") which can cause probe deferral for
> the DSS when the encoder and panel modules are not loaded yet. As the
> omapdss module contains drivers for the DSS and all its children, this
> results in the internal encoders being probed before the DSS. Missing
> runtime PM handling around register access then caused imprecise
> external aborts.
> 
> Patch 1/4 moves population of the DSS children from arch code to the
> omapdss driver. This prevents the DSS children from being probed before
> the DSS. The change can be considered as a workaround in the sense that
> runtime PM of the DSS children should operate correctly even when the
> DSS probe fail. However, given that reducing the amount of arch code is
> an improvement in itself, I believe the solution to be acceptable.
> 
> Patches 2/4 and 3/4 then ensure that the HDMI4 and DSI devices are
> active at bind and probe time respectively, in order to access hardware
> registers there.
> 
> Patch 4/4 finally fixes the internal encoders crash in their runtime PM
> handlers by moving DISPC runtime PM state management to omapdrm. This is
> the biggest change compared to v3 that went for a less intrusive (in my
> opinion at least) workaround. As Tomi and Tony were in favour of a real
> fix instead of a workaround, I gave the RFC fix more testing on
> Beagleboard, Pandaboard and AM57xx EVM and concluded it should be safe.
> 
> Patch 1/4 touches both the mach-omap2 and omapdss, and has been acked by
> Tony to be merged through the DRM tree.
> 
> Laurent Pinchart (4):
>   drm/omap: Populate DSS children in omapdss driver
>   drm/omap: hdmi4: Ensure the device is active during bind
>   drm/omap: dsi: Ensure the device is active during probe
>   drm/omap: Move DISPC runtime PM handling to omapdrm
> 
>  arch/arm/mach-omap2/display.c       | 111 +++++++++++++---------------
>  drivers/gpu/drm/omapdrm/dss/dsi.c   |  14 ++--
>  drivers/gpu/drm/omapdrm/dss/dss.c   |  11 ++-
>  drivers/gpu/drm/omapdrm/dss/hdmi4.c |  37 +++-------
>  drivers/gpu/drm/omapdrm/dss/hdmi5.c |  27 -------
>  drivers/gpu/drm/omapdrm/dss/venc.c  |   7 --
>  drivers/gpu/drm/omapdrm/omap_crtc.c |   6 ++
>  7 files changed, 83 insertions(+), 130 deletions(-)

Thanks, looks good to me. I'll push to drm-misc-fixes.

 Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-11-12 10:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-10 11:16 [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Laurent Pinchart
2018-11-10 11:16 ` [PATCH v3 1/4] drm/omap: Populate DSS children in omapdss driver Laurent Pinchart
2018-11-11  0:45   ` Sebastian Reichel
2018-11-10 11:16 ` [PATCH v3 2/4] drm/omap: hdmi4: Ensure the device is active during bind Laurent Pinchart
2018-11-11  0:48   ` Sebastian Reichel
2018-11-10 11:16 ` [PATCH v3 3/4] drm/omap: dsi: Ensure the device is active during probe Laurent Pinchart
2018-11-11  0:48   ` Sebastian Reichel
2018-11-10 11:16 ` [PATCH v3 4/4] drm/omap: Move DISPC runtime PM handling to omapdrm Laurent Pinchart
2018-11-11  1:45   ` Sebastian Reichel
2018-11-12 10:05 ` [PATCH v3 0/4] omapdrm: Fix runtime PM issues at module load and unload time Tomi Valkeinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.