| Message ID | 20170410150207.4125-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 10, 2017 at 04:02:05PM +0100, Chris Wilson wrote: > Allow the caller to use the fast_timeout_us to specify how long to wait > within the atomic section, rather than transparently switching to a > sleeping loop for larger values. This is required as some callsites may > need a long wait and are in an atomic section. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index eb38392a2435..53c8457869f6 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -1601,7 +1601,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv, > * > * Otherwise, the wait will timeout after @slow_timeout_ms milliseconds. > * For atomic context @slow_timeout_ms must be zero and @fast_timeout_us > - * must be not larger than 10 microseconds. > + * must be not larger than 20,0000 microseconds. So we don't enforce this any more ? > * > * Note that this routine assumes the caller holds forcewake asserted, it is > * not suitable for very long waits. See intel_wait_for_register() if you > @@ -1623,16 +1623,17 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv, > int ret; > > /* Catch any overuse of this function */ > - might_sleep_if(fast_timeout_us > 10 || slow_timeout_ms); > + might_sleep_if(slow_timeout_ms); > > - if (fast_timeout_us > 10) > - ret = _wait_for(done, fast_timeout_us, 10); > - else > + ret = -ETIMEDOUT; > + if (fast_timeout_us && fast_timeout_us < 20000) > ret = _wait_for_atomic(done, fast_timeout_us, 0); > if (ret) > ret = wait_for(done, slow_timeout_ms); What if someone passes fast=20001us and slow=0ms ? Maybe like this: might_sleep_if(fast_timeout_us > 20000 || slow_timeout_ms); if (fast_timeout_us && fast_timeout_us < 20000) ret = _wait_for_atomic(done, fast_timeout_us, 0); else slow_timeout_ms += fast_timeout_us/1000; if (slow_timeout_ms) ret = wait_for(done, slow_timeout_ms); -Michal
On Mon, Apr 10, 2017 at 05:20:32PM +0200, Michal Wajdeczko wrote: > On Mon, Apr 10, 2017 at 04:02:05PM +0100, Chris Wilson wrote: > > Allow the caller to use the fast_timeout_us to specify how long to wait > > within the atomic section, rather than transparently switching to a > > sleeping loop for larger values. This is required as some callsites may > > need a long wait and are in an atomic section. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > > index eb38392a2435..53c8457869f6 100644 > > --- a/drivers/gpu/drm/i915/intel_uncore.c > > +++ b/drivers/gpu/drm/i915/intel_uncore.c > > @@ -1601,7 +1601,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv, > > * > > * Otherwise, the wait will timeout after @slow_timeout_ms milliseconds. > > * For atomic context @slow_timeout_ms must be zero and @fast_timeout_us > > - * must be not larger than 10 microseconds. > > + * must be not larger than 20,0000 microseconds. > > So we don't enforce this any more ? I considered adding a GEM_BUG_ON / GEM_WARN_ON, but really just not listening to them and hitting the sleep was my answer to sillyness. Atomic spinning for 20ms is not acceptable. :| > > * Note that this routine assumes the caller holds forcewake asserted, it is > > * not suitable for very long waits. See intel_wait_for_register() if you > > @@ -1623,16 +1623,17 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv, > > int ret; > > > > /* Catch any overuse of this function */ > > - might_sleep_if(fast_timeout_us > 10 || slow_timeout_ms); > > + might_sleep_if(slow_timeout_ms); > > > > - if (fast_timeout_us > 10) > > - ret = _wait_for(done, fast_timeout_us, 10); > > - else > > + ret = -ETIMEDOUT; > > + if (fast_timeout_us && fast_timeout_us < 20000) > > ret = _wait_for_atomic(done, fast_timeout_us, 0); > > if (ret) > > ret = wait_for(done, slow_timeout_ms); > > What if someone passes fast=20001us and slow=0ms ? > Maybe like this: > > might_sleep_if(fast_timeout_us > 20000 || slow_timeout_ms); > > if (fast_timeout_us && fast_timeout_us < 20000) > ret = _wait_for_atomic(done, fast_timeout_us, 0); > else > slow_timeout_ms += fast_timeout_us/1000; No. Just shoot it down in review. -Chris
On Mon, Apr 10, 2017 at 04:23:15PM -0000, Patchwork wrote: > == Series Details == > > Series: series starting with [1/3] drm/i915: Stop second guessing the caller for intel_uncore_wait_for_register() (rev2) > URL : https://patchwork.freedesktop.org/series/22786/ > State : warning > > == Summary == > > Series 22786v2 Series without cover letter > https://patchwork.freedesktop.org/api/1.0/series/22786/revisions/2/mbox/ > > Test core_auth: > Subgroup basic-auth: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test drv_getparams_basic: > Subgroup basic-subslice-total: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test drv_module_reload: > Subgroup basic-no-display: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-reload: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-reload-final: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_basic: > Subgroup create-close: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_busy: > Subgroup basic-hang-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_close_race: > Subgroup basic-threads: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_cs_tlb: > Subgroup basic-default: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_ctx_basic: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_ctx_create: > Subgroup basic-files: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_ctx_param: > Subgroup basic-default: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_ctx_switch: > Subgroup basic-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-default-heavy: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_exec_basic: > Subgroup gtt-render: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup readonly-bsd: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup readonly-default: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup readonly-render: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > Test gem_exec_create: > Subgroup basic: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_exec_fence: > Subgroup await-hang-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-await-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-wait-default: > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup nb-await-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-uc: > dmesg-warn -> FAIL (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-batch-kernel-default-wb: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-uc-pro-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-uc-prw-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-uc-ro-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-uc-rw-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-uc-set-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > dmesg-warn -> PASS (fi-snb-2520m) fdo#100643 > Subgroup basic-wb-pro-default: > dmesg-warn -> PASS (fi-snb-2600) fdo#100643 > WARNING: Long output truncated Ping? -Chris
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index eb38392a2435..53c8457869f6 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -1601,7 +1601,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv, * * Otherwise, the wait will timeout after @slow_timeout_ms milliseconds. * For atomic context @slow_timeout_ms must be zero and @fast_timeout_us - * must be not larger than 10 microseconds. + * must be not larger than 20,0000 microseconds. * * Note that this routine assumes the caller holds forcewake asserted, it is * not suitable for very long waits. See intel_wait_for_register() if you @@ -1623,16 +1623,17 @@ int __intel_wait_for_register_fw(struct drm_i915_private *dev_priv, int ret; /* Catch any overuse of this function */ - might_sleep_if(fast_timeout_us > 10 || slow_timeout_ms); + might_sleep_if(slow_timeout_ms); - if (fast_timeout_us > 10) - ret = _wait_for(done, fast_timeout_us, 10); - else + ret = -ETIMEDOUT; + if (fast_timeout_us && fast_timeout_us < 20000) ret = _wait_for_atomic(done, fast_timeout_us, 0); if (ret) ret = wait_for(done, slow_timeout_ms); + if (out_value) *out_value = reg_value; + return ret; #undef done }
Allow the caller to use the fast_timeout_us to specify how long to wait within the atomic section, rather than transparently switching to a sleeping loop for larger values. This is required as some callsites may need a long wait and are in an atomic section. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)