| Message ID | 20210119144912.12653-6-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] drm/i915/gem: Almagamate clflushes on suspend | expand |
On Tue, 19 Jan 2021 at 14:49, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Simplify the frontbuffer unpin by removing the lock requirement. The LRU > bumping was primarily to protect the GTT from being evicted and from > frontbuffers being eagerly shrunk. Now we protect frontbuffers from the > shrinker, and we avoid accidentally evicting from the GTT, so the > benefit from bumping LRU is no more, and we can save more time by not. For the GTT evict case, where/how do we currently try to prevent accidental eviction for fb?
Quoting Matthew Auld (2021-01-19 16:38:04) > On Tue, 19 Jan 2021 at 14:49, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > Simplify the frontbuffer unpin by removing the lock requirement. The LRU > > bumping was primarily to protect the GTT from being evicted and from > > frontbuffers being eagerly shrunk. Now we protect frontbuffers from the > > shrinker, and we avoid accidentally evicting from the GTT, so the > > benefit from bumping LRU is no more, and we can save more time by not. > > For the GTT evict case, where/how do we currently try to prevent > accidental eviction for fb? Our preference is to try with NOEVICT, and then use smaller partial mappings, reducing the risk of evicting anything that may be reused in the near future. The goal is to really only use the full GTT mapping for when HW needs access to the whole object. However, we could apply the same rule as we do for the shrinker as leave frontbuffer objects until the second pass. Such as --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -61,6 +61,19 @@ mark_free(struct drm_mm_scan *scan, return drm_mm_scan_add_block(scan, &vma->node); } +static bool skip_vma(struct i915_vma *vma) +{ + if (i915_vma_is_active(vma)) + return true; + + if (i915_is_ggtt(vma) && + vma->obj && + i915_gem_object_is_framebuffer(vma->obj)) + return true; + + return false; +} + /** * i915_gem_evict_something - Evict vmas to make room for binding a new one * @vm: address space to evict from @@ -150,7 +163,7 @@ i915_gem_evict_something(struct i915_address_space *vm, * To notice when we complete one full cycle, we record the * first active element seen, before moving it to the tail. */ - if (active != ERR_PTR(-EAGAIN) && i915_vma_is_active(vma)) { + if (active != ERR_PTR(-EAGAIN) && skip_vma(vma)) { if (!active) active = vma; That would be better if we mark the vma as being used by display. -Chris
On Tue, 19 Jan 2021 at 17:02, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Matthew Auld (2021-01-19 16:38:04) > > On Tue, 19 Jan 2021 at 14:49, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > > > > Simplify the frontbuffer unpin by removing the lock requirement. The LRU > > > bumping was primarily to protect the GTT from being evicted and from > > > frontbuffers being eagerly shrunk. Now we protect frontbuffers from the > > > shrinker, and we avoid accidentally evicting from the GTT, so the > > > benefit from bumping LRU is no more, and we can save more time by not. > > > > For the GTT evict case, where/how do we currently try to prevent > > accidental eviction for fb? > > Our preference is to try with NOEVICT, and then use smaller partial > mappings, reducing the risk of evicting anything that may be reused in > the near future. The goal is to really only use the full GTT mapping for > when HW needs access to the whole object. Ah, right. > > However, we could apply the same rule as we do for the shrinker as leave > frontbuffer objects until the second pass. Such as > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > @@ -61,6 +61,19 @@ mark_free(struct drm_mm_scan *scan, > return drm_mm_scan_add_block(scan, &vma->node); > } > > +static bool skip_vma(struct i915_vma *vma) > +{ > + if (i915_vma_is_active(vma)) > + return true; > + > + if (i915_is_ggtt(vma) && > + vma->obj && > + i915_gem_object_is_framebuffer(vma->obj)) > + return true; > + > + return false; > +} > + > /** > * i915_gem_evict_something - Evict vmas to make room for binding a new one > * @vm: address space to evict from > @@ -150,7 +163,7 @@ i915_gem_evict_something(struct i915_address_space *vm, > * To notice when we complete one full cycle, we record the > * first active element seen, before moving it to the tail. > */ > - if (active != ERR_PTR(-EAGAIN) && i915_vma_is_active(vma)) { > + if (active != ERR_PTR(-EAGAIN) && skip_vma(vma)) { > if (!active) > active = vma; > > That would be better if we mark the vma as being used by display. Makes sense. For whichever method, Reviewed-by: Matthew Auld <matthew.auld@intel.com> > -Chris
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index b614987eddf1..3c120a4a6800 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -1430,7 +1430,7 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, */ ret = i915_vma_pin_fence(vma); if (ret != 0 && INTEL_GEN(dev_priv) < 4) { - i915_gem_object_unpin_from_display_plane(vma); + i915_vma_unpin(vma); vma = ERR_PTR(ret); goto err; } @@ -1448,12 +1448,9 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, void intel_unpin_fb_vma(struct i915_vma *vma, unsigned long flags) { - i915_gem_object_lock(vma->obj, NULL); if (flags & PLANE_HAS_FENCE) i915_vma_unpin_fence(vma); - i915_gem_object_unpin_from_display_plane(vma); - i915_gem_object_unlock(vma->obj); - + i915_vma_unpin(vma); i915_vma_put(vma); } diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c index 6be5d8946c69..8edb9b2cdbeb 100644 --- a/drivers/gpu/drm/i915/display/intel_overlay.c +++ b/drivers/gpu/drm/i915/display/intel_overlay.c @@ -360,7 +360,7 @@ static void intel_overlay_release_old_vma(struct intel_overlay *overlay) intel_frontbuffer_flip_complete(overlay->i915, INTEL_FRONTBUFFER_OVERLAY(overlay->crtc->pipe)); - i915_gem_object_unpin_from_display_plane(vma); + i915_vma_unpin(vma); i915_vma_put(vma); } @@ -861,7 +861,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay, return 0; out_unpin: - i915_gem_object_unpin_from_display_plane(vma); + i915_vma_unpin(vma); out_pin_section: atomic_dec(&dev_priv->gpu_error.pending_fb_pin); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c b/drivers/gpu/drm/i915/gem/i915_gem_domain.c index f0379b550dfc..666e4c60d610 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c @@ -433,48 +433,6 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, return vma; } -static void i915_gem_object_bump_inactive_ggtt(struct drm_i915_gem_object *obj) -{ - struct drm_i915_private *i915 = to_i915(obj->base.dev); - struct i915_vma *vma; - - if (list_empty(&obj->vma.list)) - return; - - mutex_lock(&i915->ggtt.vm.mutex); - spin_lock(&obj->vma.lock); - for_each_ggtt_vma(vma, obj) { - if (!drm_mm_node_allocated(&vma->node)) - continue; - - GEM_BUG_ON(vma->vm != &i915->ggtt.vm); - list_move_tail(&vma->vm_link, &vma->vm->bound_list); - } - spin_unlock(&obj->vma.lock); - mutex_unlock(&i915->ggtt.vm.mutex); - - if (i915_gem_object_is_shrinkable(obj)) { - unsigned long flags; - - spin_lock_irqsave(&i915->mm.obj_lock, flags); - - if (obj->mm.madv == I915_MADV_WILLNEED && - !atomic_read(&obj->mm.shrink_pin)) - list_move_tail(&obj->mm.link, &i915->mm.shrink_list); - - spin_unlock_irqrestore(&i915->mm.obj_lock, flags); - } -} - -void -i915_gem_object_unpin_from_display_plane(struct i915_vma *vma) -{ - /* Bump the LRU to try and avoid premature eviction whilst flipping */ - i915_gem_object_bump_inactive_ggtt(vma->obj); - - i915_vma_unpin(vma); -} - /** * Moves a single object to the CPU read, and possibly write domain. * @obj: object to act on @@ -615,9 +573,6 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, else err = i915_gem_object_set_to_cpu_domain(obj, write_domain); - /* And bump the LRU for this access */ - i915_gem_object_bump_inactive_ggtt(obj); - i915_gem_object_unlock(obj); if (write_domain) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 72c699088be0..7ccf786c41a9 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -500,7 +500,6 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj, u32 alignment, const struct i915_ggtt_view *view, unsigned int flags); -void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma); void i915_gem_object_make_unshrinkable(struct drm_i915_gem_object *obj); void i915_gem_object_make_shrinkable(struct drm_i915_gem_object *obj);
Simplify the frontbuffer unpin by removing the lock requirement. The LRU bumping was primarily to protect the GTT from being evicted and from frontbuffers being eagerly shrunk. Now we protect frontbuffers from the shrinker, and we avoid accidentally evicting from the GTT, so the benefit from bumping LRU is no more, and we can save more time by not. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/display/intel_display.c | 7 +-- drivers/gpu/drm/i915/display/intel_overlay.c | 4 +- drivers/gpu/drm/i915/gem/i915_gem_domain.c | 45 -------------------- drivers/gpu/drm/i915/gem/i915_gem_object.h | 1 - 4 files changed, 4 insertions(+), 53 deletions(-)