OMAPDSS: Check if RPM enabled before trying to change state

From: Jassi Brar <redacted>

If the runtime PM of the device is disabled (for example in resume from
suspend path), it doesn't make sense to attempt pm_runtime_get/put, esp
when their return values affect the control flow path.

Signed-off-by: Jassi Brar <redacted>
---

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

 drivers/video/omap2/dss/dispc.c |    6 ++++++
 drivers/video/omap2/dss/dsi.c   |    6 ++++++
 drivers/video/omap2/dss/dss.c   |    6 ++++++
 drivers/video/omap2/dss/hdmi.c  |    6 ++++++
 drivers/video/omap2/dss/rfbi.c  |    6 ++++++
 drivers/video/omap2/dss/venc.c  |    6 ++++++
 6 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 4749ac3..2c3266f 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c

@@ -372,6 +372,9 @@ int dispc_runtime_get(void)
 
 	DSSDBG("dispc_runtime_get\n");
 
+	if (!pm_runtime_enabled(&dispc.pdev->dev))
+		return 0;
+
 	r = pm_runtime_get_sync(&dispc.pdev->dev);
 	WARN_ON(r < 0);
 	return r < 0 ? r : 0;

@@ -383,6 +386,9 @@ void dispc_runtime_put(void)
 
 	DSSDBG("dispc_runtime_put\n");
 
+	if (!pm_runtime_enabled(&dispc.pdev->dev))
+		return;
+
 	r = pm_runtime_put_sync(&dispc.pdev->dev);
 	WARN_ON(r < 0);
 }

diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index ca8382d..6db4cb1 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c

@@ -1062,6 +1062,9 @@ int dsi_runtime_get(struct platform_device *dsidev)
 
 	DSSDBG("dsi_runtime_get\n");
 
+	if (!pm_runtime_enabled(&dsi->pdev->dev))
+		return 0;
+
 	r = pm_runtime_get_sync(&dsi->pdev->dev);
 	WARN_ON(r < 0);
 	return r < 0 ? r : 0;

@@ -1074,6 +1077,9 @@ void dsi_runtime_put(struct platform_device *dsidev)
 
 	DSSDBG("dsi_runtime_put\n");
 
+	if (!pm_runtime_enabled(&dsi->pdev->dev))
+		return;
+
 	r = pm_runtime_put_sync(&dsi->pdev->dev);
 	WARN_ON(r < 0);
 }

diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index 7706323..5e52224 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c

@@ -719,6 +719,9 @@ static int dss_runtime_get(void)
 
 	DSSDBG("dss_runtime_get\n");
 
+	if (!pm_runtime_enabled(&dss.pdev->dev))
+		return 0;
+
 	r = pm_runtime_get_sync(&dss.pdev->dev);
 	WARN_ON(r < 0);
 	return r < 0 ? r : 0;

@@ -730,6 +733,9 @@ static void dss_runtime_put(void)
 
 	DSSDBG("dss_runtime_put\n");
 
+	if (!pm_runtime_enabled(&dss.pdev->dev))
+		return;
+
 	r = pm_runtime_put_sync(&dss.pdev->dev);
 	WARN_ON(r < 0 && r != -EBUSY);
 }

diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 0738090..900e621 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c

@@ -123,6 +123,9 @@ static int hdmi_runtime_get(void)
 
 	DSSDBG("hdmi_runtime_get\n");
 
+	if (!pm_runtime_enabled(&hdmi.pdev->dev))
+		return 0;
+
 	r = pm_runtime_get_sync(&hdmi.pdev->dev);
 	WARN_ON(r < 0);
 	if (r < 0)

@@ -137,6 +140,9 @@ static void hdmi_runtime_put(void)
 
 	DSSDBG("hdmi_runtime_put\n");
 
+	if (!pm_runtime_enabled(&hdmi.pdev->dev))
+		return;
+
 	r = pm_runtime_put_sync(&hdmi.pdev->dev);
 	WARN_ON(r < 0);
 }

diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 3d8c206..401384a 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c

@@ -129,6 +129,9 @@ static int rfbi_runtime_get(void)
 
 	DSSDBG("rfbi_runtime_get\n");
 
+	if (!pm_runtime_enabled(&rfbi.pdev->dev))
+		return 0;
+
 	r = pm_runtime_get_sync(&rfbi.pdev->dev);
 	WARN_ON(r < 0);
 	return r < 0 ? r : 0;

@@ -140,6 +143,9 @@ static void rfbi_runtime_put(void)
 
 	DSSDBG("rfbi_runtime_put\n");
 
+	if (!pm_runtime_enabled(&rfbi.pdev->dev))
+		return;
+
 	r = pm_runtime_put_sync(&rfbi.pdev->dev);
 	WARN_ON(r < 0);
 }

diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index 2b89739..edd8710 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c

@@ -390,6 +390,9 @@ static int venc_runtime_get(void)
 
 	DSSDBG("venc_runtime_get\n");
 
+	if (!pm_runtime_enabled(&venc.pdev->dev))
+		return 0;
+
 	r = pm_runtime_get_sync(&venc.pdev->dev);
 	WARN_ON(r < 0);
 	return r < 0 ? r : 0;

@@ -401,6 +404,9 @@ static void venc_runtime_put(void)
 
 	DSSDBG("venc_runtime_put\n");
 
+	if (!pm_runtime_enabled(&venc.pdev->dev))
+		return;
+
 	r = pm_runtime_put_sync(&venc.pdev->dev);
 	WARN_ON(r < 0);
 }

-- 
1.7.4.1

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

From: Jassi Brar <redacted>

If the runtime PM of the device is disabled (for example in resume from
suspend path), it doesn't make sense to attempt pm_runtime_get/put, esp
when their return values affect the control flow path.

This looks strange. When the driver does pm_runtime_get() it expects the
HW to be functional and registers accessible. If we just skip the
pm_runtime_get(), how can the code work?

Signed-off-by: Jassi Brar <redacted>
---

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

 Tomi

On 25 June 2012 11:50, Tomi Valkeinen [off-list ref] wrote:

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

....

quoted

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

Yes, it returns -EACCESS because RPM on devices is disabled during the
period from suspend-start to resume-finished.

On Mon, 2012-06-25 at 14:19 +0530, Jassi Brar wrote:

On 25 June 2012 11:50, Tomi Valkeinen [off-list ref] wrote:

quoted

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

....

quoted

quoted

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

Yes, it returns -EACCESS because RPM on devices is disabled during the
period from suspend-start to resume-finished.

So... You didn't answer my first comment, how can the code work? The
driver needs to enable the HW and the call to pm_runtime_get() is
skipped. Won't this lead to crash as the DSS registers are accessed
without the HW in enabled state? And what happens if the
pm_runtime_get() call is skipped, but pm_runtime_put() is not?

 Tomi

On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen [off-list ref] wrote:

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

quoted

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

On slightly different but related issue, currently OMAPDSS always
spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
because pm_runtime_put* always return -ENOSYS without
CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
maybe WARN_ON should check for -ENOSYS, I don't know..


-- 
Gražvydas

On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:

On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

quoted

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

On slightly different but related issue, currently OMAPDSS always
spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
because pm_runtime_put* always return -ENOSYS without
CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
maybe WARN_ON should check for -ENOSYS, I don't know..

Hmm. I guess I'm missing some understanding about runtime PM. omapdss
uses runtime PM to enable the underlying DSS hardware. If there's no
runtime PM, how does the driver work? Or is it the job of
hwmod/omap_device to keep all the hardware always enabled if runtime PM
is not compiled in?

 Tomi

On 25 June 2012 15:00, Tomi Valkeinen [off-list ref] wrote:

On Mon, 2012-06-25 at 14:19 +0530, Jassi Brar wrote:

quoted

On 25 June 2012 11:50, Tomi Valkeinen [off-list ref] wrote:

quoted

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

....

quoted

quoted

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

Yes, it returns -EACCESS because RPM on devices is disabled during the
period from suspend-start to resume-finished.

So... You didn't answer my first comment, how can the code work?

Sorry, don't know why I thought I didn't miss anything.

The driver needs to enable the HW and the call to pm_runtime_get() is
skipped. Won't this lead to crash as the DSS registers are accessed
without the HW in enabled state?

Hmm...  how does the extant code in hdmi driver ensures DSS is up and running ?
While it does sound important even to my limited knowledge of OMAPDSS,
I see rpm of HDMI, VENC and RFBI only dependent on DISPC, not DSS.

And for DISPC these drivers already hold a reference in their display
enable/resume and keep it until disable/suspend. So we don't lose
DISPC anytime it is really required.

And what happens if the pm_runtime_get() call is skipped, but pm_runtime_put() is not?

Not sure in what newly introduced scenario by this patch, because
get/put both check for pm_enabled before proceeding. Am I overlooking
something?

thnx

On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote:

On 25 June 2012 15:00, Tomi Valkeinen [off-list ref] wrote:

quoted

The driver needs to enable the HW and the call to pm_runtime_get() is
skipped. Won't this lead to crash as the DSS registers are accessed
without the HW in enabled state?

