| Message ID | 1394726418-10831-4-git-send-email-deepak.s@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 13, 2014 at 09:30:18PM +0530, deepak.s@linux.intel.com wrote: > From: Deepak S <deepak.s@intel.com> > > We are adding a module paramter to control rps boost. By default, we > enable the boost for better performace. Based on the need (perf/power) > we can either enable/disable. > > v2: Addressed rps default comment (Jani) > > Signed-off-by: Deepak S <deepak.s@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 16 +++++++++++++++- > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > 3 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 607042b..7808319 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2106,6 +2106,7 @@ struct i915_params { > int panel_use_ssc; > int vbt_sdvo_panel_type; > int enable_rc6; > + int enable_rps_boost; Should be bool like Jani said. And then it should be thrown somewhere somewhere at the end of the structure next to the other bools. > int enable_fbc; > int enable_ppgtt; > int enable_psr; > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 92b0b41..23a4700 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1002,6 +1002,17 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv) > return !atomic_xchg(&file_priv->rps_wait_boost, true); > } > > +static int intel_enable_rps_boost(struct drm_device *dev) > +{ > + /* No RPS Boost before Ironlake */ This comment is still wrong. I'd just drop it, everyone should know what the gen check below means. > + if (INTEL_INFO(dev)->gen < 6) > + return 0; > + > + /* Respect the kernel parameter if it is set */ This comment too seems rather obvious. I'd drop it as well. > + return i915.enable_rps_boost; > + > +} This function is still just a wrapper for i915.enable_rps_boost since __wait_seqno() already does the gen check. You could just check i915.enable_rps_boost directly in __wait_seqno(). The other option is to just drop the gen check from __wait_seqno() and just let this function take care of it. Hmm. Yeah that might be the nicest choice in fact. > + > /** > * __wait_seqno - wait until execution of seqno has finished > * @ring: the ring expected to report seqno > @@ -1042,8 +1053,11 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno, > > timeout_expire = timeout ? jiffies + timespec_to_jiffies_timeout(timeout) : 0; > > - if (INTEL_INFO(dev)->gen >= 6 && can_wait_boost(file_priv)) { > + if (INTEL_INFO(dev)->gen >= 6 && can_wait_boost(file_priv) && > + intel_enable_rps_boost(ring->dev)) { Indentation is quite wrong. There's also trailing whitespace around these parts. Please run patches through checkpatch.pl before submitting. > + > gen6_rps_boost(dev_priv); > + > if (file_priv) > mod_delayed_work(dev_priv->wq, > &file_priv->mm.idle_work, > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index a66ffb6..2d207e3 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -34,6 +34,7 @@ struct i915_params i915 __read_mostly = { > .panel_use_ssc = -1, > .vbt_sdvo_panel_type = -1, > .enable_rc6 = -1, > + .enable_rps_boost = 1, true > .enable_fbc = -1, > .enable_hangcheck = true, > .enable_ppgtt = -1, > @@ -78,6 +79,10 @@ 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(enable_rps_boost, i915.enable_rps_boost, int, 0600); bool > +MODULE_PARM_DESC(enable_rps_boost, > + "Enable/Disable boost RPS frequency (default: enabled (1))"); default: true > + > module_param_named(enable_fbc, i915.enable_fbc, int, 0600); > MODULE_PARM_DESC(enable_fbc, > "Enable frame buffer compression for power savings " > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 3/13/2014 11:46 PM, Ville Syrjälä wrote: > On Thu, Mar 13, 2014 at 09:30:18PM +0530, deepak.s@linux.intel.com wrote: >> From: Deepak S <deepak.s@intel.com> >> >> We are adding a module paramter to control rps boost. By default, we >> enable the boost for better performace. Based on the need (perf/power) >> we can either enable/disable. >> >> v2: Addressed rps default comment (Jani) >> >> Signed-off-by: Deepak S <deepak.s@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/i915_gem.c | 16 +++++++++++++++- >> drivers/gpu/drm/i915/i915_params.c | 5 +++++ >> 3 files changed, 21 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 607042b..7808319 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2106,6 +2106,7 @@ struct i915_params { >> int panel_use_ssc; >> int vbt_sdvo_panel_type; >> int enable_rc6; >> + int enable_rps_boost; > > Should be bool like Jani said. And then it should be thrown somewhere > somewhere at the end of the structure next to the other bools. I will address this. >> int enable_fbc; >> int enable_ppgtt; >> int enable_psr; >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 92b0b41..23a4700 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -1002,6 +1002,17 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv) >> return !atomic_xchg(&file_priv->rps_wait_boost, true); >> } >> >> +static int intel_enable_rps_boost(struct drm_device *dev) >> +{ >> + /* No RPS Boost before Ironlake */ > > This comment is still wrong. I'd just drop it, everyone should know what > the gen check below means. Ok >> + if (INTEL_INFO(dev)->gen < 6) >> + return 0; >> + >> + /* Respect the kernel parameter if it is set */ > > This comment too seems rather obvious. I'd drop it as well. Ok >> + return i915.enable_rps_boost; >> + >> +} > > This function is still just a wrapper for i915.enable_rps_boost since > __wait_seqno() already does the gen check. You could just check > i915.enable_rps_boost directly in __wait_seqno(). The other option is > to just drop the gen check from __wait_seqno() and just let this > function take care of it. Hmm. Yeah that might be the nicest choice in > fact. Agreed. Does not make sense to have multiple platform check's. >> + >> /** >> * __wait_seqno - wait until execution of seqno has finished >> * @ring: the ring expected to report seqno >> @@ -1042,8 +1053,11 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno, >> >> timeout_expire = timeout ? jiffies + timespec_to_jiffies_timeout(timeout) : 0; >> >> - if (INTEL_INFO(dev)->gen >= 6 && can_wait_boost(file_priv)) { >> + if (INTEL_INFO(dev)->gen >= 6 && can_wait_boost(file_priv) && >> + intel_enable_rps_boost(ring->dev)) { > > Indentation is quite wrong. There's also trailing whitespace around > these parts. Please run patches through checkpatch.pl before submitting. > >> + >> gen6_rps_boost(dev_priv); >> + >> if (file_priv) >> mod_delayed_work(dev_priv->wq, >> &file_priv->mm.idle_work, >> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c >> index a66ffb6..2d207e3 100644 >> --- a/drivers/gpu/drm/i915/i915_params.c >> +++ b/drivers/gpu/drm/i915/i915_params.c >> @@ -34,6 +34,7 @@ struct i915_params i915 __read_mostly = { >> .panel_use_ssc = -1, >> .vbt_sdvo_panel_type = -1, >> .enable_rc6 = -1, >> + .enable_rps_boost = 1, > > true > >> .enable_fbc = -1, >> .enable_hangcheck = true, >> .enable_ppgtt = -1, >> @@ -78,6 +79,10 @@ 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(enable_rps_boost, i915.enable_rps_boost, int, 0600); > > bool > >> +MODULE_PARM_DESC(enable_rps_boost, >> + "Enable/Disable boost RPS frequency (default: enabled (1))"); > > default: true > >> + >> module_param_named(enable_fbc, i915.enable_fbc, int, 0600); >> MODULE_PARM_DESC(enable_fbc, >> "Enable frame buffer compression for power savings " >> -- >> 1.8.4.2 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 607042b..7808319 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2106,6 +2106,7 @@ struct i915_params { int panel_use_ssc; int vbt_sdvo_panel_type; int enable_rc6; + int enable_rps_boost; int enable_fbc; int enable_ppgtt; int enable_psr; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 92b0b41..23a4700 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1002,6 +1002,17 @@ static bool can_wait_boost(struct drm_i915_file_private *file_priv) return !atomic_xchg(&file_priv->rps_wait_boost, true); } +static int intel_enable_rps_boost(struct drm_device *dev) +{ + /* No RPS Boost before Ironlake */ + if (INTEL_INFO(dev)->gen < 6) + return 0; + + /* Respect the kernel parameter if it is set */ + return i915.enable_rps_boost; + +} + /** * __wait_seqno - wait until execution of seqno has finished * @ring: the ring expected to report seqno @@ -1042,8 +1053,11 @@ static int __wait_seqno(struct intel_ring_buffer *ring, u32 seqno, timeout_expire = timeout ? jiffies + timespec_to_jiffies_timeout(timeout) : 0; - if (INTEL_INFO(dev)->gen >= 6 && can_wait_boost(file_priv)) { + if (INTEL_INFO(dev)->gen >= 6 && can_wait_boost(file_priv) && + intel_enable_rps_boost(ring->dev)) { + gen6_rps_boost(dev_priv); + if (file_priv) mod_delayed_work(dev_priv->wq, &file_priv->mm.idle_work, diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index a66ffb6..2d207e3 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -34,6 +34,7 @@ struct i915_params i915 __read_mostly = { .panel_use_ssc = -1, .vbt_sdvo_panel_type = -1, .enable_rc6 = -1, + .enable_rps_boost = 1, .enable_fbc = -1, .enable_hangcheck = true, .enable_ppgtt = -1, @@ -78,6 +79,10 @@ 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(enable_rps_boost, i915.enable_rps_boost, int, 0600); +MODULE_PARM_DESC(enable_rps_boost, + "Enable/Disable boost RPS frequency (default: enabled (1))"); + module_param_named(enable_fbc, i915.enable_fbc, int, 0600); MODULE_PARM_DESC(enable_fbc, "Enable frame buffer compression for power savings "