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

[Jassi Brar <jaswinder.singh@linaro.org>]

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