On Fri, Sep 28, 2012 at 1:55 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Fri, Sep 28, 2012 at 12:54 PM, Rob Clark <rob.clark@xxxxxxxxxx> 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. hmm.. well I have been thinking that some of what is currently in the connectors needs to move into the encoders.. this would help, although will take some time. >>> 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. > - an alternative would be a new ->global_adjust_modes after the > ->adjust_mode stage that gets all the new adjusted_modes and could > check global resource constrains. > > Imo anything more complicated than that has dubious value in a common > framework. I also dunno yet how (or if at all) we should bake in > handling of planes restrictions ... > > For panel framework integration I don't think we need much, this is > likely just a driver internal thing. well.. I guess it where the panel driver fits in KMS. I think it would be common that we need to send a command to the panel to turn off / disable backlight / etc. Versus an hdmi/dp/etc monitor which would just do this automatically when it stops receiving a signal. So it doesn't seem like a bad idea to not assume that your connector is completely passive. But I could buy the argument that this should be part of crtc helpers v2. BR, -R > Cheers, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
