[PATCH] drm/i915: Do not lie about atomic wait granularity

[PATCH] drm/i915: Do not lie about atomic wait granularity

[Tvrtko Ursulin]

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

Currently the wait_for_atomic_us only allows for a millisecond
granularity which is not nice towards callers requesting small
micro-second waits.

Re-implement it so micro-second granularity is really supported
and not just in the name of the macro.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
Danger - this might break things which currently work by accident!
---
 drivers/gpu/drm/i915/intel_drv.h | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f620023ed134..9e8a1202194c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -63,10 +63,25 @@
 	ret__;								\
 })
 
+#define _wait_for_atomic(COND, US) ({ \
+	unsigned long end__; \
+	int ret__ = 0; \
+	get_cpu(); \
+	end__ = (local_clock() >> 10) + (US) + 1; \
+	while (!(COND)) { \
+		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
+			ret__ = -ETIMEDOUT; \
+			break; \
+		} \
+		cpu_relax(); \
+	} \
+	put_cpu(); \
+	ret__; \
+})
+
 #define wait_for(COND, MS) _wait_for(COND, MS, 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)
+#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 KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
-- 
1.9.1

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

Re: [PATCH] drm/i915: Do not lie about atomic wait granularity

[Chris Wilson]

On Mon, Feb 01, 2016 at 01:17:35PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Currently the wait_for_atomic_us only allows for a millisecond
> granularity which is not nice towards callers requesting small
> micro-second waits.
> 
> Re-implement it so micro-second granularity is really supported
> and not just in the name of the macro.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
> Danger - this might break things which currently work by accident!
> ---
>  drivers/gpu/drm/i915/intel_drv.h | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f620023ed134..9e8a1202194c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -63,10 +63,25 @@
>  	ret__;								\
>  })
>  
> +#define _wait_for_atomic(COND, US) ({ \
> +	unsigned long end__; \
> +	int ret__ = 0; \
> +	get_cpu(); \

Hmm, by virtue of its name (and original intent), we are expected to be
in an atomic context and could just do a BUG_ON(!in_atomic()) to catch
misuse. Since the removal of the panic modeset, all callers outside of
intel_uncore.c are definitely abusing this and we would be better to use
a usleep[_range]() variant instead.
-Chris

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

✗ Fi.CI.BAT: failure for drm/i915: Do not lie about atomic wait granularity

[Patchwork]

== Summary ==

Series 2979v1 drm/i915: Do not lie about atomic wait granularity
http://patchwork.freedesktop.org/api/1.0/series/2979/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (skl-i5k-2)
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:156  pass:147  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:159  pass:147  dwarn:0   dfail:0   fail:0   skip:12 
bsw-nuc-2        total:159  pass:129  dwarn:0   dfail:0   fail:0   skip:30 
byt-nuc          total:159  pass:136  dwarn:0   dfail:0   fail:0   skip:23 
hsw-brixbox      total:159  pass:146  dwarn:0   dfail:0   fail:0   skip:13 
ilk-hp8440p      total:159  pass:111  dwarn:0   dfail:0   fail:0   skip:48 
ivb-t430s        total:159  pass:145  dwarn:0   dfail:0   fail:0   skip:14 
skl-i5k-2        total:159  pass:143  dwarn:2   dfail:0   fail:0   skip:14 
snb-dellxps      total:159  pass:137  dwarn:0   dfail:0   fail:0   skip:22 
snb-x220t        total:159  pass:137  dwarn:0   dfail:0   fail:1   skip:21 

HANGED hsw-gt2 in igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a

Results at /archive/results/CI_IGT_test/Patchwork_1336/

6b1049b84dcd979f631d15b2ada325d8e5b2c4e1 drm-intel-nightly: 2016y-01m-29d-22h-50m-57s UTC integration manifest
8db7a9e3078beff29c8016e570ecfe70db1aaca4 drm/i915: Do not lie about atomic wait granularity

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

Re: [PATCH] drm/i915: Do not lie about atomic wait granularity

[Tvrtko Ursulin]

On 01/02/16 13:30, Chris Wilson wrote:
> On Mon, Feb 01, 2016 at 01:17:35PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Currently the wait_for_atomic_us only allows for a millisecond
>> granularity which is not nice towards callers requesting small
>> micro-second waits.
>>
>> Re-implement it so micro-second granularity is really supported
>> and not just in the name of the macro.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>> Danger - this might break things which currently work by accident!
>> ---
>>   drivers/gpu/drm/i915/intel_drv.h | 21 ++++++++++++++++++---
>>   1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index f620023ed134..9e8a1202194c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -63,10 +63,25 @@
>>   	ret__;								\
>>   })
>>
>> +#define _wait_for_atomic(COND, US) ({ \
>> +	unsigned long end__; \
>> +	int ret__ = 0; \
>> +	get_cpu(); \
>
> Hmm, by virtue of its name (and original intent), we are expected to be
> in an atomic context and could just do a BUG_ON(!in_atomic()) to catch
> misuse. Since the removal of the panic modeset, all callers outside of
> intel_uncore.c are definitely abusing this and we would be better to use
> a usleep[_range]() variant instead.

I considered a WARN_ON_ONCE and a BUILD_BUG_ON for very long waits but 
chickened out on both.

I'll respin with a WARN_ON_ONCE(!in_atomic)) to start with.

Regards,

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

Re: [PATCH] drm/i915: Do not lie about atomic wait granularity

[Tvrtko Ursulin]

On 01/02/16 14:15, Tvrtko Ursulin wrote:
>
> On 01/02/16 13:30, Chris Wilson wrote:
>> On Mon, Feb 01, 2016 at 01:17:35PM +0000, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> Currently the wait_for_atomic_us only allows for a millisecond
>>> granularity which is not nice towards callers requesting small
>>> micro-second waits.
>>>
>>> Re-implement it so micro-second granularity is really supported
>>> and not just in the name of the macro.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> Danger - this might break things which currently work by accident!
>>> ---
>>>   drivers/gpu/drm/i915/intel_drv.h | 21 ++++++++++++++++++---
>>>   1 file changed, 18 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index f620023ed134..9e8a1202194c 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -63,10 +63,25 @@
>>>       ret__;                                \
>>>   })
>>>
>>> +#define _wait_for_atomic(COND, US) ({ \
>>> +    unsigned long end__; \
>>> +    int ret__ = 0; \
>>> +    get_cpu(); \
>>
>> Hmm, by virtue of its name (and original intent), we are expected to be
>> in an atomic context and could just do a BUG_ON(!in_atomic()) to catch
>> misuse. Since the removal of the panic modeset, all callers outside of
>> intel_uncore.c are definitely abusing this and we would be better to use
>> a usleep[_range]() variant instead.
>
> I considered a WARN_ON_ONCE and a BUILD_BUG_ON for very long waits but
> chickened out on both.
>
> I'll respin with a WARN_ON_ONCE(!in_atomic)) to start with.

Can't really do that it seems since in_atomic() will be always false on 
non-fully-preemptible kernels.

Could do the current cpu comparison trick to catch false timeouts due 
callers from non-atomic sections but not sure if it is worth it. So it 
looks like manual audit of call sites to me.

Or find a time source with micro-second resolution which does not go 
backwards on CPU migrations?

Regards,

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