| Message ID | 1463499808-3335-4-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, May 17, 2016 at 06:43:24PM +0300, Mika Kuoppala wrote: > Usually the condition we are after appears within very short time. > Spin few times before going into sleep. With this approximately > half of the wait_for in init path will take the fast path without > sleeping. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> Some numbers on how much time this saved would be nice. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 488141929a7a..c225605c727c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -39,7 +39,7 @@ > #include <drm/drm_atomic.h> > > /** > - * _wait_for_ms - magic (register) wait macro > + * __wait_for_ms - magic (register) wait macro > * > * Does the right thing for modeset paths when run under kdgb or similar atomic > * contexts. Note that it's important that we check the condition again after > @@ -50,17 +50,22 @@ > * drm_can_sleep() can be removed and in_atomic()/!in_atomic() asserts > * added. > */ > -#define _wait_for_ms(COND, TIMEOUT_MS, SLEEP_US) ({ \ > +#define __wait_for_ms(COND, TIMEOUT_MS, SLEEP_US, SPIN_COUNT) ({ \ > const unsigned long timeout__ = \ > jiffies + msecs_to_jiffies(TIMEOUT_MS) + 1; \ > + unsigned int c__ = 0; \ > int ret__ = 0; \ > + \ > while (!(COND)) { \ > if (time_after(jiffies, timeout__)) { \ > if (!(COND)) \ > ret__ = -ETIMEDOUT; \ > break; \ > } \ > - if ((SLEEP_US) && drm_can_sleep()) { \ > + \ > + if (++c__ > (SPIN_COUNT) && \ > + (SLEEP_US) && \ > + drm_can_sleep()) { \ > usleep_range((SLEEP_US), (SLEEP_US) * 2); \ > } else { \ > cpu_relax(); \ > @@ -69,7 +74,8 @@ > ret__; \ > }) > > -#define wait_for(COND, MS) _wait_for_ms((COND), (MS), 1 * USEC_PER_MSEC) > +#define wait_for(COND, MS) __wait_for_ms((COND), (MS), 1 * USEC_PER_MSEC, 5) > +#define _wait_for_ms(COND, MS, US) __wait_for_ms((COND), (MS), (US), 5) > > /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */ > #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT) > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, May 17, 2016 at 06:43:24PM +0300, Mika Kuoppala wrote: > Usually the condition we are after appears within very short time. > Spin few times before going into sleep. With this approximately > half of the wait_for in init path will take the fast path without > sleeping. > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 488141929a7a..c225605c727c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -39,7 +39,7 @@ > #include <drm/drm_atomic.h> > > /** > - * _wait_for_ms - magic (register) wait macro > + * __wait_for_ms - magic (register) wait macro > * > * Does the right thing for modeset paths when run under kdgb or similar atomic > * contexts. Note that it's important that we check the condition again after > @@ -50,17 +50,22 @@ > * drm_can_sleep() can be removed and in_atomic()/!in_atomic() asserts > * added. > */ > -#define _wait_for_ms(COND, TIMEOUT_MS, SLEEP_US) ({ \ > +#define __wait_for_ms(COND, TIMEOUT_MS, SLEEP_US, SPIN_COUNT) ({ \ > const unsigned long timeout__ = \ > jiffies + msecs_to_jiffies(TIMEOUT_MS) + 1; \ > + unsigned int c__ = 0; \ > int ret__ = 0; \ > + \ > while (!(COND)) { \ > if (time_after(jiffies, timeout__)) { \ > if (!(COND)) \ > ret__ = -ETIMEDOUT; \ > break; \ > } \ > - if ((SLEEP_US) && drm_can_sleep()) { \ > + \ > + if (++c__ > (SPIN_COUNT) && \ > + (SLEEP_US) && \ > + drm_can_sleep()) { \ > usleep_range((SLEEP_US), (SLEEP_US) * 2); \ > } else { \ > cpu_relax(); \ > @@ -69,7 +74,8 @@ > ret__; \ > }) > > -#define wait_for(COND, MS) _wait_for_ms((COND), (MS), 1 * USEC_PER_MSEC) > +#define wait_for(COND, MS) __wait_for_ms((COND), (MS), 1 * USEC_PER_MSEC, 5) > +#define _wait_for_ms(COND, MS, US) __wait_for_ms((COND), (MS), (US), 5) Why 5? I did some histrograms on wait_for() on my BSW (just looking at modeset paths really), and that showed ~66% of the time we got it right on the first check, and ~33% was completed after one sleep iteration. The sleep duration didn't make much of a difference here, so the current 1-2 msec is probably not at all optimal. I also tested doing a double check intially, and IIRC that reduced the second bin by about half. I didn't check whether further spinnign would have helped more. > > /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */ > #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT) > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, May 18, 2016 at 10:20:22AM +0200, Daniel Vetter wrote: > On Tue, May 17, 2016 at 06:43:24PM +0300, Mika Kuoppala wrote: > > Usually the condition we are after appears within very short time. > > Spin few times before going into sleep. With this approximately > > half of the wait_for in init path will take the fast path without > > sleeping. > > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > Some numbers on how much time this saved would be nice. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > --- > > drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++---- > > 1 file changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 488141929a7a..c225605c727c 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -39,7 +39,7 @@ > > #include <drm/drm_atomic.h> > > > > /** > > - * _wait_for_ms - magic (register) wait macro > > + * __wait_for_ms - magic (register) wait macro > > * > > * Does the right thing for modeset paths when run under kdgb or similar atomic > > * contexts. Note that it's important that we check the condition again after > > @@ -50,17 +50,22 @@ > > * drm_can_sleep() can be removed and in_atomic()/!in_atomic() asserts > > * added. > > */ > > -#define _wait_for_ms(COND, TIMEOUT_MS, SLEEP_US) ({ \ > > +#define __wait_for_ms(COND, TIMEOUT_MS, SLEEP_US, SPIN_COUNT) ({ \ > > const unsigned long timeout__ = \ > > jiffies + msecs_to_jiffies(TIMEOUT_MS) + 1; \ > > + unsigned int c__ = 0; \ > > int ret__ = 0; \ > > + \ > > while (!(COND)) { \ > > if (time_after(jiffies, timeout__)) { \ > > if (!(COND)) \ > > ret__ = -ETIMEDOUT; \ > > break; \ > > } \ > > - if ((SLEEP_US) && drm_can_sleep()) { \ > > + \ > > + if (++c__ > (SPIN_COUNT) && \ > > + (SLEEP_US) && \ Could we kill SLEEP_US here in patch 0? -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > [ text/plain ] > On Wed, May 18, 2016 at 10:20:22AM +0200, Daniel Vetter wrote: >> On Tue, May 17, 2016 at 06:43:24PM +0300, Mika Kuoppala wrote: >> > Usually the condition we are after appears within very short time. >> > Spin few times before going into sleep. With this approximately >> > half of the wait_for in init path will take the fast path without >> > sleeping. >> > >> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> >> Some numbers on how much time this saved would be nice. >> >> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> >> > --- >> > drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++---- >> > 1 file changed, 10 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> > index 488141929a7a..c225605c727c 100644 >> > --- a/drivers/gpu/drm/i915/intel_drv.h >> > +++ b/drivers/gpu/drm/i915/intel_drv.h >> > @@ -39,7 +39,7 @@ >> > #include <drm/drm_atomic.h> >> > >> > /** >> > - * _wait_for_ms - magic (register) wait macro >> > + * __wait_for_ms - magic (register) wait macro >> > * >> > * Does the right thing for modeset paths when run under kdgb or similar atomic >> > * contexts. Note that it's important that we check the condition again after >> > @@ -50,17 +50,22 @@ >> > * drm_can_sleep() can be removed and in_atomic()/!in_atomic() asserts >> > * added. >> > */ >> > -#define _wait_for_ms(COND, TIMEOUT_MS, SLEEP_US) ({ \ >> > +#define __wait_for_ms(COND, TIMEOUT_MS, SLEEP_US, SPIN_COUNT) ({ \ >> > const unsigned long timeout__ = \ >> > jiffies + msecs_to_jiffies(TIMEOUT_MS) + 1; \ >> > + unsigned int c__ = 0; \ >> > int ret__ = 0; \ >> > + \ >> > while (!(COND)) { \ >> > if (time_after(jiffies, timeout__)) { \ >> > if (!(COND)) \ >> > ret__ = -ETIMEDOUT; \ >> > break; \ >> > } \ >> > - if ((SLEEP_US) && drm_can_sleep()) { \ >> > + \ >> > + if (++c__ > (SPIN_COUNT) && \ >> > + (SLEEP_US) && \ > > Could we kill SLEEP_US here in patch 0? Patch 1? Yes we could. -Mika > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 488141929a7a..c225605c727c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -39,7 +39,7 @@ #include <drm/drm_atomic.h> /** - * _wait_for_ms - magic (register) wait macro + * __wait_for_ms - magic (register) wait macro * * Does the right thing for modeset paths when run under kdgb or similar atomic * contexts. Note that it's important that we check the condition again after @@ -50,17 +50,22 @@ * drm_can_sleep() can be removed and in_atomic()/!in_atomic() asserts * added. */ -#define _wait_for_ms(COND, TIMEOUT_MS, SLEEP_US) ({ \ +#define __wait_for_ms(COND, TIMEOUT_MS, SLEEP_US, SPIN_COUNT) ({ \ const unsigned long timeout__ = \ jiffies + msecs_to_jiffies(TIMEOUT_MS) + 1; \ + unsigned int c__ = 0; \ int ret__ = 0; \ + \ while (!(COND)) { \ if (time_after(jiffies, timeout__)) { \ if (!(COND)) \ ret__ = -ETIMEDOUT; \ break; \ } \ - if ((SLEEP_US) && drm_can_sleep()) { \ + \ + if (++c__ > (SPIN_COUNT) && \ + (SLEEP_US) && \ + drm_can_sleep()) { \ usleep_range((SLEEP_US), (SLEEP_US) * 2); \ } else { \ cpu_relax(); \ @@ -69,7 +74,8 @@ ret__; \ }) -#define wait_for(COND, MS) _wait_for_ms((COND), (MS), 1 * USEC_PER_MSEC) +#define wait_for(COND, MS) __wait_for_ms((COND), (MS), 1 * USEC_PER_MSEC, 5) +#define _wait_for_ms(COND, MS, US) __wait_for_ms((COND), (MS), (US), 5) /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */ #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
Usually the condition we are after appears within very short time. Spin few times before going into sleep. With this approximately half of the wait_for in init path will take the fast path without sleeping. Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)