[09/17] drm/i915: Wait until the intel_wakeref idle callback is complete
diff mbox series

Message ID 20191119100929.2628356-9-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/17] drm/i915/gem: Manually dump the debug trace on GEM_BUG_ON
Related show

Commit Message

Chris Wilson Nov. 19, 2019, 10:09 a.m. UTC
When waiting for idle, serialise with any ongoing callback so that it
will have completed before completing the wait.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_wakeref.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Mika Kuoppala Nov. 19, 2019, 4:12 p.m. UTC | #1
Chris Wilson <chris@chris-wilson.co.uk> writes:

> When waiting for idle, serialise with any ongoing callback so that it
> will have completed before completing the wait.

Might be come apparent and evident when reading the patch
that introduce the intel_wakeref_unlock_wait(),
but reader is yearning for a why part.

The 'wait_for_idle' is kind of revaling of
why the need for sync tho.

-Mika

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_wakeref.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
> index 9b29176cc1ca..91feb53b2942 100644
> --- a/drivers/gpu/drm/i915/intel_wakeref.c
> +++ b/drivers/gpu/drm/i915/intel_wakeref.c
> @@ -109,8 +109,15 @@ void __intel_wakeref_init(struct intel_wakeref *wf,
>  
>  int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
>  {
> -	return wait_var_event_killable(&wf->wakeref,
> -				       !intel_wakeref_is_active(wf));
> +	int err;
> +
> +	err = wait_var_event_killable(&wf->wakeref,
> +				      !intel_wakeref_is_active(wf));
> +	if (err)
> +		return err;
> +
> +	intel_wakeref_unlock_wait(wf);
> +	return 0;
>  }
>  
>  static void wakeref_auto_timeout(struct timer_list *t)
> -- 
> 2.24.0
Chris Wilson Nov. 19, 2019, 4:35 p.m. UTC | #2
Quoting Mika Kuoppala (2019-11-19 16:12:18)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > When waiting for idle, serialise with any ongoing callback so that it
> > will have completed before completing the wait.
> 
> Might be come apparent and evident when reading the patch
> that introduce the intel_wakeref_unlock_wait(),
> but reader is yearning for a why part.
> 
> The 'wait_for_idle' is kind of revaling of
> why the need for sync tho.

I don't have a particular example where by not waiting for the callback
to finish, we found ourselves inspecting stale state. But that's the
worry -- that any caller proceeds assuming the wakeref mutexes are no
longer in use, when in fact some CPU is still accessing that memory.
Tends to only be a worry before free, which in this case entails module
unload.

I needed the "unlock_wait" for engine-pm in one of the selftests, as I
was adding that, I saw no harm in dotting the 'i' and crossing the 't'
here as well.
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
index 9b29176cc1ca..91feb53b2942 100644
--- a/drivers/gpu/drm/i915/intel_wakeref.c
+++ b/drivers/gpu/drm/i915/intel_wakeref.c
@@ -109,8 +109,15 @@  void __intel_wakeref_init(struct intel_wakeref *wf,
 
 int intel_wakeref_wait_for_idle(struct intel_wakeref *wf)
 {
-	return wait_var_event_killable(&wf->wakeref,
-				       !intel_wakeref_is_active(wf));
+	int err;
+
+	err = wait_var_event_killable(&wf->wakeref,
+				      !intel_wakeref_is_active(wf));
+	if (err)
+		return err;
+
+	intel_wakeref_unlock_wait(wf);
+	return 0;
 }
 
 static void wakeref_auto_timeout(struct timer_list *t)