[Intel-gfx] [RFC] dpms handling on atomic drivers

[Daniel Vetter]

>> 2. Cloned configurations
>>
>> Currently dpms is set per-connector, and the crtc helpers only shut down
>> the specific encoder. Only when all connectors are off will it shut down
>> the crtc, too. That pretty much defeats the point of the new helpers of
>> always enabling/disabling a given output pipeline holesale and in the same
>> order. i915 modeset tries to have the cake and eat it with some clever
>> bit-frobbing on the encoder (to select black output when needed) but
>> otherwise only enable/disable the entire pipe. Imo that was stupid and not
>> worth the complexity at all, furthermore it needs changes in all drivers
>> to implement. Especially since X doesn't care about per-connector dpms.
>> And even for multi-seat dpms will switch only per-crtc.
>>
>> Proposal: Only shut down anything (and then the hole output pipe with all
>> cloned outputs) when all connector's dpms property is set to off. And
>> enable it again as soon as one property goes to on.
>
> Well, it isn't quite the behavior I would expect..  at least not if
> atomic ioctl has a "preserve properties not explicitly set" mode.  I
> would expect one of the displays to turn off.  Or at least go black
> (but preferably off..)
>
> we could move the property to the encoder with only two states
> (on/off) and somehow emulate old dpms w/ some helpers to do a full
> modeset if/when needed?  Not really sure if that helps anything..

I guess I need to extend a bit here. My idea is to track the dpms
state (all 4 states) in the connector state. But that is only to keep
up the illusion for the legacy dpms interface. The real state would be
the active boolean in the crtc. And I think that would also be the one
we'd expose in the atomic ioctl.

And yeah that would all be in an atomic helper to implement legacy
dpms on top of atomic, so drivers could overwrite this if needed.

>> 3. No internal representation of dpms state
>>
>> Because of the above nonsense it's essentially not possible to find out in
>> a generic way whether the crtc is actually on. Which means that no generic
>> (core or helper code) can figure out whether e.g. vblanks still happen.
>> In the atomic helpers I just do a drm_vblank_get and look at the return
>> value to figure out whether the crtc is on or not. This is one giant mess.
>>
>> Proposal. Add a new boolean ->active to the crtc state, which will track
>> the dpms state of the crtc. Add a helper to the atomic core functions
>> which will compute ->active from the state update according to the
>> proposals for issue 1&2. Require that all callers of
>> ->atomich_check/commit update ->active properly and require
>> implementations to obey it. That means the atomic helpers will switch from
>> looking at ->enable to looking at ->active to decided whether a crtc an
>> all its outputs should be enabled or not.
>
> hmm.. then I'm starting to get a bit fuzzy on the distinction between
> enabled and active..  I guess I should go back and look more closely
> at the differentiation between 'enabled' and 'active' in current i915
> helpers..
>
> Seems like we should be able to just repurpose enabled in the new
> helpers, to not automatically be true when a mode is set, but instead
> be true iff:
>   1) >=1 attached connector is ON
>   2) valid mode is set
>   3) >=1 attached plane has an fb
>
> seems like the existing problem w/ enabled is just to do with how
> current helpers use it.. but maybe I'm overlooking something..

Well 3) is imo wrong, since black screen with no fbs is imo a valid
state (hopefully only transitional). And the core/helper will ensure
that 1&2 never get out of sync (you won't get an -EINVAL though since
existing userspace does stupid stuff that violates this, the core
setCrtc ioctl cleans that up though).

That aside I guess I need to elaborate on what makes dpms special in
i915, and why there's a real difference between crtc->enable == true
&& ->active == false and crtc->enable == false in i915. For complex
configs we do resource checking (shared dplls) and that's done in the
modeset. For a pipe which has been disabled just with dpms we then
guarantee that we'll keep these resources reserve and so will always
be able to enable the pipe again. If you disable the pipe completely
(i.e. set crtc->enable to false) we'll release these resources. E.g.
in the i915 share dpll code we have both an active refcount (does the
ppl need to be running) and a reference mask (which crtc is
referencing this pll, even when the crtc is disabled with dpms).

Now atomic has less of an issue with that since a given config should
always keep working. Well, until you have multi-seat and the other
compositor might have stolen some shared resources meanwhile. So I
still think a separate knob to set the hw state and make the hw state
independent from reserving required resources is rather useful.

Note that even with the old crtc helper code there's a difference
between dpms and set_config(NULL): crtc->mode_set can fail and is only
called from set-config, not from the dpms code. Before the rewrite
i915 used that to handle shared dplls and failing the modeset if we
couldn't find a free dpll. I don't know whether there's any other
driver though that uses this facility.

I hope that explains why I want to keep separate enable and active
crtc states and don't just want to completely remove dpms from
drivers. Note that for the actual driver backend there will be no
difference at all, since my plan is that the atomic helpers would
implement exactly your idea. So drivers using the crtc+plane helper
vfuncs wouldn't care at all about all this. The overall point is to
enforce consistency, and that's best done with no driver specific code
at all ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch