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

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

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