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

[Rob Clark]

On Fri, Sep 28, 2012 at 1:55 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Fri, Sep 28, 2012 at 12:54 PM, Rob Clark <rob.clark at 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.

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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel