| Message ID | 20211129134735.628712-12-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove short term pins from execbuf. | expand |
On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst <maarten.lankhorst@linux.intel.com> wrote: > > Now that we cannot unbind kill the currently locked object directly Can this be reworded slightly? Not sure what is meant by "unbind kill" here. > because we're removing short term pinning, we may have to unbind the > object from gtt manually, using a i915_gem_evict_vm() call. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > index 65fc6ff5f59d..6d557bb9926f 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > @@ -357,8 +357,22 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) > vma = i915_gem_object_ggtt_pin_ww(obj, &ww, &view, 0, 0, flags); > } > > - /* The entire mappable GGTT is pinned? Unexpected! */ > - GEM_BUG_ON(vma == ERR_PTR(-ENOSPC)); > + /* > + * The entire mappable GGTT is pinned? Unexpected! > + * Try to evict the object we locked too, as normally we skip it > + * due to lack of short term pinning inside execbuf. > + */ > + if (vma == ERR_PTR(-ENOSPC)) { > + ret = mutex_lock_interruptible(&ggtt->vm.mutex); > + if (!ret) { > + ret = i915_gem_evict_vm(&ggtt->vm, &ww); Would it make sense to pass an extra flag for the above ggtt_pin(maybe PIN_EVICT_SHARED)? Such that evict_for_something can handle the already locked object and then also any vma sharing the same dma-resv object here? Or at least trying to nuke the entire vm, just for the mappable portion seems maybe overkill? Or perhaps we never expect to hit this in the real world? Reviewed-by: Matthew Auld <matthew.auld@intel.com> > + mutex_unlock(&ggtt->vm.mutex); > + } > + if (ret) > + goto err_reset; > + vma = i915_gem_object_ggtt_pin_ww(obj, &ww, &view, 0, 0, flags); > + } > + GEM_WARN_ON(vma == ERR_PTR(-ENOSPC)); > } > if (IS_ERR(vma)) { > ret = PTR_ERR(vma); > -- > 2.34.0 >
On 09-12-2021 13:17, Matthew Auld wrote: > On Mon, 29 Nov 2021 at 13:58, Maarten Lankhorst > <maarten.lankhorst@linux.intel.com> wrote: >> Now that we cannot unbind kill the currently locked object directly > Can this be reworded slightly? Not sure what is meant by "unbind kill" here. Oops, the word 'kill' doesn't belong here. >> because we're removing short term pinning, we may have to unbind the >> object from gtt manually, using a i915_gem_evict_vm() call. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> index 65fc6ff5f59d..6d557bb9926f 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c >> @@ -357,8 +357,22 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) >> vma = i915_gem_object_ggtt_pin_ww(obj, &ww, &view, 0, 0, flags); >> } >> >> - /* The entire mappable GGTT is pinned? Unexpected! */ >> - GEM_BUG_ON(vma == ERR_PTR(-ENOSPC)); >> + /* >> + * The entire mappable GGTT is pinned? Unexpected! >> + * Try to evict the object we locked too, as normally we skip it >> + * due to lack of short term pinning inside execbuf. >> + */ >> + if (vma == ERR_PTR(-ENOSPC)) { >> + ret = mutex_lock_interruptible(&ggtt->vm.mutex); >> + if (!ret) { >> + ret = i915_gem_evict_vm(&ggtt->vm, &ww); > Would it make sense to pass an extra flag for the above ggtt_pin(maybe > PIN_EVICT_SHARED)? Such that evict_for_something can handle the > already locked object and then also any vma sharing the same dma-resv > object here? Or at least trying to nuke the entire vm, just for the > mappable portion seems maybe overkill? Or perhaps we never expect to > hit this in the real world? > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> Yeah, effect would be the same though. When fully reworking eviction and vm locks, it might be better to do so though. > >> + mutex_unlock(&ggtt->vm.mutex); >> + } >> + if (ret) >> + goto err_reset; >> + vma = i915_gem_object_ggtt_pin_ww(obj, &ww, &view, 0, 0, flags); >> + } >> + GEM_WARN_ON(vma == ERR_PTR(-ENOSPC)); >> } >> if (IS_ERR(vma)) { >> ret = PTR_ERR(vma); >> -- >> 2.34.0 >>
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 65fc6ff5f59d..6d557bb9926f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -357,8 +357,22 @@ static vm_fault_t vm_fault_gtt(struct vm_fault *vmf) vma = i915_gem_object_ggtt_pin_ww(obj, &ww, &view, 0, 0, flags); } - /* The entire mappable GGTT is pinned? Unexpected! */ - GEM_BUG_ON(vma == ERR_PTR(-ENOSPC)); + /* + * The entire mappable GGTT is pinned? Unexpected! + * Try to evict the object we locked too, as normally we skip it + * due to lack of short term pinning inside execbuf. + */ + if (vma == ERR_PTR(-ENOSPC)) { + ret = mutex_lock_interruptible(&ggtt->vm.mutex); + if (!ret) { + ret = i915_gem_evict_vm(&ggtt->vm, &ww); + mutex_unlock(&ggtt->vm.mutex); + } + if (ret) + goto err_reset; + vma = i915_gem_object_ggtt_pin_ww(obj, &ww, &view, 0, 0, flags); + } + GEM_WARN_ON(vma == ERR_PTR(-ENOSPC)); } if (IS_ERR(vma)) { ret = PTR_ERR(vma);
Now that we cannot unbind kill the currently locked object directly because we're removing short term pinning, we may have to unbind the object from gtt manually, using a i915_gem_evict_vm() call. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)