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

[John Harrison]

On 1/15/2019 12:17, Chris Wilson wrote:
> 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.
Fair enough. I just wanted to check it was deliberate and not some 
accidental remnant of some other change that was dropped along the way.

>
>>>    #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
One can but dream...

> 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).
That sounds like we should already be overflowing the current limit of 16?!

>
> I was also wondering if we used 7b + negative byte for the overflow
> would generate better code.
Meaning limit the count to 127 and use a signed char cast? Thus test for 
-ve rather than BIT(x)? Maybe. Although the above comment makes me 
nervous that even 127 might not be sufficient for very much longer. New 
hardware always brings more planes and heads!

>
> 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

I think some kind of comment along those lines would be worth having.

With that comment added:
Reviewed-by: John Harrison <John.C.Harrison at Intel.com>