[Intel-gfx] [RFC PATCH] drm/i915/skl: Add DC6 disabling as a power well

[Patrik Jakobsson]

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?

-Patrik

> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx