[Intel-gfx] [PATCH v2 11/12] drm/i915/gen9: Add boot parameter for disabling DC6

[Patrik Jakobsson]

On Wed, Nov 11, 2015 at 09:04:09PM +0200, Imre Deak wrote:
> On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote:
> > v2: Use _unsafe (Jani)
> > 
> > Signed-off-by: Patrik Jakobsson <patrik.jakobsson at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         | 1 +
> >  drivers/gpu/drm/i915/i915_params.c      | 6 ++++++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++--
> >  3 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index c0252ef..5628c5a 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2639,6 +2639,7 @@ struct i915_params {
> >  	int panel_use_ssc;
> >  	int vbt_sdvo_panel_type;
> >  	int enable_rc6;
> > +	int enable_dc6;
> >  	int enable_fbc;
> >  	int enable_ppgtt;
> >  	int enable_execlists;
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 368df67..6457f3a 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -32,6 +32,7 @@ struct i915_params i915 __read_mostly = {
> >  	.panel_use_ssc = -1,
> >  	.vbt_sdvo_panel_type = -1,
> >  	.enable_rc6 = -1,
> > +	.enable_dc6 = 1,
> >  	.enable_fbc = -1,
> >  	.enable_execlists = -1,
> >  	.enable_hangcheck = true,
> > @@ -79,6 +80,11 @@ MODULE_PARM_DESC(enable_rc6,
> >  	"For example, 3 would enable rc6 and deep rc6, and 7 would
> > enable everything. "
> >  	"default: -1 (use per-chip default)");
> >  
> > +module_param_named_unsafe(enable_dc6, i915.enable_dc6, int, 0400);
> > +MODULE_PARM_DESC(enable_dc6,
> > +	"Enable power-saving display C-state 6. "
> > +	"(0 = disable; 1 = enable [default])");
> > +
> 
> It would be more generic to have something like enable_dc, -1=per-chip
> default, 0=disable, 1=up to dc5, 2=up to dc6.

I'm not sure if this parameter is going to stay for long but if it does I
suppose we should have DC9 as well. But do we really need this level of control?
My intention was to work around the DC6 corner case. Do you think we could make
good use of a more generic interface? Perhaps useful for testing? If so, I
definitely think we should go with your more generic solution. Otherwise I'd
rather keep it simple. Feel free to override my decision here.

Also, what would 0=disable be? Not starting the DMC at all or DC3/4?

> 
> >  module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
> >  MODULE_PARM_DESC(enable_fbc,
> >  	"Enable frame buffer compression for power savings "
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 95c3fcc..62c1273 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -708,7 +708,7 @@ static void gen9_dc_off_power_well_enable(struct
> > drm_i915_private *dev_priv,
> >  static void gen9_dc_off_power_well_disable(struct drm_i915_private
> > *dev_priv,
> >  					  struct i915_power_well
> > *power_well)
> >  {
> > -	if (IS_SKYLAKE(dev_priv))
> > +	if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
> >  		skl_enable_dc6(dev_priv);
> >  	else
> >  		gen9_enable_dc5(dev_priv);
> > @@ -720,7 +720,7 @@ static void gen9_dc_off_power_well_sync_hw(struct
> > drm_i915_private *dev_priv,
> >  	if (power_well->count > 0) {
> >  		gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> >  	} else {
> > -		if (IS_SKYLAKE(dev_priv))
> > +		if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
> >  			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC6);
> >  		else
> >  			gen9_set_dc_state(dev_priv,
> > DC_STATE_EN_UPTO_DC5);