[PATCH 1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

[PATCH 1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Required to enable correct wait_for_atomic checks.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/Kconfig.debug | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
index 8f404103341d..43400adc95db 100644
--- a/drivers/gpu/drm/i915/Kconfig.debug
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -18,6 +18,7 @@ config DRM_I915_WERROR
 config DRM_I915_DEBUG
         bool "Enable additional driver debugging"
         depends on DRM_I915
+        select PREEMPT_COUNT
         default n
         help
           Choose this option to turn on extra driver debugging that may affect
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

usleep_range is not recommended for waits shorten than 10us.

Make the wait_for_us use the atomic variant for such waits.

To do so we need to disable the !in_atomic warning for such uses
and also disable preemption since the macro is written in a way
to only be safe to be used in atomic context (local_clock() and
no second COND check after the timeout).

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3156d8df7921..e21bf6e6f119 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -69,20 +69,21 @@
 })
 
 #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
-#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
 
 /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
 #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
-# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
 #else
-# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
 #endif
 
-#define _wait_for_atomic(COND, US) ({ \
+#define _wait_for_atomic(COND, US, ATOMIC) ({ \
 	unsigned long end__; \
 	int ret__ = 0; \
-	_WAIT_FOR_ATOMIC_CHECK; \
-	BUILD_BUG_ON((US) > 50000); \
+	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
+	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
+	if (!(ATOMIC)) \
+		preempt_disable(); \
 	end__ = (local_clock() >> 10) + (US) + 1; \
 	while (!(COND)) { \
 		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
@@ -97,11 +98,23 @@
 		} \
 		cpu_relax(); \
 	} \
+	if (!(ATOMIC)) \
+		preempt_enable(); \
 	ret__; \
 })
 
-#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 wait_for_us(COND, US) \
+({ \
+	int ret__; \
+	if ((US) > 10) \
+		ret__ = _wait_for((COND), (US), 10); \
+	else \
+		ret__ = _wait_for_atomic((COND), (US), 0); \
+	ret__; \
+})
+
+#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
+#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
 
 #define KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Imre Deak]

On ti, 2016-06-28 at 12:51 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> usleep_range is not recommended for waits shorten than 10us.
> 
> Make the wait_for_us use the atomic variant for such waits.
> 
> To do so we need to disable the !in_atomic warning for such uses
> and also disable preemption since the macro is written in a way
> to only be safe to be used in atomic context (local_clock() and
> no second COND check after the timeout).
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3156d8df7921..e21bf6e6f119 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -69,20 +69,21 @@
>  })
>  
>  #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
> -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
>  
>  /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>  #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
>  #else
> -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
>  #endif
>  
> -#define _wait_for_atomic(COND, US) ({ \
> +#define _wait_for_atomic(COND, US, ATOMIC) ({ \
>  	unsigned long end__; \
>  	int ret__ = 0; \
> -	_WAIT_FOR_ATOMIC_CHECK; \
> -	BUILD_BUG_ON((US) > 50000); \
> +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
> +	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
> +	if (!(ATOMIC)) \
> +		preempt_disable(); \

Disabling preemption for this purpose (scheduling a timeout) could be
frowned upon, although for 10us may be not an issue. Another
possibility would be to use cpu_clock() instead which would have some
overhead in case of scheduling away from the initial CPU, but we'd only
incur it for the non-atomic <10us case, so would be negligible imo.
You'd also have to re-check the condition with that solution.

Also could you explain how can we ignore hard IRQs as hinted by the
comment in _wait_for_atomic()?

>  	end__ = (local_clock() >> 10) + (US) + 1; \
>  	while (!(COND)) { \
>  		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
> @@ -97,11 +98,23 @@
>  		} \
>  		cpu_relax(); \
>  	} \
> +	if (!(ATOMIC)) \
> +		preempt_enable(); \
>  	ret__; \
>  })
>  
> -#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 wait_for_us(COND, US) \
> +({ \
> +	int ret__; \
> +	if ((US) > 10) \
> +		ret__ = _wait_for((COND), (US), 10); \
> +	else \
> +		ret__ = _wait_for_atomic((COND), (US), 0); \
> +	ret__; \
> +})
> +
> +#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
> +#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
>  
>  #define KHz(x) (1000 * (x))
>  #define MHz(x) KHz(1000 * (x))
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Chris Wilson]

On Tue, Jun 28, 2016 at 12:51:50PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> usleep_range is not recommended for waits shorten than 10us.
> 
> Make the wait_for_us use the atomic variant for such waits.
> 
> To do so we need to disable the !in_atomic warning for such uses
> and also disable preemption since the macro is written in a way
> to only be safe to be used in atomic context (local_clock() and
> no second COND check after the timeout).
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3156d8df7921..e21bf6e6f119 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -69,20 +69,21 @@
>  })
>  
>  #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
> -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
>  
>  /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>  #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
>  #else
> -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
>  #endif
>  
> -#define _wait_for_atomic(COND, US) ({ \
> +#define _wait_for_atomic(COND, US, ATOMIC) ({ \

__busywait_for
__busyspin_for

(Thinking __usleep_until and maybe __busyspin_until)

>  	unsigned long end__; \
>  	int ret__ = 0; \
> -	_WAIT_FOR_ATOMIC_CHECK; \
> -	BUILD_BUG_ON((US) > 50000); \
> +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
> +	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
> +	if (!(ATOMIC)) \
> +		preempt_disable(); \

>  	end__ = (local_clock() >> 10) + (US) + 1; \
>  	while (!(COND)) { \
>  		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
> @@ -97,11 +98,23 @@
>  		} \

if (!ATOMIC && need_resched())
	break;

?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging
URL   : https://patchwork.freedesktop.org/series/9226/
State : success

