[PATCH 11/23] drm: omapdrm: Check DSS manager state in the enable/disable helpers

[Laurent Pinchart]

Hi Daniel,

On Wednesday 11 May 2016 09:40:14 Daniel Vetter wrote:
> On Tue, May 10, 2016 at 04:28:22PM +0300, Tomi Valkeinen wrote:
> > On 26/04/16 23:35, Laurent Pinchart wrote:
> > > The omapdrm DSS manager enable/disable operations check the DSS manager
> > > state to avoid double enabling/disabling. Move that code to the DSS
> > > manager to decrease the dependency of the DRM layer to the DSS layer.
> > 
> > Shouldn't omapdrm know if the CRTC is enabled or not, and avoid
> > double-enable/disable by just looking at its internal state?
> > 
> > If so, we could remove dispc_mgr_is_enabled() call as you do, and add a
> > WARN_ON() to omapdss if the mgr is already enabled/disabled to catch
> > bugs in omapdrm.
> 
> Yeah, atomic helpers guarantees you that it'll never screw this up and
> disable/enable twice.

It's more complex than that. The omapdrm driver has a complicated call stack, 
going through omapdss subdrivers that make verification difficult at the 
moment. The CRTCs are not enabled/disabled directly in response to the DRM 
core's calls to the CRTC enable and disable operations, but through the 
omapdss driver from the encoder helper enable and disable operations. That's 
something I'd like to clean up, but I'd rather not mess with it right now.

> The only tricky bit to consider is boot-up, where the firmware might have
> left something enabled, but by default the reset helpers assume everything
> is off, and just reset sw state to all off.
> 
> Either patch up that state to match (you only need to set very few things,
> no need to recover all the details about the mode), or manually shut
> things down. But you need to make sure that at driver load sw matches hw
> state.
> -Daniel

-- 
Regards,

Laurent Pinchart