drm/i915: Serialise i915_active_wait() with its retirement
diff mbox series

Message ID 20191129093908.631527-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915: Serialise i915_active_wait() with its retirement
Related show

Commit Message

Chris Wilson Nov. 29, 2019, 9:39 a.m. UTC
As the i915_active.retire() may be running on another CPU as we detect
that the i915_active is idle, we may not wait for the retirement itself.
Wait for the remote callback by waiting for the retirement worker.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_active.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Tvrtko Ursulin Nov. 29, 2019, 11:15 a.m. UTC | #1
On 29/11/2019 09:39, Chris Wilson wrote:
> As the i915_active.retire() may be running on another CPU as we detect
> that the i915_active is idle, we may not wait for the retirement itself.
> Wait for the remote callback by waiting for the retirement worker.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_active.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 479195ecbc6c..e8630ee33336 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -469,6 +469,7 @@ int i915_active_wait(struct i915_active *ref)
>   	if (wait_var_event_interruptible(ref, i915_active_is_idle(ref)))
>   		return -EINTR;
>   
> +	flush_work(&ref->work);
>   	return 0;
>   }
>   
> 

Hm, but wake_up_war is in the worker so how does wait_var_event wake the 
waiter up before it has been retired?

Regards,

Tvrtko
Chris Wilson Nov. 29, 2019, 11:28 a.m. UTC | #2
Quoting Tvrtko Ursulin (2019-11-29 11:15:46)
> 
> On 29/11/2019 09:39, Chris Wilson wrote:
> > As the i915_active.retire() may be running on another CPU as we detect
> > that the i915_active is idle, we may not wait for the retirement itself.
> > Wait for the remote callback by waiting for the retirement worker.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_active.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> > index 479195ecbc6c..e8630ee33336 100644
> > --- a/drivers/gpu/drm/i915/i915_active.c
> > +++ b/drivers/gpu/drm/i915/i915_active.c
> > @@ -469,6 +469,7 @@ int i915_active_wait(struct i915_active *ref)
> >       if (wait_var_event_interruptible(ref, i915_active_is_idle(ref)))
> >               return -EINTR;
> >   
> > +     flush_work(&ref->work);
> >       return 0;
> >   }
> >   
> > 
> 
> Hm, but wake_up_war is in the worker so how does wait_var_event wake the 
> waiter up before it has been retired?

Remember the wait_event pattern is to skip the wait if COND is already
met. So since the first thing the retirement does is the
atomic_dec_and_test(), we can see ref->count == 0 very early, long
before ref->retire() is called. Our selftest is checking that if
i915_active_wait() reports completion, the callback has also run and
that the i915_active can then be destroyed.
-Chris
Tvrtko Ursulin Nov. 29, 2019, 11:38 a.m. UTC | #3
On 29/11/2019 11:28, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2019-11-29 11:15:46)
>>
>> On 29/11/2019 09:39, Chris Wilson wrote:
>>> As the i915_active.retire() may be running on another CPU as we detect
>>> that the i915_active is idle, we may not wait for the retirement itself.
>>> Wait for the remote callback by waiting for the retirement worker.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=112424
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    drivers/gpu/drm/i915/i915_active.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
>>> index 479195ecbc6c..e8630ee33336 100644
>>> --- a/drivers/gpu/drm/i915/i915_active.c
>>> +++ b/drivers/gpu/drm/i915/i915_active.c
>>> @@ -469,6 +469,7 @@ int i915_active_wait(struct i915_active *ref)
>>>        if (wait_var_event_interruptible(ref, i915_active_is_idle(ref)))
>>>                return -EINTR;
>>>    
>>> +     flush_work(&ref->work);
>>>        return 0;
>>>    }
>>>    
>>>
>>
>> Hm, but wake_up_war is in the worker so how does wait_var_event wake the
>> waiter up before it has been retired?
> 
> Remember the wait_event pattern is to skip the wait if COND is already
> met. So since the first thing the retirement does is the
> atomic_dec_and_test(), we can see ref->count == 0 very early, long
> before ref->retire() is called. Our selftest is checking that if
> i915_active_wait() reports completion, the callback has also run and
> that the i915_active can then be destroyed.

True!

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 479195ecbc6c..e8630ee33336 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -469,6 +469,7 @@  int i915_active_wait(struct i915_active *ref)
 	if (wait_var_event_interruptible(ref, i915_active_is_idle(ref)))
 		return -EINTR;
 
+	flush_work(&ref->work);
 	return 0;
 }