== Summary ==

Series 9226v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/1/mbox

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                fail       -> PASS       (ro-byt-n2820)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:2   dfail:1   fail:2   skip:22 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-byt-n2820     total:229  pass:179  dwarn:0   dfail:1   fail:4   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:22   pass:0    dwarn:7   dfail:0   fail:0   skip:14 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 
ro-bsw-n3050 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1318/

3a50b42 drm-intel-nightly: 2016y-06m-28d-11h-06m-59s UTC integration manifest
552511a drm/i915: Use atomic waits for short non-atomic ones
ad4618c drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

On 28/06/16 13:19, Imre Deak wrote:
> On ti, 2016-06-28 at 12:51 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> usleep_range is not recommended for waits shorten than 10us.
>>
>> Make the wait_for_us use the atomic variant for such waits.
>>
>> To do so we need to disable the !in_atomic warning for such uses
>> and also disable preemption since the macro is written in a way
>> to only be safe to be used in atomic context (local_clock() and
>> no second COND check after the timeout).
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
>>   1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 3156d8df7921..e21bf6e6f119 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -69,20 +69,21 @@
>>   })
>>
>>   #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
>> -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
>>
>>   /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>>   #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
>> -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
>> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
>>   #else
>> -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
>> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
>>   #endif
>>
>> -#define _wait_for_atomic(COND, US) ({ \
>> +#define _wait_for_atomic(COND, US, ATOMIC) ({ \
>>   	unsigned long end__; \
>>   	int ret__ = 0; \
>> -	_WAIT_FOR_ATOMIC_CHECK; \
>> -	BUILD_BUG_ON((US) > 50000); \
>> +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
>> +	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
>> +	if (!(ATOMIC)) \
>> +		preempt_disable(); \
>
> Disabling preemption for this purpose (scheduling a timeout) could be
> frowned upon, although for 10us may be not an issue. Another

Possibly, but I don't see how to otherwise do it.

And about the number itself - I chose 10us just because usleep_range is 
not recommended for <10us due setup overhead.

> possibility would be to use cpu_clock() instead which would have some
> overhead in case of scheduling away from the initial CPU, but we'd only
> incur it for the non-atomic <10us case, so would be negligible imo.
> You'd also have to re-check the condition with that solution.

How would you implement it with cpu_clock? What would you do when 
re-scheduled?

> Also could you explain how can we ignore hard IRQs as hinted by the
> comment in _wait_for_atomic()?

Hm, in retrospect it does not look safe. Upside that after your fixes 
from today it will be, since all remaining callers are with interrupts 
disabled. And downside that the patch from this thread is not safe then 
and would need the condition put back in. Possibly only in the !ATOMIC 
case but that might be too fragile for the future.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

[Chris Wilson]

On Tue, Jun 28, 2016 at 12:51:49PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Required to enable correct wait_for_atomic checks.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Imre Deak]

On ti, 2016-06-28 at 14:29 +0100, Tvrtko Ursulin wrote:
> On 28/06/16 13:19, Imre Deak wrote:
> > On ti, 2016-06-28 at 12:51 +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > 
> > > usleep_range is not recommended for waits shorten than 10us.
> > > 
> > > Make the wait_for_us use the atomic variant for such waits.
> > > 
> > > To do so we need to disable the !in_atomic warning for such uses
> > > and also disable preemption since the macro is written in a way
> > > to only be safe to be used in atomic context (local_clock() and
> > > no second COND check after the timeout).
> > > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
> > >   1 file changed, 21 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3156d8df7921..e21bf6e6f119 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -69,20 +69,21 @@
> > >   })
> > > 
> > >   #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
> > > -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
> > > 
> > >   /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
> > >   #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> > > -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
> > > +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
> > >   #else
> > > -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
> > > +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
> > >   #endif
> > > 
> > > -#define _wait_for_atomic(COND, US) ({ \
> > > +#define _wait_for_atomic(COND, US, ATOMIC) ({ \
> > >   	unsigned long end__; \
> > >   	int ret__ = 0; \
> > > -	_WAIT_FOR_ATOMIC_CHECK; \
> > > -	BUILD_BUG_ON((US) > 50000); \
> > > +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
> > > +	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
> > > +	if (!(ATOMIC)) \
> > > +		preempt_disable(); \
> > 
> > Disabling preemption for this purpose (scheduling a timeout) could be
> > frowned upon, although for 10us may be not an issue. Another
> 
> Possibly, but I don't see how to otherwise do it.
> 
> And about the number itself - I chose 10us just because usleep_range is 
> not recommended for <10us due setup overhead.
> 
> > possibility would be to use cpu_clock() instead which would have some
> > overhead in case of scheduling away from the initial CPU, but we'd only
> > incur it for the non-atomic <10us case, so would be negligible imo.
> > You'd also have to re-check the condition with that solution.
> 
> How would you implement it with cpu_clock? What would you do when 
> re-scheduled?

By calculating the expiry in the beginning with cpu_clock()
using raw_smp_processor_id() and then calling cpu_clock() in
time_after() with the same CPU id. cpu_clock() would then internally
handle the scheduling away scenario.

> > Also could you explain how can we ignore hard IRQs as hinted by the
> > comment in _wait_for_atomic()?
> 
> Hm, in retrospect it does not look safe. Upside that after your fixes 
> from today it will be, since all remaining callers are with interrupts 
> disabled.

Well, except for the GuC path, but that's for a 10ms timeout, so
probably doesn't matter (or else we have a bigger problem).

> And downside that the patch from this thread is not safe then 
> and would need the condition put back in. Possibly only in the !ATOMIC 
> case but that might be too fragile for the future.

I'd say we'd need the extra check at least whenever hard IRQs are not
disabled. Even then there could be NMIs or some other background stuff
(ME) that could be a problem. OTOH we'd incur the overhead from the
extra check only in the exceptional timeout case, so I think doing it
in all cases wouldn't be a big problem.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Chris Wilson]

On Tue, Jun 28, 2016 at 02:29:33PM +0100, Tvrtko Ursulin wrote:
> How would you implement it with cpu_clock? What would you do when
> re-scheduled?

unsigned long base;
int cpu;
int ret;

preempt_disable();
cpu = smp_processor_id();
base = local_clock() >> 10;
for (;;) {
	u64 now = local_clock() >> 10;
	preempt_enable();

	if (COND) {
		ret = 0;
		break;
	}

	if (now - base >= timeout) {
		ret = -ETIMEOUT;
		break;
	}
	
	cpu_relax();

	preempt_disable();
	if (unlikely(cpu != smp_processor_id()) {
		timeout -= now - base;
		cpu = smp_processor_id();
		base = local_clock() >> 10;
	}
}
ret;

Borrowed from udelay()
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Chris Wilson]

On Tue, Jun 28, 2016 at 02:55:28PM +0100, Chris Wilson wrote:
> On Tue, Jun 28, 2016 at 02:29:33PM +0100, Tvrtko Ursulin wrote:
> > How would you implement it with cpu_clock? What would you do when
> > re-scheduled?
> 
> unsigned long base;
> int cpu;
> int ret;
> 
> preempt_disable();
> cpu = smp_processor_id();
> base = local_clock() >> 10;
> for (;;) {
> 	u64 now = local_clock() >> 10;
> 	preempt_enable();
> 
> 	if (COND) {
> 		ret = 0;
> 		break;
> 	}
> 
> 	if (now - base >= timeout) {
> 		ret = -ETIMEOUT;
> 		break;
> 	}
> 	
> 	cpu_relax();
> 
> 	preempt_disable();
> 	if (unlikely(cpu != smp_processor_id()) {
> 		timeout -= now - base;

For this, we should scale everything to ns (u64).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

On 28/06/16 14:53, Imre Deak wrote:
> On ti, 2016-06-28 at 14:29 +0100, Tvrtko Ursulin wrote:
>> On 28/06/16 13:19, Imre Deak wrote:
>>> On ti, 2016-06-28 at 12:51 +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> usleep_range is not recommended for waits shorten than 10us.
>>>>
>>>> Make the wait_for_us use the atomic variant for such waits.
>>>>
>>>> To do so we need to disable the !in_atomic warning for such uses
>>>> and also disable preemption since the macro is written in a way
>>>> to only be safe to be used in atomic context (local_clock() and
>>>> no second COND check after the timeout).
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Imre Deak <imre.deak@intel.com>
>>>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
>>>>    1 file changed, 21 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 3156d8df7921..e21bf6e6f119 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -69,20 +69,21 @@
>>>>    })
>>>>
>>>>    #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
>>>> -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
>>>>
>>>>    /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>>>>    #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
>>>> -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
>>>> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
>>>>    #else
>>>> -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
>>>> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
>>>>    #endif
>>>>
>>>> -#define _wait_for_atomic(COND, US) ({ \
>>>> +#define _wait_for_atomic(COND, US, ATOMIC) ({ \
>>>>    	unsigned long end__; \
>>>>    	int ret__ = 0; \
>>>> -	_WAIT_FOR_ATOMIC_CHECK; \
>>>> -	BUILD_BUG_ON((US) > 50000); \
>>>> +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
>>>> +	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
>>>> +	if (!(ATOMIC)) \
>>>> +		preempt_disable(); \
>>>
>>> Disabling preemption for this purpose (scheduling a timeout) could be
>>> frowned upon, although for 10us may be not an issue. Another
>>
>> Possibly, but I don't see how to otherwise do it.
>>
>> And about the number itself - I chose 10us just because usleep_range is
>> not recommended for <10us due setup overhead.
>>
>>> possibility would be to use cpu_clock() instead which would have some
>>> overhead in case of scheduling away from the initial CPU, but we'd only
>>> incur it for the non-atomic <10us case, so would be negligible imo.
>>> You'd also have to re-check the condition with that solution.
>>
>> How would you implement it with cpu_clock? What would you do when
>> re-scheduled?
>
> By calculating the expiry in the beginning with cpu_clock()
> using raw_smp_processor_id() and then calling cpu_clock() in
> time_after() with the same CPU id. cpu_clock() would then internally
> handle the scheduling away scenario.

Right, but that is also not ideal since if the two cpu_clocks differ the 
running time domain is not identical to the timeout one. Probably would 
not matter but feels hacky.

>>> Also could you explain how can we ignore hard IRQs as hinted by the
>>> comment in _wait_for_atomic()?
>>
>> Hm, in retrospect it does not look safe. Upside that after your fixes
>> from today it will be, since all remaining callers are with interrupts
>> disabled.
>
> Well, except for the GuC path, but that's for a 10ms timeout, so
> probably doesn't matter (or else we have a bigger problem).

I've just sent a patch for that.

>> And downside that the patch from this thread is not safe then
>> and would need the condition put back in. Possibly only in the !ATOMIC
>> case but that might be too fragile for the future.
>
> I'd say we'd need the extra check at least whenever hard IRQs are not
> disabled. Even then there could be NMIs or some other background stuff
> (ME) that could be a problem. OTOH we'd incur the overhead from the
> extra check only in the exceptional timeout case, so I think doing it
> in all cases wouldn't be a big problem.

Yeah I'll put it in.

Regards,

Tvrtko



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

On 28/06/16 15:14, Chris Wilson wrote:
> On Tue, Jun 28, 2016 at 02:55:28PM +0100, Chris Wilson wrote:
>> On Tue, Jun 28, 2016 at 02:29:33PM +0100, Tvrtko Ursulin wrote:
>>> How would you implement it with cpu_clock? What would you do when
>>> re-scheduled?
>>
>> unsigned long base;
>> int cpu;
>> int ret;
>>
>> preempt_disable();
>> cpu = smp_processor_id();
>> base = local_clock() >> 10;
>> for (;;) {
>> 	u64 now = local_clock() >> 10;
>> 	preempt_enable();
>>
>> 	if (COND) {
>> 		ret = 0;
>> 		break;
>> 	}
>>
>> 	if (now - base >= timeout) {
>> 		ret = -ETIMEOUT;
>> 		break;
>> 	}
>> 	
>> 	cpu_relax();
>>
>> 	preempt_disable();
>> 	if (unlikely(cpu != smp_processor_id()) {
>> 		timeout -= now - base;
>
> For this, we should scale everything to ns (u64).

In other words not scale. Is this type of loop more preferable to you 
guys vs how it looked in this original patch?

Only difference is the preempt off section is shorter here, but I don't 
think that is interesting for the atomic waits case.


Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

Hi,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.7-rc5 next-20160628]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Tvrtko-Ursulin/drm-i915-debug-Select-PREEMPT_COUNT-when-enabling-debugging/20160628-200101
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=m68k 

All warnings (new ones prefixed by >>):

warning: (PREEMPT && DRM_I915_DEBUG && DEBUG_ATOMIC_SLEEP) selects PREEMPT_COUNT which has unmet direct dependencies (COLDFIRE)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 36967 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Chris Wilson]

On Tue, Jun 28, 2016 at 03:40:24PM +0100, Tvrtko Ursulin wrote:
> 
> On 28/06/16 15:14, Chris Wilson wrote:
> >On Tue, Jun 28, 2016 at 02:55:28PM +0100, Chris Wilson wrote:
> >>On Tue, Jun 28, 2016 at 02:29:33PM +0100, Tvrtko Ursulin wrote:
> >>>How would you implement it with cpu_clock? What would you do when
> >>>re-scheduled?
> >>
> >>unsigned long base;
> >>int cpu;
> >>int ret;
> >>
> >>preempt_disable();
> >>cpu = smp_processor_id();
> >>base = local_clock() >> 10;
> >>for (;;) {
> >>	u64 now = local_clock() >> 10;
> >>	preempt_enable();
> >>
> >>	if (COND) {
> >>		ret = 0;
> >>		break;
> >>	}
> >>
> >>	if (now - base >= timeout) {
> >>		ret = -ETIMEOUT;
> >>		break;
> >>	}
> >>	
> >>	cpu_relax();
> >>
> >>	preempt_disable();
> >>	if (unlikely(cpu != smp_processor_id()) {
> >>		timeout -= now - base;
> >
> >For this, we should scale everything to ns (u64).
> 
> In other words not scale. Is this type of loop more preferable to
> you guys vs how it looked in this original patch?
> 
> Only difference is the preempt off section is shorter here, but I
> don't think that is interesting for the atomic waits case.

Right, for the current (correct) atomic users we simply don't care. It's
the sleepers where adding a 10us unpreemptible section is likely to be
frowned upon. I wonder if we should even go as far as adding a
cond_resched().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

usleep_range is not recommended for waits shorten than 10us.

Make the wait_for_us use the atomic variant for such waits.

To do so we need to reimplement the _wait_for_atomic macro to
be safe with regards to preemption and interrupts.

v2: Reimplement _wait_for_atomic to be irq and preemption safe.
    (Chris Wilson and Imre Deak)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 46 ++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3156d8df7921..a200fd2be908 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -69,7 +69,6 @@
 })
 
 #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
-#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
 
 /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
 #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
@@ -78,25 +77,44 @@
 # define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
 #endif
 
-#define _wait_for_atomic(COND, US) ({ \
-	unsigned long end__; \
-	int ret__ = 0; \
+#define _wait_for_atomic(COND, US) \
+({ \
+	int cpu, ret, timeout = (US) * 1000; \
+	u64 base; \
 	_WAIT_FOR_ATOMIC_CHECK; \
 	BUILD_BUG_ON((US) > 50000); \
-	end__ = (local_clock() >> 10) + (US) + 1; \
-	while (!(COND)) { \
-		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
-			/* Unlike the regular wait_for(), this atomic variant \
-			 * cannot be preempted (and we'll just ignore the issue\
-			 * of irq interruptions) and so we know that no time \
-			 * has passed since the last check of COND and can \
-			 * immediately report the timeout. \
-			 */ \
-			ret__ = -ETIMEDOUT; \
+	preempt_disable(); \
+	cpu = smp_processor_id(); \
+	base = local_clock(); \
+	for (;;) { \
+		u64 now = local_clock(); \
+		preempt_enable(); \
+		if (COND) { \
+			ret = 0; \
+			break; \
+		} \
+		if (now - base >= timeout) { \
+			ret = -ETIMEDOUT; \
 			break; \
 		} \
 		cpu_relax(); \
+		preempt_disable(); \
+		if (unlikely(cpu != smp_processor_id())) { \
+			timeout -= now - base; \
+			cpu = smp_processor_id(); \
+			base = local_clock(); \
+		} \
 	} \
+	ret; \
+})
+
+#define wait_for_us(COND, US) \
+({ \
+	int ret__; \
+	if ((US) > 10) \
+		ret__ = _wait_for((COND), (US), 10); \
+	else \
+		ret__ = _wait_for_atomic((COND), (US)); \
 	ret__; \
 })
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

usleep_range is not recommended for waits shorten than 10us.

Make the wait_for_us use the atomic variant for such waits.

To do so we need to reimplement the _wait_for_atomic macro to
be safe with regards to preemption and interrupts.

v2: Reimplement _wait_for_atomic to be irq and preemption safe.
    (Chris Wilson and Imre Deak)

v3: Fixed in_atomic check due rebase error.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 56 ++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3156d8df7921..265f76157876 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -69,39 +69,57 @@
 })
 
 #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
-#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
 
 /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
 #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
-# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
 #else
-# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
 #endif
 
-#define _wait_for_atomic(COND, US) ({ \
-	unsigned long end__; \
-	int ret__ = 0; \
-	_WAIT_FOR_ATOMIC_CHECK; \
+#define _wait_for_atomic(COND, US, ATOMIC) \
+({ \
+	int cpu, ret, timeout = (US) * 1000; \
+	u64 base; \
+	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
 	BUILD_BUG_ON((US) > 50000); \
-	end__ = (local_clock() >> 10) + (US) + 1; \
-	while (!(COND)) { \
-		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
-			/* Unlike the regular wait_for(), this atomic variant \
-			 * cannot be preempted (and we'll just ignore the issue\
-			 * of irq interruptions) and so we know that no time \
-			 * has passed since the last check of COND and can \
-			 * immediately report the timeout. \
-			 */ \
-			ret__ = -ETIMEDOUT; \
+	preempt_disable(); \
+	cpu = smp_processor_id(); \
+	base = local_clock(); \
+	for (;;) { \
+		u64 now = local_clock(); \
+		preempt_enable(); \
+		if (COND) { \
+			ret = 0; \
+			break; \
+		} \
+		if (now - base >= timeout) { \
+			ret = -ETIMEDOUT; \
 			break; \
 		} \
 		cpu_relax(); \
+		preempt_disable(); \
+		if (unlikely(cpu != smp_processor_id())) { \
+			timeout -= now - base; \
+			cpu = smp_processor_id(); \
+			base = local_clock(); \
+		} \
 	} \
+	ret; \
+})
+
+#define wait_for_us(COND, US) \
+({ \
+	int ret__; \
+	if ((US) > 10) \
+		ret__ = _wait_for((COND), (US), 10); \
+	else \
+		ret__ = _wait_for_atomic((COND), (US), 0); \
 	ret__; \
 })
 
-#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 wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
+#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
 
 #define KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev2)
URL   : https://patchwork.freedesktop.org/series/9226/
State : warning

== Summary ==

Series 9226v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/2/mbox

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-skl-i5-6260u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-b:
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
                pass       -> SKIP       (fi-skl-i5-6260u)

fi-hsw-i7-4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
fi-skl-i5-6260u  total:229  pass:200  dwarn:1   dfail:0   fail:2   skip:26 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:2   dfail:1   fail:2   skip:22 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:176  dwarn:1   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1323/

3a50b42 drm-intel-nightly: 2016y-06m-28d-11h-06m-59s UTC integration manifest
75007ef drm/i915: Use atomic waits for short non-atomic ones
ad4a4c5 drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev3)
URL   : https://patchwork.freedesktop.org/series/9226/
State : success

== Summary ==

Series 9226v3 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/3/mbox

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:2   dfail:1   fail:2   skip:22 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:176  dwarn:1   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1324/

3a50b42 drm-intel-nightly: 2016y-06m-28d-11h-06m-59s UTC integration manifest
dff9772 drm/i915: Use atomic waits for short non-atomic ones
0e1ff4c drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Use atomic waits for short non-atomic ones

[Imre Deak]

On Tue, 2016-06-28 at 15:38 +0100, Tvrtko Ursulin wrote:
> On 28/06/16 14:53, Imre Deak wrote:
> > On ti, 2016-06-28 at 14:29 +0100, Tvrtko Ursulin wrote:
> > > On 28/06/16 13:19, Imre Deak wrote:
> > > > On ti, 2016-06-28 at 12:51 +0100, Tvrtko Ursulin wrote:
> > > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > > 
> > > > > usleep_range is not recommended for waits shorten than 10us.
> > > > > 
> > > > > Make the wait_for_us use the atomic variant for such waits.
> > > > > 
> > > > > To do so we need to disable the !in_atomic warning for such uses
> > > > > and also disable preemption since the macro is written in a way
> > > > > to only be safe to be used in atomic context (local_clock() and
> > > > > no second COND check after the timeout).
> > > > > 
> > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/intel_drv.h | 29 +++++++++++++++++++++--------
> > > > >    1 file changed, 21 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index 3156d8df7921..e21bf6e6f119 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -69,20 +69,21 @@
> > > > >    })
> > > > > 
> > > > >    #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
> > > > > -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
> > > > > 
> > > > >    /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
> > > > >    #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> > > > > -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
> > > > > +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
> > > > >    #else
> > > > > -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
> > > > > +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
> > > > >    #endif
> > > > > 
> > > > > -#define _wait_for_atomic(COND, US) ({ \
> > > > > +#define _wait_for_atomic(COND, US, ATOMIC) ({ \
> > > > >    	unsigned long end__; \
> > > > >    	int ret__ = 0; \
> > > > > -	_WAIT_FOR_ATOMIC_CHECK; \
> > > > > -	BUILD_BUG_ON((US) > 50000); \
> > > > > +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
> > > > > +	BUILD_BUG_ON((ATOMIC) && (US) > 50000); \
> > > > > +	if (!(ATOMIC)) \
> > > > > +		preempt_disable(); \
> > > > 
> > > > Disabling preemption for this purpose (scheduling a timeout) could be
> > > > frowned upon, although for 10us may be not an issue. Another
> > > 
> > > Possibly, but I don't see how to otherwise do it.
> > > 
> > > And about the number itself - I chose 10us just because usleep_range is
> > > not recommended for <10us due setup overhead.
> > > 
> > > > possibility would be to use cpu_clock() instead which would have some
> > > > overhead in case of scheduling away from the initial CPU, but we'd only
> > > > incur it for the non-atomic <10us case, so would be negligible imo.
> > > > You'd also have to re-check the condition with that solution.
> > > 
> > > How would you implement it with cpu_clock? What would you do when
> > > re-scheduled?
> > 
> > By calculating the expiry in the beginning with cpu_clock()
> > using raw_smp_processor_id() and then calling cpu_clock() in
> > time_after() with the same CPU id. cpu_clock() would then internally
> > handle the scheduling away scenario.
> 
> Right, but that is also not ideal since if the two cpu_clocks differ the 
> running time domain is not identical to the timeout one.

Hm, this is what I meant:

int cpu = raw_smp_processor_id();
end = cpu_clock(cpu) + timeout;
while (!time_after(cpu_clock(cpu), end))
   check condition...

So cpu_clock() would be always called with the same CPU id and hence it
would return timestamps from the same time domain. In case of
scheduling away at any point cpu_clock() would internally detect this
and return the timestamp from the original CPU time domain.

This also has the advantage that in case of a stable, synchronized TSC
- which should be the generic case - cpu_clock() simply translates to
reading the TSC, without disabling pre-emption.

I don't see any problem with Chris' approach either though.

> Probably would not matter but feels hacky.
> 
> > > > Also could you explain how can we ignore hard IRQs as hinted by the
> > > > comment in _wait_for_atomic()?
> > > 
> > > Hm, in retrospect it does not look safe. Upside that after your fixes
> > > from today it will be, since all remaining callers are with interrupts
> > > disabled.
> > 
> > Well, except for the GuC path, but that's for a 10ms timeout, so
> > probably doesn't matter (or else we have a bigger problem).
> 
> I've just sent a patch for that.
> 
> > > And downside that the patch from this thread is not safe then
> > > and would need the condition put back in. Possibly only in the !ATOMIC
> > > case but that might be too fragile for the future.
> > 
> > I'd say we'd need the extra check at least whenever hard IRQs are not
> > disabled. Even then there could be NMIs or some other background stuff
> > (ME) that could be a problem. OTOH we'd incur the overhead from the
> > extra check only in the exceptional timeout case, so I think doing it
> > in all cases wouldn't be a big problem.
> 
> Yeah I'll put it in.
> 
> Regards,
> 
> Tvrtko
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Use atomic waits for short non-atomic ones

