[02/12] drm/i915: Do not wait atomically for display clocks
diff mbox

Message ID 1454411190-15721-3-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin Feb. 2, 2016, 11:06 a.m. UTC
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(-)

Comments

Chris Wilson Feb. 2, 2016, noon UTC | #1
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
Dave Gordon Feb. 2, 2016, 2:08 p.m. UTC | #2
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.
Chris Wilson Feb. 2, 2016, 3:39 p.m. UTC | #3
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

Patch
diff mbox

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);