Re: [PATCH] drm: call connector->dpms(OFF) when disabling

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 28, 2012 at 01:55:19PM +0200, Daniel Vetter wrote:
> On Fri, Sep 28, 2012 at 12:54 PM, Rob Clark <rob.clark@linaro.org> wrote:
> > Maybe it just makes sense to always do connector->dpms(OFF) before
> > unhooking the chain, rather than directly calling dpms on the
> > encoder/crtc?
> 
> Well, that makes the entire (optional) ->disable stuff a bit more
> awkward. The thing imo really is that the crtc helper assume that the
> connectors do not hold any hw state, whereas you're omap driver
> violates that assumption.
> 
> For the intel driver we've fixed this by doing any sink handling (e.g.
> for dp) in the encoder->dpms functions. So I think the right way for
> omap is to do the same (or conclude that the crtc helpers are a bad
> fit and ditch them). Having connectors that are special, but only in
> some of the cases imo makes squat sense for a generic helper library.
> 
> >> Which is imo a clear sign that the crtc helper is a misfit for your hw and
> >> you should stop using it ;-) And adding special-case handling like this
> >> into common code, especially if it weakens the semantic concepts in the
> >> helper layer is a recipe for a maintainance disaster a few years down the
> >> road. Hence
> >
> > well, I think by the time we start adding atomic-modeset and
> > common/dsi panel framework, there might be need for a new set of
> > helpers.. but I'm not sure that the hw is quite strange enough to need
> > an omap specific set of helpers.  Maybe I start w/ something in
> > omapdrm but then refactor into common functions once a few drivers are
> > converted to atomic modeset and panel framework.
> 
> I see a few ways forward with the crtc helpers and atomic modeset:
> - bake the assumption into the code that drivers using the crtc
> helpers don't have any shared global resources and drop all these
> checks. This would boil down to writing a new set_config to handle
> global modeset changes (instead of changes to just one crtc).
> Obviously the current possible_encoders/possible_crtcs would still be
> checked.
> - like above, but add a driver-global ->check callback before starting
> the modeset sequence, but with the new configuration already applied
> to the kms structures.

That's essentially what my intel_atomic.c code does. Somewhat ironically,
since your modeset rework, that code is now more suited for other drivers
and I need to go and rewrite it for i915.

-- 
Ville Syrjälä
Intel OTC