[Chris Wilson]

On Tue, Jun 28, 2016 at 05:20:43PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> usleep_range is not recommended for waits shorten than 10us.
> 
> Make the wait_for_us use the atomic variant for such waits.
> 
> To do so we need to reimplement the _wait_for_atomic macro to
> be safe with regards to preemption and interrupts.
> 
> v2: Reimplement _wait_for_atomic to be irq and preemption safe.
>     (Chris Wilson and Imre Deak)
> 
> v3: Fixed in_atomic check due rebase error.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 56 ++++++++++++++++++++++++++--------------
>  1 file changed, 37 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3156d8df7921..265f76157876 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -69,39 +69,57 @@
>  })
>  
>  #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
> -#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
>  
>  /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
>  #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
> -# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
>  #else
> -# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
> +# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
>  #endif
>  
> -#define _wait_for_atomic(COND, US) ({ \
> -	unsigned long end__; \
> -	int ret__ = 0; \
> -	_WAIT_FOR_ATOMIC_CHECK; \
> +#define _wait_for_atomic(COND, US, ATOMIC) \
> +({ \
> +	int cpu, ret, timeout = (US) * 1000; \
> +	u64 base; \
> +	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
>  	BUILD_BUG_ON((US) > 50000); \
> -	end__ = (local_clock() >> 10) + (US) + 1; \
> -	while (!(COND)) { \
> -		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
> -			/* Unlike the regular wait_for(), this atomic variant \
> -			 * cannot be preempted (and we'll just ignore the issue\
> -			 * of irq interruptions) and so we know that no time \
> -			 * has passed since the last check of COND and can \
> -			 * immediately report the timeout. \
> -			 */ \
> -			ret__ = -ETIMEDOUT; \
> +	preempt_disable(); \
> +	cpu = smp_processor_id(); \
> +	base = local_clock(); \
> +	for (;;) { \
> +		u64 now = local_clock(); \
> +		preempt_enable(); \
> +		if (COND) { \
> +			ret = 0; \
> +			break; \
> +		} \
> +		if (now - base >= timeout) { \
> +			ret = -ETIMEDOUT; \
>  			break; \
>  		} \
>  		cpu_relax(); \
> +		preempt_disable(); \
> +		if (unlikely(cpu != smp_processor_id())) { \
> +			timeout -= now - base; \
> +			cpu = smp_processor_id(); \
> +			base = local_clock(); \
> +		} \
>  	} \
> +	ret; \
> +})
> +
> +#define wait_for_us(COND, US) \
> +({ \
> +	int ret__; \

       /* usleep_range() is not recommended for waits shorten than 10us. */ \

> +	if ((US) > 10) \

Do we want __builtin_constant_p(US) to be sure this folds away?

Can you resend this as a new series, I'm 100% sure if CI picked it up?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev3)

[Chris Wilson]

On Tue, Jun 28, 2016 at 05:03:20PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev3)
> URL   : https://patchwork.freedesktop.org/series/9226/
> State : success
> 
> == Summary ==
> 
> Series 9226v3 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/3/mbox
> 
> Test kms_pipe_crc_basic:
>         Subgroup suspend-read-crc-pipe-c:
>                 dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

Should we ask QA to enable CONFIG_I915_DEBUG and spend some time fire
fighting?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Ro.CI.BAT: success for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev3)

[Tvrtko Ursulin]

On 28/06/16 20:59, Chris Wilson wrote:
> On Tue, Jun 28, 2016 at 05:03:20PM -0000, Patchwork wrote:
>> == Series Details ==
>>
>> Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev3)
>> URL   : https://patchwork.freedesktop.org/series/9226/
>> State : success
>>
>> == Summary ==
>>
>> Series 9226v3 Series without cover letter
>> http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/3/mbox
>>
>> Test kms_pipe_crc_basic:
>>          Subgroup suspend-read-crc-pipe-c:
>>                  dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
>
> Should we ask QA to enable CONFIG_I915_DEBUG and spend some time fire
> fighting?

Yes why not, we can always backtrack if it uncovers too much. :)

I will respin with the __builtin_constant_p since I was considering that 
myself.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

usleep_range is not recommended for waits shorten than 10us.

Make the wait_for_us use the atomic variant for such waits.

To do so we need to reimplement the _wait_for_atomic macro to
be safe with regards to preemption and interrupts.

v2: Reimplement _wait_for_atomic to be irq and preemption safe.
    (Chris Wilson and Imre Deak)

v3: Fixed in_atomic check due rebase error.
v4: Build bug on non-constant timeouts.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 57 ++++++++++++++++++++++++++--------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3156d8df7921..21fb296dbba7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -69,39 +69,58 @@
 })
 
 #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
-#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
 
 /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
 #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
-# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
 #else
-# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
 #endif
 
-#define _wait_for_atomic(COND, US) ({ \
-	unsigned long end__; \
-	int ret__ = 0; \
-	_WAIT_FOR_ATOMIC_CHECK; \
+#define _wait_for_atomic(COND, US, ATOMIC) \
+({ \
+	int cpu, ret, timeout = (US) * 1000; \
+	u64 base; \
+	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
 	BUILD_BUG_ON((US) > 50000); \
-	end__ = (local_clock() >> 10) + (US) + 1; \
-	while (!(COND)) { \
-		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
-			/* Unlike the regular wait_for(), this atomic variant \
-			 * cannot be preempted (and we'll just ignore the issue\
-			 * of irq interruptions) and so we know that no time \
-			 * has passed since the last check of COND and can \
-			 * immediately report the timeout. \
-			 */ \
-			ret__ = -ETIMEDOUT; \
+	preempt_disable(); \
+	cpu = smp_processor_id(); \
+	base = local_clock(); \
+	for (;;) { \
+		u64 now = local_clock(); \
+		preempt_enable(); \
+		if (COND) { \
+			ret = 0; \
+			break; \
+		} \
+		if (now - base >= timeout) { \
+			ret = -ETIMEDOUT; \
 			break; \
 		} \
 		cpu_relax(); \
+		preempt_disable(); \
+		if (unlikely(cpu != smp_processor_id())) { \
+			timeout -= now - base; \
+			cpu = smp_processor_id(); \
+			base = local_clock(); \
+		} \
 	} \
+	ret; \
+})
+
+#define wait_for_us(COND, US) \
+({ \
+	int ret__; \
+	BUILD_BUG_ON(!__builtin_constant_p(US)); \
+	if ((US) > 10) \
+		ret__ = _wait_for((COND), (US), 10); \
+	else \
+		ret__ = _wait_for_atomic((COND), (US), 0); \
 	ret__; \
 })
 
-#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 wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
+#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
 
 #define KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev4)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev4)
URL   : https://patchwork.freedesktop.org/series/9226/
State : failure

== Summary ==

Series 9226v4 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/4/mbox

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (ro-byt-n2820)
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i7-5557U)

fi-hsw-i7-4770k  total:229  pass:194  dwarn:0   dfail:0   fail:2   skip:33 
fi-kbl-qkkr      total:229  pass:161  dwarn:29  dfail:0   fail:0   skip:39 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:201  dwarn:2   dfail:1   fail:3   skip:22 
ro-bdw-i7-5557U  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 
ro-bsw-n3050 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1326/

a90c989 drm-intel-nightly: 2016y-06m-29d-09h-24m-21s UTC integration manifest
f47ea06 drm/i915: Use atomic waits for short non-atomic ones
5d6ee68 drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v5] drm/i915: Use atomic waits for short non-atomic ones

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

usleep_range is not recommended for waits shorten than 10us.

Make the wait_for_us use the atomic variant for such waits.

To do so we need to reimplement the _wait_for_atomic macro to
be safe with regards to preemption and interrupts.

v2: Reimplement _wait_for_atomic to be irq and preemption safe.
    (Chris Wilson and Imre Deak)

v3: Fixed in_atomic check due rebase error.
v4: Build bug on non-constant timeouts.
v5: Compile away cpu migration code in atomic paths.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 62 ++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3156d8df7921..98a5be4ec8c5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -69,39 +69,63 @@
 })
 
 #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
-#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
 
 /* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
 #if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
-# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) WARN_ON_ONCE((ATOMIC) && !in_atomic())
 #else
-# define _WAIT_FOR_ATOMIC_CHECK do { } while (0)
+# define _WAIT_FOR_ATOMIC_CHECK(ATOMIC) do { } while (0)
 #endif
 
-#define _wait_for_atomic(COND, US) ({ \
-	unsigned long end__; \
-	int ret__ = 0; \
-	_WAIT_FOR_ATOMIC_CHECK; \
+#define _wait_for_atomic(COND, US, ATOMIC) \
+({ \
+	int cpu, ret, timeout = (US) * 1000; \
+	u64 base; \
+	_WAIT_FOR_ATOMIC_CHECK(ATOMIC); \
 	BUILD_BUG_ON((US) > 50000); \
-	end__ = (local_clock() >> 10) + (US) + 1; \
-	while (!(COND)) { \
-		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
-			/* Unlike the regular wait_for(), this atomic variant \
-			 * cannot be preempted (and we'll just ignore the issue\
-			 * of irq interruptions) and so we know that no time \
-			 * has passed since the last check of COND and can \
-			 * immediately report the timeout. \
-			 */ \
-			ret__ = -ETIMEDOUT; \
+	if (!(ATOMIC)) { \
+		preempt_disable(); \
+		cpu = smp_processor_id(); \
+	} \
+	base = local_clock(); \
+	for (;;) { \
+		u64 now = local_clock(); \
+		if (!(ATOMIC)) \
+			preempt_enable(); \
+		if (COND) { \
+			ret = 0; \
+			break; \
+		} \
+		if (now - base >= timeout) { \
+			ret = -ETIMEDOUT; \
 			break; \
 		} \
 		cpu_relax(); \
+		if (!(ATOMIC)) { \
+			preempt_disable(); \
+			if (unlikely(cpu != smp_processor_id())) { \
+				timeout -= now - base; \
+				cpu = smp_processor_id(); \
+				base = local_clock(); \
+			} \
+		} \
 	} \
