| Message ID | 1454332655-30400-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Feb 01, 2016 at 01:17:35PM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Currently the wait_for_atomic_us only allows for a millisecond > granularity which is not nice towards callers requesting small > micro-second waits. > > Re-implement it so micro-second granularity is really supported > and not just in the name of the macro. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > Danger - this might break things which currently work by accident! > --- > drivers/gpu/drm/i915/intel_drv.h | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f620023ed134..9e8a1202194c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -63,10 +63,25 @@ > ret__; \ > }) > > +#define _wait_for_atomic(COND, US) ({ \ > + unsigned long end__; \ > + int ret__ = 0; \ > + get_cpu(); \ Hmm, by virtue of its name (and original intent), we are expected to be in an atomic context and could just do a BUG_ON(!in_atomic()) to catch misuse. Since the removal of the panic modeset, all callers outside of intel_uncore.c are definitely abusing this and we would be better to use a usleep[_range]() variant instead. -Chris
On 01/02/16 13:30, Chris Wilson wrote: > On Mon, Feb 01, 2016 at 01:17:35PM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Currently the wait_for_atomic_us only allows for a millisecond >> granularity which is not nice towards callers requesting small >> micro-second waits. >> >> Re-implement it so micro-second granularity is really supported >> and not just in the name of the macro. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> Danger - this might break things which currently work by accident! >> --- >> drivers/gpu/drm/i915/intel_drv.h | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index f620023ed134..9e8a1202194c 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -63,10 +63,25 @@ >> ret__; \ >> }) >> >> +#define _wait_for_atomic(COND, US) ({ \ >> + unsigned long end__; \ >> + int ret__ = 0; \ >> + get_cpu(); \ > > Hmm, by virtue of its name (and original intent), we are expected to be > in an atomic context and could just do a BUG_ON(!in_atomic()) to catch > misuse. Since the removal of the panic modeset, all callers outside of > intel_uncore.c are definitely abusing this and we would be better to use > a usleep[_range]() variant instead. I considered a WARN_ON_ONCE and a BUILD_BUG_ON for very long waits but chickened out on both. I'll respin with a WARN_ON_ONCE(!in_atomic)) to start with. Regards, Tvrtko
On 01/02/16 14:15, Tvrtko Ursulin wrote: > > On 01/02/16 13:30, Chris Wilson wrote: >> On Mon, Feb 01, 2016 at 01:17:35PM +0000, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Currently the wait_for_atomic_us only allows for a millisecond >>> granularity which is not nice towards callers requesting small >>> micro-second waits. >>> >>> Re-implement it so micro-second granularity is really supported >>> and not just in the name of the macro. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> Danger - this might break things which currently work by accident! >>> --- >>> drivers/gpu/drm/i915/intel_drv.h | 21 ++++++++++++++++++--- >>> 1 file changed, 18 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_drv.h >>> b/drivers/gpu/drm/i915/intel_drv.h >>> index f620023ed134..9e8a1202194c 100644 >>> --- a/drivers/gpu/drm/i915/intel_drv.h >>> +++ b/drivers/gpu/drm/i915/intel_drv.h >>> @@ -63,10 +63,25 @@ >>> ret__; \ >>> }) >>> >>> +#define _wait_for_atomic(COND, US) ({ \ >>> + unsigned long end__; \ >>> + int ret__ = 0; \ >>> + get_cpu(); \ >> >> Hmm, by virtue of its name (and original intent), we are expected to be >> in an atomic context and could just do a BUG_ON(!in_atomic()) to catch >> misuse. Since the removal of the panic modeset, all callers outside of >> intel_uncore.c are definitely abusing this and we would be better to use >> a usleep[_range]() variant instead. > > I considered a WARN_ON_ONCE and a BUILD_BUG_ON for very long waits but > chickened out on both. > > I'll respin with a WARN_ON_ONCE(!in_atomic)) to start with. Can't really do that it seems since in_atomic() will be always false on non-fully-preemptible kernels. Could do the current cpu comparison trick to catch false timeouts due callers from non-atomic sections but not sure if it is worth it. So it looks like manual audit of call sites to me. Or find a time source with micro-second resolution which does not go backwards on CPU migrations? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f620023ed134..9e8a1202194c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -63,10 +63,25 @@ ret__; \ }) +#define _wait_for_atomic(COND, US) ({ \ + unsigned long end__; \ + int ret__ = 0; \ + get_cpu(); \ + end__ = (local_clock() >> 10) + (US) + 1; \ + while (!(COND)) { \ + if (time_after((unsigned long)(local_clock() >> 10), end__)) { \ + ret__ = -ETIMEDOUT; \ + break; \ + } \ + cpu_relax(); \ + } \ + put_cpu(); \ + ret__; \ +}) + #define wait_for(COND, MS) _wait_for(COND, MS, 1) -#define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0) -#define wait_for_atomic_us(COND, US) _wait_for((COND), \ - DIV_ROUND_UP((US), 1000), 0) +#define wait_for_atomic(COND, MS) _wait_for_atomic(COND, (MS * 1000)) +#define wait_for_atomic_us(COND, US) _wait_for_atomic((COND), (US)) #define KHz(x) (1000 * (x)) #define MHz(x) KHz(1000 * (x))