| Message ID | 1574743899-17638-4-git-send-email-ankit.p.navik@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Dynamic EU configuration of Slice/Sub-slice/EU | expand |
On 26/11/2019 04:51, Ankit Navik wrote: > High resolution timer is used for predictive governor to control > eu/slice/subslice based on workloads. > > param is provided to enable/disable/update timer configuration > > V2: > * Fix code style. > * Move predictive_load_timer into a drm_i915_private > structure. > * Make generic function to set optimum config. (Tvrtko Ursulin) > > V3: > * Rebase. > * Fix race condition for predictive load set. > * Add slack to start hrtimer for more power efficient. (Tvrtko Ursulin) > > V4: > * Fix data type and initialization of mutex to protect predictive load > state. > * Move predictive timer init to i915_gem_init_early. (Tvrtko Ursulin) > * Move debugfs to kernel parameter. > > V5: > * Rebase. > * Remove mutex for pred_timer > > V6: > * Rebase. > * Fix warnings. > > Cc: Vipin Anand <vipin.anand@intel.com> > Signed-off-by: Ankit Navik <ankit.p.navik@intel.com> > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/gt/intel_deu.c | 104 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/gt/intel_deu.h | 31 +++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 4 ++ > drivers/gpu/drm/i915/i915_gem.c | 4 ++ > drivers/gpu/drm/i915/i915_params.c | 4 ++ > drivers/gpu/drm/i915/i915_params.h | 1 + > 7 files changed, 149 insertions(+) > create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.c > create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index e0fd10c..c1a98f3 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -77,6 +77,7 @@ obj-y += gt/ > gt-y += \ > gt/intel_breadcrumbs.o \ > gt/intel_context.o \ > + gt/intel_deu.o \ > gt/intel_engine_cs.o \ > gt/intel_engine_heartbeat.o \ > gt/intel_engine_pm.o \ > diff --git a/drivers/gpu/drm/i915/gt/intel_deu.c b/drivers/gpu/drm/i915/gt/intel_deu.c > new file mode 100644 > index 0000000..6c5b01c > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_deu.c > @@ -0,0 +1,104 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + * > + * Authors: > + * Ankit Navik <ankit.p.navik@intel.com> > + */ > + > +/** > + * DOC: Dynamic EU Control (DEU) > + * > + * DEU tries to re-configure EU allocation during runtime by predictive load > + * calculation of command queue to gain power saving. > + * It is transparent to user space and completely handled in the kernel. > + */ > + > +#include "intel_deu.h" > +#include "i915_drv.h" > +#include "gem/i915_gem_context.h" > + > +/* > + * Anything above threshold is considered as HIGH load, less is considered > + * as LOW load and equal is considered as MEDIUM load. > + * > + * The threshold value of three active requests pending. > + */ > +#define PENDING_THRESHOLD_MEDIUM 3 > + > +#define SLACK_TIMER_NSEC 1000000 /* Timer range in nano second */ > + > +enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer) > +{ > + struct drm_i915_private *dev_priv = > + container_of(hrtimer, typeof(*dev_priv), pred_timer); > + struct i915_gem_context *ctx; > + enum gem_load_type load_type; > + unsigned int req_pending; > + > + list_for_each_entry(ctx, &dev_priv->gem.contexts.list, link) { > + req_pending = atomic_read(&ctx->req_cnt); > + > + /* > + * Transitioning to low state whenever pending request is zero > + * would cause vacillation between low and high state. > + */ > + if (req_pending == 0) > + continue; > + > + if (req_pending > PENDING_THRESHOLD_MEDIUM) > + load_type = LOAD_TYPE_HIGH; > + else if (req_pending == PENDING_THRESHOLD_MEDIUM) > + load_type = LOAD_TYPE_MEDIUM; > + else > + load_type = LOAD_TYPE_LOW; > + > + i915_gem_context_set_load_type(ctx, load_type); > + } Now ideally we don't want to iterate contexts from a timer for more than one reason. Most interesting one is that the configuration you are setting here is not actually applied until __execlists_update_reg_state. Which runs only when contexts is getting pinned (or re-pinned). You mentioned you did some experiment where you did something on context pinning and that it did not work so well. I don't know what that was though. I don't think that was ever posted? What I am thinking is this: You drop the timer altogether. Instead in __execlists_update_reg_state you look at your gem_context->req_cnt and implement your logic there. You convert your selected configuration to struct intel_sseu (so counts to bitmasks) and pass it to intel_sseu_make_rpcs which does the right thing. > + > + hrtimer_forward_now(hrtimer, > + ms_to_ktime(dev_priv->predictive_load_enable)); > + > + return HRTIMER_RESTART; > +} > + > +/** > + * intel_deu_init - Initialize dynamic EU > + * @dev_priv: i915 device instance > + * > + * This function is called at driver load > + */ > +void intel_deu_init(struct drm_i915_private *dev_priv) > +{ > + dev_priv->predictive_load_enable = i915_modparams.deu_enable; > + hrtimer_init(&dev_priv->pred_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + dev_priv->pred_timer.function = predictive_load_cb; > + > + if (dev_priv->predictive_load_enable) { > + if (!hrtimer_active(&dev_priv->pred_timer)) > + hrtimer_start_range_ns(&dev_priv->pred_timer, > + ms_to_ktime(dev_priv->predictive_load_enable), > + SLACK_TIMER_NSEC, > + HRTIMER_MODE_REL_PINNED); > + } else { > + hrtimer_cancel(&dev_priv->pred_timer); Why do you need to stop something which hasn't been started? And more importantly, who stops the timer on driver unload? > + } > +} > diff --git a/drivers/gpu/drm/i915/gt/intel_deu.h b/drivers/gpu/drm/i915/gt/intel_deu.h > new file mode 100644 > index 0000000..3b4b16f > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_deu.h > @@ -0,0 +1,31 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > + > +#ifndef __INTEL_DEU_H__ > +#define __INTEL_DEU_H__ > + > +struct drm_i915_private; > + > +void intel_deu_init(struct drm_i915_private *dev_priv); > + > +#endif /* __INTEL_DEU_H__ */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3064ddf..5553537 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1002,6 +1002,8 @@ struct drm_i915_private { > /* optimal slice/subslice/EU configration state */ > struct i915_sseu_optimum_config *opt_config; > > + /* protects predictive load state */ > + struct hrtimer pred_timer; > int predictive_load_enable; > > unsigned int fsb_freq, mem_freq, is_ddr3; > @@ -1768,6 +1770,8 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); > #endif > extern const struct dev_pm_ops i915_pm_ops; > > +extern enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer); > + > int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent); > void i915_driver_remove(struct drm_i915_private *i915); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 61395b0..ee711ce 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -45,6 +45,7 @@ > #include "gem/i915_gem_context.h" > #include "gem/i915_gem_ioctls.h" > #include "gem/i915_gem_pm.h" > +#include "gt/intel_deu.h" > #include "gt/intel_context.h" > #include "gt/intel_engine_user.h" > #include "gt/intel_gt.h" > @@ -1416,6 +1417,9 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv) > i915_gem_init__mm(dev_priv); > > spin_lock_init(&dev_priv->fb_tracking.lock); > + > + /* Dynamic EU timer initialization for predictive load */ > + intel_deu_init(dev_priv); > } > > void i915_gem_cleanup_early(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 1dd1f36..a5a3a6e 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -101,6 +101,10 @@ i915_param_named_unsafe(disable_power_well, int, 0400, > > i915_param_named_unsafe(enable_ips, int, 0600, "Enable IPS (default: true)"); > > +i915_param_named_unsafe(deu_enable, int, 0600, > + "Enable dynamic EU control for power savings " > + "(0=disable deu predictive timer [default], 150=optimal deu predictive timer)"); > + > i915_param_named(fastboot, int, 0600, > "Try to skip unnecessary mode sets at boot time " > "(0=disabled, 1=enabled) " > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index 31b88f2..cf0903b 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -54,6 +54,7 @@ struct drm_printer; > param(int, disable_power_well, -1) \ > param(int, enable_ips, 1) \ > param(int, invert_brightness, 0) \ > + param(int, deu_enable, 0) \ > param(int, enable_guc, 0) \ > param(int, guc_log_level, -1) \ > param(char *, guc_firmware_path, NULL) \ > Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-11-26 10:51:22) > You mentioned you did some experiment where you did something on context > pinning and that it did not work so well. I don't know what that was > though. I don't think that was ever posted? > > What I am thinking is this: You drop the timer altogether. Instead in > __execlists_update_reg_state you look at your gem_context->req_cnt and > implement your logic there. I noticed the same non-sequitur. Except I would push that either the entire measurement and hence patch series is bogus (beyond the patches themselves being trivially broken, tested much?), or that it really should be done from a timer and also adjust pinned contexts ala reconfigure_sseu. A bunch of selftests and igt proving that different sseu setups do consume different power (i.e. that we can adjust sseu correctly), along with demonstrating good workloads where autotuning produces beneficial results is a must. Also given Tvrtko's stats, this could all be done from userspace with an extended CONTEXT_PARAM_SSEU, so why would we not do it that way? -Chris
On 26/11/2019 11:09, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-11-26 10:51:22) >> You mentioned you did some experiment where you did something on context >> pinning and that it did not work so well. I don't know what that was >> though. I don't think that was ever posted? >> >> What I am thinking is this: You drop the timer altogether. Instead in >> __execlists_update_reg_state you look at your gem_context->req_cnt and >> implement your logic there. > > I noticed the same non-sequitur. Except I would push that either the > entire measurement and hence patch series is bogus (beyond the patches > themselves being trivially broken, tested much?), or that it really > should be done from a timer and also adjust pinned contexts ala > reconfigure_sseu. Yeah, if doing it at pin time would not show the power benefit that would mean looking at req_cnt at pin time does not work, while looking at it half a timer period ago, on average, works. Which would be very intriguing. We'd probably want nice graphs in this case overlaying power, request counts, selected EU config, etc. > A bunch of selftests and igt proving that different sseu setups do > consume different power (i.e. that we can adjust sseu correctly), > along with demonstrating good workloads where autotuning produces > beneficial results is a must. > > Also given Tvrtko's stats, this could all be done from userspace with an > extended CONTEXT_PARAM_SSEU, so why would we not do it that way? You mean patching and recompiling userspace? I don't think that would work for them. Regards, Tvrtko
On 26/11/2019 11:31, Tvrtko Ursulin wrote: > On 26/11/2019 11:09, Chris Wilson wrote: >> Quoting Tvrtko Ursulin (2019-11-26 10:51:22) >>> You mentioned you did some experiment where you did something on context >>> pinning and that it did not work so well. I don't know what that was >>> though. I don't think that was ever posted? >>> >>> What I am thinking is this: You drop the timer altogether. Instead in >>> __execlists_update_reg_state you look at your gem_context->req_cnt and >>> implement your logic there. >> >> I noticed the same non-sequitur. Except I would push that either the >> entire measurement and hence patch series is bogus (beyond the patches >> themselves being trivially broken, tested much?), or that it really >> should be done from a timer and also adjust pinned contexts ala >> reconfigure_sseu. > > Yeah, if doing it at pin time would not show the power benefit that > would mean looking at req_cnt at pin time does not work, while looking > at it half a timer period ago, on average, works. Which would be very > intriguing. We'd probably want nice graphs in this case overlaying > power, request counts, selected EU config, etc. Another thing to try, if simple bucketing of req_cnt to load level at pin time will not work, could be a time-weighted moving average of the same count, but also driven from context pinning. Regards, Tvrtko
Quoting Tvrtko Ursulin (2019-11-26 11:31:14) > > On 26/11/2019 11:09, Chris Wilson wrote: > > Also given Tvrtko's stats, this could all be done from userspace with an > > extended CONTEXT_PARAM_SSEU, so why would we not do it that way? > > You mean patching and recompiling userspace? I don't think that would > work for them. No. I was thinking about some proof-of-concept userspace daemon using per-context stats and tuning each context from a timer. Almost exactly as proposed here, but it would be very nice validation that our kernel API are not getting in the way of userspace creativity. :) But who am I to talk, I think C is a scripting language ;) -Chris
On 26/11/2019 13:21, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2019-11-26 11:31:14) >> >> On 26/11/2019 11:09, Chris Wilson wrote: >>> Also given Tvrtko's stats, this could all be done from userspace with an >>> extended CONTEXT_PARAM_SSEU, so why would we not do it that way? >> >> You mean patching and recompiling userspace? I don't think that would >> work for them. > > No. I was thinking about some proof-of-concept userspace daemon using > per-context stats and tuning each context from a timer. Almost exactly > as proposed here, but it would be very nice validation that our kernel > API are not getting in the way of userspace creativity. :) > > But who am I to talk, I think C is a scripting language ;) Ah, I overlooked you wrote _extended_ CONTEXT_PARAM_SSEU. Regards, Tvrtko
Hi Chris, Thank you for your feedback. > On 26/11/2019 10:52, Charis Wilson wrote: > > Quoting Tvrtko Ursulin (2019-11-26 10:51:22) > > You mentioned you did some experiment where you did something on > > context pinning and that it did not work so well. I don't know what > > that was though. I don't think that was ever posted? > > > > What I am thinking is this: You drop the timer altogether. Instead in > > __execlists_update_reg_state you look at your gem_context->req_cnt and > > implement your logic there. > > I noticed the same non-sequitur. Except I would push that either the entire > measurement and hence patch series is bogus (beyond the patches themselves > being trivially broken, tested much?), or that it really should be done from a > timer and also adjust pinned contexts ala reconfigure_sseu. > > A bunch of selftests and igt proving that different sseu setups do consume > different power (i.e. that we can adjust sseu correctly), along with > demonstrating good workloads where autotuning produces beneficial results is > a must. > > Also given Tvrtko's stats, this could all be done from userspace with an extended > CONTEXT_PARAM_SSEU, so why would we not do it that way? > -Chris We have verified this patch series on KBL, GLK & CML platforms with well known standard benchmarks as mentioned in the cover letters. Power savings are verified with various tools (RAPL counters, Moonsoon power monitor and socwatch). We have also verified Battery Life use cases which shows significant power savings. So I hope you will understand the we have taken proper measurements before pushing the patch series. Thanks & Regards, Ankit
> On 26/11/2019 04:21, Tvrtko Ursulin wrote: > > On 26/11/2019 04:51, Ankit Navik wrote: > > High resolution timer is used for predictive governor to control > > eu/slice/subslice based on workloads. > > > > param is provided to enable/disable/update timer configuration > > > > V2: > > * Fix code style. > > * Move predictive_load_timer into a drm_i915_private > > structure. > > * Make generic function to set optimum config. (Tvrtko Ursulin) > > > > V3: > > * Rebase. > > * Fix race condition for predictive load set. > > * Add slack to start hrtimer for more power efficient. (Tvrtko > > Ursulin) > > > > V4: > > * Fix data type and initialization of mutex to protect predictive load > > state. > > * Move predictive timer init to i915_gem_init_early. (Tvrtko Ursulin) > > * Move debugfs to kernel parameter. > > > > V5: > > * Rebase. > > * Remove mutex for pred_timer > > > > V6: > > * Rebase. > > * Fix warnings. > > > > Cc: Vipin Anand <vipin.anand@intel.com> > > Signed-off-by: Ankit Navik <ankit.p.navik@intel.com> > > --- > > drivers/gpu/drm/i915/Makefile | 1 + > > drivers/gpu/drm/i915/gt/intel_deu.c | 104 > ++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/gt/intel_deu.h | 31 +++++++++++ > > drivers/gpu/drm/i915/i915_drv.h | 4 ++ > > drivers/gpu/drm/i915/i915_gem.c | 4 ++ > > drivers/gpu/drm/i915/i915_params.c | 4 ++ > > drivers/gpu/drm/i915/i915_params.h | 1 + > > 7 files changed, 149 insertions(+) > > create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.c > > create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.h > > > > diff --git a/drivers/gpu/drm/i915/Makefile > > b/drivers/gpu/drm/i915/Makefile index e0fd10c..c1a98f3 100644 > > --- a/drivers/gpu/drm/i915/Makefile > > +++ b/drivers/gpu/drm/i915/Makefile > > @@ -77,6 +77,7 @@ obj-y += gt/ > > gt-y += \ > > gt/intel_breadcrumbs.o \ > > gt/intel_context.o \ > > + gt/intel_deu.o \ > > gt/intel_engine_cs.o \ > > gt/intel_engine_heartbeat.o \ > > gt/intel_engine_pm.o \ > > diff --git a/drivers/gpu/drm/i915/gt/intel_deu.c > > b/drivers/gpu/drm/i915/gt/intel_deu.c > > new file mode 100644 > > index 0000000..6c5b01c > > --- /dev/null > > +++ b/drivers/gpu/drm/i915/gt/intel_deu.c > > @@ -0,0 +1,104 @@ > > +/* > > + * Copyright © 2019 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person > > +obtaining a > > + * copy of this software and associated documentation files (the > > +"Software"), > > + * to deal in the Software without restriction, including without > > +limitation > > + * the rights to use, copy, modify, merge, publish, distribute, > > +sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom > > +the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including > > +the next > > + * paragraph) shall be included in all copies or substantial portions > > +of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > +EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > +MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > EVENT > > +SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, > DAMAGES > > +OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > +ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > > +OTHER > > + * DEALINGS IN THE SOFTWARE. > > + * > > + * Authors: > > + * Ankit Navik <ankit.p.navik@intel.com> > > + */ > > + > > +/** > > + * DOC: Dynamic EU Control (DEU) > > + * > > + * DEU tries to re-configure EU allocation during runtime by > > +predictive load > > + * calculation of command queue to gain power saving. > > + * It is transparent to user space and completely handled in the kernel. > > + */ > > + > > +#include "intel_deu.h" > > +#include "i915_drv.h" > > +#include "gem/i915_gem_context.h" > > + > > +/* > > + * Anything above threshold is considered as HIGH load, less is > > +considered > > + * as LOW load and equal is considered as MEDIUM load. > > + * > > + * The threshold value of three active requests pending. > > + */ > > +#define PENDING_THRESHOLD_MEDIUM 3 > > + > > +#define SLACK_TIMER_NSEC 1000000 /* Timer range in nano second */ > > + > > +enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer) { > > + struct drm_i915_private *dev_priv = > > + container_of(hrtimer, typeof(*dev_priv), pred_timer); > > + struct i915_gem_context *ctx; > > + enum gem_load_type load_type; > > + unsigned int req_pending; > > + > > + list_for_each_entry(ctx, &dev_priv->gem.contexts.list, link) { > > + req_pending = atomic_read(&ctx->req_cnt); > > + > > + /* > > + * Transitioning to low state whenever pending request is zero > > + * would cause vacillation between low and high state. > > + */ > > + if (req_pending == 0) > > + continue; > > + > > + if (req_pending > PENDING_THRESHOLD_MEDIUM) > > + load_type = LOAD_TYPE_HIGH; > > + else if (req_pending == PENDING_THRESHOLD_MEDIUM) > > + load_type = LOAD_TYPE_MEDIUM; > > + else > > + load_type = LOAD_TYPE_LOW; > > + > > + i915_gem_context_set_load_type(ctx, load_type); > > + } > > Now ideally we don't want to iterate contexts from a timer for more than one > reason. Most interesting one is that the configuration you are setting here is not > actually applied until __execlists_update_reg_state. > Which runs only when contexts is getting pinned (or re-pinned). > > You mentioned you did some experiment where you did something on context > pinning and that it did not work so well. I don't know what that was though. I > don't think that was ever posted? > > What I am thinking is this: You drop the timer altogether. Instead in > __execlists_update_reg_state you look at your gem_context->req_cnt and > implement your logic there. > > You convert your selected configuration to struct intel_sseu (so counts to > bitmasks) and pass it to intel_sseu_make_rpcs which does the right thing. Ok, we will drop the timer and verify the results with the suggestion. Thanks & Regards, Ankit > > > + > > + hrtimer_forward_now(hrtimer, > > + ms_to_ktime(dev_priv->predictive_load_enable)); > > + > > + return HRTIMER_RESTART; > > +} > > + > > +/** > > + * intel_deu_init - Initialize dynamic EU > > + * @dev_priv: i915 device instance > > + * > > + * This function is called at driver load */ void > > +intel_deu_init(struct drm_i915_private *dev_priv) { > > + dev_priv->predictive_load_enable = i915_modparams.deu_enable; > > + hrtimer_init(&dev_priv->pred_timer, CLOCK_MONOTONIC, > HRTIMER_MODE_REL); > > + dev_priv->pred_timer.function = predictive_load_cb; > > + > > + if (dev_priv->predictive_load_enable) { > > + if (!hrtimer_active(&dev_priv->pred_timer)) > > + hrtimer_start_range_ns(&dev_priv->pred_timer, > > + ms_to_ktime(dev_priv->predictive_load_enable), > > + SLACK_TIMER_NSEC, > > + HRTIMER_MODE_REL_PINNED); > > + } else { > > + hrtimer_cancel(&dev_priv->pred_timer); > > Why do you need to stop something which hasn't been started? > > And more importantly, who stops the timer on driver unload? > > > + } > > +} > > diff --git a/drivers/gpu/drm/i915/gt/intel_deu.h > > b/drivers/gpu/drm/i915/gt/intel_deu.h > > new file mode 100644 > > index 0000000..3b4b16f > > --- /dev/null > > +++ b/drivers/gpu/drm/i915/gt/intel_deu.h > > @@ -0,0 +1,31 @@ > > +/* > > + * Copyright © 2019 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person > > +obtaining a > > + * copy of this software and associated documentation files (the > > +"Software"), > > + * to deal in the Software without restriction, including without > > +limitation > > + * the rights to use, copy, modify, merge, publish, distribute, > > +sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom > > +the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including > > +the next > > + * paragraph) shall be included in all copies or substantial portions > > +of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > > +EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > > +MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > EVENT > > +SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, > DAMAGES > > +OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > > +ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR > > +OTHER > > + * DEALINGS IN THE SOFTWARE. > > + */ > > + > > +#ifndef __INTEL_DEU_H__ > > +#define __INTEL_DEU_H__ > > + > > +struct drm_i915_private; > > + > > +void intel_deu_init(struct drm_i915_private *dev_priv); > > + > > +#endif /* __INTEL_DEU_H__ */ > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index 3064ddf..5553537 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1002,6 +1002,8 @@ struct drm_i915_private { > > /* optimal slice/subslice/EU configration state */ > > struct i915_sseu_optimum_config *opt_config; > > > > + /* protects predictive load state */ > > + struct hrtimer pred_timer; > > int predictive_load_enable; > > > > unsigned int fsb_freq, mem_freq, is_ddr3; @@ -1768,6 +1770,8 @@ > > long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); > > #endif > > extern const struct dev_pm_ops i915_pm_ops; > > > > +extern enum hrtimer_restart predictive_load_cb(struct hrtimer > > +*hrtimer); > > + > > int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent); > > void i915_driver_remove(struct drm_i915_private *i915); > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > > b/drivers/gpu/drm/i915/i915_gem.c index 61395b0..ee711ce 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -45,6 +45,7 @@ > > #include "gem/i915_gem_context.h" > > #include "gem/i915_gem_ioctls.h" > > #include "gem/i915_gem_pm.h" > > +#include "gt/intel_deu.h" > > #include "gt/intel_context.h" > > #include "gt/intel_engine_user.h" > > #include "gt/intel_gt.h" > > @@ -1416,6 +1417,9 @@ void i915_gem_init_early(struct drm_i915_private > *dev_priv) > > i915_gem_init__mm(dev_priv); > > > > spin_lock_init(&dev_priv->fb_tracking.lock); > > + > > + /* Dynamic EU timer initialization for predictive load */ > > + intel_deu_init(dev_priv); > > } > > > > void i915_gem_cleanup_early(struct drm_i915_private *dev_priv) diff > > --git a/drivers/gpu/drm/i915/i915_params.c > > b/drivers/gpu/drm/i915/i915_params.c > > index 1dd1f36..a5a3a6e 100644 > > --- a/drivers/gpu/drm/i915/i915_params.c > > +++ b/drivers/gpu/drm/i915/i915_params.c > > @@ -101,6 +101,10 @@ i915_param_named_unsafe(disable_power_well, int, > > 0400, > > > > i915_param_named_unsafe(enable_ips, int, 0600, "Enable IPS (default: > > true)"); > > > > +i915_param_named_unsafe(deu_enable, int, 0600, > > + "Enable dynamic EU control for power savings " > > + "(0=disable deu predictive timer [default], 150=optimal deu > > +predictive timer)"); > + > > i915_param_named(fastboot, int, 0600, > > "Try to skip unnecessary mode sets at boot time " > > "(0=disabled, 1=enabled) " > > diff --git a/drivers/gpu/drm/i915/i915_params.h > > b/drivers/gpu/drm/i915/i915_params.h > > index 31b88f2..cf0903b 100644 > > --- a/drivers/gpu/drm/i915/i915_params.h > > +++ b/drivers/gpu/drm/i915/i915_params.h > > @@ -54,6 +54,7 @@ struct drm_printer; > > param(int, disable_power_well, -1) \ > > param(int, enable_ips, 1) \ > > param(int, invert_brightness, 0) \ > > + param(int, deu_enable, 0) \ > > param(int, enable_guc, 0) \ > > param(int, guc_log_level, -1) \ > > param(char *, guc_firmware_path, NULL) \ > > > > Regards, > > Tvrtko
On 26/11/2019 13:16, Tvrtko Ursulin wrote: > > On 26/11/2019 11:31, Tvrtko Ursulin wrote: >> On 26/11/2019 11:09, Chris Wilson wrote: >>> Quoting Tvrtko Ursulin (2019-11-26 10:51:22) >>>> You mentioned you did some experiment where you did something on >>>> context >>>> pinning and that it did not work so well. I don't know what that was >>>> though. I don't think that was ever posted? >>>> >>>> What I am thinking is this: You drop the timer altogether. Instead in >>>> __execlists_update_reg_state you look at your gem_context->req_cnt and >>>> implement your logic there. >>> >>> I noticed the same non-sequitur. Except I would push that either the >>> entire measurement and hence patch series is bogus (beyond the patches >>> themselves being trivially broken, tested much?), or that it really >>> should be done from a timer and also adjust pinned contexts ala >>> reconfigure_sseu. >> >> Yeah, if doing it at pin time would not show the power benefit that >> would mean looking at req_cnt at pin time does not work, while looking >> at it half a timer period ago, on average, works. Which would be very >> intriguing. We'd probably want nice graphs in this case overlaying >> power, request counts, selected EU config, etc. > > Another thing to try, if simple bucketing of req_cnt to load level at > pin time will not work, could be a time-weighted moving average of the > same count, but also driven from context pinning. Yet another interesting experiment would be to try context busyness instead of request counts. Crudely, look at context's GPU busy time (I have patches for this) per evaluation period and configure accordingly. This should be able to track better in theory I think, but probably has it's own problems. Hard to say without trying and comparing. Implementation wise, a kthread periodically reconfiguring contexts would work I think. Like: every second per context query context engine busyness calculate relative busyness re-configure sseu Regards, Tvrtko
On 26/11/2019 04:51, Ankit Navik wrote: > High resolution timer is used for predictive governor to control > eu/slice/subslice based on workloads. > > param is provided to enable/disable/update timer configuration > > V2: > * Fix code style. > * Move predictive_load_timer into a drm_i915_private > structure. > * Make generic function to set optimum config. (Tvrtko Ursulin) > > V3: > * Rebase. > * Fix race condition for predictive load set. > * Add slack to start hrtimer for more power efficient. (Tvrtko Ursulin) > > V4: > * Fix data type and initialization of mutex to protect predictive load > state. > * Move predictive timer init to i915_gem_init_early. (Tvrtko Ursulin) > * Move debugfs to kernel parameter. > > V5: > * Rebase. > * Remove mutex for pred_timer > > V6: > * Rebase. > * Fix warnings. > > Cc: Vipin Anand <vipin.anand@intel.com> > Signed-off-by: Ankit Navik <ankit.p.navik@intel.com> > --- > drivers/gpu/drm/i915/Makefile | 1 + > drivers/gpu/drm/i915/gt/intel_deu.c | 104 ++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/gt/intel_deu.h | 31 +++++++++++ > drivers/gpu/drm/i915/i915_drv.h | 4 ++ > drivers/gpu/drm/i915/i915_gem.c | 4 ++ > drivers/gpu/drm/i915/i915_params.c | 4 ++ > drivers/gpu/drm/i915/i915_params.h | 1 + > 7 files changed, 149 insertions(+) > create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.c > create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.h > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index e0fd10c..c1a98f3 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -77,6 +77,7 @@ obj-y += gt/ > gt-y += \ > gt/intel_breadcrumbs.o \ > gt/intel_context.o \ > + gt/intel_deu.o \ > gt/intel_engine_cs.o \ > gt/intel_engine_heartbeat.o \ > gt/intel_engine_pm.o \ > diff --git a/drivers/gpu/drm/i915/gt/intel_deu.c b/drivers/gpu/drm/i915/gt/intel_deu.c > new file mode 100644 > index 0000000..6c5b01c > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_deu.c > @@ -0,0 +1,104 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + * > + * Authors: > + * Ankit Navik <ankit.p.navik@intel.com> > + */ > + > +/** > + * DOC: Dynamic EU Control (DEU) > + * > + * DEU tries to re-configure EU allocation during runtime by predictive load > + * calculation of command queue to gain power saving. > + * It is transparent to user space and completely handled in the kernel. > + */ > + > +#include "intel_deu.h" > +#include "i915_drv.h" > +#include "gem/i915_gem_context.h" > + > +/* > + * Anything above threshold is considered as HIGH load, less is considered > + * as LOW load and equal is considered as MEDIUM load. > + * > + * The threshold value of three active requests pending. > + */ > +#define PENDING_THRESHOLD_MEDIUM 3 > + > +#define SLACK_TIMER_NSEC 1000000 /* Timer range in nano second */ > + > +enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer) > +{ > + struct drm_i915_private *dev_priv = > + container_of(hrtimer, typeof(*dev_priv), pred_timer); > + struct i915_gem_context *ctx; > + enum gem_load_type load_type; > + unsigned int req_pending; > + > + list_for_each_entry(ctx, &dev_priv->gem.contexts.list, link) { > + req_pending = atomic_read(&ctx->req_cnt); > + Another thought here. SSEU configuration is a shared resource affecting some engines with some commands. For instance number of requests targeting the blitter engine sounds completely irrelevant to this decision. Requests targeting video engines may or may not be relevant, depends if they are using media fixed function or non-fixed function pipeline (I think at least they can do this.). Render is always relevant. I had some patches which count per engine, but in a world of virtual engines that does not work so well. Maybe solution is to track per struct intel_context. That should solve the virtual engine problem, but I am unsure how to deal with the media angle. I can sketch out the intel_context request tracking after the holiday break as a starting point. Regards, Tvrtko > + /* > + * Transitioning to low state whenever pending request is zero > + * would cause vacillation between low and high state. > + */ > + if (req_pending == 0) > + continue; > + > + if (req_pending > PENDING_THRESHOLD_MEDIUM) > + load_type = LOAD_TYPE_HIGH; > + else if (req_pending == PENDING_THRESHOLD_MEDIUM) > + load_type = LOAD_TYPE_MEDIUM; > + else > + load_type = LOAD_TYPE_LOW; > + > + i915_gem_context_set_load_type(ctx, load_type); > + } > + > + hrtimer_forward_now(hrtimer, > + ms_to_ktime(dev_priv->predictive_load_enable)); > + > + return HRTIMER_RESTART; > +} > + > +/** > + * intel_deu_init - Initialize dynamic EU > + * @dev_priv: i915 device instance > + * > + * This function is called at driver load > + */ > +void intel_deu_init(struct drm_i915_private *dev_priv) > +{ > + dev_priv->predictive_load_enable = i915_modparams.deu_enable; > + hrtimer_init(&dev_priv->pred_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + dev_priv->pred_timer.function = predictive_load_cb; > + > + if (dev_priv->predictive_load_enable) { > + if (!hrtimer_active(&dev_priv->pred_timer)) > + hrtimer_start_range_ns(&dev_priv->pred_timer, > + ms_to_ktime(dev_priv->predictive_load_enable), > + SLACK_TIMER_NSEC, > + HRTIMER_MODE_REL_PINNED); > + } else { > + hrtimer_cancel(&dev_priv->pred_timer); > + } > +} > diff --git a/drivers/gpu/drm/i915/gt/intel_deu.h b/drivers/gpu/drm/i915/gt/intel_deu.h > new file mode 100644 > index 0000000..3b4b16f > --- /dev/null > +++ b/drivers/gpu/drm/i915/gt/intel_deu.h > @@ -0,0 +1,31 @@ > +/* > + * Copyright © 2019 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > + * copy of this software and associated documentation files (the "Software"), > + * to deal in the Software without restriction, including without limitation > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the next > + * paragraph) shall be included in all copies or substantial portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > + > +#ifndef __INTEL_DEU_H__ > +#define __INTEL_DEU_H__ > + > +struct drm_i915_private; > + > +void intel_deu_init(struct drm_i915_private *dev_priv); > + > +#endif /* __INTEL_DEU_H__ */ > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3064ddf..5553537 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1002,6 +1002,8 @@ struct drm_i915_private { > /* optimal slice/subslice/EU configration state */ > struct i915_sseu_optimum_config *opt_config; > > + /* protects predictive load state */ > + struct hrtimer pred_timer; > int predictive_load_enable; > > unsigned int fsb_freq, mem_freq, is_ddr3; > @@ -1768,6 +1770,8 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); > #endif > extern const struct dev_pm_ops i915_pm_ops; > > +extern enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer); > + > int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent); > void i915_driver_remove(struct drm_i915_private *i915); > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 61395b0..ee711ce 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -45,6 +45,7 @@ > #include "gem/i915_gem_context.h" > #include "gem/i915_gem_ioctls.h" > #include "gem/i915_gem_pm.h" > +#include "gt/intel_deu.h" > #include "gt/intel_context.h" > #include "gt/intel_engine_user.h" > #include "gt/intel_gt.h" > @@ -1416,6 +1417,9 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv) > i915_gem_init__mm(dev_priv); > > spin_lock_init(&dev_priv->fb_tracking.lock); > + > + /* Dynamic EU timer initialization for predictive load */ > + intel_deu_init(dev_priv); > } > > void i915_gem_cleanup_early(struct drm_i915_private *dev_priv) > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 1dd1f36..a5a3a6e 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -101,6 +101,10 @@ i915_param_named_unsafe(disable_power_well, int, 0400, > > i915_param_named_unsafe(enable_ips, int, 0600, "Enable IPS (default: true)"); > > +i915_param_named_unsafe(deu_enable, int, 0600, > + "Enable dynamic EU control for power savings " > + "(0=disable deu predictive timer [default], 150=optimal deu predictive timer)"); > + > i915_param_named(fastboot, int, 0600, > "Try to skip unnecessary mode sets at boot time " > "(0=disabled, 1=enabled) " > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index 31b88f2..cf0903b 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -54,6 +54,7 @@ struct drm_printer; > param(int, disable_power_well, -1) \ > param(int, enable_ips, 1) \ > param(int, invert_brightness, 0) \ > + param(int, deu_enable, 0) \ > param(int, enable_guc, 0) \ > param(int, guc_log_level, -1) \ > param(char *, guc_firmware_path, NULL) \ >
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index e0fd10c..c1a98f3 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -77,6 +77,7 @@ obj-y += gt/ gt-y += \ gt/intel_breadcrumbs.o \ gt/intel_context.o \ + gt/intel_deu.o \ gt/intel_engine_cs.o \ gt/intel_engine_heartbeat.o \ gt/intel_engine_pm.o \ diff --git a/drivers/gpu/drm/i915/gt/intel_deu.c b/drivers/gpu/drm/i915/gt/intel_deu.c new file mode 100644 index 0000000..6c5b01c --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_deu.c @@ -0,0 +1,104 @@ +/* + * Copyright © 2019 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Authors: + * Ankit Navik <ankit.p.navik@intel.com> + */ + +/** + * DOC: Dynamic EU Control (DEU) + * + * DEU tries to re-configure EU allocation during runtime by predictive load + * calculation of command queue to gain power saving. + * It is transparent to user space and completely handled in the kernel. + */ + +#include "intel_deu.h" +#include "i915_drv.h" +#include "gem/i915_gem_context.h" + +/* + * Anything above threshold is considered as HIGH load, less is considered + * as LOW load and equal is considered as MEDIUM load. + * + * The threshold value of three active requests pending. + */ +#define PENDING_THRESHOLD_MEDIUM 3 + +#define SLACK_TIMER_NSEC 1000000 /* Timer range in nano second */ + +enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer) +{ + struct drm_i915_private *dev_priv = + container_of(hrtimer, typeof(*dev_priv), pred_timer); + struct i915_gem_context *ctx; + enum gem_load_type load_type; + unsigned int req_pending; + + list_for_each_entry(ctx, &dev_priv->gem.contexts.list, link) { + req_pending = atomic_read(&ctx->req_cnt); + + /* + * Transitioning to low state whenever pending request is zero + * would cause vacillation between low and high state. + */ + if (req_pending == 0) + continue; + + if (req_pending > PENDING_THRESHOLD_MEDIUM) + load_type = LOAD_TYPE_HIGH; + else if (req_pending == PENDING_THRESHOLD_MEDIUM) + load_type = LOAD_TYPE_MEDIUM; + else + load_type = LOAD_TYPE_LOW; + + i915_gem_context_set_load_type(ctx, load_type); + } + + hrtimer_forward_now(hrtimer, + ms_to_ktime(dev_priv->predictive_load_enable)); + + return HRTIMER_RESTART; +} + +/** + * intel_deu_init - Initialize dynamic EU + * @dev_priv: i915 device instance + * + * This function is called at driver load + */ +void intel_deu_init(struct drm_i915_private *dev_priv) +{ + dev_priv->predictive_load_enable = i915_modparams.deu_enable; + hrtimer_init(&dev_priv->pred_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + dev_priv->pred_timer.function = predictive_load_cb; + + if (dev_priv->predictive_load_enable) { + if (!hrtimer_active(&dev_priv->pred_timer)) + hrtimer_start_range_ns(&dev_priv->pred_timer, + ms_to_ktime(dev_priv->predictive_load_enable), + SLACK_TIMER_NSEC, + HRTIMER_MODE_REL_PINNED); + } else { + hrtimer_cancel(&dev_priv->pred_timer); + } +} diff --git a/drivers/gpu/drm/i915/gt/intel_deu.h b/drivers/gpu/drm/i915/gt/intel_deu.h new file mode 100644 index 0000000..3b4b16f --- /dev/null +++ b/drivers/gpu/drm/i915/gt/intel_deu.h @@ -0,0 +1,31 @@ +/* + * Copyright © 2019 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + */ + +#ifndef __INTEL_DEU_H__ +#define __INTEL_DEU_H__ + +struct drm_i915_private; + +void intel_deu_init(struct drm_i915_private *dev_priv); + +#endif /* __INTEL_DEU_H__ */ diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3064ddf..5553537 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1002,6 +1002,8 @@ struct drm_i915_private { /* optimal slice/subslice/EU configration state */ struct i915_sseu_optimum_config *opt_config; + /* protects predictive load state */ + struct hrtimer pred_timer; int predictive_load_enable; unsigned int fsb_freq, mem_freq, is_ddr3; @@ -1768,6 +1770,8 @@ long i915_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg); #endif extern const struct dev_pm_ops i915_pm_ops; +extern enum hrtimer_restart predictive_load_cb(struct hrtimer *hrtimer); + int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent); void i915_driver_remove(struct drm_i915_private *i915); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 61395b0..ee711ce 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -45,6 +45,7 @@ #include "gem/i915_gem_context.h" #include "gem/i915_gem_ioctls.h" #include "gem/i915_gem_pm.h" +#include "gt/intel_deu.h" #include "gt/intel_context.h" #include "gt/intel_engine_user.h" #include "gt/intel_gt.h" @@ -1416,6 +1417,9 @@ void i915_gem_init_early(struct drm_i915_private *dev_priv) i915_gem_init__mm(dev_priv); spin_lock_init(&dev_priv->fb_tracking.lock); + + /* Dynamic EU timer initialization for predictive load */ + intel_deu_init(dev_priv); } void i915_gem_cleanup_early(struct drm_i915_private *dev_priv) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 1dd1f36..a5a3a6e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -101,6 +101,10 @@ i915_param_named_unsafe(disable_power_well, int, 0400, i915_param_named_unsafe(enable_ips, int, 0600, "Enable IPS (default: true)"); +i915_param_named_unsafe(deu_enable, int, 0600, + "Enable dynamic EU control for power savings " + "(0=disable deu predictive timer [default], 150=optimal deu predictive timer)"); + i915_param_named(fastboot, int, 0600, "Try to skip unnecessary mode sets at boot time " "(0=disabled, 1=enabled) " diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 31b88f2..cf0903b 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -54,6 +54,7 @@ struct drm_printer; param(int, disable_power_well, -1) \ param(int, enable_ips, 1) \ param(int, invert_brightness, 0) \ + param(int, deu_enable, 0) \ param(int, enable_guc, 0) \ param(int, guc_log_level, -1) \ param(char *, guc_firmware_path, NULL) \
High resolution timer is used for predictive governor to control eu/slice/subslice based on workloads. param is provided to enable/disable/update timer configuration V2: * Fix code style. * Move predictive_load_timer into a drm_i915_private structure. * Make generic function to set optimum config. (Tvrtko Ursulin) V3: * Rebase. * Fix race condition for predictive load set. * Add slack to start hrtimer for more power efficient. (Tvrtko Ursulin) V4: * Fix data type and initialization of mutex to protect predictive load state. * Move predictive timer init to i915_gem_init_early. (Tvrtko Ursulin) * Move debugfs to kernel parameter. V5: * Rebase. * Remove mutex for pred_timer V6: * Rebase. * Fix warnings. Cc: Vipin Anand <vipin.anand@intel.com> Signed-off-by: Ankit Navik <ankit.p.navik@intel.com> --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/gt/intel_deu.c | 104 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_deu.h | 31 +++++++++++ drivers/gpu/drm/i915/i915_drv.h | 4 ++ drivers/gpu/drm/i915/i915_gem.c | 4 ++ drivers/gpu/drm/i915/i915_params.c | 4 ++ drivers/gpu/drm/i915/i915_params.h | 1 + 7 files changed, 149 insertions(+) create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.c create mode 100644 drivers/gpu/drm/i915/gt/intel_deu.h