+	ret; \
+})
+
+#define wait_for_us(COND, US) \
+({ \
+	int ret__; \
+	BUILD_BUG_ON(!__builtin_constant_p(US)); \
+	if ((US) > 10) \
+		ret__ = _wait_for((COND), (US), 10); \
+	else \
+		ret__ = _wait_for_atomic((COND), (US), 0); \
 	ret__; \
 })
 
-#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 wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000, 1)
+#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US), 1)
 
 #define KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5] drm/i915: Use atomic waits for short non-atomic ones

[Chris Wilson]

On Wed, Jun 29, 2016 at 12:27:22PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> usleep_range is not recommended for waits shorten than 10us.
> 
> Make the wait_for_us use the atomic variant for such waits.
> 
> To do so we need to reimplement the _wait_for_atomic macro to
> be safe with regards to preemption and interrupts.
> 
> v2: Reimplement _wait_for_atomic to be irq and preemption safe.
>     (Chris Wilson and Imre Deak)
> 
> v3: Fixed in_atomic check due rebase error.
> v4: Build bug on non-constant timeouts.
> v5: Compile away cpu migration code in atomic paths.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

I like the polish.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Using wait_for_hybrid() is really tempting, just need to kick Mika to
finish intel_wait_for_register()...
-Chris
-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev5)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev5)
URL   : https://patchwork.freedesktop.org/series/9226/
State : failure

== Summary ==

Series 9226v5 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/5/mbox

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (ro-byt-n2820)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (fi-hsw-i7-4770k)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)

fi-hsw-i7-4770k  total:103  pass:86   dwarn:0   dfail:0   fail:0   skip:16 
fi-kbl-qkkr      total:229  pass:161  dwarn:29  dfail:0   fail:0   skip:39 
fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25 
fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:154  dwarn:0   dfail:1   fail:4   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40 
ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1327/

a90c989 drm-intel-nightly: 2016y-06m-29d-09h-24m-21s UTC integration manifest
d5eba7b drm/i915: Use atomic waits for short non-atomic ones
41208f4 drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev5)

[Tvrtko Ursulin]

On 29/06/16 12:50, Patchwork wrote:
> == Series Details ==
>
> Series: series starting with [1/2] drm/i915/debug: Select PREEMPT_COUNT when enabling debugging (rev5)
> URL   : https://patchwork.freedesktop.org/series/9226/
> State : failure
>
> == Summary ==
>
> Series 9226v5 Series without cover letter
> http://patchwork.freedesktop.org/api/1.0/series/9226/revisions/5/mbox
>
> Test drv_module_reload_basic:
>                  dmesg-warn -> PASS       (ro-byt-n2820)
> Test gem_exec_suspend:
>          Subgroup basic-s3:
>                  pass       -> INCOMPLETE (fi-hsw-i7-4770k)

Sporadic known failure, can't find the BAT.

> Test kms_pipe_crc_basic:
>          Subgroup suspend-read-crc-pipe-a:
>                  dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
>          Subgroup suspend-read-crc-pipe-c:
>                  dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
>
> fi-hsw-i7-4770k  total:103  pass:86   dwarn:0   dfail:0   fail:0   skip:16
> fi-kbl-qkkr      total:229  pass:161  dwarn:29  dfail:0   fail:0   skip:39
> fi-skl-i5-6260u  total:229  pass:202  dwarn:0   dfail:0   fail:2   skip:25
> fi-skl-i7-6700k  total:229  pass:188  dwarn:0   dfail:0   fail:2   skip:39
> fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53
> ro-bdw-i5-5250u  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23
> ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38
> ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49
> ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45
> ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31
> ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31
> ro-ilk-i7-620lm  total:229  pass:154  dwarn:0   dfail:1   fail:4   skip:70
> ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65
> ro-ivb-i7-3770   total:229  pass:186  dwarn:0   dfail:1   fail:2   skip:40
> ro-ivb2-i7-3770  total:229  pass:190  dwarn:0   dfail:1   fail:2   skip:36
> ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19
> ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48
> ro-bdw-i7-5557U failed to connect after reboot
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1327/
>
> a90c989 drm-intel-nightly: 2016y-06m-29d-09h-24m-21s UTC integration manifest
> d5eba7b drm/i915: Use atomic waits for short non-atomic ones
> 41208f4 drm/i915/debug: Select PREEMPT_COUNT when enabling debugging

Merged to dinq.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx