[PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[jaswinder.singh]

From: Jassi Brar <jaswinder.singh@linaro.org>

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 <jaswinder.singh@linaro.org>
---

 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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
> 
> 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 <jaswinder.singh@linaro.org>
> ---
> 
>  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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 25 June 2012 11:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
....
>>  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.

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 937 bytes --]

On Mon, 2012-06-25 at 14:19 +0530, Jassi Brar wrote:
> On 25 June 2012 11:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> ....
> >>  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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 25 June 2012 15:00, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-25 at 14:19 +0530, Jassi Brar wrote:
>> On 25 June 2012 11:50, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>> ....
>> >>  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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2444 bytes --]

On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote:
> On 25 June 2012 15:00, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

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

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 25 June 2012 18:11, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote:
>> On 25 June 2012 15:00, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
>> > 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.


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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3936 bytes --]

On Mon, 2012-06-25 at 19:01 +0530, Jassi Brar wrote:
> On 25 June 2012 18:11, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote:
> >> On 25 June 2012 15:00, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >
> >> > 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.

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.

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 25 June 2012 19:19, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-25 at 19:01 +0530, Jassi Brar wrote:

>> > /* 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.


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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2510 bytes --]

On Mon, 2012-06-25 at 22:36 +0530, Jassi Brar wrote:
> On 25 June 2012 19:19, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-06-25 at 19:01 +0530, Jassi Brar wrote:
> 
> >> > /* 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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 26 June 2012 12:49, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Mon, 2012-06-25 at 22:36 +0530, Jassi Brar wrote:

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

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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Andy Green]

On 06/26/12 16:32, the mail apparently from Jassi Brar included:
> On 26 June 2012 12:49, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Mon, 2012-06-25 at 22:36 +0530, Jassi Brar wrote:
>
>>>>
>>>> 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.
>
>>> 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.

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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3964 bytes --]

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
184)
[ 2267.362457] [<c0169b6c>] (sysfs_write_file+0x100/0x184) from [<c0109008>] (vfs_write+0xb4/0x148)
[ 2267.371795] [<c0109008>] (vfs_write+0xb4/0x148) from [<c0109290>] (sys_write+0x40/0x70)
[ 2267.380310] [<c0109290>] (sys_write+0x40/0x70) from [<c0013d60>] (ret_fast_syscall+0x0/0x3c)
[ 2267.389282] ---[ end trace 54fe7eea726ac84d ]---
[ 2267.394592] dpm_run_callback(): platform_pm_resume+0x0/0x50 returns -13
[ 2267.401641] PM: Device omapdss failed to resume: error -13

I don't remember seeing that with earlier kernel versions, but I'm not
100% sure.

Looking at the log, I can see that both DSS and DISPC runtime_resume
callbacks are called early, and successfully. Later the system resume
callback tries to enable the displays that were enabled when the system
went to suspend, which fails because dispc_runtime_get() returns -EACCES
(and pm_runtime_enabled() returns false).

Interestingly, during suspend dispc_runtime_put() is called, and at that
point pm_runtime_enabled() also returns false, but pm_runtime_put_sync()
still returns 0 instead of -EACCES.

I'll need to study this more, but I don't think this is a problem
related to omapdss's handling of runtime PM, but rather handling system
suspend. omapdss's handling of system suspend is in a rather bad state.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 26 June 2012 14:37, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 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
> 184)
> [ 2267.362457] [<c0169b6c>] (sysfs_write_file+0x100/0x184) from [<c0109008>] (vfs_write+0xb4/0x148)
> [ 2267.371795] [<c0109008>] (vfs_write+0xb4/0x148) from [<c0109290>] (sys_write+0x40/0x70)
> [ 2267.380310] [<c0109290>] (sys_write+0x40/0x70) from [<c0013d60>] (ret_fast_syscall+0x0/0x3c)
> [ 2267.389282] ---[ end trace 54fe7eea726ac84d ]---
> [ 2267.394592] dpm_run_callback(): platform_pm_resume+0x0/0x50 returns -13
> [ 2267.401641] PM: Device omapdss failed to resume: error -13
>
Seems similar, but I only tested OMAP4 HDMI.
thanks.

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2902 bytes --]

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,


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 26 June 2012 17:33, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> 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.
>
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

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



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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:
> On 26 June 2012 17:33, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > 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.
> >
> 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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 26 June 2012 20:38, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:
>> On 26 June 2012 17:33, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > 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.
>> >
>> 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.

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On Tue, 2012-06-26 at 20:39 +0530, Jassi Brar wrote:
> On 26 June 2012 20:38, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:
> >> On 26 June 2012 17:33, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> > 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.
> >> >
> >> 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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 26 June 2012 20:41, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-06-26 at 20:39 +0530, Jassi Brar wrote:
>> On 26 June 2012 20:38, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> > On Tue, 2012-06-26 at 20:19 +0530, Jassi Brar wrote:
>> >> On 26 June 2012 17:33, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> >> > 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.
>> >> >
>> >> 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.

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1554 bytes --]

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 27 June 2012 00:14, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2012-06-26 at 22:31 +0530, Jassi Brar wrote:
>>
>> > 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.


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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

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

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 27 June 2012 11:28, Tomi Valkeinen <tomi.valkeinen@ti.com> 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!

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3862 bytes --]

On Wed, 2012-06-27 at 13:11 +0530, Jassi Brar wrote:
> On 27 June 2012 11:28, Tomi Valkeinen <tomi.valkeinen@ti.com> 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.

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.

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 27 June 2012 13:43, Tomi Valkeinen <tomi.valkeinen@ti.com> 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.

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

On Wed, 2012-06-27 at 20:23 +0530, Jassi Brar wrote:
> On 27 June 2012 13:43, Tomi Valkeinen <tomi.valkeinen@ti.com> 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.

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 28 June 2012 12:11, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Wed, 2012-06-27 at 20:23 +0530, Jassi Brar wrote:
>> On 27 June 2012 13:43, Tomi Valkeinen <tomi.valkeinen@ti.com> 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.
>
> 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.


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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

On Thu, 2012-06-28 at 13:16 +0530, Jassi Brar wrote:
> On 28 June 2012 12:11, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2012-06-27 at 20:23 +0530, Jassi Brar wrote:
> >> On 27 June 2012 13:43, Tomi Valkeinen <tomi.valkeinen@ti.com> 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.
> >
> > 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.
> 
> 
> >> 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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Grazvydas Ignotas]

On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>>
>>  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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1126 bytes --]

On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:
> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> >>
> >>  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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Rajendra Nayak]

On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:
> On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:
>> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen<tomi.valkeinen@ti.com>  wrote:
>>> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>>>>
>>>>   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
>

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2379 bytes --]

On Mon, 2012-06-25 at 18:12 +0530, Rajendra Nayak wrote:
> On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:
> > On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:
> >> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen<tomi.valkeinen@ti.com>  wrote:
> >>> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
> >>>>
> >>>>   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();
> }

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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Rajendra Nayak]

On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:
> On Mon, 2012-06-25 at 18:12 +0530, Rajendra Nayak wrote:
>> On Monday 25 June 2012 06:00 PM, Tomi Valkeinen wrote:
>>> On Mon, 2012-06-25 at 15:05 +0300, Grazvydas Ignotas wrote:
>>>> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen<tomi.valkeinen@ti.com>   wrote:
>>>>> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>>>>>>
>>>>>>    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();
>> }
>
> 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?

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
>

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Grazvydas Ignotas]

CCing some PM people, maybe they can comment?

On Tue, Jun 26, 2012 at 7:51 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:
>>
>> 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.
>
>> 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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Alan Stern]

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 <rnayak@ti.com> wrote:
> > On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:
> >>
> >> 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
> > 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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1951 bytes --]

On Tue, 2012-06-26 at 10:34 -0400, Alan Stern wrote:
> 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 <rnayak@ti.com> wrote:
> > > On Monday 25 June 2012 06:20 PM, Tomi Valkeinen wrote:
> > >>
> > >> 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


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Alan Stern]

On Tue, 26 Jun 2012, Tomi Valkeinen wrote:

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

Re: [PATCH] OMAPDSS: Check if RPM enabled before trying to change state

[Jassi Brar]

On 25 June 2012 17:35, Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Mon, Jun 25, 2012 at 9:20 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Sat, 2012-06-23 at 13:36 +0530, jaswinder.singh@linaro.org wrote:
>>>
>>>  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