drm/i915: Add support for asynchronous display power disabling
authorImre Deak <imre.deak@intel.com>
Mon, 13 May 2019 19:25:33 +0000 (22:25 +0300)
committerImre Deak <imre.deak@intel.com>
Tue, 14 May 2019 11:06:10 +0000 (14:06 +0300)
commite0da2d63ab3ac746d61b8861085fd3abbef011c6
tree8eb6ec1c0ba853f80baa5576e293a124b54e6c90
parentee70080a52f19251a58212925dfaf6c9b0a07316
drm/i915: Add support for asynchronous display power disabling

By disabling a power domain asynchronously we can restrict holding a
reference on that power domain to the actual code sequence that
requires the power to be on for the HW access it's doing, by also
avoiding unneeded on-off-on togglings of the power domain (since the
disabling happens with a delay).

One benefit is potential power saving due to the following two reasons:
1. The fact that we will now be holding the reference only for the
   necessary duration by the end of the patchset. While simply not
   delaying the disabling has the same benefit, it has the problem that
   frequent on-off-on power switching has its own power cost (see the 2.
   point below) and the debug trace for power well on/off events will
   cause a lot of dmesg spam (see details about this further below).
2. Avoiding the power cost of freuqent on-off-on power switching. This
   requires us to find the optimal disabling delay based on the measured
   power cost of on->off and off->on switching of each power well vs.
   the power of keeping the given power well on.

   In this patchset I'm not providing this optimal delay for two
   reasons:
   a) I don't have the means yet to perform the measurement (with high
      enough signal-to-noise ratio, or with the help of an energy
      counter that takes switching into account). I'm currently looking
      for a way to measure this.

   b) Before reducing the disabling delay we need an alternative way for
      debug tracing powerwell on/off events. Simply avoiding/throttling
      the debug messages is not a solution, see further below.

   Note that even in the case where we can't measure any considerable
   power cost of frequent on-off switching of powerwells, it still would
   make sense to do the disabling asynchronously (with 0 delay) to avoid
   blocking on the disabling. On VLV I measured this disabling time
   overhead to be 1ms on average with a worst case of 4ms.

In the case of the AUX power domains on ICL we would also need to keep
the sequence where we hold the power reference short, the way it would
be by the end of this patchset where we hold it only for the actual AUX
transfer. Anything else would make the locking we need for ICL TypeC
ports (whenever we hold a reference on any AUX power domain) rather
problematic, adding for instance unnecessary lockdep dependencies to
the required TypeC port lock.

I chose the disabling delay to be 100msec for now to avoid the unneeded
toggling (and so not to introduce dmesg spamming) in the DP MST sideband
signaling code. We could optimize this delay later, once we have the
means to measure the switching power cost (see above).

Note that simply removing/throttling the debug tracing for power well
on/off events is not a solution. We need to know the exact spots of
these events and cannot rely only on incorrect register accesses caught
(due to not holding a wakeref at the time of access). Incorrect
powerwell enabling/disabling could lead to other problems, for instance
we need to keep certain powerwells enabled for the duration of modesets
and AUX transfers.

v2:
- Clarify the commit log parts about power cost measurement and the
  problem of simply removing/throttling debug tracing. (Chris)
- Optimize out local wakeref vars at intel_runtime_pm_put_raw() and
  intel_display_power_put_async() call sites if
  CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n. (Chris)
- Rebased on v2 of the wakeref w/o power-on guarantee patch.
- Add missing docbook headers.
v3:
- Checkpatch spelling/missing-empty-line fix.
v4:
- Fix unintended local wakeref var optimization when using
  call-arguments with side-effects, by using inline funcs instead of
  macros. In this patch in particular this will fix the
  intel_display_power_grab_async_put_ref()->intel_runtime_pm_put_raw()
  call).

  No size change in practice (would be the same disregarding the
  corresponding change in intel_display_power_grab_async_put_ref()):
  $ size i915-macro.ko
     text    data     bss     dec     hex filename
  2455190  105890   10272 2571352  273c58 i915-macro.ko
  $ size i915-inline.ko
     text    data     bss     dec     hex filename
  2455195  105890   10272 2571357  273c5d i915-inline.ko

  Kudos to Stan for reporting the raw-wakeref WARNs this issue caused. His
  config has CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n, which I didn't retest
  after v1, and we are also not testing this config in CI.

  Now tested both with CONFIG_DRM_I915_DEBUG_RUNTIME_PM=y/n on ICL,
  connecting both Chamelium and regular DP, HDMI sinks.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20190513192533.12586-1-imre.deak@intel.com
drivers/gpu/drm/i915/i915_drv.h
drivers/gpu/drm/i915/intel_runtime_pm.c
drivers/gpu/drm/i915/intel_runtime_pm.h