| Message ID | 1447084107-8521-12-git-send-email-patrik.jakobsson@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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);
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. 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);
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);
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])"); + 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);
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(-)