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

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

[-- Attachment #1: Type: text/plain, Size: 3638 bytes --]

On Thu, 2012-06-28 at 13:16 +0530, Jassi Brar wrote:
> On 28 June 2012 12:11, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2012-06-27 at 20:23 +0530, Jassi Brar wrote:
> >> On 27 June 2012 13:43, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> >
> >> > 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...
> >> >
> >> IIUC, I have similar opinion.
> >> Each panel having its own suspend/resume sounds like inter-dependency trouble.
> >
> > What do you mean with that? If we just consider omapdss and the panel
> > drivers, I see no dependency trouble. Panels are independent of each
> > other, and omapdss is supposed to handle any locking & refcounting
> > related to multiple panels already, as from omapdss's point of view
> > panel suspend is the same as panel disable.
> >
> > And if we take omapfb/omapdrm into equation, well, in any case it
> > couldn't be any worse than the current one where suspend is handled by
> > omapdss.
> >
> I just anticipate it not trivial keeping  omap_dss_device.state in
> sync with omap_dss_driver.suspend/resume
> when the latter is made system suspend/resume and former still
> affected by hdmi_panel_disable/enable.

This state variable is something I'd like to get rid of. While I think
it could work fine if we added some locking to omap_dss_device, I'd
actually like to get rid of omap_dss_device also. (totally another
issue, let's not go there).

But anyway, the state variable should only be written by the panel
driver, so I don't see a big problem there. The panel's
disable/enable/suspend/resume touch the state, but the panel driver
should protect that with a mutex.

> Perhaps .disable and .suspend would need merging?

Yes, I already have a patch that removes suspend/resume from
omap_dss_device, as they are identical to disable/enable. I haven't
merged it yet, as there's some funny code in omapdrm that breaks after
the change, and I haven't had time to look at it.

> But as I said, it 'sounds' like.  It all may be straight forward - you
> would know better.
> 
> 
> >> I too would prefer suspend/resume propagating from omap-fb/drm, which
> >> imho fits better with the notion of a linux device(omapdss is only
> >> backend). Though I don't have strong feelings about how core then take
> >> various panels up/down optimally.
> >
> > One one hand, I see the combination of omapdss (or the "output" side of
> > omapdss) and a panel as a whole entity. I mean, if you just load omapdss
> > and a panel driver, but no omapfb/omapdrm, you already have a working
> > panel. You don't _need_ omapfb/omapdrm there. And in that sense it'd
> > make sense if the panels did handle their own suspend/resume.
> >
> Well, I would think if omapdss+panels (backend) serve none other
> omapfb/drm, they should simply lie dormant hogging no resources if the
> omapfb/drm driver isn't loaded (if that isn't already the case).

That should be the case.

I need to look more carefully what fb/drm already do. If they already
have good suspend mechanisms and expect to be the ones handling system
suspend, then I guess the choice is quite clear.

 Tomi


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]