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.