On Wed, 2012-06-27 at 13:11 +0530, Jassi Brar wrote: > On 27 June 2012 11:28, Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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:
signature.asc
Description: This is a digitally signed message part
