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

[PATCH 0/3] 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/3 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/3 and 3/3 then ensure that the HDMI4 and DSI devices are
active at bind and probe time respectively, in order to access hardware
registers there.

Tony, patch 1/3 touches both the mach-omap2 and omapdss. Would you be
fine merging it through the DRM tree ? I don't think there's a risk of
conflict during the v4.20-rc cycle.

Laurent Pinchart (3):
  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

 arch/arm/mach-omap2/display.c       | 111 +++++++++++++---------------
 drivers/gpu/drm/omapdrm/dss/dsi.c   |  25 +++++--
 drivers/gpu/drm/omapdrm/dss/dss.c   |  11 ++-
 drivers/gpu/drm/omapdrm/dss/hdmi4.c |  10 ++-
 4 files changed, 91 insertions(+), 66 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[PATCH 1/3] 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 arch code by a call to
of_platform_populate(). While this approach can be considered as
correct, it performs a task in arch code that can be equally well
performed in driver code. To simplify arch code, move the
of_platform_populate() call to the omapdss driver, and cleanup at remove
time by calling of_platform_depopulate().

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

[PATCH 2/3] 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

[PATCH 3/3] 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.

Additionally we need to introduce a hack in the DSI runtime suspend and
resume callbacks, as they try to manage the runtime PM state of the
DISPC, which is not available at DSI probe time. The proper fix is to
move DISPC runtime PM handling to omapdrm completely, but it requires
more testing.

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 | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 394c129cfb3b..c10dac9dfe4c 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)
@@ -5470,7 +5473,8 @@ 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);
+	if (dsi->dss && dsi->dss->dispc)
+		dispc_runtime_put(dsi->dss->dispc);
 
 	return 0;
 }
@@ -5480,9 +5484,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 */
-- 
Regards,

Laurent Pinchart

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

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

[Tomi Valkeinen]

Hi Laurent,

On 01/11/18 12:25, 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/3 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/3 and 3/3 then ensure that the HDMI4 and DSI devices are
> active at bind and probe time respectively, in order to access hardware
> registers there.
> 
> Tony, patch 1/3 touches both the mach-omap2 and omapdss. Would you be
> fine merging it through the DRM tree ? I don't think there's a risk of
> conflict during the v4.20-rc cycle.

Thanks for debugging this! I have to say I really don't like these
(well, 2 is fine), as they feel like hacks.

We do dispc_runtime_get/put in the HDMI driver's suspend/resume too, so
don't we need similar hack (as you add in dsi.c) there also?

So we have two problems (aside missing runtime_get/put calls in those
few places):

- All DSS submodules depend on DSS (dss core)

- All DSS encoders depend on DISPC. The dependency is there only when
we're starting the video stream.

Can we not defer the probe of the submodules until the dependencies have
been probed? Are there any other ways to manage such device
dependencies? Possibly we could handle the driver registration so that
we only register encoders when dss and dispc have been probed, but that
feels a bit hacky too.

And considering these issues, is the assumption that "There's no reason
to delay initialization of most of the driver (such as mapping memory
I/O or enabling runtime PM) to the component bind handler." still valid?
 edb715dffdee71bb8216ee4d71c0714d932e9acf doesn't really state any
benefit for this move either. Clearly there's a reason to do this in
bind, although it doesn't solve the dispc dependency.

 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: [PATCH 0/3] omapdrm: Fix runtime PM issues at module load and unload time

[Laurent Pinchart]

Ho Tomi,

On Thursday, 1 November 2018 13:47:40 EET Tomi Valkeinen wrote:
> On 01/11/18 12:25, 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/3 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/3 and 3/3 then ensure that the HDMI4 and DSI devices are
> > active at bind and probe time respectively, in order to access hardware
> > registers there.
> > 
> > Tony, patch 1/3 touches both the mach-omap2 and omapdss. Would you be
> > fine merging it through the DRM tree ? I don't think there's a risk of
> > conflict during the v4.20-rc cycle.
> 
> Thanks for debugging this! I have to say I really don't like these
> (well, 2 is fine), as they feel like hacks.

I assume you also have nothing against the first hunk of patch 3/3.

> We do dispc_runtime_get/put in the HDMI driver's suspend/resume too, so
> don't we need similar hack (as you add in dsi.c) there also?

We would if we had to access HDMI registers at probe time.

> So we have two problems (aside missing runtime_get/put calls in those
> few places):
> 
> - All DSS submodules depend on DSS (dss core)
> 
> - All DSS encoders depend on DISPC. The dependency is there only when
> we're starting the video stream.
> 
> Can we not defer the probe of the submodules until the dependencies have
> been probed?

That would be difficult, as the submodules don't know about the DSS. It goes 
the other way round, it's the DSS that gathers a list of submodules.

Furthermore the DSS probe is deferred due to the devices connected to the DPI 
and SDI outputs not being probed yet. See dpi_init_output_port(), the call to 
omapdss_of_find_connected_device() returns -EPROBE_DEFER. This is why the DSS 
probe is deferred if you load the omapdss module before all the other modules 
for the external components. Getting hold if the next device in the chain was 
needed to reverse the chain control direction.

I would like to also point out that regardless of the underlying issue, I 
think creating the DSS children in the DSS driver instead of mach-omap2 code 
is the right thing to do, as the DSS really handles the bus through which the 
children are accessed. This is similar to how an I2C adapter creates the I2C 
slaves.

> Are there any other ways to manage such device dependencies? Possibly we
> could handle the driver registration so that we only register encoders when
> dss and dispc have been probed, but that feels a bit hacky too.

I also think that's an even bigger hack.

> And considering these issues, is the assumption that "There's no reason
> to delay initialization of most of the driver (such as mapping memory
> I/O or enabling runtime PM) to the component bind handler." still valid?
> edb715dffdee71bb8216ee4d71c0714d932e9acf doesn't really state any
> benefit for this move either. Clearly there's a reason to do this in
> bind, although it doesn't solve the dispc dependency.

We could try moving code back to the bind handler, but that would get in the 
way of probe deferral until bridges are available. In order to solve that we 
would need to convert all bridges to the component framework, which we can't 
do as they're used by display drivers that don't use components. Lovely, isn't 
it ?

There's a nearly infinite amount of problems to fix and I wish I could tackle 
them all at the same time, but that's unfortunately not possible. I would like 
at some point in the future to add an asynchronous bind mechanism to DRM 
bridge itself, whose usage would be optional, unlike the component framework. 
This would help with probe handling, but it's way down the road.

-- 
Regards,

Laurent Pinchart



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

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

[Tomi Valkeinen]

On 01/11/18 14:13, Laurent Pinchart wrote:

>> Thanks for debugging this! I have to say I really don't like these
>> (well, 2 is fine), as they feel like hacks.
> 
> I assume you also have nothing against the first hunk of patch 3/3.

Yes. And after our discussion, I think 1 is fine too, so the only
problem is the latter part of patch 3.

>> Can we not defer the probe of the submodules until the dependencies have
>> been probed?
> 
> That would be difficult, as the submodules don't know about the DSS. It goes 
> the other way round, it's the DSS that gathers a list of submodules.
> 
> Furthermore the DSS probe is deferred due to the devices connected to the DPI 
> and SDI outputs not being probed yet. See dpi_init_output_port(), the call to 
> omapdss_of_find_connected_device() returns -EPROBE_DEFER. This is why the DSS 
> probe is deferred if you load the omapdss module before all the other modules 
> for the external components. Getting hold if the next device in the chain was 
> needed to reverse the chain control direction.

Ok.

> I would like to also point out that regardless of the underlying issue, I 
> think creating the DSS children in the DSS driver instead of mach-omap2 code 
> is the right thing to do, as the DSS really handles the bus through which the 
> children are accessed. This is similar to how an I2C adapter creates the I2C 
> slaves.

Agreed.

>> And considering these issues, is the assumption that "There's no reason
>> to delay initialization of most of the driver (such as mapping memory
>> I/O or enabling runtime PM) to the component bind handler." still valid?
>> edb715dffdee71bb8216ee4d71c0714d932e9acf doesn't really state any
>> benefit for this move either. Clearly there's a reason to do this in
>> bind, although it doesn't solve the dispc dependency.
> 
> We could try moving code back to the bind handler, but that would get in the 
> way of probe deferral until bridges are available. In order to solve that we 
> would need to convert all bridges to the component framework, which we can't 
> do as they're used by display drivers that don't use components. Lovely, isn't 
> it ?
> 
> There's a nearly infinite amount of problems to fix and I wish I could tackle 
> them all at the same time, but that's unfortunately not possible. I would like 
> at some point in the future to add an asynchronous bind mechanism to DRM 
> bridge itself, whose usage would be optional, unlike the component framework. 
> This would help with probe handling, but it's way down the road.

Yep, this seems to be rather complex issue. So, maybe we can go with the
third patch too, but perhaps split it in two, as the first hunk is fine.
And perhaps add mark the rest clearly as HACK, also in the suspend.

 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: [PATCH 0/3] omapdrm: Fix runtime PM issues at module load and unload time

[Tony Lindgren]

Hi,

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [181101 12:13]:
> On Thursday, 1 November 2018 13:47:40 EET Tomi Valkeinen wrote:
> > We do dispc_runtime_get/put in the HDMI driver's suspend/resume too, so
> > don't we need similar hack (as you add in dsi.c) there also?
> 
> We would if we had to access HDMI registers at probe time.

With these I'm still seeing the following issue with hdmi on rmmod
of omapdrm related modules as hdmi->dss is NULL in hdmi_runtime_resume.

Regards,

Tony

8< ------
Unable to handle kernel NULL pointer dereference at virtual address 00000278
...
PC is at hdmi_runtime_resume+0xc/0x1c [omapdss]
LR is at __rpm_callback+0x144/0x1d8
...
(hdmi_runtime_resume [omapdss]) from [<c06079b4>] (__rpm_callback+0x144/0x1d8)
(__rpm_callback) from [<c0607a68>] (rpm_callback+0x20/0x80)
(rpm_callback) from [<c06075f0>] (rpm_resume+0x60c/0x828)
(rpm_resume) from [<c0607858>] (__pm_runtime_resume+0x4c/0x64)
(__pm_runtime_resume) from [<c05fc7ec>] (device_release_driver_internal+0x130/0x234)
(device_release_driver_internal) from [<c05fc934>] (driver_detach+0x38/0x6c)
(driver_detach) from [<c05fb698>] (bus_remove_driver+0x4c/0xa4)
(bus_remove_driver) from [<c05fe23c>] (platform_unregister_drivers+0x20/0x2c)
(platform_unregister_drivers) from [<c01f0fe0>] (sys_delete_module+0x1c0/0x230)
(sys_delete_module) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Laurent Pinchart]

Hi Tony,

On Thursday, 1 November 2018 17:58:56 EET Tony Lindgren wrote:
> * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [181101 12:13]:
> > On Thursday, 1 November 2018 13:47:40 EET Tomi Valkeinen wrote:
> > > We do dispc_runtime_get/put in the HDMI driver's suspend/resume too, so
> > > don't we need similar hack (as you add in dsi.c) there also?
> > 
> > We would if we had to access HDMI registers at probe time.
> 
> With these I'm still seeing the following issue with hdmi on rmmod
> of omapdrm related modules as hdmi->dss is NULL in hdmi_runtime_resume.

This is actually what I expected, but to my surprise the problem didn't occur 
on my system, I don't know why. I'll try to investigate.

> Regards,
> 
> Tony
> 
> 8< ------
> Unable to handle kernel NULL pointer dereference at virtual address 00000278
> ...
> PC is at hdmi_runtime_resume+0xc/0x1c [omapdss]
> LR is at __rpm_callback+0x144/0x1d8
> ...
> (hdmi_runtime_resume [omapdss]) from [<c06079b4>]
> (__rpm_callback+0x144/0x1d8) (__rpm_callback) from [<c0607a68>]
> (rpm_callback+0x20/0x80)
> (rpm_callback) from [<c06075f0>] (rpm_resume+0x60c/0x828)
> (rpm_resume) from [<c0607858>] (__pm_runtime_resume+0x4c/0x64)
> (__pm_runtime_resume) from [<c05fc7ec>]
> (device_release_driver_internal+0x130/0x234)
> (device_release_driver_internal) from [<c05fc934>]
> (driver_detach+0x38/0x6c) (driver_detach) from [<c05fb698>]
> (bus_remove_driver+0x4c/0xa4)
> (bus_remove_driver) from [<c05fe23c>]
> (platform_unregister_drivers+0x20/0x2c) (platform_unregister_drivers) from
> [<c01f0fe0>] (sys_delete_module+0x1c0/0x230) (sys_delete_module) from
> [<c0101000>] (ret_fast_syscall+0x0/0x28)

-- 
Regards,

Laurent Pinchart



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

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

[Laurent Pinchart]

Hi Tony,

On Thursday, 1 November 2018 18:17:43 EET Laurent Pinchart wrote:
> On Thursday, 1 November 2018 17:58:56 EET Tony Lindgren wrote:
> > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [181101 12:13]:
> >> On Thursday, 1 November 2018 13:47:40 EET Tomi Valkeinen wrote:
> >>> We do dispc_runtime_get/put in the HDMI driver's suspend/resume too,
> >>> so don't we need similar hack (as you add in dsi.c) there also?
> >> 
> >> We would if we had to access HDMI registers at probe time.
> > 
> > With these I'm still seeing the following issue with hdmi on rmmod
> > of omapdrm related modules as hdmi->dss is NULL in hdmi_runtime_resume.
> 
> This is actually what I expected, but to my surprise the problem didn't
> occur on my system, I don't know why. I'll try to investigate.

I've submitted a v2 which should hopefully fix this.

-- 
Regards,

Laurent Pinchart



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

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

[Tony Lindgren]

* Laurent Pinchart <laurent.pinchart@ideasonboard.com> [181105 15:14]:
> Hi Tony,
> 
> On Thursday, 1 November 2018 18:17:43 EET Laurent Pinchart wrote:
> > On Thursday, 1 November 2018 17:58:56 EET Tony Lindgren wrote:
> > > * Laurent Pinchart <laurent.pinchart@ideasonboard.com> [181101 12:13]:
> > >> On Thursday, 1 November 2018 13:47:40 EET Tomi Valkeinen wrote:
> > >>> We do dispc_runtime_get/put in the HDMI driver's suspend/resume too,
> > >>> so don't we need similar hack (as you add in dsi.c) there also?
> > >> 
> > >> We would if we had to access HDMI registers at probe time.
> > > 
> > > With these I'm still seeing the following issue with hdmi on rmmod
> > > of omapdrm related modules as hdmi->dss is NULL in hdmi_runtime_resume.
> > 
> > This is actually what I expected, but to my surprise the problem didn't
> > occur on my system, I don't know why. I'll try to investigate.
> 
> I've submitted a v2 which should hopefully fix this.

Yes thanks seems to fix it, noticed one issue there though
for hdmi.

Regards,

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