[Intel-gfx] [PATCH] drm/i915: Fix mismatch between misplaced vma check and vma insert

[Chris Wilson]

Quoting Tang, CQ (2020-12-16 17:27:40)
> 
> 
> > -----Original Message-----
> > From: Chris Wilson <chris at chris-wilson.co.uk>
> > Sent: Wednesday, December 16, 2020 12:43 AM
> > To: Tang, CQ <cq.tang at intel.com>; intel-gfx at lists.freedesktop.org
> > Cc: stable@ <vger.kernel.orgstable at vger.kernel.org>
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix mismatch between misplaced
> > vma check and vma insert
> > 
> > Quoting Tang, CQ (2020-12-16 00:51:21)
> > >
> > >
> > > > -----Original Message-----
> > > > From: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Sent: Tuesday, December 15, 2020 2:02 PM
> > > > To: Tang, CQ <cq.tang at intel.com>; intel-gfx at lists.freedesktop.org
> > > > Cc: stable@ <vger.kernel.org stable at vger.kernel.org>
> > > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix mismatch between
> > > > misplaced vma check and vma insert
> > > >
> > > > Quoting Tang, CQ (2020-12-15 21:50:53)
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > > Sent: Tuesday, December 15, 2020 12:31 PM
> > > > > > To: intel-gfx at lists.freedesktop.org
> > > > > > Cc: Chris Wilson <chris at chris-wilson.co.uk>; Tang, CQ
> > > > > > <cq.tang at intel.com>; stable at vger.kernel.org
> > > > > > Subject: [PATCH] drm/i915: Fix mismatch between misplaced vma
> > > > > > check and vma insert
> > > > > >
> > > > > > When inserting a VMA, we restrict the placement to the low 4G
> > > > > > unless the caller opts into using the full range. This was done
> > > > > > to allow usersapce the opportunity to transition slowly from a
> > > > > > 32b address space, and to avoid breaking inherent 32b
> > > > > > assumptions of some
> > > > commands.
> > > > > >
> > > > > > However, for insert we limited ourselves to 4G-4K, but on
> > > > > > verification we allowed the full 4G. This causes some attempts
> > > > > > to bind a new buffer to sporadically fail with -ENOSPC, but at
> > > > > > other times be
> > > > bound successfully.
> > > > > >
> > > > > > commit 48ea1e32c39d ("drm/i915/gen9: Set PIN_ZONE_4G end to
> > 4GB
> > > > > > - 1
> > > > > > page") suggests that there is a genuine problem with stateless
> > > > > > addressing that cannot utilize the last page in 4G and so we
> > > > > > purposefully
> > > > excluded it.
> > > > > >
> > > > > > Reported-by: CQ Tang <cq.tang at intel.com>
> > > > > > Fixes: 48ea1e32c39d ("drm/i915/gen9: Set PIN_ZONE_4G end to 4GB
> > > > > > - 1
> > > > > > page")
> > > > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > > > Cc: CQ Tang <cq.tang at intel.com>
> > > > > > Cc: stable at vger.kernel.org
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > index 193996144c84..2ff32daa50bd 100644
> > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> > > > > > @@ -382,7 +382,7 @@ eb_vma_misplaced(const struct
> > > > > > drm_i915_gem_exec_object2 *entry,
> > > > > >               return true;
> > > > > >
> > > > > >       if (!(flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS) &&
> > > > > > -         (vma->node.start + vma->node.size - 1) >> 32)
> > > > > > +         (vma->node.start + vma->node.size + 4095) >> 32)
> > > > >
> > > > > Why 4095 not 4096?
> > > >
> > > > It's the nature of the test that we need an inclusive bound.
> > > >
> > > > Consider an object of size 4G - 4K, that is allowed to fit within our 32b GTT.
> > > >
> > > >       4G - 4k + 4K = 4G == 1 << 32: => vma misplaced
> > > >
> > > >       4G - 4k + 4k - 1 = 4G -1 = 0xffffffff => vma ok
> > >
> > > How do we trigger this code?  I run gem_exec_params at larger-than-life-
> > batch but did not see this code is executed.
> > > Basically how do we triggre first attempt to pin the object in place.
> > 
> > It's the first pin tried, but the incoming execobj.offset must be available and
> > the object itself must be ready to be pinned. That's true for the current tree
> > on all current gen.
> 
> For gem_exec_params at larger-than-life-batch subtest, I only see i915_vma_misplaced() be called when EXEC_OBJECT_SUPPORTS_48B_ADDRESS flags is specified, and the test passes.
> I want to catch the bug before you fixed here. So a 4GB object should be OK, because before your fix, i915_vma_misplaced() returns false.
> I did specify execobj.offset=0, but the driver code goes to i915_vma_insert() directly and return -ENOSPC.
> 
> How do I make gem_exec_params at larger-than-life-batch code to catch this bug?

Prior to the patch, upstream on current gen would always take the fast
pin and hit the bug. It will only fail to take that path if it was not
_allowed_ to place the object at offset 0.

To explicitly test that the page is excluded we would use softpin.
-Chris