[Intel-gfx] [PATCH 42/46] drm/i915: Enlarge vma->pin_count

[Chris Wilson]

Quoting John Harrison (2019-01-15 19:57:19)
> On 1/7/2019 03:55, Chris Wilson wrote:
> > Previously we only accommodated having a vma pinned by a small number of
> > users, with the maximum being pinned for use by the display engine. As
> > such, we used a small bitfield only large enough to allow the vma to
> > be pinned twice (for back/front buffers) in each scanout plane. Keeping
> > the maximum permissible pin_count small allows us to quickly catch a
> > potential leak. However, as we want to split a 4096B page into 64
> > different cachelines and pin each cacheline for use by a different
> > timeline, we will exceed the current maximum permissible vma->pin_count
> > and so time has come to enlarge it.
> >
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >   drivers/gpu/drm/i915/i915_gem_gtt.h | 26 +++++++++++++-------------
> >   drivers/gpu/drm/i915/i915_vma.h     | 28 +++++++++-------------------
> >   2 files changed, 22 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index bd679c8c56dd..03ade71b8d9a 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -642,19 +642,19 @@ int i915_gem_gtt_insert(struct i915_address_space *vm,
> >   
> >   /* Flags used by pin/bind&friends. */
> >   #define PIN_NONBLOCK                BIT_ULL(0)
> > -#define PIN_MAPPABLE         BIT_ULL(1)
> > -#define PIN_ZONE_4G          BIT_ULL(2)
> > -#define PIN_NONFAULT         BIT_ULL(3)
> > -#define PIN_NOEVICT          BIT_ULL(4)
> > -
> > -#define PIN_MBZ                      BIT_ULL(5) /* I915_VMA_PIN_OVERFLOW */
> > -#define PIN_GLOBAL           BIT_ULL(6) /* I915_VMA_GLOBAL_BIND */
> > -#define PIN_USER             BIT_ULL(7) /* I915_VMA_LOCAL_BIND */
> > -#define PIN_UPDATE           BIT_ULL(8)
> > -
> > -#define PIN_HIGH             BIT_ULL(9)
> > -#define PIN_OFFSET_BIAS              BIT_ULL(10)
> > -#define PIN_OFFSET_FIXED     BIT_ULL(11)
> > +#define PIN_NONFAULT         BIT_ULL(1)
> > +#define PIN_NOEVICT          BIT_ULL(2)
> > +#define PIN_MAPPABLE         BIT_ULL(3)
> > +#define PIN_ZONE_4G          BIT_ULL(4)
> > +#define PIN_HIGH             BIT_ULL(5)
> > +#define PIN_OFFSET_BIAS              BIT_ULL(6)
> > +#define PIN_OFFSET_FIXED     BIT_ULL(7)
> > +
> > +#define PIN_MBZ                      BIT_ULL(8) /* I915_VMA_PIN_OVERFLOW */
> > +#define PIN_GLOBAL           BIT_ULL(9) /* I915_VMA_GLOBAL_BIND */
> > +#define PIN_USER             BIT_ULL(10) /* I915_VMA_LOCAL_BIND */
> > +#define PIN_UPDATE           BIT_ULL(11)
> > +
> The upper bits need moving to accommodate the larger count. And the 
> HIGH/OFFSET_* fields are not shared with vma-flags so can be moved down 
> with the other pin only flags. But I don't see a reason to shuffle the 
> lower bits around? MAPPABLE to NOEVICT were 1,2,3,4 but are now 3,4,1,2. 
> Is there some semantic meaning to the new order?

Just that:
 - bias, mappable, zone_4g: address limit specifiers
   + high: not strictly an address limit, but an address direction to search
   + fixed: address override, limits still apply though
 - nonblock, nonfault, noevict: search specifiers

I just hadn't had an excuse to reorder them for a while.

> >   #define PIN_OFFSET_MASK             (-I915_GTT_PAGE_SIZE)
> >   
> >   #endif
> > diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> > index 7252abc73d3e..266b226ebef2 100644
> > --- a/drivers/gpu/drm/i915/i915_vma.h
> > +++ b/drivers/gpu/drm/i915/i915_vma.h
> > @@ -70,30 +70,20 @@ struct i915_vma {
> >        */
> >       unsigned int open_count;
> >       unsigned long flags;
> > -     /**
> > -      * How many users have pinned this object in GTT space. The following
> > -      * users can each hold at most one reference: pwrite/pread, execbuffer
> > -      * (objects are not allowed multiple times for the same batchbuffer),
> > -      * and the framebuffer code. When switching/pageflipping, the
> > -      * framebuffer code has at most two buffers pinned per crtc.
> > -      *
> > -      * In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3
> > -      * bits with absolutely no headroom. So use 4 bits.
> > -      */
> Is it not worth keeping some comment about the maximum pin count being 
> bounded so 8-bits is guaranteed to be sufficient? Also, is the old 
> comment actually valid? Surely modern hardware has more than two CRTCs 
> so the limit of 7 was wrong anyway? Maybe even have a compile time 
> assert that the mask size is greater than max(1 + 1 + 1 + 1 + 
> 2*MAX_CRTC, PAGE_SIZE/CACHELINE_SIZE)?

Is a comment accurate? rotfl

Also I think we are up to 3*NUM_PLANES*NUM_CRTCS, but can't be quite sure
with the atomic state tracking, so it might just be still 2 (but just
wait until we have an actual flip queue).

I was also wondering if we used 7b + negative byte for the overflow
would generate better code.

Still a comment towards that this should be bounded to a small number
hence the "validity" in checking for overflow as part of the flags might
be in order.
-Chris