Hi Jani,
On 9/20/2023 11:03 AM, Jani Nikula wrote:
On Mon, 18 Sep 2023, Nirmoy Das <nirmoy.das@xxxxxxxxx> wrote:
From: Chris Wilson <chris.p.wilson@xxxxxxxxx>
When runtime pm is first woken, it will synchronously call the
registered callbacks for the device. These callbacks
may pull in their own forest of locks, which we do not want to
conflate with the intel_wakeref.mutex. A second minor benefit to
reducing the coverage of the mutex, is that it will reduce
contention for frequent sleeps and wakes (such as when being used
for soft-rc6).
Signed-off-by: Chris Wilson <chris.p.wilson@xxxxxxxxx>
Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
Is this patch a dependency on the subsequent patches in the series? If
yes, what's the rationale?
When we do GGTT update with gpu request, we need to take pm wakeref for
GT as well as for the engine and
without this patch/improvement there will be lockdep warning as we are
not allowed to alloc memory with
vm->mutex held which can happen when the runtime pm woken up 1st.
If not, please submit separately. None of
this is is obvious in the commit messages.
So I would like to keep this patch in the series. I will add rationale
in the cover letter and
resend with your below suggestions.
Thanks,
Nirmoy
---
drivers/gpu/drm/i915/intel_wakeref.c | 43 ++++++++++++++--------------
1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_wakeref.c b/drivers/gpu/drm/i915/intel_wakeref.c
index 718f2f1b6174..af7b4cb5b4d7 100644
--- a/drivers/gpu/drm/i915/intel_wakeref.c
+++ b/drivers/gpu/drm/i915/intel_wakeref.c
@@ -10,21 +10,11 @@
#include "intel_wakeref.h"
#include "i915_drv.h"
-static void rpm_get(struct intel_wakeref *wf)
-{
- wf->wakeref = intel_runtime_pm_get(&wf->i915->runtime_pm);
-}
-
-static void rpm_put(struct intel_wakeref *wf)
-{
- intel_wakeref_t wakeref = fetch_and_zero(&wf->wakeref);
-
- intel_runtime_pm_put(&wf->i915->runtime_pm, wakeref);
- INTEL_WAKEREF_BUG_ON(!wakeref);
-}
-
int __intel_wakeref_get_first(struct intel_wakeref *wf)
{
+ intel_wakeref_t wakeref = intel_runtime_pm_get(&wf->i915->runtime_pm);
No non-trivial function calls in the initializer please.
+ int err = 0;
Until now err was only for handling error returns. If it's also for
returning success, it should probably be named ret.
+
/*
* Treat get/put as different subclasses, as we may need to run
* the put callback from under the shrinker and do not want to
@@ -32,41 +22,50 @@ int __intel_wakeref_get_first(struct intel_wakeref *wf)
* upon acquiring the wakeref.
*/
mutex_lock_nested(&wf->mutex, SINGLE_DEPTH_NESTING);
- if (!atomic_read(&wf->count)) {
- int err;
- rpm_get(wf);
+ if (likely(!atomic_read(&wf->count))) {
Adding the likely should be a separate patch with rationale, not a
random drive-by change. (And maybe it just should not be added at all.)
+ INTEL_WAKEREF_BUG_ON(wf->wakeref);
+ wf->wakeref = fetch_and_zero(&wakeref);
fetch_and_zero() should just die. All it does here is make things more
confusing, not less. Please don't add new users.
The get and put helpers could probably stay, modified, to make this more
readable.
err = wf->ops->get(wf);
if (unlikely(err)) {
- rpm_put(wf);
- mutex_unlock(&wf->mutex);
- return err;
+ wakeref = xchg(&wf->wakeref, 0);
+ wake_up_var(&wf->wakeref);
e.g. here this bit is duplicated in ____intel_wakeref_put_last().
+ goto unlock;
}
smp_mb__before_atomic(); /* release wf->count */
}
atomic_inc(&wf->count);
+ INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
+
+unlock:
mutex_unlock(&wf->mutex);
+ if (unlikely(wakeref))
+ intel_runtime_pm_put(&wf->i915->runtime_pm, wakeref);
- INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
- return 0;
+ return err;
}
static void ____intel_wakeref_put_last(struct intel_wakeref *wf)
{
+ intel_wakeref_t wakeref = 0;
+
INTEL_WAKEREF_BUG_ON(atomic_read(&wf->count) <= 0);
if (unlikely(!atomic_dec_and_test(&wf->count)))
goto unlock;
/* ops->put() must reschedule its own release on error/deferral */
if (likely(!wf->ops->put(wf))) {
- rpm_put(wf);
+ INTEL_WAKEREF_BUG_ON(!wf->wakeref);
+ wakeref = xchg(&wf->wakeref, 0);
wake_up_var(&wf->wakeref);
}
unlock:
mutex_unlock(&wf->mutex);
+ if (wakeref)
+ intel_runtime_pm_put(&wf->i915->runtime_pm, wakeref);
}
void __intel_wakeref_put_last(struct intel_wakeref *wf, unsigned long flags)