Hmm...  how does the extant code in hdmi driver ensures DSS is up and running ?
While it does sound important even to my limited knowledge of OMAPDSS,
I see rpm of HDMI, VENC and RFBI only dependent on DISPC, not DSS.

DSS device is parent to all the DSS subdevices. So when a subdevice is
enabled, DSS device is enabled first.

But anyway, I wasn't referring to the DSS part of OMAPDSS, but to
omapdss generally. If we do this:

/* this is skipped, if runtime PM is disabled */
dispc_runtime_get();

/* this accesses a register, but the HW is disabled? */
dispc_read_reg(FOO);

So again, I don't understand how the underlying HW gets enabled. Or does
hwmod/omap_device code make sure that it's enabled while the board is
being resumed? If so, what would happen if we continue the above
scenario as follows:

/* this is skipped, if runtime PM is disabled */
dispc_runtime_get();

/* this accesses a register, the HW is kept enabled by hwmod */
dispc_read_reg(FOO);

/* at some time later the resume procedure ends, and hwmod doesn't keep
the HW enabled any more */

/* this accesses a register, the HW is disabled */
dispc_read_reg(FOO);

And for DISPC these drivers already hold a reference in their display
enable/resume and keep it until disable/suspend. So we don't lose
DISPC anytime it is really required.

If all the displays are disabled, nobody keeps a reference to dispc.

quoted

And what happens if the pm_runtime_get() call is skipped, but pm_runtime_put() is not?

Not sure in what newly introduced scenario by this patch, because
get/put both check for pm_enabled before proceeding. Am I overlooking
something?

Currently (for example) dispc_runtime_get/put call
pm_runtime_get/put_sync. When somebody uses dispc_runtime_get, the same
somebody knows it needs to call dispc_runtime_put later.

Now, what happens if dispc_runtime_get is called when runtime PM is
disabled (i.e. pm_runtime_get_sync is skipped), but runtime PM is
enabled later when that somebody calls dispc_runtime_put (i.e.
pm_runtime_put_sync is _not_ skipped)?

 Tomi

On 25 June 2012 17:35, Grazvydas Ignotas [off-list ref] wrote:

On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen [off-list ref] wrote:

quoted

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

quoted

 Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

On slightly different but related issue, currently OMAPDSS always
spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
because pm_runtime_put* always return -ENOSYS without
CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
maybe WARN_ON should check for -ENOSYS, I don't know..

I didn't check, but this patch should already fix that I think ?

IMHO, for omapdss, we need not differentiate between -ENOSYS and
-EACCESS because anyway the ultimate functions dispc_runtime_resume()
and dispc_runtime_suspend()  can't report failure (they always return
0).

-j

On Mon, 2012-06-25 at 18:12 +0530, Rajendra Nayak wrote:

On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:

quoted

On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:

quoted

On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen[off-list ref]  wrote:

quoted

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

quoted

  Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

On slightly different but related issue, currently OMAPDSS always
spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
because pm_runtime_put* always return -ENOSYS without
CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
maybe WARN_ON should check for -ENOSYS, I don't know..

Hmm. I guess I'm missing some understanding about runtime PM. omapdss
uses runtime PM to enable the underlying DSS hardware. If there's no
runtime PM, how does the driver work? Or is it the job of
hwmod/omap_device to keep all the hardware always enabled if runtime PM
is not compiled in?

Yes, the below trick keeps all hwmods always enabled post the initial
setup if runtime PM is disabled.

from arch/arm/mach-omap2/io.c

static void __init omap_hwmod_init_postsetup(void)
{
         u8 postsetup_state;

         /* Set the default postsetup state for all hwmods */
#ifdef CONFIG_PM_RUNTIME
         postsetup_state = _HWMOD_STATE_IDLE;
#else
         postsetup_state = _HWMOD_STATE_ENABLED;
#endif
         omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);

2 more quoted lines

Ah, ok, thanks.

Do you know how the drivers should handle CONFIG_PM_RUNTIME=n?
Are they supposed to handle the error values returned by runtime PM
functions somehow, or should they use #ifdef CONFIG_PM_RUNTIME?

Both options sound a bit difficult to me... With the first one it's
difficult to see if there was an actual error and we should somehow
react to it, or is everything fine and we just shouldn't care about
runtime PM. The second one requires ifdefs in many places.

 Tomi

On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:

On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:

quoted

On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen[off-list ref]  wrote:

quoted

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

quoted

  Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

On slightly different but related issue, currently OMAPDSS always
spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
because pm_runtime_put* always return -ENOSYS without
CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
maybe WARN_ON should check for -ENOSYS, I don't know..

Hmm. I guess I'm missing some understanding about runtime PM. omapdss
uses runtime PM to enable the underlying DSS hardware. If there's no
runtime PM, how does the driver work? Or is it the job of
hwmod/omap_device to keep all the hardware always enabled if runtime PM
is not compiled in?

Yes, the below trick keeps all hwmods always enabled post the initial
setup if runtime PM is disabled.

from arch/arm/mach-omap2/io.c

static void __init omap_hwmod_init_postsetup(void)
{
         u8 postsetup_state;

         /* Set the default postsetup state for all hwmods */
#ifdef CONFIG_PM_RUNTIME
         postsetup_state = _HWMOD_STATE_IDLE;
#else
         postsetup_state = _HWMOD_STATE_ENABLED;
#endif
         omap_hwmod_for_each(_set_hwmod_postsetup_state, &postsetup_state);

         omap_pm_if_early_init();
}

  Tomi

On 25 June 2012 18:11, Tomi Valkeinen [off-list ref] wrote:

On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote:

quoted

On 25 June 2012 15:00, Tomi Valkeinen [off-list ref] wrote:

quoted

quoted

The driver needs to enable the HW and the call to pm_runtime_get() is
skipped. Won't this lead to crash as the DSS registers are accessed
without the HW in enabled state?

Hmm...  how does the extant code in hdmi driver ensures DSS is up and running ?
While it does sound important even to my limited knowledge of OMAPDSS,
I see rpm of HDMI, VENC and RFBI only dependent on DISPC, not DSS.

DSS device is parent to all the DSS subdevices. So when a subdevice is
enabled, DSS device is enabled first.

But anyway, I wasn't referring to the DSS part of OMAPDSS, but to
omapdss generally. If we do this:

/* this is skipped, if runtime PM is disabled */
dispc_runtime_get();

I hope you do realize that there is difference between "PM is disabled
on a device"
and "the device is in some low-power state".   pm_runtime_enabled()
checks for the former.
So under this light...

/* this accesses a register, but the HW is disabled? */
dispc_read_reg(FOO);

.... the H/W is already always enabled if CONFIG_PM_RUNTIME is not defined.

If CONFIG_PM_RUNTIME is indeed defined,  pm_runtime_enabled() will
always return true after pm_runtime_enable()  unless someone disables
it explicitly - omapdss or the RPM stack(during suspend/resume).
OMAPDSS never does so in the lifetime of a driver.  So the only period
in which pm_runtime_enabled() returns false, is when the platform is
suspending, suspended or resuming.

quoted

quoted

And what happens if the pm_runtime_get() call is skipped, but pm_runtime_put() is not?

Not sure in what newly introduced scenario by this patch, because
get/put both check for pm_enabled before proceeding. Am I overlooking
something?

Currently (for example) dispc_runtime_get/put call
pm_runtime_get/put_sync. When somebody uses dispc_runtime_get, the same
somebody knows it needs to call dispc_runtime_put later.

Now, what happens if dispc_runtime_get is called when runtime PM is
disabled (i.e. pm_runtime_get_sync is skipped), but runtime PM is
enabled later when that somebody calls dispc_runtime_put (i.e.
pm_runtime_put_sync is _not_ skipped)?

As I said, for omapdss, PM is disabled (not device deactivated) only
during rpm suspend/resume.
And it should be no different than any lock protected section
preempted by suspend-resume before reaching its end.

On Mon, 2012-06-25 at 19:01 +0530, Jassi Brar wrote:

On 25 June 2012 18:11, Tomi Valkeinen [off-list ref] wrote:

quoted

On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote:

quoted

On 25 June 2012 15:00, Tomi Valkeinen [off-list ref] wrote:

quoted

quoted

The driver needs to enable the HW and the call to pm_runtime_get() is
skipped. Won't this lead to crash as the DSS registers are accessed
without the HW in enabled state?

Hmm...  how does the extant code in hdmi driver ensures DSS is up and running ?
While it does sound important even to my limited knowledge of OMAPDSS,
I see rpm of HDMI, VENC and RFBI only dependent on DISPC, not DSS.

DSS device is parent to all the DSS subdevices. So when a subdevice is
enabled, DSS device is enabled first.

But anyway, I wasn't referring to the DSS part of OMAPDSS, but to
omapdss generally. If we do this:

/* this is skipped, if runtime PM is disabled */
dispc_runtime_get();

I hope you do realize that there is difference between "PM is disabled
on a device"
and "the device is in some low-power state".   pm_runtime_enabled()
checks for the former.
So under this light...

quoted

/* this accesses a register, but the HW is disabled? */
dispc_read_reg(FOO);

.... the H/W is already always enabled if CONFIG_PM_RUNTIME is not defined.

If CONFIG_PM_RUNTIME is indeed defined,  pm_runtime_enabled() will
always return true after pm_runtime_enable()  unless someone disables
it explicitly - omapdss or the RPM stack(during suspend/resume).
OMAPDSS never does so in the lifetime of a driver.  So the only period
in which pm_runtime_enabled() returns false, is when the platform is
suspending, suspended or resuming.

Right. So what happens in my example above?

Normally if the driver does dispc_runtime_get() and dispc_read_reg(),
the first call will enable the HW so the reg read works. 

But if the pm_runtime is disabled, say, during system suspend, with your
patch dispc_runtime_get() will just return 0 without doing anything, and
the dispc_read_reg() will crash because the HW is disabled (because
nobody enabled it).

Perhaps I'm missing something, but I don't quite understand how this
works.

quoted

quoted

quoted

And what happens if the pm_runtime_get() call is skipped, but pm_runtime_put() is not?

Not sure in what newly introduced scenario by this patch, because
get/put both check for pm_enabled before proceeding. Am I overlooking
something?

Currently (for example) dispc_runtime_get/put call
pm_runtime_get/put_sync. When somebody uses dispc_runtime_get, the same
somebody knows it needs to call dispc_runtime_put later.

Now, what happens if dispc_runtime_get is called when runtime PM is
disabled (i.e. pm_runtime_get_sync is skipped), but runtime PM is
enabled later when that somebody calls dispc_runtime_put (i.e.
pm_runtime_put_sync is _not_ skipped)?

As I said, for omapdss, PM is disabled (not device deactivated) only
during rpm suspend/resume.
And it should be no different than any lock protected section
preempted by suspend-resume before reaching its end.

I'm not sure if I understand... If the driver does dispc_runtime_get()
while the PM is disabled, say, during system resume, dispc_runtime_get()
will do nothing and return 0. The driver thinks it succeeded, and will
call dispc_runtime_put() later.

Calling the dispc_runtime_put() could happen very soon, while runtime PM
is still disabled, in which case everything works fine. But there's no
rule to say dispc_runtime_put() has to be called very soon after
dispc_runtime_get(). The driver might as well call put later, when
runtime PM is enabled.

This would end up with a pm_runtime_put call without a matching
pm_runtime_get call.

 Tomi

On 25 June 2012 19:19, Tomi Valkeinen [off-list ref] wrote:

On Mon, 2012-06-25 at 19:01 +0530, Jassi Brar wrote:

quoted

quoted

/* this accesses a register, but the HW is disabled? */
dispc_read_reg(FOO);

.... the H/W is already always enabled if CONFIG_PM_RUNTIME is not defined.

If CONFIG_PM_RUNTIME is indeed defined,  pm_runtime_enabled() will
always return true after pm_runtime_enable()  unless someone disables
it explicitly - omapdss or the RPM stack(during suspend/resume).
OMAPDSS never does so in the lifetime of a driver.  So the only period
in which pm_runtime_enabled() returns false, is when the platform is
suspending, suspended or resuming.

Right. So what happens in my example above?

Normally if the driver does dispc_runtime_get() and dispc_read_reg(),
the first call will enable the HW so the reg read works.

But if the pm_runtime is disabled, say, during system suspend, with your
patch dispc_runtime_get() will just return 0 without doing anything, and
the dispc_read_reg() will crash because the HW is disabled (because
nobody enabled it).

Hmm, I am not sure if new calls would/should be made to dispc.c after
the system has suspended and before resumed. That is, anything other
than from runtime_resume/suspend callbacks of DSS, DISPC, HDMI, VENC
and RFBI, which rightly don't touch any dss reg but only
enable/disable a clock.
As we know, a subsystem should make sure any active work is cleared
out before suspending and set some flag so that nothing runs until it
has resumed. I don't say we can't crash the system with this patch,
but then we would be violating rules of suspend-resume.

quoted

As I said, for omapdss, PM is disabled (not device deactivated) only
during rpm suspend/resume.
And it should be no different than any lock protected section
preempted by suspend-resume before reaching its end.

I'm not sure if I understand... If the driver does dispc_runtime_get()
while the PM is disabled, say, during system resume, dispc_runtime_get()
will do nothing and return 0. The driver thinks it succeeded, and will
call dispc_runtime_put() later.

Calling the dispc_runtime_put() could happen very soon, while runtime PM
is still disabled, in which case everything works fine. But there's no
rule to say dispc_runtime_put() has to be called very soon after
dispc_runtime_get(). The driver might as well call put later, when
runtime PM is enabled.

This would end up with a pm_runtime_put call without a matching
pm_runtime_get call.

Again, we need to see if there is really some situation where
something new is attempted before the subsystem has resumed. If there
is indeed, maybe omapdss need to flag it's suspended and prevent such
thing until it has resumed.
Btw, even without this patch, when dispc_runtime_get() does return
error under rpm disabled, we disturb the dev.power.usage_count balance
by not calling dispc_runtime_put()
This patch doesn't make everything perfect, but only improve upon the
current situation.

thnx

On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:

On Mon, 2012-06-25 at 18:12 +0530, Rajendra Nayak wrote:

quoted

On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:

quoted

On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:

quoted

On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen[off-list ref]   wrote:

quoted

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:

quoted

   Currenlty HDMI fails to come up in the suspend-resume path.
This patch helps that real-world scenario.

What is the problem there? It'd be good to explain the problem in the
patch description. Does the pm_runtime_get return -EACCES?

On slightly different but related issue, currently OMAPDSS always
spits lots of backtraces when it's compiled without CONFIG_PM_RUNTIME,
because pm_runtime_put* always return -ENOSYS without
CONFIG_PM_RUNTIME. So something like this patch proposes is needed, or
maybe WARN_ON should check for -ENOSYS, I don't know..

Hmm. I guess I'm missing some understanding about runtime PM. omapdss
uses runtime PM to enable the underlying DSS hardware. If there's no
runtime PM, how does the driver work? Or is it the job of
hwmod/omap_device to keep all the hardware always enabled if runtime PM
is not compiled in?

Yes, the below trick keeps all hwmods always enabled post the initial
setup if runtime PM is disabled.

from arch/arm/mach-omap2/io.c

static void __init omap_hwmod_init_postsetup(void)
{
          u8 postsetup_state;

          /* Set the default postsetup state for all hwmods */
#ifdef CONFIG_PM_RUNTIME
          postsetup_state = _HWMOD_STATE_IDLE;
#else
          postsetup_state = _HWMOD_STATE_ENABLED;
#endif
          omap_hwmod_for_each(_set_hwmod_postsetup_state,&postsetup_state);

9 more quoted lines

hmm, I always though with CONFIG_RUNTIME_PM=n, the functions would
be stubbed to return success and not failure. And the _pm_runtime_resume
function indeed seems to return 1, which is not failure but just saying
that your device is already active/enabled.
The _pm_runtime_suspend and _pm_runtime_idle do return a -ENOSYS, which
is something only returned when CONFIG_RUNTIME_PM=n, so if you really
want to handle failing pm_runtime_put_sync cases, maybe you still can.
But then, I don't know if there is anything you can do to recover from
a failing pm_runtime_put_sync, except for warning the user maybe.

Both options sound a bit difficult to me... With the first one it's
difficult to see if there was an actual error and we should somehow
react to it, or is everything fine and we just shouldn't care about
runtime PM. The second one requires ifdefs in many places.

  Tomi

On Mon, 2012-06-25 at 22:36 +0530, Jassi Brar wrote:

On 25 June 2012 19:19, Tomi Valkeinen [off-list ref] wrote:

quoted

On Mon, 2012-06-25 at 19:01 +0530, Jassi Brar wrote:

quoted

quoted

quoted

/* this accesses a register, but the HW is disabled? */
dispc_read_reg(FOO);

.... the H/W is already always enabled if CONFIG_PM_RUNTIME is not defined.

If CONFIG_PM_RUNTIME is indeed defined,  pm_runtime_enabled() will
always return true after pm_runtime_enable()  unless someone disables
it explicitly - omapdss or the RPM stack(during suspend/resume).
OMAPDSS never does so in the lifetime of a driver.  So the only period
in which pm_runtime_enabled() returns false, is when the platform is
suspending, suspended or resuming.

Right. So what happens in my example above?

Normally if the driver does dispc_runtime_get() and dispc_read_reg(),
the first call will enable the HW so the reg read works.

But if the pm_runtime is disabled, say, during system suspend, with your
patch dispc_runtime_get() will just return 0 without doing anything, and
the dispc_read_reg() will crash because the HW is disabled (because
nobody enabled it).

Hmm, I am not sure if new calls would/should be made to dispc.c after
the system has suspended and before resumed. That is, anything other
than from runtime_resume/suspend callbacks of DSS, DISPC, HDMI, VENC
and RFBI, which rightly don't touch any dss reg but only
enable/disable a clock.

They do touch the registers. For example, dispc's callbacks save and
restore the registers. The HW should be fully functional during the
callbacks. The point of the callbacks is to suspend/resume the HW in
question, which of course requires accessing the HW.

As we know, a subsystem should make sure any active work is cleared
out before suspending and set some flag so that nothing runs until it
has resumed. I don't say we can't crash the system with this patch,
but then we would be violating rules of suspend-resume.

Let's go back a bit. I feel like I'm missing some pieces of information,
as I still don't quite grasp the problem.

In the patch you said this fixes an issue with HDMI. Can you tell more
about the problem? What code path is being run? Any error messages?

I tried system suspend with omap4-sdp and panda, with 3.5-rc2, but
neither board seems to wake up from the suspend. Does it work for you?

 Tomi

On 06/26/12 16:32, the mail apparently from Jassi Brar included:

On 26 June 2012 12:49, Tomi Valkeinen [off-list ref] wrote:

quoted

On Mon, 2012-06-25 at 22:36 +0530, Jassi Brar wrote:

quoted

quoted

quoted

Normally if the driver does dispc_runtime_get() and dispc_read_reg(),
the first call will enable the HW so the reg read works.

But if the pm_runtime is disabled, say, during system suspend, with your
patch dispc_runtime_get() will just return 0 without doing anything, and
the dispc_read_reg() will crash because the HW is disabled (because
nobody enabled it).

Hmm, I am not sure if new calls would/should be made to dispc.c after
the system has suspended and before resumed. That is, anything other
than from runtime_resume/suspend callbacks of DSS, DISPC, HDMI, VENC
and RFBI, which rightly don't touch any dss reg but only
enable/disable a clock.

They do touch the registers. For example, dispc's callbacks save and
restore the registers. The HW should be fully functional during the
callbacks. The point of the callbacks is to suspend/resume the HW in
question, which of course requires accessing the HW.

DISPC being held by HDMI, VENC and RFBI would be the last to suspend
and first to resume. And it won't have its registers touched between
dispc_runtime_suspend() and dispc_runtime_resume(), which seems ok to
me (?)
HDMI, VENC and RFBI directly fooling around with DISPC regs would have
been a problem, which isn't the case.

quoted

quoted

As we know, a subsystem should make sure any active work is cleared
out before suspending and set some flag so that nothing runs until it
has resumed. I don't say we can't crash the system with this patch,
but then we would be violating rules of suspend-resume.

Let's go back a bit. I feel like I'm missing some pieces of information,
as I still don't quite grasp the problem.

In the patch you said this fixes an issue with HDMI. Can you tell more
about the problem? What code path is being run? Any error messages?

16 more quoted lines

We don't have access to Blaze and don't test that tree against it, but 
it's worth trying on PandaBoard ES which we do have and test against 
(omap4plus_defconfig).

Here, mem suspend is working with HDMI raster coming back on resume, but 
we don't always get a desktop redraw (suspending again can "correct" 
that).  Jassi's patches are present in this tree.

A slightly side-issue, I have a TV here that only issues hpd 700ms after 
the Panda provides 5V at the HDMI link.  It has always been touch-and-go 
if dss will recognize it or not, compared to a monitor which issues hpd 
high within some us of the link being powered.

The patches from Jassi about permanently enabling the external HDMI PHY 
chip section that performs level-conversion for hpd, and the existing 
work to use irq management of hpd, seems to have really solved detecting 
that TV for the first time.

-Andy

-- 
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  - 
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

On 26 June 2012 12:49, Tomi Valkeinen [off-list ref] wrote:

On Mon, 2012-06-25 at 22:36 +0530, Jassi Brar wrote:

quoted

quoted

Normally if the driver does dispc_runtime_get() and dispc_read_reg(),
the first call will enable the HW so the reg read works.

But if the pm_runtime is disabled, say, during system suspend, with your
patch dispc_runtime_get() will just return 0 without doing anything, and
the dispc_read_reg() will crash because the HW is disabled (because
nobody enabled it).

Hmm, I am not sure if new calls would/should be made to dispc.c after
the system has suspended and before resumed. That is, anything other
than from runtime_resume/suspend callbacks of DSS, DISPC, HDMI, VENC
and RFBI, which rightly don't touch any dss reg but only
enable/disable a clock.

They do touch the registers. For example, dispc's callbacks save and
restore the registers. The HW should be fully functional during the
callbacks. The point of the callbacks is to suspend/resume the HW in
question, which of course requires accessing the HW.

DISPC being held by HDMI, VENC and RFBI would be the last to suspend
and first to resume. And it won't have its registers touched between
dispc_runtime_suspend() and dispc_runtime_resume(), which seems ok to
me (?)
HDMI, VENC and RFBI directly fooling around with DISPC regs would have
been a problem, which isn't the case.

quoted

As we know, a subsystem should make sure any active work is cleared
out before suspending and set some flag so that nothing runs until it
has resumed. I don't say we can't crash the system with this patch,
but then we would be violating rules of suspend-resume.

Let's go back a bit. I feel like I'm missing some pieces of information,
as I still don't quite grasp the problem.

In the patch you said this fixes an issue with HDMI. Can you tell more
about the problem? What code path is being run? Any error messages?

I tried system suspend with omap4-sdp and panda, with 3.5-rc2, but
neither board seems to wake up from the suspend. Does it work for you?

Something non-omapdss in vanilla breaks suspend/resume.
Without this patch I see the upstream's display broken after the
suspend attempt.
    $ echo mem > /sys/power/state

I work on TILT tree, which has suspend/resume working after some more
local patches.
    http://git.linaro.org/gitweb?p=people/andygreen/kernel-tilt.git;a=shortlog;h=refs/heads/tilt-3.4

I don't have SDP so not sure, but it should simply be testable with
Panda4460 and the omap4plus_defconfig there.  Please feel free to ask
if you have any issue checking that out.

Thanks.

On Tue, 2012-06-26 at 14:02 +0530, Jassi Brar wrote:

Something non-omapdss in vanilla breaks suspend/resume.

I was able to reproduce (probably) the same issue with omap3 overo. Does
this looks familiar:

[ 2267.140197] ------------[ cut here ]------------
[ 2267.145172] WARNING: at drivers/video/omap2/dss/dispc.c:377 dispc_runtime_get+0x60/0x7c [omapdss]
()
[ 2267.154846] Modules linked in: omapfb panel_generic_dpi omapdss cfbimgblt cfbfillrect cfbcopyarea
 [last unloaded: omapdss]
