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

[Alex Deucher]

On Fri, Sep 28, 2012 at 8:19 AM, Rob Clark <rob.clark at linaro.org> wrote:
> 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.

Thinking about modern hw, I'd probably only expose connectors and
monitors (or maybe call them displays).  You could hang backlight
controls and brightness, etc on the monitor objects.  Also you could
have a multiple monitors connected to a single connector (DP 1.2 or
virtual adapters (wireless or remote display for example)).
Everything else (crtcs, plls, encoders, etc.) would be driver
controlled and validated.

Alex