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 works around an issue in the runtime PM handlers of the internal encoders that try to manage the DISPC runtime PM state when the DISPC may not be available. This is a bit of a hack and should be fixed by moving DISPC runtime PM handling out of the internal encoders runtime PM handlers, but that will require more testing and should not be rushed during the -rc period.
Tony, this series should fix the issues you've reported. I have tested it on both Pandaboard and AM57xx EVM, loading and unloading modules and exercising the display with kmstest.
Patch 1/4 touches both the mach-omap2 and omapdss. Would it be OK merging it through the DRM tree ? I don't think there's a risk of conflict during the v4.20-rc cycle.
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: Work around missing DISPC in runtime PM handlers
arch/arm/mach-omap2/display.c | 111 +++++++++++++--------------- drivers/gpu/drm/omapdrm/dss/dsi.c | 29 ++++++-- drivers/gpu/drm/omapdrm/dss/dss.c | 11 ++- drivers/gpu/drm/omapdrm/dss/hdmi4.c | 29 ++++++-- drivers/gpu/drm/omapdrm/dss/hdmi5.c | 19 ++++- drivers/gpu/drm/omapdrm/dss/venc.c | 19 ++++- 6 files changed, 140 insertions(+), 78 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 --- 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);
* Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 15:10]:
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
Please merge this together with the DSS fixes:
Acked-by: Tony Lindgren tony@atomide.com
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 --- 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..36be9a36d664 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_get(hdmi); return r; }
* Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 15:10]:
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
Looks good to me except one typo below..
index cf6230eac31a..36be9a36d664 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_get(hdmi); return r;
}
At err_runtime_put you should call hdmi_runtime_put() instead of hdmi_runtime_get(), right? :)
Regards,
Tony
Hi Tony,
On Monday, 5 November 2018 22:02:47 EET Tony Lindgren wrote:
- Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 15:10]:
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
Looks good to me except one typo below..
index cf6230eac31a..36be9a36d664 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
[snip]
@@ -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_get(hdmi); return r;
}
At err_runtime_put you should call hdmi_runtime_put() instead of hdmi_runtime_get(), right? :)
Oops. Of course. Thank you for the review. This will be fixed in v3.
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 --- 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)
* Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 15:10]:
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
The runtime suspend and resume callbacks for the DSI, HDMI4, HDMI5 and VENC try to manage the runtime PM state of the DISPC, which can be unavailable at probe time (due to the DSS probe being deferred for instance) and remove time (due to the DISPC being removed already). The proper fix is to move DISPC runtime PM handling out of the encoders runtime PM handlers, but that will require more development and testing that would be reasonable for an -rc bug fix.
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 | 22 ++++++++++++++++++---- drivers/gpu/drm/omapdrm/dss/hdmi4.c | 19 +++++++++++++++---- drivers/gpu/drm/omapdrm/dss/hdmi5.c | 19 +++++++++++++++---- drivers/gpu/drm/omapdrm/dss/venc.c | 19 +++++++++++++++---- 4 files changed, 63 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c index b9d5ad7e67d8..a1b9e6659b1e 100644 --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5473,7 +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); + /* + * FIXME: DISPC runtime PM handling should be controlled from omapdrm, + * see dsi_runtime_resume(). + */ + if (dsi->dss && dsi->dss->dispc) + dispc_runtime_put(dsi->dss->dispc);
return 0; } @@ -5483,9 +5488,18 @@ 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; + /* + * FIXME: The device is resumed from the probe function before the dss + * is available, in order to read a hardware configuration register. + * This doesn't require resuming the DISPC, so make it conditional. The + * DISPC runtime PM handling should instead be controlled from omapdrm, + * which is already partly the case, but needs additional testing. + */ + if (dsi->dss && dsi->dss->dispc) { + 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 36be9a36d664..21d1147c0827 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c @@ -845,7 +845,12 @@ static int hdmi_runtime_suspend(struct device *dev) { struct omap_hdmi *hdmi = dev_get_drvdata(dev);
- dispc_runtime_put(hdmi->dss->dispc); + /* + * FIXME: DISPC runtime PM handling should be controlled from omapdrm, + * see dsi_runtime_resume(). + */ + if (hdmi->dss && hdmi->dss->dispc) + dispc_runtime_put(hdmi->dss->dispc);
return 0; } @@ -855,9 +860,15 @@ 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; + /* + * FIXME: DISPC runtime PM handling should be controlled from omapdrm, + * see dsi_runtime_resume(). + */ + if (hdmi->dss && hdmi->dss->dispc) { + r = dispc_runtime_get(hdmi->dss->dispc); + if (r < 0) + return r; + }
return 0; } diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi5.c b/drivers/gpu/drm/omapdrm/dss/hdmi5.c index b0e4a7463f8c..75e2cb74a66c 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c @@ -829,7 +829,12 @@ static int hdmi_runtime_suspend(struct device *dev) { struct omap_hdmi *hdmi = dev_get_drvdata(dev);
- dispc_runtime_put(hdmi->dss->dispc); + /* + * FIXME: DISPC runtime PM handling should be controlled from omapdrm, + * see dsi_runtime_resume(). + */ + if (hdmi->dss && hdmi->dss->dispc) + dispc_runtime_put(hdmi->dss->dispc);
return 0; } @@ -839,9 +844,15 @@ 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; + /* + * FIXME: DISPC runtime PM handling should be controlled from omapdrm, + * see dsi_runtime_resume(). + */ + if (hdmi->dss && hdmi->dss->dispc) { + r = dispc_runtime_get(hdmi->dss->dispc); + if (r < 0) + return r; + }
return 0; } diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c index ff0b18c8e4ac..c39ad81fbc54 100644 --- a/drivers/gpu/drm/omapdrm/dss/venc.c +++ b/drivers/gpu/drm/omapdrm/dss/venc.c @@ -946,7 +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); + /* + * FIXME: DISPC runtime PM handling should be controlled from omapdrm, + * see dsi_runtime_resume(). + */ + if (venc->dss && venc->dss->dispc) + dispc_runtime_put(venc->dss->dispc);
return 0; } @@ -956,9 +961,15 @@ 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; + /* + * FIXME: DISPC runtime PM handling should be controlled from omapdrm, + * see dsi_runtime_resume(). + */ + if (venc->dss && venc->dss->dispc) { + r = dispc_runtime_get(venc->dss->dispc); + if (r < 0) + return r; + }
if (venc->tv_dac_clk) clk_prepare_enable(venc->tv_dac_clk);
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 or remove time).
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/dss/dsi.c | 21 ---------------- drivers/gpu/drm/omapdrm/dss/hdmi4.c | 38 ----------------------------- drivers/gpu/drm/omapdrm/dss/hdmi5.c | 38 ----------------------------- drivers/gpu/drm/omapdrm/dss/venc.c | 18 -------------- drivers/gpu/drm/omapdrm/omap_crtc.c | 6 +++++ 5 files changed, 6 insertions(+), 115 deletions(-)
This patch applies on top of the "[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time" series. It demonstrates what I think is the proper long term fix for the issue addressed by patch 4/4. Due to its nature, I don't think this patch should be applied for v4.20 as it qualifies for very careful testing, hence my proposal to fix the runtime PM problem with 4/4 and to queue this patch for v4.21.
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c index a1b9e6659b1e..36123c086d97 100644 --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -5473,33 +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);
- /* - * FIXME: DISPC runtime PM handling should be controlled from omapdrm, - * see dsi_runtime_resume(). - */ - if (dsi->dss && dsi->dss->dispc) - 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; - - /* - * FIXME: The device is resumed from the probe function before the dss - * is available, in order to read a hardware configuration register. - * This doesn't require resuming the DISPC, so make it conditional. The - * DISPC runtime PM handling should instead be controlled from omapdrm, - * which is already partly the case, but needs additional testing. - */ - if (dsi->dss && dsi->dss->dispc) { - 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 21d1147c0827..164501642ed1 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c @@ -841,43 +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); - - /* - * FIXME: DISPC runtime PM handling should be controlled from omapdrm, - * see dsi_runtime_resume(). - */ - if (hdmi->dss && hdmi->dss->dispc) - 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; - - /* - * FIXME: DISPC runtime PM handling should be controlled from omapdrm, - * see dsi_runtime_resume(). - */ - if (hdmi->dss && hdmi->dss->dispc) { - 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", }, {}, @@ -888,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 75e2cb74a66c..9e8556f67a29 100644 --- a/drivers/gpu/drm/omapdrm/dss/hdmi5.c +++ b/drivers/gpu/drm/omapdrm/dss/hdmi5.c @@ -825,43 +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); - - /* - * FIXME: DISPC runtime PM handling should be controlled from omapdrm, - * see dsi_runtime_resume(). - */ - if (hdmi->dss && hdmi->dss->dispc) - 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; - - /* - * FIXME: DISPC runtime PM handling should be controlled from omapdrm, - * see dsi_runtime_resume(). - */ - if (hdmi->dss && hdmi->dss->dispc) { - 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", }, @@ -873,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 c39ad81fbc54..b5f52727f8b1 100644 --- a/drivers/gpu/drm/omapdrm/dss/venc.c +++ b/drivers/gpu/drm/omapdrm/dss/venc.c @@ -946,30 +946,12 @@ static int venc_runtime_suspend(struct device *dev) if (venc->tv_dac_clk) clk_disable_unprepare(venc->tv_dac_clk);
- /* - * FIXME: DISPC runtime PM handling should be controlled from omapdrm, - * see dsi_runtime_resume(). - */ - if (venc->dss && venc->dss->dispc) - 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; - - /* - * FIXME: DISPC runtime PM handling should be controlled from omapdrm, - * see dsi_runtime_resume(). - */ - if (venc->dss && venc->dss->dispc) { - 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,
* Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 19:23]:
This patch applies on top of the "[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time" series. It demonstrates what I think is the proper long term fix for the issue addressed by patch 4/4. Due to its nature, I don't think this patch should be applied for v4.20 as it qualifies for very careful testing, hence my proposal to fix the runtime PM problem with 4/4 and to queue this patch for v4.21.
To me this seems like a less risky fix compared to conditional runtime PM calls in patch 4. Conditional calls with usecounts seem to always break one way or another.. So would be nice to go with this one if possible.
Regards,
Tony
Hi Tony,
On Monday, 5 November 2018 22:14:46 EET Tony Lindgren wrote:
- Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 19:23]:
This patch applies on top of the "[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time" series. It demonstrates what I think is the proper long term fix for the issue addressed by patch 4/4. Due to its nature, I don't think this patch should be applied for v4.20 as it qualifies for very careful testing, hence my proposal to fix the runtime PM problem with 4/4 and to queue this patch for v4.21.
To me this seems like a less risky fix compared to conditional runtime PM calls in patch 4. Conditional calls with usecounts seem to always break one way or another.. So would be nice to go with this one if possible.
If Tomi is fine with this, and after careful testing, I have no issue with merging this patch squashed with 4/4 for 4.20-rc.
On 05/11/18 23:46, Laurent Pinchart wrote:
Hi Tony,
On Monday, 5 November 2018 22:14:46 EET Tony Lindgren wrote:
- Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 19:23]:
This patch applies on top of the "[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time" series. It demonstrates what I think is the proper long term fix for the issue addressed by patch 4/4. Due to its nature, I don't think this patch should be applied for v4.20 as it qualifies for very careful testing, hence my proposal to fix the runtime PM problem with 4/4 and to queue this patch for v4.21.
To me this seems like a less risky fix compared to conditional runtime PM calls in patch 4. Conditional calls with usecounts seem to always break one way or another.. So would be nice to go with this one if possible.
If Tomi is fine with this, and after careful testing, I have no issue with merging this patch squashed with 4/4 for 4.20-rc.
I didn't try it yet, but it makes sense and looks good to me, so I think we should try to go with this one instead of the 4/4.
Btw, Peter also reported this one on linux-next on beagle-xm:
http://horus.dhcp.ti.com:7777/epowaxifuk.go
Venc has always been a bit odd with the clocks, and with a quick look at the code I didn't see what's wrong there, unless we have a mismatch with the runtime_get/puts.
Tomi
On 06/11/18 11:22, Tomi Valkeinen wrote:
On 05/11/18 23:46, Laurent Pinchart wrote:
Hi Tony,
On Monday, 5 November 2018 22:14:46 EET Tony Lindgren wrote:
- Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 19:23]:
This patch applies on top of the "[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time" series. It demonstrates what I think is the proper long term fix for the issue addressed by patch 4/4. Due to its nature, I don't think this patch should be applied for v4.20 as it qualifies for very careful testing, hence my proposal to fix the runtime PM problem with 4/4 and to queue this patch for v4.21.
To me this seems like a less risky fix compared to conditional runtime PM calls in patch 4. Conditional calls with usecounts seem to always break one way or another.. So would be nice to go with this one if possible.
If Tomi is fine with this, and after careful testing, I have no issue with merging this patch squashed with 4/4 for 4.20-rc.
I didn't try it yet, but it makes sense and looks good to me, so I think we should try to go with this one instead of the 4/4.
Btw, Peter also reported this one on linux-next on beagle-xm:
Ops, didn't notice that was an internal pastebin. Here's the crash:
[ 180.053192] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa050040 [ 180.060913] pgd = 42d2749f [ 180.063629] [fa050040] *pgd=48011452(bad) [ 180.067687] Internal error: : 1028 [#1] PREEMPT SMP ARM [ 180.072937] Modules linked in: [ 180.076019] CPU: 0 PID: 3072 Comm: halt Not tainted 4.19.0-rc8-next-20181019-00090-gec9a27467a7f #2 [ 180.085083] Hardware name: Generic OMAP36xx (Flattened Device Tree) [ 180.091400] PC is at dss_set_dac_pwrdn_bgz+0x4/0x18 [ 180.096313] LR is at venc_display_disable+0x2c/0x50 [ 180.101196] pc : [<c0558e98>] lr : [<c0564d94>] psr: 60070013 [ 180.107513] sp : da405e30 ip : db3b4d80 fp : c0b619d4 [ 180.112762] r10: c0b619e4 r9 : c0e76760 r8 : db1cf844 [ 180.118011] r7 : c0ea8870 r6 : db758000 r5 : db758008 r4 : db758060 [ 180.124542] r3 : fa050c00 r2 : fa050000 r1 : 00000000 r0 : db709c00 [ 180.131103] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 180.138275] Control: 10c5387d Table: 9a138019 DAC: 00000051 [ 180.144042] Process halt (pid: 3072, stack limit = 0x785a09e7) [ 180.149902] Stack: (0xda405e30 to 0xda406000) [ 180.154296] 5e20: db2b9d90 db17de10 db1cf810 c05735d4 [ 180.162506] 5e40: db2b9d90 c05585dc db1cf81c c058a3f4 d3327d25 4321fedc 00000000 c0e05108 [ 180.170745] 5e60: c0e34654 fee1dead 00022958 00000058 00000000 c0156b44 4321fedc c0156d00 [ 180.178955] 5e80: c0e09708 ffffffff db078000 c0155318 c0e7681c 00000002 db078000 df9222c0 [ 180.187164] 5ea0: db3b4d80 db074000 db78b000 db78b200 ebee9635 0000000e da405f0c c088e078 [ 180.195404] 5ec0: 00000000 c0143118 000a801d dabaf310 db2bcb10 c088ea14 db074000 00000014 [ 180.203613] 5ee0: 00000000 d3327d25 da404000 ffffe000 c0e05108 00000000 da405f5c 00000001 [ 180.211822] 5f00: da404000 00000025 da405f1c c088ea14 db074000 c0e05108 00000000 c0143e1c [ 180.220062] 5f20: a0070013 d3327d25 da404000 db053000 db3b4d80 00000000 00000014 c0e05108 [ 180.228271] 5f40: c0101204 da404000 00000025 c019ae20 da404000 c0146038 da195300 00000014 [ 180.236480] 5f60: 00000000 00000000 00000c00 00000000 00000000 00000000 00000000 d3327d25 [ 180.244720] 5f80: da195300 d3327d25 00000002 00000000 00000002 00000000 00000058 c0101204 [ 180.252929] 5fa0: da404000 c0101000 00000000 00000002 fee1dead 28121969 4321fedc 00022958 [ 180.261138] 5fc0: 00000000 00000002 00000000 00000058 00000000 00000001 00000000 00000000 [ 180.269378] 5fe0: 50313900 beb4ec78 00011158 b6e4e39c 20070010 fee1dead 00000000 00000000 [ 180.277587] [<c0558e98>] (dss_set_dac_pwrdn_bgz) from [<c0564d94>] (venc_display_disable+0x2c/0x50) [ 180.286682] [<c0564d94>] (venc_display_disable) from [<c05735d4>] (tvc_disable+0x28/0x34) [ 180.294921] [<c05735d4>] (tvc_disable) from [<c05585dc>] (dss_shutdown+0x34/0x38) [ 180.302429] [<c05585dc>] (dss_shutdown) from [<c058a3f4>] (device_shutdown+0x160/0x20c) [ 180.310485] [<c058a3f4>] (device_shutdown) from [<c0156b44>] (kernel_power_off+0x2c/0x70) [ 180.318695] [<c0156b44>] (kernel_power_off) from [<c0156d00>] (sys_reboot+0x130/0x1e8) [ 180.326660] [<c0156d00>] (sys_reboot) from [<c0101000>] (ret_fast_syscall+0x0/0x54) [ 180.334350] Exception stack(0xda405fa8 to 0xda405ff0) [ 180.339447] 5fa0: 00000000 00000002 fee1dead 28121969 4321fedc 00022958 [ 180.347656] 5fc0: 00000000 00000002 00000000 00000058 00000000 00000001 00000000 00000000 [ 180.355865] 5fe0: 50313900 beb4ec78 00011158 b6e4e39c [ 180.360961] Code: e5821040 e12fff1e e7f001f2 e5902004 (e5923040) [ 180.367065] ---[ end trace 8d96f3954c4321c5 ]--- [ 180.371856] In-band Error seen by MPU at address 0 [ 180.376739] ------------[ cut here ]------------ [ 180.381408] WARNING: CPU: 0 PID: 3072 at drivers/bus/omap_l3_smx.c:166 omap3_l3_app_irq+0xac/0x118 [ 180.390411] Modules linked in: [ 180.393463] CPU: 0 PID: 3072 Comm: halt Tainted: G D 4.19.0-rc8-next-20181019-00090-gec9a27467a7f
Hi Tomi,
On Tuesday, 6 November 2018 14:47:51 EET Tomi Valkeinen wrote:
On 06/11/18 11:22, Tomi Valkeinen wrote:
On 05/11/18 23:46, Laurent Pinchart wrote:
On Monday, 5 November 2018 22:14:46 EET Tony Lindgren wrote:
- Laurent Pinchart laurent.pinchart@ideasonboard.com [181105 19:23]:
This patch applies on top of the "[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time" series. It demonstrates what I think is the proper long term fix for the issue addressed by patch 4/4. Due to its nature, I don't think this patch should be applied for v4.20 as it qualifies for very careful testing, hence my proposal to fix the runtime PM problem with 4/4 and to queue this patch for v4.21.
To me this seems like a less risky fix compared to conditional runtime PM calls in patch 4. Conditional calls with usecounts seem to always break one way or another.. So would be nice to go with this one if possible.
If Tomi is fine with this, and after careful testing, I have no issue with merging this patch squashed with 4/4 for 4.20-rc.
I didn't try it yet, but it makes sense and looks good to me, so I think we should try to go with this one instead of the 4/4.
I've tested the patch on both Pandaboard and AM57xx EVM without any noticing any issue. I would appreciate if you could test it as well before we deem it fit for v4.20.
Btw, Peter also reported this one on linux-next on beagle-xm:
Ops, didn't notice that was an internal pastebin. Here's the crash:
[ 180.053192] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfa050040 [ 180.060913] pgd = 42d2749f [ 180.063629] [fa050040] *pgd=48011452(bad) [ 180.067687] Internal error: : 1028 [#1] PREEMPT SMP ARM [ 180.072937] Modules linked in: [ 180.076019] CPU: 0 PID: 3072 Comm: halt Not tainted 4.19.0-rc8-next-20181019-00090-gec9a27467a7f #2 [ 180.085083] Hardware name: Generic OMAP36xx (Flattened Device Tree) [ 180.091400] PC is at dss_set_dac_pwrdn_bgz+0x4/0x18 [ 180.096313] LR is at venc_display_disable+0x2c/0x50 [ 180.101196] pc : [<c0558e98>] lr : [<c0564d94>] psr: 60070013 [ 180.107513] sp : da405e30 ip : db3b4d80 fp : c0b619d4 [ 180.112762] r10: c0b619e4 r9 : c0e76760 r8 : db1cf844 [ 180.118011] r7 : c0ea8870 r6 : db758000 r5 : db758008 r4 : db758060 [ 180.124542] r3 : fa050c00 r2 : fa050000 r1 : 00000000 r0 : db709c00 [ 180.131103] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none [ 180.138275] Control: 10c5387d Table: 9a138019 DAC: 00000051 [ 180.144042] Process halt (pid: 3072, stack limit = 0x785a09e7) [ 180.149902] Stack: (0xda405e30 to 0xda406000)
[snip]
[ 180.277587] [<c0558e98>] (dss_set_dac_pwrdn_bgz) from [<c0564d94>] (venc_display_disable+0x2c/0x50) [ 180.286682] [<c0564d94>] (venc_display_disable) from [<c05735d4>] (tvc_disable+0x28/0x34) [ 180.294921] [<c05735d4>] (tvc_disable) from [<c05585dc>] (dss_shutdown+0x34/0x38) [ 180.302429] [<c05585dc>] (dss_shutdown) from [<c058a3f4>] (device_shutdown+0x160/0x20c) [ 180.310485] [<c058a3f4>] (device_shutdown) from [<c0156b44>] (kernel_power_off+0x2c/0x70) [ 180.318695] [<c0156b44>] (kernel_power_off) from [<c0156d00>] (sys_reboot+0x130/0x1e8) [ 180.326660] [<c0156d00>] (sys_reboot) from [<c0101000>] (ret_fast_syscall+0x0/0x54) [ 180.334350] Exception stack(0xda405fa8 to 0xda405ff0) [ 180.339447] 5fa0: 00000000 00000002 fee1dead 28121969 4321fedc 00022958 [ 180.347656] 5fc0: 00000000 00000002 00000000 00000058 00000000 00000001 00000000 00000000 [ 180.355865] 5fe0: 50313900 beb4ec78 00011158 b6e4e39c [ 180.360961] Code: e5821040 e12fff1e e7f001f2 e5902004 (e5923040) [ 180.367065] ---[ end trace 8d96f3954c4321c5 ]--- [ 180.371856] In-band Error seen by MPU at address 0 [ 180.376739] ------------[ cut here ]------------ [ 180.381408] WARNING: CPU: 0 PID: 3072 at drivers/bus/omap_l3_smx.c:166 omap3_l3_app_irq+0xac/0x118 [ 180.390411] Modules linked in: [ 180.393463] CPU: 0 PID: 3072 Comm: halt Tainted: G D 4.19.0-rc8-next-20181019-00090-gec9a27467a7f
I dove into my boxes of development board and managed to find a Beagleboard- xM. After dusting it and being amazed it was still working, I tested v4.20-rc1 and couldn't reproduce the above issue, neither with nor without the regression fixes.
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Tomi Valkeinen -
Tony Lindgren