| Message ID | 1454411190-15721-2-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 02, 2016 at 11:06:19AM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > This is for callers who want micro-second precision but are not > waiting from the atomic context. linux/time.h provides us with USEC_PER_MSEC that would help to break up these large numbers better for human consumption. 2000 -> 2*USEC_PER_SEC 10 -> 10*USEC_PER_MSEC Maybe: #define wait_for_seconds(x) ((x)*USEC_PER_SEC) #define wait_for_milliseconds(x) ((x)*USEC_PER_MSEC) if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, wait_for_seconds(5) /* timeout */, wait_for_millseconds(10) /* interval */)) > @@ -55,7 +55,7 @@ > break; \ > } \ > if ((W) && drm_can_sleep()) { \ Note after the atomic conversion, we can also do the !atomic assert here and kill the drm_can_sleep() check -Chris
On 02/02/16 11:06, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > This is for callers who want micro-second precision but are not > waiting from the atomic context. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 3 +-- > drivers/gpu/drm/i915/intel_drv.h | 9 +++++---- > drivers/gpu/drm/i915/intel_psr.c | 2 +- > 3 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e2bea710614f..fb8a76ec6ade 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1788,11 +1788,10 @@ static void wait_panel_status(struct intel_dp *intel_dp, > I915_READ(pp_stat_reg), > I915_READ(pp_ctrl_reg)); > > - if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) { > + if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000000, 10000)) > DRM_ERROR("Panel status timeout: status %08x control %08x\n", > I915_READ(pp_stat_reg), > I915_READ(pp_ctrl_reg)); > - } > > DRM_DEBUG_KMS("Wait complete\n"); > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f620023ed134..779d17a9fcce 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -45,8 +45,8 @@ > * having timed out, since the timeout could be due to preemption or similar and > * we've never had a chance to check the condition before the timeout. > */ > -#define _wait_for(COND, MS, W) ({ \ > - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \ > +#define _wait_for(COND, US, W) ({ \ > + unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1; \ > int ret__ = 0; \ > while (!(COND)) { \ > if (time_after(jiffies, timeout__)) { \ > @@ -55,7 +55,7 @@ > break; \ > } \ > if ((W) && drm_can_sleep()) { \ > - usleep_range((W)*1000, (W)*2000); \ > + usleep_range((W), (W)*2); \ > } else { \ > cpu_relax(); \ > } \ > @@ -63,7 +63,8 @@ > ret__; \ > }) > > -#define wait_for(COND, MS) _wait_for(COND, MS, 1) > +#define wait_for(COND, MS) _wait_for(COND, (MS) * 1000, 1000) > +#define wait_for_us(COND, US) _wait_for(COND, US, 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) Don't these latter two macros need to be altered since the underlying _wait_for() now takes times in us rather than ms? .Dave. > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 9ccff3011523..e12377963839 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -492,7 +492,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp) > > /* Wait till PSR is idle */ > if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) & > - EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10)) > + EDP_PSR_STATUS_STATE_MASK) == 0, 2000000, 10000)) > DRM_ERROR("Timed out waiting for PSR Idle State\n"); > > dev_priv->psr.active = false; >
On 02/02/16 13:35, Dave Gordon wrote: > On 02/02/16 11:06, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> This is for callers who want micro-second precision but are not >> waiting from the atomic context. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 3 +-- >> drivers/gpu/drm/i915/intel_drv.h | 9 +++++---- >> drivers/gpu/drm/i915/intel_psr.c | 2 +- >> 3 files changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c >> index e2bea710614f..fb8a76ec6ade 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -1788,11 +1788,10 @@ static void wait_panel_status(struct intel_dp >> *intel_dp, >> I915_READ(pp_stat_reg), >> I915_READ(pp_ctrl_reg)); >> >> - if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) { >> + if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000000, >> 10000)) >> DRM_ERROR("Panel status timeout: status %08x control %08x\n", >> I915_READ(pp_stat_reg), >> I915_READ(pp_ctrl_reg)); >> - } >> >> DRM_DEBUG_KMS("Wait complete\n"); >> } >> diff --git a/drivers/gpu/drm/i915/intel_drv.h >> b/drivers/gpu/drm/i915/intel_drv.h >> index f620023ed134..779d17a9fcce 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -45,8 +45,8 @@ >> * having timed out, since the timeout could be due to preemption or >> similar and >> * we've never had a chance to check the condition before the timeout. >> */ >> -#define _wait_for(COND, MS, W) ({ \ >> - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \ >> +#define _wait_for(COND, US, W) ({ \ >> + unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1; \ >> int ret__ = 0; \ >> while (!(COND)) { \ >> if (time_after(jiffies, timeout__)) { \ >> @@ -55,7 +55,7 @@ >> break; \ >> } \ >> if ((W) && drm_can_sleep()) { \ >> - usleep_range((W)*1000, (W)*2000); \ >> + usleep_range((W), (W)*2); \ >> } else { \ >> cpu_relax(); \ >> } \ >> @@ -63,7 +63,8 @@ >> ret__; \ >> }) >> >> -#define wait_for(COND, MS) _wait_for(COND, MS, 1) >> +#define wait_for(COND, MS) _wait_for(COND, (MS) * 1000, 1000) >> +#define wait_for_us(COND, US) _wait_for(COND, US, 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) > > Don't these latter two macros need to be altered since the underlying > _wait_for() now takes times in us rather than ms? Yes you are right, thanks! (Consequence of re-ordering and re-basing.) Regards, Tvrtko
On 02/02/16 11:57, Chris Wilson wrote: > On Tue, Feb 02, 2016 at 11:06:19AM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> This is for callers who want micro-second precision but are not >> waiting from the atomic context. > > linux/time.h provides us with USEC_PER_MSEC that would help to break up > these large numbers better for human consumption. > > 2000 -> 2*USEC_PER_SEC > 10 -> 10*USEC_PER_MSEC > > Maybe: > > #define wait_for_seconds(x) ((x)*USEC_PER_SEC) > #define wait_for_milliseconds(x) ((x)*USEC_PER_MSEC) > > if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, > wait_for_seconds(5) /* timeout */, > wait_for_millseconds(10) /* interval */)) There are only two callers where it would be a bit interesting so it just feels like needless change to me at the moment. Better to keep the established conventions for these two macros. >> @@ -55,7 +55,7 @@ >> break; \ >> } \ >> if ((W) && drm_can_sleep()) { \ > > Note after the atomic conversion, we can also do the !atomic assert here > and kill the drm_can_sleep() check Noted. Maybe I'll put a comment somewhere. Regards, Tvrtko
On Tue, Feb 02, 2016 at 02:04:55PM +0000, Tvrtko Ursulin wrote: > > > On 02/02/16 11:57, Chris Wilson wrote: > >On Tue, Feb 02, 2016 at 11:06:19AM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>This is for callers who want micro-second precision but are not > >>waiting from the atomic context. > > > >linux/time.h provides us with USEC_PER_MSEC that would help to break up > >these large numbers better for human consumption. > > > >2000 -> 2*USEC_PER_SEC > >10 -> 10*USEC_PER_MSEC > > > >Maybe: > > > >#define wait_for_seconds(x) ((x)*USEC_PER_SEC) > >#define wait_for_milliseconds(x) ((x)*USEC_PER_MSEC) > > > >if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, > > wait_for_seconds(5) /* timeout */, > > wait_for_millseconds(10) /* interval */)) > > There are only two callers where it would be a bit interesting so it > just feels like needless change to me at the moment. Better to keep > the established conventions for these two macros. I am a bit more concerned that there are any users of _wait_for() outside of the header. -Chris
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index e2bea710614f..fb8a76ec6ade 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1788,11 +1788,10 @@ static void wait_panel_status(struct intel_dp *intel_dp, I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); - if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) { + if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000000, 10000)) DRM_ERROR("Panel status timeout: status %08x control %08x\n", I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); - } DRM_DEBUG_KMS("Wait complete\n"); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f620023ed134..779d17a9fcce 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -45,8 +45,8 @@ * having timed out, since the timeout could be due to preemption or similar and * we've never had a chance to check the condition before the timeout. */ -#define _wait_for(COND, MS, W) ({ \ - unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1; \ +#define _wait_for(COND, US, W) ({ \ + unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1; \ int ret__ = 0; \ while (!(COND)) { \ if (time_after(jiffies, timeout__)) { \ @@ -55,7 +55,7 @@ break; \ } \ if ((W) && drm_can_sleep()) { \ - usleep_range((W)*1000, (W)*2000); \ + usleep_range((W), (W)*2); \ } else { \ cpu_relax(); \ } \ @@ -63,7 +63,8 @@ ret__; \ }) -#define wait_for(COND, MS) _wait_for(COND, MS, 1) +#define wait_for(COND, MS) _wait_for(COND, (MS) * 1000, 1000) +#define wait_for_us(COND, US) _wait_for(COND, US, 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) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 9ccff3011523..e12377963839 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -492,7 +492,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp) /* Wait till PSR is idle */ if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) & - EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10)) + EDP_PSR_STATUS_STATE_MASK) == 0, 2000000, 10000)) DRM_ERROR("Timed out waiting for PSR Idle State\n"); dev_priv->psr.active = false;