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

Wed Dec 16 21:53:53 UTC 2020

> -----Original Message-----
> From: Chris Wilson <chris at chris-wilson.co.uk>
> Sent: Wednesday, December 16, 2020 12:44 PM
> 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 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.

It looks the upstream code has different behavior than internal DII.  With DII I used softpin flag, but did not see fast pin and hit the bug.

--CQ

> -Chris