[Intel-gfx] [RFC PATCH] drm/i915/skl: Add DC6 disabling as a power well
Patrik Jakobsson
patrik.jakobsson at linux.intel.com
Fri Sep 25 01:56:31 PDT 2015
On Thu, Sep 24, 2015 at 06:20:14PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 24, 2015 at 02:50:16PM +0200, Patrik Jakobsson wrote:
> > On Wed, Sep 23, 2015 at 01:18:00PM +0200, Patrik Jakobsson wrote:
> > > On Wed, Sep 23, 2015 at 10:43:00AM +0200, Daniel Vetter wrote:
> > > > On Mon, Sep 21, 2015 at 10:00:45AM +0200, Patrik Jakobsson wrote:
> > > > > On Wed, Sep 16, 2015 at 11:10:07PM +0300, Ville Syrjälä wrote:
> > > > > > On Fri, Sep 11, 2015 at 01:55:22PM +0200, Patrik Jakobsson wrote:
> > > > > > > We need to be able to control if DC6 is allowed or not. Much like
> > > > > > > requesting power to a specific piece of the hardware we need to be able
> > > > > > > to request that we don't enter DC6 during certain hw access.
> > > > > > >
> > > > > > > To solve this without introducing too much infrastructure I'm hooking
> > > > > > > into the power well / power domain framework. DC6 prevention is modeled
> > > > > > > much like an enabled power well. Thus I'm using the terminology on/off
> > > > > > > for DC states instead of enable/disable.
> > > > > > >
> > > > > > > The problem that started this work is the need for DC6 to be disabled
> > > > > > > when accessing DP_AUX_A during CRTC on/off. That is also fixed in this
> > > > > > > patch.
> > > > > > >
> > > > > > > This is posted as an RFC since DMC and DC state handling is being
> > > > > > > reworked and will possibly affect the outcome of this patch. The patch
> > > > > > > has known warnings.
> > > > > > >
> > > > > > > Signed-off-by: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/i915/intel_ddi.c | 9 +++++
> > > > > > > drivers/gpu/drm/i915/intel_drv.h | 2 +
> > > > > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 69 +++++++++++++++++++++++++--------
> > > > > > > 3 files changed, 64 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > index 4823184..c2c1ad2 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > > > @@ -2288,6 +2288,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
> > > > > > > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
> > > > > > > struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > > > >
> > > > > > > + intel_display_power_get(dev_priv, POWER_DOMAIN_AUX_A);
> > > > > > > +
> > > > > >
> > > > > > These I think shouldn't be necessary with my
> > > > > > intel_display_port_aux_power_domain() stuff since intel_dp_aux_ch() will
> > > > > > itself grab the appropriate power domain.
> > > > > >
> > > > > > That's of course assuming that AUX is the only reason why we need to
> > > > > > keep DC6 disabled here.
> > > > > >
> > > > >
> > > > > The upside with having get/put around bigger aux transfers is that we don't get
> > > > > tons of enable/disable lines in the log. My vote is that we keep this but also
> > > > > have your fine-grained get/puts.
> > > >
> > > > Imo the correct solution to avoid this is by adding a slight bit of
> > > > hystersis to the power well code. Which means that yes, we reinvent
> > > > another feature of the core power_domain code in our home-grown solution -
> > > > I hate it when my years old predictions come true ;-)
> > > >
> > > > Sprinkling higher-level get/put calls all over the place is imo just
> > > > leaking the abstraction, which isn't good.
> > > > -Daniel
> > >
> > > With Ville's patches the problem is not as bad as I first thought. We can add
> > > hysteresis later if needed.
> > >
> > > -Patrik
> >
> > So I discovered that we cannot have DC5 and DC6 as seperate power wells since
> > they are mutually exclusive. As Ville pointed out we don't use DC5 for anything
> > so we could get away for now with just DC6 as a power well.
> >
> > As I see it there are three ways to go about this:
> >
> > A. Add AUX A to Power well 2.
> > This would power up PW2 during DP Aux A even though we don't need it but since
> > we get the side effect of DC6 being disabled it should work.
> >
> > B. Add DC6 off as a power well and remove DC5 off.
> > Fairly straight forward but assumes we don't need DC5 control.
> >
> > C. Add multi-state support for the DC power well.
> > Would be the nice way but perhaps a bit overkill. Good thing about this would be
> > that we can incorporate DC9 control for BXT and unify more of the DC code.
> >
> > So A, B or C?
>
> After some spitballing with Imre we came up with the following power well layout,
> which I think matches the documented seuqences in the spec the best:
>
> display well:
> domains:
> all
> enable:
> init display sequence
> enable pch reset handshake
> enable pw1 and miscio
> enable cdclk pll
> enable dbuf
> dc_state_en = up_to_dc6
> disable:
> dc_state_en = disable
> uninit display sequence
> disable dbuf
> disable cdclk pll
> disable pw1 and miscio
>
> dc6 off well:
> domains:
> gmbus, dc5 off well domains
> enable:
> dc_state_en = up_to_dc5
> disable:
> dc_state_en = up_to_dc6
>
> dc5 off well:
> domains:
> aux A, pw2 well domains
> enable:
> dc_state_en = disable
> disable:
> dc_state_en = up_to_dc5
>
> pw2 well:
> domains:
> ddi B/C/D/E, aux B/C/D/E, vga, audio, pipe B/C, pfit B/C
> enable:
> enable pw2
> disable:
> disable pw2
>
Ok, that looks good, but what I'm trying to get at is that if we're going to
have two seperate power wells for DC5 and DC6 how do we keep track of DC5
enable/disable in DC6 enable/disable?
E.g.
- DC6 enable (dc_state_en = up_to_dc5)
- DC5 enable (dc_state_en = disable)
- DC6 disable (dc_state_en = up_to_dc6)
The last line would incorrectly set dc_state_en = up_to_dc6 when it should be
dc_state_en = disable because DC5 is still enabled.
Currently we don't have the above case since DC5 is never enabled (dc_state_en =
disable) but it's still nasty. And with Aux A on DC5 (as in the description
above) we would hit this problem.
-Patrik
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list