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

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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