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