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(-)
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);
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
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; }
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
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)
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);
- else
dsi_runtime_put(dsi);} else { dsi->num_lanes_supported = 3;
}
r = dsi_init_output(dsi); if (r)
-- Regards,
Laurent Pinchart
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,
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,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,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
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
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Sebastian Reichel -
Tomi Valkeinen