| Message ID | 1454411190-15721-3-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 02, 2016 at 11:06:20AM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Looks like this code does not need to wait atomically since it > otherwise takes the mutex. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 304fc9637026..a7530cf612d7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk) > val |= LCPLL_CD_SOURCE_FCLK; > I915_WRITE(LCPLL_CTL, val); > > - if (wait_for_atomic_us(I915_READ(LCPLL_CTL) & > - LCPLL_CD_SOURCE_FCLK_DONE, 1)) > + if (wait_for_us(I915_READ(LCPLL_CTL) & > + LCPLL_CD_SOURCE_FCLK_DONE, 1)) Thinking about wait_for_seconds and friends from before, does this read better as if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE, wait_for_microseconds(1)) ? -Chris
On 02/02/16 12:00, Chris Wilson wrote: > On Tue, Feb 02, 2016 at 11:06:20AM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Looks like this code does not need to wait atomically since it >> otherwise takes the mutex. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 304fc9637026..a7530cf612d7 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk) >> val |= LCPLL_CD_SOURCE_FCLK; >> I915_WRITE(LCPLL_CTL, val); >> >> - if (wait_for_atomic_us(I915_READ(LCPLL_CTL) & >> - LCPLL_CD_SOURCE_FCLK_DONE, 1)) >> + if (wait_for_us(I915_READ(LCPLL_CTL) & >> + LCPLL_CD_SOURCE_FCLK_DONE, 1)) > > Thinking about wait_for_seconds and friends from before, does this read > better as > > if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE, > wait_for_microseconds(1)) > ? > -Chris No, not really, because it confuses a function (or macro) that tests for a condition with one that converts between different units of time, yet they both have names beginning wait_for... And people might expect a function called wait_for_microseconds() to actually wait for some number of microseconds! There's an ambiguity in English anyway e.g. "wait for a bus" (event) vs "wait for 10 minutes" (duration). But there's no need to propagate natural-language foolishness into code. What we're really trying to express is ({ while (!condition && !timedout) delay(interval) resultis timedout; }) but there's still a question about, why should the granularity of the delay be related to the precision of the timespec? With these patches, we've got a situation where if the timeout is specified in us, the delay between rechecking the condition is 1us, but if the timeout is in ms, there's a 1ms recheck interval. .Dave.
On Tue, Feb 02, 2016 at 02:08:44PM +0000, Dave Gordon wrote: > On 02/02/16 12:00, Chris Wilson wrote: > >On Tue, Feb 02, 2016 at 11:06:20AM +0000, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >>Looks like this code does not need to wait atomically since it > >>otherwise takes the mutex. > >> > >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >>--- > >> drivers/gpu/drm/i915/intel_display.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>index 304fc9637026..a7530cf612d7 100644 > >>--- a/drivers/gpu/drm/i915/intel_display.c > >>+++ b/drivers/gpu/drm/i915/intel_display.c > >>@@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk) > >> val |= LCPLL_CD_SOURCE_FCLK; > >> I915_WRITE(LCPLL_CTL, val); > >> > >>- if (wait_for_atomic_us(I915_READ(LCPLL_CTL) & > >>- LCPLL_CD_SOURCE_FCLK_DONE, 1)) > >>+ if (wait_for_us(I915_READ(LCPLL_CTL) & > >>+ LCPLL_CD_SOURCE_FCLK_DONE, 1)) > > > >Thinking about wait_for_seconds and friends from before, does this read > >better as > > > >if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE, > > wait_for_microseconds(1)) > >? > >-Chris > > No, not really, because it confuses a function (or macro) that tests > for a condition with one that converts between different units of > time, yet they both have names beginning wait_for... > > And people might expect a function called wait_for_microseconds() to > actually wait for some number of microseconds! So, suggest an improvement. ms_to_us, msec_to_usec, MSEC_TO_USEC, SEC_TO_USEC, timeout_in_microseconds, timeout_in_millseconds, timeout_in_seconds. But since this is specific to the wait_for interface, they need to reflect that, so wait_timeout_in_seconds, but that is too general (now we are treading on wait_event territory), so back to wait_for_seconds_timeout()? On the other hand, msecs_to_jiffies(). My question is whether having a universal wait_for() macro is preferrable over having both wait_for() and wait_for_us() and passing in million+ numbers. > There's an ambiguity in English anyway e.g. "wait for a bus" (event) > vs "wait for 10 minutes" (duration). But there's no need to > propagate natural-language foolishness into code. Oh, you are misinterpreting the name. It was meant to be wait_for (the interface name) + _units. > What we're really trying to express is > > ({ while (!condition && !timedout) > delay(interval) > resultis timedout; }) > > but there's still a question about, why should the granularity of > the delay be related to the precision of the timespec? With these > patches, we've got a situation where if the timeout is specified in > us, the delay between rechecking the condition is 1us, but if the > timeout is in ms, there's a 1ms recheck interval. Why not? The original intent was for checking status changes across tens of microseconds where we wanted a coarse granularity and there was no impetus for high accuracy. Then this was copy-pasted into a new routine for microsecond resoluation, and again there was no demand for fine control over the interval. Nor has anyone really looked at whether 1us is a good figure for the "high resolution" waits - except for a couple of instances where a different value has been seemingly arbitrarily chosen. We have talked about using timeout/N or an exponential interval. If I had chosen wait_until() as the name, that would have saved a few brain cells. But I can't see a way to avoid the name proliferation for _atomic, _with_interval like wait_event. At the least, we can trim that down by avoiding encoding the units into the macro name. We also probably don't yet need the _with_interval, so the only question is whether it is possible to have a single macro suitable for waits from 1us to 1s (and a second macro wait_for_atomic that operates in microseconds for good reason). ret = wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE, 1); and ret = wait_for((I915_READ(dpll_reg) & port_mask) == expected_mask, secs_to_usecs(1)); Is it possible to have a single dtrt macro to cover such a range? -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 304fc9637026..a7530cf612d7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk) val |= LCPLL_CD_SOURCE_FCLK; I915_WRITE(LCPLL_CTL, val); - if (wait_for_atomic_us(I915_READ(LCPLL_CTL) & - LCPLL_CD_SOURCE_FCLK_DONE, 1)) + if (wait_for_us(I915_READ(LCPLL_CTL) & + LCPLL_CD_SOURCE_FCLK_DONE, 1)) DRM_ERROR("Switching to FCLK failed\n"); val = I915_READ(LCPLL_CTL); @@ -9788,8 +9788,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk) val &= ~LCPLL_CD_SOURCE_FCLK; I915_WRITE(LCPLL_CTL, val); - if (wait_for_atomic_us((I915_READ(LCPLL_CTL) & - LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1)) + if (wait_for_us((I915_READ(LCPLL_CTL) & + LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1)) DRM_ERROR("Switching back to LCPLL failed\n"); mutex_lock(&dev_priv->rps.hw_lock);