Re: [PATCH v3] drm/i915: Only report a wakeup if the waiter was truly asleep

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/04/2017 18:40, Tvrtko Ursulin wrote:
> On 06/04/2017 10:30, Chris Wilson wrote:
>> If we attempt to wake up a waiter, who is currently checking the seqno
>> it will be in the TASK_INTERRUPTIBLE state and ttwu will report success.
>> However, it is actually awake and functioning -- so delay reporting the
>> actual wake up until it sleeps.
>>
>> v2: Defend against !CONFIG_SMP
>> v3: Don't filter out calls to wake_up_process
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=100007
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 18 ++++++++++++++++--
>>  drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 ++++
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> index 9ccbf26124c6..808d3a3cda0a 100644
>> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
>> @@ -27,6 +27,12 @@
>>
>>  #include "i915_drv.h"
>>
>> +#ifdef CONFIG_SMP
>> +#define task_asleep(tsk) (!(tsk)->on_cpu)
>> +#else
>> +#define task_asleep(tsk) ((tsk) != current)
>> +#endif
>
> It really bothers to fish into this low level info which probably isn't
> intended to be used from the outside.
>
>> +
>>  static unsigned int __intel_breadcrumbs_wakeup(struct
>> intel_breadcrumbs *b)
>>  {
>>      struct intel_wait *wait;
>> @@ -37,8 +43,16 @@ static unsigned int
>> __intel_breadcrumbs_wakeup(struct intel_breadcrumbs *b)
>>      wait = b->irq_wait;
>>      if (wait) {
>>          result = ENGINE_WAKEUP_WAITER;
>> -        if (wake_up_process(wait->tsk))
>> +
>> +        /* Be careful not to report a successful wakeup if the waiter
>> +         * is currently processing the seqno, where it will have
>> +         * already called set_task_state(TASK_INTERRUPTIBLE).
>> +         */
>> +        if (task_asleep(wait->tsk))
>>              result |= ENGINE_WAKEUP_ASLEEP;
>
> And this still has the problem of not being atomic between reporting the
> two flags. So the reported status can be false which also bothers me.
>
> I will need to take some more time thinking about this.

Warning, the idea below is potentially unrefined! :)

How about a scheme where on wake_up we would atomic_inc our own wakeup 
counter, and then the signaller keeps atomic_dec_and_test one item at a 
time until it has consumed all the wakeups? The code in 
intel_breadcrumbs_hangcheck only declares a missed breadcrumb if the 
wakeup counter is one, meaning this was the first wakeup?

Regards,

Tvrtko

>> +
>> +        if (wake_up_process(wait->tsk))
>> +            result |= ENGINE_WAKEUP_SUCCESS;
>>      }
>>
>>      return result;
>> @@ -98,7 +112,7 @@ static void intel_breadcrumbs_hangcheck(unsigned
>> long data)
>>       * but we still have a waiter. Assuming all batches complete within
>>       * DRM_I915_HANGCHECK_JIFFIES [1.5s]!
>>       */
>> -    if (intel_engine_wakeup(engine) & ENGINE_WAKEUP_ASLEEP) {
>> +    if (intel_engine_wakeup(engine) == ENGINE_WAKEUP) {
>>          missed_breadcrumb(engine);
>>          mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
>>      } else {
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index cbe61d3f31da..974a5928bec9 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -663,6 +663,10 @@ static inline bool intel_engine_has_waiter(const
>> struct intel_engine_cs *engine)
>>  unsigned int intel_engine_wakeup(struct intel_engine_cs *engine);
>>  #define ENGINE_WAKEUP_WAITER BIT(0)
>>  #define ENGINE_WAKEUP_ASLEEP BIT(1)
>> +#define ENGINE_WAKEUP_SUCCESS BIT(2)
>> +#define ENGINE_WAKEUP (ENGINE_WAKEUP_WAITER | \
>> +               ENGINE_WAKEUP_ASLEEP | \
>> +               ENGINE_WAKEUP_SUCCESS)
>>
>>  void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>>  void intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine);
>>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx