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

[John Harrison]

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?


>   #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)?


> -#define I915_VMA_PIN_MASK 0xf
> -#define I915_VMA_PIN_OVERFLOW	BIT(5)
> +#define I915_VMA_PIN_MASK 0xff
> +#define I915_VMA_PIN_OVERFLOW	BIT(8)
>   
>   	/** Flags and address space this VMA is bound to */
> -#define I915_VMA_GLOBAL_BIND	BIT(6)
> -#define I915_VMA_LOCAL_BIND	BIT(7)
> +#define I915_VMA_GLOBAL_BIND	BIT(9)
> +#define I915_VMA_LOCAL_BIND	BIT(10)
>   #define I915_VMA_BIND_MASK (I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND | I915_VMA_PIN_OVERFLOW)
>   
> -#define I915_VMA_GGTT		BIT(8)
> -#define I915_VMA_CAN_FENCE	BIT(9)
> -#define I915_VMA_CLOSED		BIT(10)
> -#define I915_VMA_USERFAULT_BIT	11
> +#define I915_VMA_GGTT		BIT(11)
> +#define I915_VMA_CAN_FENCE	BIT(12)
> +#define I915_VMA_CLOSED		BIT(13)
> +#define I915_VMA_USERFAULT_BIT	14
>   #define I915_VMA_USERFAULT	BIT(I915_VMA_USERFAULT_BIT)
> -#define I915_VMA_GGTT_WRITE	BIT(12)
> +#define I915_VMA_GGTT_WRITE	BIT(15)
>   
>   	unsigned int active_count;
>   	struct rb_root active;