[ 2267.166595] [<c001b61c>] (unwind_backtrace+0x0/0xf0) from [<c0040238>] (warn_slowpath_common+0x4c
/0x64)
[ 2267.176605] [<c0040238>] (warn_slowpath_common+0x4c/0x64) from [<c004026c>] (warn_slowpath_null+0
x1c/0x24)
[ 2267.186859] [<c004026c>] (warn_slowpath_null+0x1c/0x24) from [<bf0d7918>] (dispc_runtime_get+0x60
/0x7c [omapdss])
[ 2267.197814] [<bf0d7918>] (dispc_runtime_get+0x60/0x7c [omapdss]) from [<bf0e3148>] (omapdss_dpi_d
isplay_enable+0x48/0x230 [omapdss])
[ 2267.210479] [<bf0e3148>] (omapdss_dpi_display_enable+0x48/0x230 [omapdss]) from [<bf110034>] (gen
eric_dpi_panel_check_timings+0x30/0x7c [panel_generic_dpi])
[ 2267.225311] [<bf110034>] (generic_dpi_panel_check_timings+0x30/0x7c [panel_generic_dpi]) from [<b
f11008c>] (generic_dpi_panel_resume+0xc/0x1c [panel_generic_dpi])
[ 2267.240722] [<bf11008c>] (generic_dpi_panel_resume+0xc/0x1c [panel_generic_dpi]) from [<bf0de654>
] (dss_resume_device+0x28/0x40 [omapdss])
[ 2267.253936] [<bf0de654>] (dss_resume_device+0x28/0x40 [omapdss]) from [<c02bfb94>] (bus_for_each_
dev+0x50/0x7c)
[ 2267.264678] [<c02bfb94>] (bus_for_each_dev+0x50/0x7c) from [<c02c287c>] (platform_pm_resume+0x2c/
0x50)
[ 2267.274566] [<c02c287c>] (platform_pm_resume+0x2c/0x50) from [<c02c6da8>] (dpm_run_callback.clone
.7+0x30/0xb0)
[ 2267.285186] [<c02c6da8>] (dpm_run_callback.clone.7+0x30/0xb0) from [<c02c7b2c>] (device_resume+0x
c8/0x188)
[ 2267.295471] [<c02c7b2c>] (device_resume+0xc8/0x188) from [<c02c7f54>] (dpm_resume+0xfc/0x21c)
[ 2267.304534] [<c02c7f54>] (dpm_resume+0xfc/0x21c) from [<c02c8208>] (dpm_resume_end+0xc/0x18)
[ 2267.313507] [<c02c8208>] (dpm_resume_end+0xc/0x18) from [<c007fbcc>] (suspend_devices_and_enter+0
x15c/0x2d0)
[ 2267.323913] [<c007fbcc>] (suspend_devices_and_enter+0x15c/0x2d0) from [<c007fecc>] (pm_suspend+0x
18c/0x208)
[ 2267.334259] [<c007fecc>] (pm_suspend+0x18c/0x208) from [<c007f170>] (state_store+0x120/0x134)
[ 2267.343292] [<c007f170>] (state_store+0x120/0x134) from [<c0262850>] (kobj_attr_store+0x14/0x20)
[ 2267.352661] [<c0262850>] (kobj_attr_store+0x14/0x20) from [<c0169b6c>] (sysfs_write_file+0x100/0x

27 more lines

On 26 June 2012 14:37, Tomi Valkeinen [off-list ref] wrote:

On Tue, 2012-06-26 at 14:02 +0530, Jassi Brar wrote:

quoted

Something non-omapdss in vanilla breaks suspend/resume.

I was able to reproduce (probably) the same issue with omap3 overo. Does
this looks familiar:

[ 2267.140197] ------------[ cut here ]------------
[ 2267.145172] WARNING: at drivers/video/omap2/dss/dispc.c:377 dispc_runtime_get+0x60/0x7c [omapdss]
()
[ 2267.154846] Modules linked in: omapfb panel_generic_dpi omapdss cfbimgblt cfbfillrect cfbcopyarea
 [last unloaded: omapdss]
[ 2267.166595] [<c001b61c>] (unwind_backtrace+0x0/0xf0) from [<c0040238>] (warn_slowpath_common+0x4c
/0x64)
[ 2267.176605] [<c0040238>] (warn_slowpath_common+0x4c/0x64) from [<c004026c>] (warn_slowpath_null+0
x1c/0x24)
[ 2267.186859] [<c004026c>] (warn_slowpath_null+0x1c/0x24) from [<bf0d7918>] (dispc_runtime_get+0x60
/0x7c [omapdss])
[ 2267.197814] [<bf0d7918>] (dispc_runtime_get+0x60/0x7c [omapdss]) from [<bf0e3148>] (omapdss_dpi_d
isplay_enable+0x48/0x230 [omapdss])
[ 2267.210479] [<bf0e3148>] (omapdss_dpi_display_enable+0x48/0x230 [omapdss]) from [<bf110034>] (gen
eric_dpi_panel_check_timings+0x30/0x7c [panel_generic_dpi])
[ 2267.225311] [<bf110034>] (generic_dpi_panel_check_timings+0x30/0x7c [panel_generic_dpi]) from [<b
f11008c>] (generic_dpi_panel_resume+0xc/0x1c [panel_generic_dpi])
[ 2267.240722] [<bf11008c>] (generic_dpi_panel_resume+0xc/0x1c [panel_generic_dpi]) from [<bf0de654>
] (dss_resume_device+0x28/0x40 [omapdss])
[ 2267.253936] [<bf0de654>] (dss_resume_device+0x28/0x40 [omapdss]) from [<c02bfb94>] (bus_for_each_
dev+0x50/0x7c)
[ 2267.264678] [<c02bfb94>] (bus_for_each_dev+0x50/0x7c) from [<c02c287c>] (platform_pm_resume+0x2c/
0x50)
[ 2267.274566] [<c02c287c>] (platform_pm_resume+0x2c/0x50) from [<c02c6da8>] (dpm_run_callback.clone
.7+0x30/0xb0)
[ 2267.285186] [<c02c6da8>] (dpm_run_callback.clone.7+0x30/0xb0) from [<c02c7b2c>] (device_resume+0x
c8/0x188)
[ 2267.295471] [<c02c7b2c>] (device_resume+0xc8/0x188) from [<c02c7f54>] (dpm_resume+0xfc/0x21c)
[ 2267.304534] [<c02c7f54>] (dpm_resume+0xfc/0x21c) from [<c02c8208>] (dpm_resume_end+0xc/0x18)
[ 2267.313507] [<c02c8208>] (dpm_resume_end+0xc/0x18) from [<c007fbcc>] (suspend_devices_and_enter+0
x15c/0x2d0)
[ 2267.323913] [<c007fbcc>] (suspend_devices_and_enter+0x15c/0x2d0) from [<c007fecc>] (pm_suspend+0x
18c/0x208)

11 more quoted lines

Seems similar, but I only tested OMAP4 HDMI.
thanks.

On Tue, 2012-06-26 at 15:27 +0530, Jassi Brar wrote:

Seems similar, but I only tested OMAP4 HDMI.

Would something like this one below work for you? It fixes the issues on
my overo board.

Instead of using omapdss device's suspend/resume callbacks, this one
uses PM notifier calls which happen before suspend and after resume.

I still think the suspend handling is wrong, omapdss shouldn't be
enabling and disabling panel devices like that, but this one should
remove the biggest issues with the current suspend method.

 Tomi

diff --git a/drivers/video/omap2/dss/core.c b/drivers/video/omap2/dss/core.c
index 5066eee..c35a248 100644
--- a/drivers/video/omap2/dss/core.c
+++ b/drivers/video/omap2/dss/core.c

@@ -32,6 +32,7 @@
 #include <linux/io.h>
 #include <linux/device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/suspend.h>
 
 #include <video/omapdss.h>
 

@@ -201,6 +202,30 @@ int dss_debugfs_create_file(const char *name, void (*write)(struct seq_file *))
 #endif /* CONFIG_DEBUG_FS && CONFIG_OMAP2_DSS_DEBUG_SUPPORT */
 
 /* PLATFORM DEVICE */
+static int omap_dss_pm_notif(struct notifier_block *b, unsigned long v, void *d)
+{
+	DSSDBG("pm notif %lu\n", v);
+
+	switch (v)
+	{
+	case PM_SUSPEND_PREPARE:
+		DSSDBG("suspending displays\n");
+		return dss_suspend_all_devices();
+
+	case PM_POST_SUSPEND:
+		DSSDBG("resuming displays\n");
+		return dss_resume_all_devices();
+
+	default:
+		return 0;
+	}
+}
+
+static struct notifier_block omap_dss_pm_notif_block =
+{
+	.notifier_call = omap_dss_pm_notif,
+};
+
 static int __init omap_dss_probe(struct platform_device *pdev)
 {
 	struct omap_dss_board_info *pdata = pdev->dev.platform_data;

@@ -224,6 +249,8 @@ static int __init omap_dss_probe(struct platform_device *pdev)
 	else if (pdata->default_device)
 		core.default_display_name = pdata->default_device->name;
 
+	register_pm_notifier(&omap_dss_pm_notif_block);
+
 	return 0;
 
 err_debugfs:

@@ -233,6 +260,8 @@ err_debugfs:
 
 static int omap_dss_remove(struct platform_device *pdev)
 {
+	unregister_pm_notifier(&omap_dss_pm_notif_block);
+
 	dss_uninitialize_debugfs();
 
 	dss_uninit_overlays(pdev);

@@ -247,25 +276,9 @@ static void omap_dss_shutdown(struct platform_device *pdev)
 	dss_disable_all_devices();
 }
 
-static int omap_dss_suspend(struct platform_device *pdev, pm_message_t state)
-{
-	DSSDBG("suspend %d\n", state.event);
-
-	return dss_suspend_all_devices();
-}
-
-static int omap_dss_resume(struct platform_device *pdev)
-{
-	DSSDBG("resume\n");
-
-	return dss_resume_all_devices();
-}
-
 static struct platform_driver omap_dss_driver = {
 	.remove         = omap_dss_remove,
 	.shutdown	= omap_dss_shutdown,
-	.suspend	= omap_dss_suspend,
-	.resume		= omap_dss_resume,
 	.driver         = {
 		.name   = "omapdss",
 		.owner  = THIS_MODULE,

CCing some PM people, maybe they can comment?

On Tue, Jun 26, 2012 at 7:51 AM, Rajendra Nayak [off-list ref] wrote:

On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:

quoted

Do you know how the drivers should handle CONFIG_PM_RUNTIME=n?
Are they supposed to handle the error values returned by runtime PM
functions somehow, or should they use #ifdef CONFIG_PM_RUNTIME?

hmm, I always though with CONFIG_RUNTIME_PM=n, the functions would
be stubbed to return success and not failure. And the _pm_runtime_resume
function indeed seems to return 1, which is not failure but just saying
that your device is already active/enabled.
The _pm_runtime_suspend and _pm_runtime_idle do return a -ENOSYS, which
is something only returned when CONFIG_RUNTIME_PM=n, so if you really
want to handle failing pm_runtime_put_sync cases, maybe you still can.
But then, I don't know if there is anything you can do to recover from
a failing pm_runtime_put_sync, except for warning the user maybe.

quoted

Both options sound a bit difficult to me... With the first one it's
difficult to see if there was an actual error and we should somehow
react to it, or is everything fine and we just shouldn't care about
runtime PM. The second one requires ifdefs in many places.

-- 
Gražvydas

On Tue, 26 Jun 2012, Grazvydas Ignotas wrote:

CCing some PM people, maybe they can comment?

On Tue, Jun 26, 2012 at 7:51 AM, Rajendra Nayak [off-list ref] wrote:

quoted

On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:

quoted

Do you know how the drivers should handle CONFIG_PM_RUNTIME=n?
Are they supposed to handle the error values returned by runtime PM
functions somehow, or should they use #ifdef CONFIG_PM_RUNTIME?

hmm, I always though with CONFIG_RUNTIME_PM=n, the functions would
be stubbed to return success and not failure.

Not exactly.  They are stubbed to indicate that the device cannot be 
suspended, that it is always active.

Failure to suspend a device should not be regarded as particularly bad, 
because it doesn't affect the device's functionality.  That's true even 
when CONFIG_RUNTIME_PM is enabled.

And the _pm_runtime_resume

quoted

function indeed seems to return 1, which is not failure but just saying
that your device is already active/enabled.
The _pm_runtime_suspend and _pm_runtime_idle do return a -ENOSYS, which
is something only returned when CONFIG_RUNTIME_PM=n, so if you really
want to handle failing pm_runtime_put_sync cases, maybe you still can.
But then, I don't know if there is anything you can do to recover from
a failing pm_runtime_put_sync, except for warning the user maybe.

I don't see much point in warning the user that a device was unable to 
go to low power.

Alan Stern

On 26 June 2012 17:33, Tomi Valkeinen [off-list ref] wrote:

On Tue, 2012-06-26 at 15:27 +0530, Jassi Brar wrote:

quoted

Seems similar, but I only tested OMAP4 HDMI.

Would something like this one below work for you? It fixes the issues on
my overo board.

I think this should work too (I will get to test it only tomorrow).

Though I don't think it'll fix stack spew when run without
CONFIG_PM_RUNTIME. Maybe we could simply remove the WARN_ON in the
xxx_runtime_put() as Alan noted?

-j

56 more quoted lines

-- 
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106  -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog

On Tue, 2012-06-26 at 10:34 -0400, Alan Stern wrote:

On Tue, 26 Jun 2012, Grazvydas Ignotas wrote:

quoted

CCing some PM people, maybe they can comment?

On Tue, Jun 26, 2012 at 7:51 AM, Rajendra Nayak [off-list ref] wrote:

quoted

On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:

quoted

Do you know how the drivers should handle CONFIG_PM_RUNTIME=n?
Are they supposed to handle the error values returned by runtime PM
functions somehow, or should they use #ifdef CONFIG_PM_RUNTIME?

hmm, I always though with CONFIG_RUNTIME_PM=n, the functions would
be stubbed to return success and not failure.

Not exactly.  They are stubbed to indicate that the device cannot be 
suspended, that it is always active.

Failure to suspend a device should not be regarded as particularly bad, 
because it doesn't affect the device's functionality.  That's true even 
when CONFIG_RUNTIME_PM is enabled.

This makes sense. So if CONFIG_RUNTIME_PM=n, using pm_runtime_get_sync()
will return 1, meaning the HW is already enabled, and using
pm_runtime_put_sync() will return -ENOSYS, meaning the hardware cannot
be suspended.

With CONFIG_RUNTIME_PM=y, it's a bit more complex. If I read the code
correctly, when I call pm_runtime_get_sync(), the usage counter is
always increased, even if the pm_runtime_resume() fails. So a get()
needs to be always matched with a put(), even if get() has returned an
error.

But if pm_runtime_get_sync() returns an error, it means the HW has not
been resumed successfully, and is not operational, so the code should
bail out somehow. So basically I'd use this kind of pattern everywhere I
use pm_runtime_get_sync():

---

r = pm_runtime_get_sync(dev);
if (r < 0) {
	pm_runtime_put_sync(dev);
	/* handle error */
	return -ESOMETHING;
}

/* do the work */

pm_runtime_put_sync(dev);

---

Is this correct?

 Tomi

On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:

On 26 June 2012 17:33, Tomi Valkeinen [off-list ref] wrote:

quoted

On Tue, 2012-06-26 at 15:27 +0530, Jassi Brar wrote:

quoted

Seems similar, but I only tested OMAP4 HDMI.

Would something like this one below work for you? It fixes the issues on
my overo board.

I think this should work too (I will get to test it only tomorrow).

Though I don't think it'll fix stack spew when run without
CONFIG_PM_RUNTIME. Maybe we could simply remove the WARN_ON in the
xxx_runtime_put() as Alan noted?

Yes, that's a different issue. I'll look at that also.

 Tomi

On Tue, 26 Jun 2012, Tomi Valkeinen wrote:

quoted

Failure to suspend a device should not be regarded as particularly bad, 
because it doesn't affect the device's functionality.  That's true even 
when CONFIG_RUNTIME_PM is enabled.

This makes sense. So if CONFIG_RUNTIME_PM=n, using pm_runtime_get_sync()
will return 1, meaning the HW is already enabled, and using
pm_runtime_put_sync() will return -ENOSYS, meaning the hardware cannot
be suspended.

With CONFIG_RUNTIME_PM=y, it's a bit more complex. If I read the code
correctly, when I call pm_runtime_get_sync(), the usage counter is
always increased, even if the pm_runtime_resume() fails. So a get()
needs to be always matched with a put(), even if get() has returned an
error.

Right.  Of course, it doesn't hurt to match a get() with a put() even 
when CONFIG_RUNTIME_PM=n.

But if pm_runtime_get_sync() returns an error, it means the HW has not
been resumed successfully, and is not operational, so the code should
bail out somehow. So basically I'd use this kind of pattern everywhere I
use pm_runtime_get_sync():

---

r = pm_runtime_get_sync(dev);
if (r < 0) {
	pm_runtime_put_sync(dev);

Here you could just as well call pm_runtime_put_noidle().  Since the 
device wasn't resumed, the put operation doesn't need to try to suspend 
it.

	/* handle error */
	return -ESOMETHING;
}

/* do the work */

pm_runtime_put_sync(dev);

---

Is this correct?

Yep, you've got it.

Alan Stern

On Tue, 2012-06-26 at 20:39 +0530, Jassi Brar wrote:

On 26 June 2012 20:38, Tomi Valkeinen [off-list ref] wrote:

quoted

On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:

quoted

On 26 June 2012 17:33, Tomi Valkeinen [off-list ref] wrote:

quoted

On Tue, 2012-06-26 at 15:27 +0530, Jassi Brar wrote:

quoted

Seems similar, but I only tested OMAP4 HDMI.

Would something like this one below work for you? It fixes the issues on
my overo board.

I think this should work too (I will get to test it only tomorrow).

Though I don't think it'll fix stack spew when run without
CONFIG_PM_RUNTIME. Maybe we could simply remove the WARN_ON in the
xxx_runtime_put() as Alan noted?

Yes, that's a different issue. I'll look at that also.

Well, my patch took care of that also. But I agree, that could be
added separately as well.

Well, I don't agree that your patch is correct =). I don't think it's
right to skip runtime get and put when pm_runtime_enabled() returns
false.

 Tomi

On 26 June 2012 20:38, Tomi Valkeinen [off-list ref] wrote:

On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:

quoted

On 26 June 2012 17:33, Tomi Valkeinen [off-list ref] wrote:

quoted

On Tue, 2012-06-26 at 15:27 +0530, Jassi Brar wrote:

quoted

Seems similar, but I only tested OMAP4 HDMI.

Would something like this one below work for you? It fixes the issues on
my overo board.

I think this should work too (I will get to test it only tomorrow).

Though I don't think it'll fix stack spew when run without
CONFIG_PM_RUNTIME. Maybe we could simply remove the WARN_ON in the
xxx_runtime_put() as Alan noted?

Yes, that's a different issue. I'll look at that also.

Well, my patch took care of that also. But I agree, that could be
added separately as well.

On 26 June 2012 20:41, Tomi Valkeinen [off-list ref] wrote:

On Tue, 2012-06-26 at 20:39 +0530, Jassi Brar wrote:

quoted

On 26 June 2012 20:38, Tomi Valkeinen [off-list ref] wrote:

quoted

On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:

quoted

On 26 June 2012 17:33, Tomi Valkeinen [off-list ref] wrote:

quoted

On Tue, 2012-06-26 at 15:27 +0530, Jassi Brar wrote:

quoted

Seems similar, but I only tested OMAP4 HDMI.

Would something like this one below work for you? It fixes the issues on
my overo board.

I think this should work too (I will get to test it only tomorrow).

Though I don't think it'll fix stack spew when run without
CONFIG_PM_RUNTIME. Maybe we could simply remove the WARN_ON in the
xxx_runtime_put() as Alan noted?

Yes, that's a different issue. I'll look at that also.

Well, my patch took care of that also. But I agree, that could be
added separately as well.

Well, I don't agree that your patch is correct =). I don't think it's
right to skip runtime get and put when pm_runtime_enabled() returns
false.

While I think your patch is simpler and achieve the same, I also think
your fears about this patch are unfounded.

A quick snack for thought...

But if pm_runtime_get_sync() returns an error, it means the HW has not
been resumed successfully, and is not operational,

Not always. The HW could be in RPM_ACTIVE state while PM on it could
be disabled, if the returned error is -EACCESS.   And
pm_runtime_enabled() only catches a potential -EACCESS.


BTW, I just tested your patch and it worked for me as well. But as
suspected, it doesn't help the stack spew of CONFIG_PM_RUNTIME:=n

So I understand, I only need to resend the other three patches ?

Thanks.

On Tue, 2012-06-26 at 22:31 +0530, Jassi Brar wrote:

While I think your patch is simpler and achieve the same, I also think
your fears about this patch are unfounded.

Perhaps. But I do get a bad feeling from your patch, and I don't like
when that happens =). What I fear with changes like you made is that
they'll hide problems that should be fixed in other ways.

And I think that was the case here also. I think we should not call
dispc_runtime_get() during suspend. If pm_runtime_get returns -EACCES,
we do have a possible problem, and we should not silently ignore it.

A quick snack for thought...

quoted

But if pm_runtime_get_sync() returns an error, it means the HW has not
been resumed successfully, and is not operational,

Not always. The HW could be in RPM_ACTIVE state while PM on it could
be disabled, if the returned error is -EACCESS.   And
pm_runtime_enabled() only catches a potential -EACCESS.

True. But the HW could also be in disabled state. And that would lead to
a crash when accessing the registers.

It is not a fatal error if pm_runtime_get returns -EACCES, but we sure
shouldn't ignore it (or avoid it with pm_runtime_enabled()), but handle
it. In some rare cases it could be ok to get -EACCES, but that's a
special case, not standard.

BTW, I just tested your patch and it worked for me as well. But as
suspected, it doesn't help the stack spew of CONFIG_PM_RUNTIME:=n

So I understand, I only need to resend the other three patches ?

Yes, please.

 Tomi

On 27 June 2012 00:14, Tomi Valkeinen [off-list ref] wrote:

On Tue, 2012-06-26 at 22:31 +0530, Jassi Brar wrote:

quoted

quoted

But if pm_runtime_get_sync() returns an error, it means the HW has not
been resumed successfully, and is not operational,

Not always. The HW could be in RPM_ACTIVE state while PM on it could
be disabled, if the returned error is -EACCESS.   And
pm_runtime_enabled() only catches a potential -EACCESS.

True. But the HW could also be in disabled state. And that would lead to
a crash when accessing the registers.

It is not a fatal error if pm_runtime_get returns -EACCES, but we sure
shouldn't ignore it (or avoid it with pm_runtime_enabled()), but handle
it. In some rare cases it could be ok to get -EACCES, but that's a
special case, not standard.

You are mixing up generic concepts with what we have in omapdss.
Believe me, I do understand it's bad to proceed without caring for
returned _errors_.
The way omapdss is organized -EACCESS is _not_ an  error, it just
denotes PM is disabled on the device and that DISPC is in RPM_ACTIVE
is backed by the fact that HDMI always hold a reference between
resume-suspend and DISPC goes to suspend last and resume first.

quoted

BTW, I just tested your patch and it worked for me as well. But as
suspected, it doesn't help the stack spew of CONFIG_PM_RUNTIME:=n

So I understand, I only need to resend the other three patches ?

Yes, please.

OK, will do today later.

Regards.

On Wed, 2012-06-27 at 10:12 +0530, Jassi Brar wrote:

quoted

True. But the HW could also be in disabled state. And that would lead to
a crash when accessing the registers.

It is not a fatal error if pm_runtime_get returns -EACCES, but we sure
shouldn't ignore it (or avoid it with pm_runtime_enabled()), but handle
it. In some rare cases it could be ok to get -EACCES, but that's a
special case, not standard.

You are mixing up generic concepts with what we have in omapdss.
Believe me, I do understand it's bad to proceed without caring for
returned _errors_.
The way omapdss is organized -EACCESS is _not_ an  error, it just
denotes PM is disabled on the device and that DISPC is in RPM_ACTIVE
is backed by the fact that HDMI always hold a reference between
resume-suspend and DISPC goes to suspend last and resume first.

I'm not arguing that your solution would not work with the omapdss code
we have now, and presuming the underlying frameworks work fine. But I
want omapdss to have code that works also in the future, when other
parts of omapdss change.

It doesn't matter how omapdss is organized, -EACCES _is_ an error. It
tells us that something unexpected happened, and we should react to it
somehow.

When we call, for example, dispc_runtime_get(), we normally expect that
runtime PM is enabled, and it 1) "gets" it, increasing the use count,
and 2) makes sure the HW is enabled so it can be used. Your patch breaks
both of these if runtime PM is disabled.

Sure, in the current omapdss neither is a breaking problem, because 1)
the matching dispc_runtime_put() is called also with runtime PM
disabled, and thus we don't decrease the use count, and 2) the HW
happens to be already enabled. But that's just by "luck", and tomorrow
omapdss could be different. 

If, for some reason, we need to call dispc_runtime_get/put during
suspend, the caller should either 1) realize that we're suspending,
runtime PM is disabled, but the HW is anyway enabled, and thus skip
calling both get and put, or 2) call both get and put, but handle the
-EACCES, understanding what it means and knowing the HW is anyway
enabled.

 Tomi

On 27 June 2012 11:28, Tomi Valkeinen [off-list ref] wrote:

It doesn't matter how omapdss is organized, -EACCES _is_ an error. It
tells us that something unexpected happened, and we should react to it
somehow.

  $ git show 5025ce070
Exactly how omapdss is organised is the reason -EBUSY isn't an error there :)
Otherwise, omapdss should panic that somehow 'imbalance' has been
introduced in rpm.

Sure, in the current omapdss neither is a breaking problem, because 1)
the matching dispc_runtime_put() is called also with runtime PM
disabled, and thus we don't decrease the use count, and 2) the HW
happens to be already enabled. But that's just by "luck", and tomorrow
omapdss could be different.

It's no 'luck', but it's because today omapdss takes proper care of PM
enable/disable and get/put.
Rather, if tomorrow that stops working, it would hint that we managed
to screw up the balance.
Because if omapdss suspended and disabled PM on DISPC, and still HDMI
attempted to access dss regs, that clearly means HDMI hasn't been duly
made aware of the DISPC status.

Just as preemption and suspend/resume don't introduce any race in
locking, RPM won't introduce new imbalance in get/put of omapdss.

I am afraid, we won't reach any eureka moment on this, so I would
leave us to our conditions. This patch and discussion made me look
deep into rpm, I thank you for that and for your patience.

Cheers!

On Wed, 2012-06-27 at 13:11 +0530, Jassi Brar wrote:

On 27 June 2012 11:28, Tomi Valkeinen [off-list ref] wrote:

quoted

It doesn't matter how omapdss is organized, -EACCES _is_ an error. It
tells us that something unexpected happened, and we should react to it
somehow.

  $ git show 5025ce070
Exactly how omapdss is organised is the reason -EBUSY isn't an error there :)
Otherwise, omapdss should panic that somehow 'imbalance' has been
introduced in rpm.

There's no imbalance there, as each get() is still matched by a put(),
and the use count is correct. Your patch may cause either get or put to
be skipped.

In 5025ce070 the function in question is dss_runtime_put(), and -EBUSY
_is_ an error there. Normally if you call pm_runtime_put_sync() and the
use count drops to zero, the device should be suspended. In this case,
however, it won't be suspended as a child device is still enabled. Thus,
the framework informs about this with -EBUSY.

It's ok to ignore -EBUSY, because we're not really interested about if
the device is actually suspended or not.

However, dispc_runtime_get() is a different matter, because there we
_are_ interested about the state of the HW. If we skip
dispc_runtime_get() because runtime PM is disabled, we don't know
whether the HW is enabled or not.

And even if your patch was modified to check the HW status after
pm_runtime_enabled(), and return 0 is HW is enabled and an error if HW
is disabled, it'd be wrong, because you skip the pm_runtime_get() call.
This means that the use count is not increased, and there's no guarantee
that the HW would be functional after dispc_runtime_get() returns.

quoted

Sure, in the current omapdss neither is a breaking problem, because 1)
the matching dispc_runtime_put() is called also with runtime PM
disabled, and thus we don't decrease the use count, and 2) the HW
happens to be already enabled. But that's just by "luck", and tomorrow
omapdss could be different.

It's no 'luck', but it's because today omapdss takes proper care of PM
enable/disable and get/put.
Rather, if tomorrow that stops working, it would hint that we managed
to screw up the balance.
Because if omapdss suspended and disabled PM on DISPC, and still HDMI
attempted to access dss regs, that clearly means HDMI hasn't been duly
made aware of the DISPC status.

There are two different things here. First one is how
dispc_runtime_get/put & co. should work. The second is how they are
used.

As I see, you are arguing that it's ok to have dispc_runtime_get/put
broken, as long as they are used in a way that causes no problems.

Just as preemption and suspend/resume don't introduce any race in
locking, RPM won't introduce new imbalance in get/put of omapdss.

I am afraid, we won't reach any eureka moment on this, so I would
leave us to our conditions. This patch and discussion made me look
deep into rpm, I thank you for that and for your patience.

Yes, same here. I think this discussion and related code digging has
really improved my understanding of runtime PM =). Perhaps I'll get it
correct this time with this new information.

There's still the system suspend, which I think is quite broken. The
patch I gave fixes it for the time being, but I see it as a temporary
solution.

I don't like it at all that omapdss disables and enables the panels in
omapdss's suspend/resume hooks. But I'm not sure how this should work...
Should panel drivers each have their own suspend/resume hooks, and
handle it themselves? Or should the call to suspend/resume come from
upper layers, like omapfb or omapdrm.

I made a prototype patch a few weeks ago to move the suspend to omapfb,
and it feels better than the current one, but I'm still not sure...

 Tomi

On 27 June 2012 13:43, Tomi Valkeinen [off-list ref] wrote:

I don't like it at all that omapdss disables and enables the panels in
omapdss's suspend/resume hooks. But I'm not sure how this should work...
Should panel drivers each have their own suspend/resume hooks, and
handle it themselves? Or should the call to suspend/resume come from
upper layers, like omapfb or omapdrm.

I made a prototype patch a few weeks ago to move the suspend to omapfb,
and it feels better than the current one, but I'm still not sure...

IIUC, I have similar opinion.
Each panel having its own suspend/resume sounds like inter-dependency trouble.
I too would prefer suspend/resume propagating from omap-fb/drm, which
imho fits better with the notion of a linux device(omapdss is only
backend). Though I don't have strong feelings about how core then take
various panels up/down optimally.

BR.

On Wed, 2012-06-27 at 20:23 +0530, Jassi Brar wrote:

On 27 June 2012 13:43, Tomi Valkeinen [off-list ref] wrote:

quoted

I don't like it at all that omapdss disables and enables the panels in
omapdss's suspend/resume hooks. But I'm not sure how this should work...
Should panel drivers each have their own suspend/resume hooks, and
handle it themselves? Or should the call to suspend/resume come from
upper layers, like omapfb or omapdrm.

I made a prototype patch a few weeks ago to move the suspend to omapfb,
and it feels better than the current one, but I'm still not sure...

IIUC, I have similar opinion.
Each panel having its own suspend/resume sounds like inter-dependency trouble.

What do you mean with that? If we just consider omapdss and the panel
drivers, I see no dependency trouble. Panels are independent of each
other, and omapdss is supposed to handle any locking & refcounting
related to multiple panels already, as from omapdss's point of view
panel suspend is the same as panel disable.

And if we take omapfb/omapdrm into equation, well, in any case it
couldn't be any worse than the current one where suspend is handled by
omapdss.

I too would prefer suspend/resume propagating from omap-fb/drm, which
imho fits better with the notion of a linux device(omapdss is only
backend). Though I don't have strong feelings about how core then take
various panels up/down optimally.

One one hand, I see the combination of omapdss (or the "output" side of
omapdss) and a panel as a whole entity. I mean, if you just load omapdss
and a panel driver, but no omapfb/omapdrm, you already have a working
panel. You don't _need_ omapfb/omapdrm there. And in that sense it'd
make sense if the panels did handle their own suspend/resume.

But then, in real life it may be just simpler if omapfb/omapdrm handles
the suspend.

 Tomi

On 28 June 2012 12:11, Tomi Valkeinen [off-list ref] wrote:

On Wed, 2012-06-27 at 20:23 +0530, Jassi Brar wrote:

quoted

On 27 June 2012 13:43, Tomi Valkeinen [off-list ref] wrote:

quoted

I don't like it at all that omapdss disables and enables the panels in
omapdss's suspend/resume hooks. But I'm not sure how this should work...
Should panel drivers each have their own suspend/resume hooks, and
handle it themselves? Or should the call to suspend/resume come from
upper layers, like omapfb or omapdrm.

I made a prototype patch a few weeks ago to move the suspend to omapfb,
and it feels better than the current one, but I'm still not sure...

IIUC, I have similar opinion.
Each panel having its own suspend/resume sounds like inter-dependency trouble.

What do you mean with that? If we just consider omapdss and the panel
drivers, I see no dependency trouble. Panels are independent of each
other, and omapdss is supposed to handle any locking & refcounting
related to multiple panels already, as from omapdss's point of view
panel suspend is the same as panel disable.

And if we take omapfb/omapdrm into equation, well, in any case it
couldn't be any worse than the current one where suspend is handled by
omapdss.

I just anticipate it not trivial keeping  omap_dss_device.state in
sync with omap_dss_driver.suspend/resume
when the latter is made system suspend/resume and former still
affected by hdmi_panel_disable/enable.
Perhaps .disable and .suspend would need merging?

But as I said, it 'sounds' like.  It all may be straight forward - you
would know better.

quoted

I too would prefer suspend/resume propagating from omap-fb/drm, which
imho fits better with the notion of a linux device(omapdss is only
backend). Though I don't have strong feelings about how core then take
various panels up/down optimally.

One one hand, I see the combination of omapdss (or the "output" side of
omapdss) and a panel as a whole entity. I mean, if you just load omapdss
and a panel driver, but no omapfb/omapdrm, you already have a working
panel. You don't _need_ omapfb/omapdrm there. And in that sense it'd
make sense if the panels did handle their own suspend/resume.

Well, I would think if omapdss+panels (backend) serve none other
omapfb/drm, they should simply lie dormant hogging no resources if the
omapfb/drm driver isn't loaded (if that isn't already the case).
Also one usually has tighter control over a private subsystem by
having only one "point of intervention by system" as compared to two
or more.

But then, in real life it may be just simpler if omapfb/omapdrm handles
the suspend.

Yeah, being a simpler option is always there.

On Thu, 2012-06-28 at 13:16 +0530, Jassi Brar wrote:

On 28 June 2012 12:11, Tomi Valkeinen [off-list ref] wrote:

quoted

On Wed, 2012-06-27 at 20:23 +0530, Jassi Brar wrote:

quoted

On 27 June 2012 13:43, Tomi Valkeinen [off-list ref] wrote:

quoted

I don't like it at all that omapdss disables and enables the panels in
omapdss's suspend/resume hooks. But I'm not sure how this should work...
Should panel drivers each have their own suspend/resume hooks, and
handle it themselves? Or should the call to suspend/resume come from
upper layers, like omapfb or omapdrm.

I made a prototype patch a few weeks ago to move the suspend to omapfb,
and it feels better than the current one, but I'm still not sure...

IIUC, I have similar opinion.
Each panel having its own suspend/resume sounds like inter-dependency trouble.

What do you mean with that? If we just consider omapdss and the panel
drivers, I see no dependency trouble. Panels are independent of each
other, and omapdss is supposed to handle any locking & refcounting
related to multiple panels already, as from omapdss's point of view
panel suspend is the same as panel disable.

And if we take omapfb/omapdrm into equation, well, in any case it
couldn't be any worse than the current one where suspend is handled by
omapdss.

I just anticipate it not trivial keeping  omap_dss_device.state in
sync with omap_dss_driver.suspend/resume
when the latter is made system suspend/resume and former still
affected by hdmi_panel_disable/enable.

This state variable is something I'd like to get rid of. While I think
it could work fine if we added some locking to omap_dss_device, I'd
actually like to get rid of omap_dss_device also. (totally another
issue, let's not go there).

But anyway, the state variable should only be written by the panel
driver, so I don't see a big problem there. The panel's
disable/enable/suspend/resume touch the state, but the panel driver
should protect that with a mutex.

Perhaps .disable and .suspend would need merging?

Yes, I already have a patch that removes suspend/resume from
omap_dss_device, as they are identical to disable/enable. I haven't
merged it yet, as there's some funny code in omapdrm that breaks after
the change, and I haven't had time to look at it.

But as I said, it 'sounds' like.  It all may be straight forward - you
would know better.

quoted

quoted

I too would prefer suspend/resume propagating from omap-fb/drm, which
imho fits better with the notion of a linux device(omapdss is only
backend). Though I don't have strong feelings about how core then take
various panels up/down optimally.

One one hand, I see the combination of omapdss (or the "output" side of
omapdss) and a panel as a whole entity. I mean, if you just load omapdss
and a panel driver, but no omapfb/omapdrm, you already have a working
panel. You don't _need_ omapfb/omapdrm there. And in that sense it'd
make sense if the panels did handle their own suspend/resume.

Well, I would think if omapdss+panels (backend) serve none other
omapfb/drm, they should simply lie dormant hogging no resources if the
omapfb/drm driver isn't loaded (if that isn't already the case).

That should be the case.

I need to look more carefully what fb/drm already do. If they already
have good suspend mechanisms and expect to be the ones handling system
suspend, then I guess the choice is quite clear.

 Tomi