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

[Imre Deak <imre.deak@intel.com>]

On to, 2015-11-12 at 13:51 +0100, Patrik Jakobsson wrote:
> 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@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.

Yea, we could extend this in case we make DC9 a power 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?

This is an experimental functionality and have device wide effects (at
least display wide), so I wouldn't be surprised if we can make a good
use of a more fine grained control here.

> 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?

Just not enabling DC5/6 ever in gen9_dc_off_power_well_disable(). The
idea would be to test if some failure is caused by DC5 for example.

> >  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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx