| Message ID | 20180522150830.569-4-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Chris Wilson (2018-05-22 16:08:30) > During suspend we want to flush out all active contexts and their > rendering. To do so we queue a request from the kernel's context, once > we know that request is done, we know the GPU is completely idle. To > speed up that switch bump the GPU clocks. > > Switching to the kernel context prior to idling is also used to enforce > a barrier before changing OA properties, and when evicting active > rendering from the global GTT. All cases where we do want to > race-to-idle. > > v2: Limit the boosting to only the switch before suspend. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: David Weinehall <david.weinehall@linux.intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> #v1 > Tested-by: David Weinehall <david.weinehall@linux.intel.com> #v1 > --- > drivers/gpu/drm/i915/i915_gem.c | 11 ++++++----- > drivers/gpu/drm/i915/i915_gem_context.c | 15 ++++++++++++++- > drivers/gpu/drm/i915/i915_gem_context.h | 4 +++- > drivers/gpu/drm/i915/i915_gem_evict.c | 2 +- > 4 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index a81aa124af26..37a6c9ec5d60 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3513,7 +3513,7 @@ i915_gem_idle_work_handler(struct work_struct *work) > * idle that implies a round trip through the retire worker). > */ > mutex_lock(&dev_priv->drm.struct_mutex); > - i915_gem_switch_to_kernel_context(dev_priv); > + i915_gem_switch_to_kernel_context(dev_priv, 0); > mutex_unlock(&dev_priv->drm.struct_mutex); > > /* > @@ -4958,7 +4958,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) > * not rely on its state. > */ > if (!i915_terminally_wedged(&dev_priv->gpu_error)) { > - ret = i915_gem_switch_to_kernel_context(dev_priv); > + ret = i915_gem_switch_to_kernel_context(dev_priv, > + I915_SWITCH_BOOST); > if (ret) > goto err_unlock; > > @@ -5043,7 +5044,7 @@ void i915_gem_resume(struct drm_i915_private *i915) > intel_uc_resume(i915); > > /* Always reload a context for powersaving. */ > - if (i915_gem_switch_to_kernel_context(i915)) > + if (i915_gem_switch_to_kernel_context(i915, 0)) > goto err_wedged; > > out_unlock: > @@ -5238,7 +5239,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > goto err_active; > } > > - err = i915_gem_switch_to_kernel_context(i915); > + err = i915_gem_switch_to_kernel_context(i915, 0); > if (err) > goto err_active; > > @@ -5304,7 +5305,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) > * request, ensure we are pointing at the kernel context and > * then remove it. > */ > - if (WARN_ON(i915_gem_switch_to_kernel_context(i915))) > + if (WARN_ON(i915_gem_switch_to_kernel_context(i915, 0))) > goto out_ctx; > > if (WARN_ON(i915_gem_wait_for_idle(i915, I915_WAIT_LOCKED))) > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 3fe1212b0f7e..dce1a3d9f5e6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -605,7 +605,8 @@ static bool engine_has_idle_kernel_context(struct intel_engine_cs *engine) > return intel_engine_has_kernel_context(engine); > } > > -int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915) > +int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915, > + unsigned int flags) > { > struct intel_engine_cs *engine; > enum intel_engine_id id; > @@ -636,6 +637,18 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915) > I915_FENCE_GFP); > } > > + /* > + * "Race-to-idle". > + * > + * Switching to the kernel context is often used a synchronous > + * step prior to idling, e.g. in suspend for flushing all > + * current operations to memory before sleeping. These we > + * want to complete as quickly as possible to avoid prolonged > + * stalls, so allow the gpu to boost to maximum clocks. > + */ > + if (flags & I915_SWITCH_BOOST) > + gen6_rps_boost(rq, NULL); > + Hmm, I thinking that adding this bit to wait_for_idle works better with the parking to kernel context on idling. As after that patch, we are less likely to need to add a switch-to-kernel-context on suspend and so miss boosting the outstanding work. -Chris
Hi Chris,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on next-20180517]
[cannot apply to drm-intel/for-linux-next v4.17-rc6 v4.17-rc5 v4.17-rc4 v4.17-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Prepare-GEM-for-suspend-earlier/20180524-224509
config: x86_64-randconfig-x015-201820 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/gpu/drm/i915/selftests/igt_flush_test.c: In function 'igt_flush_test':
>> drivers/gpu/drm/i915/selftests/igt_flush_test.c:61:6: error: too few arguments to function 'i915_gem_switch_to_kernel_context'
i915_gem_switch_to_kernel_context(i915)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/gpu/drm/i915/selftests/../intel_lrc.h:28:0,
from drivers/gpu/drm/i915/selftests/../i915_drv.h:63,
from drivers/gpu/drm/i915/selftests/igt_flush_test.c:7:
drivers/gpu/drm/i915/selftests/../i915_gem_context.h:297:5: note: declared here
int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/i915_gem_switch_to_kernel_context +61 drivers/gpu/drm/i915/selftests/igt_flush_test.c
98dc0454 Chris Wilson 2018-05-05 48
98dc0454 Chris Wilson 2018-05-05 49 #define wedge_on_timeout(W, DEV, TIMEOUT) \
98dc0454 Chris Wilson 2018-05-05 50 for (__init_wedge((W), (DEV), (TIMEOUT), __builtin_return_address(0)); \
98dc0454 Chris Wilson 2018-05-05 51 (W)->i915; \
98dc0454 Chris Wilson 2018-05-05 52 __fini_wedge((W)))
98dc0454 Chris Wilson 2018-05-05 53
98dc0454 Chris Wilson 2018-05-05 54 int igt_flush_test(struct drm_i915_private *i915, unsigned int flags)
98dc0454 Chris Wilson 2018-05-05 55 {
98dc0454 Chris Wilson 2018-05-05 56 struct wedge_me w;
98dc0454 Chris Wilson 2018-05-05 57
98dc0454 Chris Wilson 2018-05-05 58 cond_resched();
98dc0454 Chris Wilson 2018-05-05 59
b9777c6f Chris Wilson 2018-05-09 60 if (flags & I915_WAIT_LOCKED &&
b9777c6f Chris Wilson 2018-05-09 @61 i915_gem_switch_to_kernel_context(i915)) {
:::::: The code at line 61 was first introduced by commit
:::::: b9777c6f86ac8c21f82211ab982ca48302042ede drm/i915/selftests: Only switch to kernel context when locked
:::::: TO: Chris Wilson <chris@chris-wilson.co.uk>
:::::: CC: Chris Wilson <chris@chris-wilson.co.uk>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Chris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20180517]
[cannot apply to drm-intel/for-linux-next v4.17-rc6 v4.17-rc5 v4.17-rc4 v4.17-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Prepare-GEM-for-suspend-earlier/20180524-224509
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/i915/selftests/igt_flush_test.c:61:46: sparse: not enough arguments for function i915_gem_switch_to_kernel_context
drivers/gpu/drm/i915/selftests/igt_flush_test.c: In function 'igt_flush_test':
drivers/gpu/drm/i915/selftests/igt_flush_test.c:61:6: error: too few arguments to function 'i915_gem_switch_to_kernel_context'
i915_gem_switch_to_kernel_context(i915)) {
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/gpu/drm/i915/selftests/../intel_lrc.h:28:0,
from drivers/gpu/drm/i915/selftests/../i915_drv.h:63,
from drivers/gpu/drm/i915/selftests/igt_flush_test.c:7:
drivers/gpu/drm/i915/selftests/../i915_gem_context.h:297:5: note: declared here
int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915,
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +61 drivers/gpu/drm/i915/selftests/igt_flush_test.c
98dc0454 Chris Wilson 2018-05-05 48
98dc0454 Chris Wilson 2018-05-05 49 #define wedge_on_timeout(W, DEV, TIMEOUT) \
98dc0454 Chris Wilson 2018-05-05 50 for (__init_wedge((W), (DEV), (TIMEOUT), __builtin_return_address(0)); \
98dc0454 Chris Wilson 2018-05-05 51 (W)->i915; \
98dc0454 Chris Wilson 2018-05-05 52 __fini_wedge((W)))
98dc0454 Chris Wilson 2018-05-05 53
98dc0454 Chris Wilson 2018-05-05 54 int igt_flush_test(struct drm_i915_private *i915, unsigned int flags)
98dc0454 Chris Wilson 2018-05-05 55 {
98dc0454 Chris Wilson 2018-05-05 56 struct wedge_me w;
98dc0454 Chris Wilson 2018-05-05 57
98dc0454 Chris Wilson 2018-05-05 58 cond_resched();
98dc0454 Chris Wilson 2018-05-05 59
b9777c6f Chris Wilson 2018-05-09 60 if (flags & I915_WAIT_LOCKED &&
b9777c6f Chris Wilson 2018-05-09 @61 i915_gem_switch_to_kernel_context(i915)) {
:::::: The code at line 61 was first introduced by commit
:::::: b9777c6f86ac8c21f82211ab982ca48302042ede drm/i915/selftests: Only switch to kernel context when locked
:::::: TO: Chris Wilson <chris@chris-wilson.co.uk>
:::::: CC: Chris Wilson <chris@chris-wilson.co.uk>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index a81aa124af26..37a6c9ec5d60 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3513,7 +3513,7 @@ i915_gem_idle_work_handler(struct work_struct *work) * idle that implies a round trip through the retire worker). */ mutex_lock(&dev_priv->drm.struct_mutex); - i915_gem_switch_to_kernel_context(dev_priv); + i915_gem_switch_to_kernel_context(dev_priv, 0); mutex_unlock(&dev_priv->drm.struct_mutex); /* @@ -4958,7 +4958,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) * not rely on its state. */ if (!i915_terminally_wedged(&dev_priv->gpu_error)) { - ret = i915_gem_switch_to_kernel_context(dev_priv); + ret = i915_gem_switch_to_kernel_context(dev_priv, + I915_SWITCH_BOOST); if (ret) goto err_unlock; @@ -5043,7 +5044,7 @@ void i915_gem_resume(struct drm_i915_private *i915) intel_uc_resume(i915); /* Always reload a context for powersaving. */ - if (i915_gem_switch_to_kernel_context(i915)) + if (i915_gem_switch_to_kernel_context(i915, 0)) goto err_wedged; out_unlock: @@ -5238,7 +5239,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) goto err_active; } - err = i915_gem_switch_to_kernel_context(i915); + err = i915_gem_switch_to_kernel_context(i915, 0); if (err) goto err_active; @@ -5304,7 +5305,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915) * request, ensure we are pointing at the kernel context and * then remove it. */ - if (WARN_ON(i915_gem_switch_to_kernel_context(i915))) + if (WARN_ON(i915_gem_switch_to_kernel_context(i915, 0))) goto out_ctx; if (WARN_ON(i915_gem_wait_for_idle(i915, I915_WAIT_LOCKED))) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 3fe1212b0f7e..dce1a3d9f5e6 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -605,7 +605,8 @@ static bool engine_has_idle_kernel_context(struct intel_engine_cs *engine) return intel_engine_has_kernel_context(engine); } -int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915) +int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915, + unsigned int flags) { struct intel_engine_cs *engine; enum intel_engine_id id; @@ -636,6 +637,18 @@ int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915) I915_FENCE_GFP); } + /* + * "Race-to-idle". + * + * Switching to the kernel context is often used a synchronous + * step prior to idling, e.g. in suspend for flushing all + * current operations to memory before sleeping. These we + * want to complete as quickly as possible to avoid prolonged + * stalls, so allow the gpu to boost to maximum clocks. + */ + if (flags & I915_SWITCH_BOOST) + gen6_rps_boost(rq, NULL); + /* * Force a flush after the switch to ensure that all rendering * and operations prior to switching to the kernel context hits diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h index c3262b4dd2ee..9d9140fa91ac 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.h +++ b/drivers/gpu/drm/i915/i915_gem_context.h @@ -303,7 +303,9 @@ int i915_gem_context_open(struct drm_i915_private *i915, void i915_gem_context_close(struct drm_file *file); int i915_switch_context(struct i915_request *rq); -int i915_gem_switch_to_kernel_context(struct drm_i915_private *dev_priv); +int i915_gem_switch_to_kernel_context(struct drm_i915_private *i915, + unsigned int flags); +#define I915_SWITCH_BOOST BIT(0) void i915_gem_context_release(struct kref *ctx_ref); struct i915_gem_context * diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index 54814a196ee4..7d0f34b587ab 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -63,7 +63,7 @@ static int ggtt_flush(struct drm_i915_private *i915) * the hopes that we can then remove contexts and the like only * bound by their active reference. */ - err = i915_gem_switch_to_kernel_context(i915); + err = i915_gem_switch_to_kernel_context(i915, 0); if (err) return err;