| Message ID | 1398696424-17813-1-git-send-email-deepak.s@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 28, 2014 at 08:17:04PM +0530, deepak.s@linux.intel.com wrote: > From: Deepak S <deepak.s@linux.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) > > v3: Use bool to represent the boot parameter (Ville). > > Signed-off-by: Deepak S <deepak.s@linux.intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/i915_params.c | 5 +++++ > 3 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e81feab..6136aab 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1945,6 +1945,7 @@ struct i915_params { > bool reset; > bool disable_display; > bool disable_vtd_wa; > + bool enable_rps_boost; > }; > extern struct i915_params i915 __read_mostly; > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index b00a77e..f2b3262 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1049,7 +1049,7 @@ 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) && i915.enable_rps_boost) { The separate INTEL_INFO was because this used to be a neat dev_priv->info.gen dereference (and dev used to not be derivable from file_priv, which itself may be NULL here), but please don't add another predicate that means can_wait_boost(). -Chris
On Mon, Apr 28, 2014 at 4:47 PM, <deepak.s@linux.intel.com> wrote: > From: Deepak S <deepak.s@linux.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) > > v3: Use bool to represent the boot parameter (Ville). > > Signed-off-by: Deepak S <deepak.s@linux.intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I'm still unhappy about this since it feels like cheating in benchmarks and it gives me the impression that you guys frob this at runtime on Android ;-) A few more ideas: 1. "light-boost": We add some hysteris (either time or whether we're still above rpe or something like that) and don't boost if this is the case. I expect that we won't be able to have the full boost benefits without the downside. 2. "eco-boost". We try to boost just enough to not miss the next frame. For that the app needs to tell us (with two new execbuf flag) whether it hit or missed the last deadline. Once an app used those flags for the first time we decrease the boost target freq once per HIT_DEADLINE and until we get the first MISS_DEADLINE. The we only try to sporadically test the limit again. TCP flow control theory might be interesting for copying ideas. 3. "runtime-boost-control". The workloads with very predictable regular loads seem to be known. We can just add a new execbuf NO_BOOST flag which libva uses on all execbufs but the first one (since we don't want to drop the first frame really). Approach 3 should be the simplest to implement and also the simplest to demonstrate in the open-source libva (since that's always a merge criteria). Aside: If you really use this at runtime then you essentially create a new ABI with this patch. Which means we need open-source userspace for it anyway. -Daniel
On Monday 28 April 2014 08:42 PM, Daniel Vetter wrote: > On Mon, Apr 28, 2014 at 4:47 PM, <deepak.s@linux.intel.com> wrote: >> From: Deepak S <deepak.s@linux.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) >> >> v3: Use bool to represent the boot parameter (Ville). >> >> Signed-off-by: Deepak S <deepak.s@linux.intel.com> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > I'm still unhappy about this since it feels like cheating in > benchmarks and it gives me the impression that you guys frob this at > runtime on Android ;-) > > A few more ideas: > 1. "light-boost": We add some hysteris (either time or whether we're > still above rpe or something like that) and don't boost if this is the > case. I expect that we won't be able to have the full boost benefits > without the downside. > > 2. "eco-boost". We try to boost just enough to not miss the next > frame. For that the app needs to tell us (with two new execbuf flag) > whether it hit or missed the last deadline. Once an app used those > flags for the first time we decrease the boost target freq once per > HIT_DEADLINE and until we get the first MISS_DEADLINE. The we only try > to sporadically test the limit again. TCP flow control theory might be > interesting for copying ideas. > > 3. "runtime-boost-control". The workloads with very predictable > regular loads seem to be known. We can just add a new execbuf NO_BOOST > flag which libva uses on all execbufs but the first one (since we > don't want to drop the first frame really). > > Approach 3 should be the simplest to implement and also the simplest > to demonstrate in the open-source libva (since that's always a merge > criteria). > > Aside: If you really use this at runtime then you essentially create a > new ABI with this patch. Which means we need open-source userspace for > it anyway. > -Daniel Thanks for the review. Agreed. option 3 is better. I will work on this.
On Wed, Apr 30, 2014 at 09:19:32PM +0530, Deepak S wrote: > > On Monday 28 April 2014 08:42 PM, Daniel Vetter wrote: > >On Mon, Apr 28, 2014 at 4:47 PM, <deepak.s@linux.intel.com> wrote: > >>From: Deepak S <deepak.s@linux.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) > >> > >>v3: Use bool to represent the boot parameter (Ville). > >> > >>Signed-off-by: Deepak S <deepak.s@linux.intel.com> > >>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >I'm still unhappy about this since it feels like cheating in > >benchmarks and it gives me the impression that you guys frob this at > >runtime on Android ;-) > > > >A few more ideas: > >1. "light-boost": We add some hysteris (either time or whether we're > >still above rpe or something like that) and don't boost if this is the > >case. I expect that we won't be able to have the full boost benefits > >without the downside. > > > >2. "eco-boost". We try to boost just enough to not miss the next > >frame. For that the app needs to tell us (with two new execbuf flag) > >whether it hit or missed the last deadline. Once an app used those > >flags for the first time we decrease the boost target freq once per > >HIT_DEADLINE and until we get the first MISS_DEADLINE. The we only try > >to sporadically test the limit again. TCP flow control theory might be > >interesting for copying ideas. > > > >3. "runtime-boost-control". The workloads with very predictable > >regular loads seem to be known. We can just add a new execbuf NO_BOOST > >flag which libva uses on all execbufs but the first one (since we > >don't want to drop the first frame really). > > > >Approach 3 should be the simplest to implement and also the simplest > >to demonstrate in the open-source libva (since that's always a merge > >criteria). > > > >Aside: If you really use this at runtime then you essentially create a > >new ABI with this patch. Which means we need open-source userspace for > >it anyway. > >-Daniel > > Thanks for the review. > Agreed. option 3 is better. I will work on this. Cool. Can you please get in contact with the otc libva guys for the demonstration user? The libva patch should be approved by libva owners for merging. Wrt testcase I think we only need to update the execbuf flag tests in gem_exec_params. Having a functional test seems less useful, we want to regression test that this works through overall power consumption metrics I guess. Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index e81feab..6136aab 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1945,6 +1945,7 @@ struct i915_params { bool reset; bool disable_display; bool disable_vtd_wa; + bool enable_rps_boost; }; extern struct i915_params i915 __read_mostly; diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b00a77e..f2b3262 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1049,7 +1049,7 @@ 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) && i915.enable_rps_boost) { gen6_rps_boost(dev_priv); if (file_priv) mod_delayed_work(dev_priv->wq, diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index d05a2af..b51da7c 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -48,6 +48,7 @@ struct i915_params i915 __read_mostly = { .disable_display = 0, .enable_cmd_parser = 1, .disable_vtd_wa = 0, + .enable_rps_boost = true, }; module_param_named(modeset, i915.modeset, int, 0400); @@ -156,3 +157,7 @@ MODULE_PARM_DESC(disable_vtd_wa, "Disable all VT-d workarounds (default: false)" module_param_named(enable_cmd_parser, i915.enable_cmd_parser, int, 0600); MODULE_PARM_DESC(enable_cmd_parser, "Enable command parsing (1=enabled [default], 0=disabled)"); + +module_param_named(enable_rps_boost, i915.enable_rps_boost, bool, 0600); +MODULE_PARM_DESC(enable_rps_boost, + "Enable/Disable boost RPS frequency (default: true)");