| Message ID | 20230918170257.8586-2-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Update GGTT with MI_UPDATE_GTT on MTL | expand |
On Mon, 18 Sep 2023, Nirmoy Das <nirmoy.das@intel.com> wrote: > From: Chris Wilson <chris.p.wilson@intel.com> > > 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@intel.com> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Is this patch a dependency on the subsequent patches in the series? If yes, what's the rationale? If not, please submit separately. None of this is is obvious in the commit messages. > --- > 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)
Hi Jani, On 9/20/2023 11:03 AM, Jani Nikula wrote: > On Mon, 18 Sep 2023, Nirmoy Das <nirmoy.das@intel.com> wrote: >> From: Chris Wilson <chris.p.wilson@intel.com> >> >> 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@intel.com> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> > 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)
Hi Jani, [...] > > * 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.) Agree, this can be made in a separate patch. > > + 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. it actually looks straight forward to me and even more understandable. get/put are OK if there are multiple users, but when used in such simple context it looks a bit of an overkill. Especially when we don't need anymore the actions taken bu get and put. So that replacing the pointer with NULL is a natural process, no? Andi
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); + int err = 0; + /* * 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))) { + INTEL_WAKEREF_BUG_ON(wf->wakeref); + wf->wakeref = fetch_and_zero(&wakeref); 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); + 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)