[01/12] drm/i915: Add wait_for_us
diff mbox

Message ID 1454411190-15721-2-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>

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

Comments

Chris Wilson Feb. 2, 2016, 11:57 a.m. UTC | #1
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
Dave Gordon Feb. 2, 2016, 1:35 p.m. UTC | #2
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;
>
Tvrtko Ursulin Feb. 2, 2016, 1:58 p.m. UTC | #3
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
Tvrtko Ursulin Feb. 2, 2016, 2:04 p.m. UTC | #4
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
Chris Wilson Feb. 2, 2016, 3:43 p.m. UTC | #5
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

Patch
diff mbox

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;