| Message ID | 20210830121006.2978297-9-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Short-term pinning and async eviction. | expand |
On Mon, Aug 30, 2021 at 02:09:55PM +0200, Maarten Lankhorst wrote: >We forgot to call intel_runtime_pm_put on error, fix it! > Looks good to me. Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> >Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >Fixes: cf41a8f1dc1e ("drm/i915: Finally remove obj->mm.lock.") >Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >Cc: <stable@vger.kernel.org> # v5.13+ >--- > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > >diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c >index e382b7f2353b..5ab136ffdeb2 100644 >--- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c >+++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c >@@ -118,7 +118,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, > intel_wakeref_t wakeref = 0; > unsigned long count = 0; > unsigned long scanned = 0; >- int err; >+ int err = 0; > > /* CHV + VTD workaround use stop_machine(); need to trylock vm->mutex */ > bool trylock_vm = !ww && intel_vm_no_concurrent_access_wa(i915); >@@ -242,12 +242,15 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, > list_splice_tail(&still_in_list, phase->list); > spin_unlock_irqrestore(&i915->mm.obj_lock, flags); > if (err) >- return err; >+ break; > } > > if (shrink & I915_SHRINK_BOUND) > intel_runtime_pm_put(&i915->runtime_pm, wakeref); > >+ if (err) >+ return err; >+ > if (nr_scanned) > *nr_scanned += scanned; > return count; >-- >2.32.0 >
On Mon, 30 Aug 2021 at 13:10, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > We forgot to call intel_runtime_pm_put on error, fix it! > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Fixes: cf41a8f1dc1e ("drm/i915: Finally remove obj->mm.lock.") > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: <stable@vger.kernel.org> # v5.13+ How does the err handling work? gem_shrink is meant to return the number of pages reclaimed which is an unsigned long, and yet we are also just returning the err here? Fortunately it looks like nothing is calling gem_shrinker with an actual ww context, so nothing will hit this yet? I think the interface needs to be reworked or something. > --- > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > index e382b7f2353b..5ab136ffdeb2 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > @@ -118,7 +118,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, > intel_wakeref_t wakeref = 0; > unsigned long count = 0; > unsigned long scanned = 0; > - int err; > + int err = 0; > > /* CHV + VTD workaround use stop_machine(); need to trylock vm->mutex */ > bool trylock_vm = !ww && intel_vm_no_concurrent_access_wa(i915); > @@ -242,12 +242,15 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, > list_splice_tail(&still_in_list, phase->list); > spin_unlock_irqrestore(&i915->mm.obj_lock, flags); > if (err) > - return err; > + break; > } > > if (shrink & I915_SHRINK_BOUND) > intel_runtime_pm_put(&i915->runtime_pm, wakeref); > > + if (err) > + return err; > + > if (nr_scanned) > *nr_scanned += scanned; > return count; > -- > 2.32.0 >
On Wed, 29 Sept 2021 at 09:37, Matthew Auld <matthew.william.auld@gmail.com> wrote: > > On Mon, 30 Aug 2021 at 13:10, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: > > > > We forgot to call intel_runtime_pm_put on error, fix it! > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Fixes: cf41a8f1dc1e ("drm/i915: Finally remove obj->mm.lock.") > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: <stable@vger.kernel.org> # v5.13+ > > How does the err handling work? gem_shrink is meant to return the > number of pages reclaimed which is an unsigned long, and yet we are > also just returning the err here? Fortunately it looks like nothing is > calling gem_shrinker with an actual ww context, so nothing will hit > this yet? I think the interface needs to be reworked or something. Can we just remove the ww context argument, or is that needed for something in the future? > > > --- > > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > > index e382b7f2353b..5ab136ffdeb2 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c > > @@ -118,7 +118,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, > > intel_wakeref_t wakeref = 0; > > unsigned long count = 0; > > unsigned long scanned = 0; > > - int err; > > + int err = 0; > > > > /* CHV + VTD workaround use stop_machine(); need to trylock vm->mutex */ > > bool trylock_vm = !ww && intel_vm_no_concurrent_access_wa(i915); > > @@ -242,12 +242,15 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, > > list_splice_tail(&still_in_list, phase->list); > > spin_unlock_irqrestore(&i915->mm.obj_lock, flags); > > if (err) > > - return err; > > + break; > > } > > > > if (shrink & I915_SHRINK_BOUND) > > intel_runtime_pm_put(&i915->runtime_pm, wakeref); > > > > + if (err) > > + return err; > > + > > if (nr_scanned) > > *nr_scanned += scanned; > > return count; > > -- > > 2.32.0 > >
Op 29-09-2021 om 10:37 schreef Matthew Auld: > On Mon, 30 Aug 2021 at 13:10, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> We forgot to call intel_runtime_pm_put on error, fix it! >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Fixes: cf41a8f1dc1e ("drm/i915: Finally remove obj->mm.lock.") >> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: <stable@vger.kernel.org> # v5.13+ > How does the err handling work? gem_shrink is meant to return the > number of pages reclaimed which is an unsigned long, and yet we are > also just returning the err here? Fortunately it looks like nothing is > calling gem_shrinker with an actual ww context, so nothing will hit > this yet? I think the interface needs to be reworked or something. We should probably make it signed in the future when used. It should never hit the LONG_MAX limit, since max value returned would be ULONG_MAX >> PAGE_SHIFT, assuming the entire address space is filled with evictable buffers. I've kept the ww lock, in case we want to evict in the future. Without ww context the buffers are trylocked, with ww we can evict the entire address space as much as possible. In most cases we only want to evict idle objects, in that case no ww is needed. Pushed just this patch, thanks for feedback. :) ~Maarten
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c index e382b7f2353b..5ab136ffdeb2 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_shrinker.c @@ -118,7 +118,7 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, intel_wakeref_t wakeref = 0; unsigned long count = 0; unsigned long scanned = 0; - int err; + int err = 0; /* CHV + VTD workaround use stop_machine(); need to trylock vm->mutex */ bool trylock_vm = !ww && intel_vm_no_concurrent_access_wa(i915); @@ -242,12 +242,15 @@ i915_gem_shrink(struct i915_gem_ww_ctx *ww, list_splice_tail(&still_in_list, phase->list); spin_unlock_irqrestore(&i915->mm.obj_lock, flags); if (err) - return err; + break; } if (shrink & I915_SHRINK_BOUND) intel_runtime_pm_put(&i915->runtime_pm, wakeref); + if (err) + return err; + if (nr_scanned) *nr_scanned += scanned; return count;
We forgot to call intel_runtime_pm_put on error, fix it! Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Fixes: cf41a8f1dc1e ("drm/i915: Finally remove obj->mm.lock.") Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: <stable@vger.kernel.org> # v5.13+ --- drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)