[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time

[PATCH v2 0/4] omapdrm: Fix runtime PM issues at module load and unload time

[Laurent Pinchart]

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(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH v2 1/4] drm/omap: Populate DSS children in omapdss driver

[Laurent Pinchart]

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);
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 1/4] drm/omap: Populate DSS children in omapdss driver

[Tony Lindgren]

* 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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/4] drm/omap: hdmi4: Ensure the device is active during bind

[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>
---
 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;
 }
 
-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v2 2/4] drm/omap: hdmi4: Ensure the device is active during bind

[Tony Lindgren]

* 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] drm/omap: hdmi4: Ensure the device is active during bind

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart



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

[PATCH v2 3/4] drm/omap: dsi: Ensure the device is active during probe

[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>
---
 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

Re: [PATCH v2 3/4] drm/omap: dsi: Ensure the device is active during probe

[Tony Lindgren]

* 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>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/4] drm/omap: Work around missing DISPC in runtime PM handlers

[Laurent Pinchart]

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);
-- 
Regards,

Laurent Pinchart

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

[RFC/PATCH] drm/omap: Move DISPC runtime PM handling to omapdrm

[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 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,
-- 
Regards,

Laurent Pinchart

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

Re: [RFC/PATCH] drm/omap: Move DISPC runtime PM handling to omapdrm

[Tony Lindgren]

* 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC/PATCH] drm/omap: Move DISPC runtime PM handling to omapdrm

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart



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

Re: [RFC/PATCH] drm/omap: Move DISPC runtime PM handling to omapdrm

[Tomi Valkeinen]

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

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

Re: [RFC/PATCH] drm/omap: Move DISPC runtime PM handling to omapdrm

[Tomi Valkeinen]

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:
> 
> http://horus.dhcp.ti.com:7777/epowaxifuk.go

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 



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

Re: [RFC/PATCH] drm/omap: Move DISPC runtime PM handling to omapdrm

[Laurent Pinchart]

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:
> > 
> > http://horus.dhcp.ti.com:7777/epowaxifuk.go
> 
> 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.

-- 
Regards,

Laurent Pinchart



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