[Intel-gfx] [PATCH 6/6] drm/i915: implement HSW display sequences for package C8+

[Daniel Vetter]

On Thu, Jun 06, 2013 at 07:49:49AM +0100, Chris Wilson wrote:
> On Wed, Jun 05, 2013 at 02:21:56PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > 
> > This patch implements "Display Sequences for Package C8", from the
> > "Display Mode Set Sequence" section from the Haswell documentation.
> > 
> > Notice that even if we allow PC8+, there's no guarantee that we will
> > actually enter PC8+, since the other devices also need to allow it.
> > Also notice that we need i915.disable_power_well=1 in order to test
> > this feature: we don't allow PC8+ if the power well is still enabled.
> > 
> > v2: - Rebase
> >     - Implement many review comments form Daniel Vetter
> >     - Call intel_prepare_ddi and i915_gem_init_swizzling when
> >       returning from C8+ so we can actually see the screen contents
> 
> Really don't like the colour you've chosen in places, and there seem to
> be a lot of placeholder code.
> 
> Highlights:
>  - intel_aux_display_shutdown_forbid / allow ->
>    intel_aux_display_get / put
> 
>    Not convinced by "aux" there either. Perhaps,
>    intel_display_wakelock_get() instead. Or we could go with
>    intel_display_forcewake_get(). Hmm, forcewake is better as we already
>    have that concept in our code (and wakelock may end up being confused
>    with the system-wide wakelocks.)

aux_display is my bikeshed ;-) The idea was that we have a russian doll
stack of display power knobs:
- the power well to shut of everything but edp on pipe A
- the power well for all display pipes (not yet on haswell)
- power control for aux stuff like gmbus/dp aux
- the real pci device

In reality it's probably more a tree since rc6 forcewake also hangs in
there somewhere. Maybe we could also track a few more things explicitly,
e.g. the refclock plls could probably be refcounted by their users (e.g.
ddi plls).

In the end I guess most of these need to be refcounted, and child power
domains need to also grab a reference on their parents on the 0->1
transition. One idea I'm pondering is whether we should make all those
power domains explicit by mapping them to platform devices we create.
Upsides:
- Someone clearly thought about funy races and locking issues when writing
  the pm runtime stuff. Using it would avoid us reinventing that wheel.
- struct device is the main object for pm in linux, so if we can make our
  aux device (like i2c) childs of the correct power domain (what I've
  called "aux display") instead of the pci device we might gain a bit in
  integration.
- Full runtime PM at the pci level seems to be on the table anyway.

>  - move your c8 state into a struct:
>    struct i915_package_c8 {
>            struct mutex mutex;
> 	   union {
> 		   struct hsw_package_c8_registers hsw_regfile;
> 	   };
> 	   bool enabled; /* was allowing_package_c8 */
> 	   int forcewake; /* was c8_forbid_refcnt */
>    };
> 
>  - The interrupt register handling is messy. We already track the values
>    of the ones that we change, the others should be static. Looks
>    mostly correct (barring a race with an interrupt!).

I've inked in today for starting my irq review (thanks to vecs), I think
we should do that first and then look again at the irq prep patches. But I
agree that in theory we should only need to clear out the unused
interrupts once in preinstall. For paranoia we could add a few WARNs in
the uninstall hooks.

>  - Hooking into modeset_global_resources seems strange and never
>    explained.

Modeset is a bit ugly for power domain refcounting (it's the same mess for
the power well as for pc8+ here). The issue I think is that we don't have
a resource acquire/release stage where platform code could hook in.
crtc_disable would be at the right spot, but is also called with the dpms
off stuff (so not suitable for everything). Off isn't called
unconditionally when shutting down a pipe for real (if we reuse the pipe
the cleanup is done in ->mode_set). And crtc_enable is too late for some
cases since global_modeset_resources and crtc mode_set already need to
touch the hw.

Maybe we could use the for_each_crtc loop we already have and grab a
refcount for each crtc. If we switch the power well code to refcounting
that would nicely unify things, too.

Another issue I'm not yet clear on is dpms handling. Atm we ignore all
that stuff for it (since neither modeset_global_resources nor mode_set
hooks are called, and the hw seems to completely forget register contents,
so this must be done). Probably the simplest solution is to call these
hooks in the dpms on path, too. But atm that plan runs afoul of the
resource accounting we still have in there (pll sharing and refcounting
mostly).

Tbh I don't have a clear overall idea for this yet.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch