On Mon, 2012-06-25 at 19:01 +0530, Jassi Brar wrote: > On 25 June 2012 18:11, Tomi Valkeinen <tomi.valkeinen@xxxxxx> wrote: > > On Mon, 2012-06-25 at 17:57 +0530, Jassi Brar wrote: > >> On 25 June 2012 15:00, Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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:
signature.asc
Description: This is a digitally signed message part
