Re: [Intel-gfx] [PATCH 6/6] drm/i915/gem: Drop lru bumping on display unpinning

From: Matthew Auld <matthew.william.auld@gmail.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 6/6] drm/i915/gem: Drop lru bumping on display unpinning
Date: Tue, 19 Jan 2021 17:14:45 +0000	[thread overview]
Message-ID: <CAM0jSHO3cbwrmX-YzxSStNmwdxNBOOf1mVib0pTSVn=cFuUiYQ@mail.gmail.com> (raw)
In-Reply-To: <161107575499.7548.13117018595783582424@build.alporthouse.com>

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx