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

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