[2/2] drm/i915/gem: Don't try to map and fence large scanout buffers (v6)

Commit Message

Kasireddy, Vivek Feb. 4, 2022, 1:22 a.m. UTC

On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2 or
more framebuffers/scanout buffers results in only one that is mappable/
fenceable. Therefore, pageflipping between these 2 FBs where only one
is mappable/fenceable creates latencies large enough to miss alternate
vblanks thereby producing less optimal framerate.

This mainly happens because when i915_gem_object_pin_to_display_plane()
is called to pin one of the FB objs, the associated vma is identified
as misplaced and therefore i915_vma_unbind() is called which unbinds and
evicts it. This misplaced vma gets subseqently pinned only when
i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
results in a latency of ~10ms and happens every other vblank/repaint cycle.
Therefore, to fix this issue, we try to see if there is space to map
at-least two objects of a given size and return early if there isn't. This
would ensure that we do not try with PIN_MAPPABLE for any objects that
are too big to map thereby preventing unncessary unbind.

Testcase:
Running Weston and weston-simple-egl on an Alderlake_S (ADLS) platform
with a 8K@60 mode results in only ~40 FPS. Since upstream Weston submits
a frame ~7ms before the next vblank, the latencies seen between atomic
commit and flip event are 7, 24 (7 + 16.66), 7, 24..... suggesting that
it misses the vblank every other frame.

Here is the ftrace snippet that shows the source of the ~10ms latency:
              i915_gem_object_pin_to_display_plane() {
0.102 us   |    i915_gem_object_set_cache_level();
                i915_gem_object_ggtt_pin_ww() {
0.390 us   |      i915_vma_instance();
0.178 us   |      i915_vma_misplaced();
                  i915_vma_unbind() {
                  __i915_active_wait() {
0.082 us   |        i915_active_acquire_if_busy();
0.475 us   |      }
                  intel_runtime_pm_get() {
0.087 us   |        intel_runtime_pm_acquire();
0.259 us   |      }
                  __i915_active_wait() {
0.085 us   |        i915_active_acquire_if_busy();
0.240 us   |      }
                  __i915_vma_evict() {
                    ggtt_unbind_vma() {
                      gen8_ggtt_clear_range() {
10507.255 us |        }
10507.689 us |      }
10508.516 us |   }

v2: Instead of using bigjoiner checks, determine whether a scanout
    buffer is too big by checking to see if it is possible to map
    two of them into the ggtt.

v3 (Ville):
- Count how many fb objects can be fit into the available holes
  instead of checking for a hole twice the object size.
- Take alignment constraints into account.
- Limit this large scanout buffer check to >= Gen 11 platforms.

v4:
- Remove existing heuristic that checks just for size. (Ville)
- Return early if we find space to map at-least two objects. (Tvrtko)
- Slightly update the commit message.

v5: (Tvrtko)
- Rename the function to indicate that the object may be too big to
  map into the aperture.
- Account for guard pages while calculating the total size required
  for the object.
- Do not subject all objects to the heuristic check and instead
  consider objects only of a certain size.
- Do the hole walk using the rbtree.
- Preserve the existing PIN_NONBLOCK logic.
- Drop the PIN_MAPPABLE check while pinning the VMA.

v6: (Tvrtko)
- Return 0 on success and the specific error code on failure to
  preserve the existing behavior.

Cc: Ville Syrjälä <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Tvrtko Ursulin <[e-mail]>
Cc: Manasi Navare <[e-mail]>
Signed-off-by: Vivek Kasireddy <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
 1 file changed, 90 insertions(+), 30 deletions(-)

Comments

Tvrtko Ursulin Feb. 7, 2022, 10:49 a.m. UTC | #1

On 04/02/2022 01:22, Vivek Kasireddy wrote:
> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2 or
> more framebuffers/scanout buffers results in only one that is mappable/
> fenceable. Therefore, pageflipping between these 2 FBs where only one
> is mappable/fenceable creates latencies large enough to miss alternate
> vblanks thereby producing less optimal framerate.
> 
> This mainly happens because when i915_gem_object_pin_to_display_plane()
> is called to pin one of the FB objs, the associated vma is identified
> as misplaced and therefore i915_vma_unbind() is called which unbinds and
> evicts it. This misplaced vma gets subseqently pinned only when
> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> Therefore, to fix this issue, we try to see if there is space to map
> at-least two objects of a given size and return early if there isn't. This
> would ensure that we do not try with PIN_MAPPABLE for any objects that
> are too big to map thereby preventing unncessary unbind.
> 
> Testcase:
> Running Weston and weston-simple-egl on an Alderlake_S (ADLS) platform
> with a 8K@60 mode results in only ~40 FPS. Since upstream Weston submits
> a frame ~7ms before the next vblank, the latencies seen between atomic
> commit and flip event are 7, 24 (7 + 16.66), 7, 24..... suggesting that
> it misses the vblank every other frame.
> 
> Here is the ftrace snippet that shows the source of the ~10ms latency:
>                i915_gem_object_pin_to_display_plane() {
> 0.102 us   |    i915_gem_object_set_cache_level();
>                  i915_gem_object_ggtt_pin_ww() {
> 0.390 us   |      i915_vma_instance();
> 0.178 us   |      i915_vma_misplaced();
>                    i915_vma_unbind() {
>                    __i915_active_wait() {
> 0.082 us   |        i915_active_acquire_if_busy();
> 0.475 us   |      }
>                    intel_runtime_pm_get() {
> 0.087 us   |        intel_runtime_pm_acquire();
> 0.259 us   |      }
>                    __i915_active_wait() {
> 0.085 us   |        i915_active_acquire_if_busy();
> 0.240 us   |      }
>                    __i915_vma_evict() {
>                      ggtt_unbind_vma() {
>                        gen8_ggtt_clear_range() {
> 10507.255 us |        }
> 10507.689 us |      }
> 10508.516 us |   }
> 
> v2: Instead of using bigjoiner checks, determine whether a scanout
>      buffer is too big by checking to see if it is possible to map
>      two of them into the ggtt.
> 
> v3 (Ville):
> - Count how many fb objects can be fit into the available holes
>    instead of checking for a hole twice the object size.
> - Take alignment constraints into account.
> - Limit this large scanout buffer check to >= Gen 11 platforms.
> 
> v4:
> - Remove existing heuristic that checks just for size. (Ville)
> - Return early if we find space to map at-least two objects. (Tvrtko)
> - Slightly update the commit message.
> 
> v5: (Tvrtko)
> - Rename the function to indicate that the object may be too big to
>    map into the aperture.
> - Account for guard pages while calculating the total size required
>    for the object.
> - Do not subject all objects to the heuristic check and instead
>    consider objects only of a certain size.
> - Do the hole walk using the rbtree.
> - Preserve the existing PIN_NONBLOCK logic.
> - Drop the PIN_MAPPABLE check while pinning the VMA.
> 
> v6: (Tvrtko)
> - Return 0 on success and the specific error code on failure to
>    preserve the existing behavior.
> 
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Tvrtko Ursulin <[e-mail]>
> Cc: Manasi Navare <[e-mail]>
> Signed-off-by: Vivek Kasireddy <[e-mail]>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
>   1 file changed, 90 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e3a2c2a0e156..39f0d17550c3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -46,6 +46,7 @@
>   #include "gem/i915_gem_mman.h"
>   #include "gem/i915_gem_region.h"
>   #include "gem/i915_gem_userptr.h"
> +#include "gem/i915_gem_tiling.h"
>   #include "gt/intel_engine_user.h"
>   #include "gt/intel_gt.h"
>   #include "gt/intel_gt_pm.h"
> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
>   	spin_unlock(&obj->vma.lock);
>   }
>   
> +static int
> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> +				 u64 alignment, u64 flags)
> +{
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> +	struct drm_mm_node *hole;
> +	u64 hole_start, hole_end, start, end;
> +	u64 fence_size, fence_alignment;
> +	unsigned int count = 0;
> +
> +	/*
> +	 * If the required space is larger than the available
> +	 * aperture, we will not able to find a slot for the
> +	 * object and unbinding the object now will be in
> +	 * vain. Worse, doing so may cause us to ping-pong
> +	 * the object in and out of the Global GTT and
> +	 * waste a lot of cycles under the mutex.
> +	 */
> +	if (obj->base.size > ggtt->mappable_end)
> +		return -E2BIG;
> +
> +	/*
> +	 * If NONBLOCK is set the caller is optimistically
> +	 * trying to cache the full object within the mappable
> +	 * aperture, and *must* have a fallback in place for
> +	 * situations where we cannot bind the object. We
> +	 * can be a little more lax here and use the fallback
> +	 * more often to avoid costly migrations of ourselves
> +	 * and other objects within the aperture.
> +	 */
> +	if (!(flags & PIN_NONBLOCK))
> +		return 0;
> +
> +	/*
> +	 * We only consider objects whose size is at-least a quarter of
> +	 * the aperture to be too big and subject them to the new
> +	 * heuristic below.
> +	 */
> +	if (obj->base.size < ggtt->mappable_end / 4)
> +		return 0;
> +
> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> +	    !i915_gem_object_is_framebuffer(obj))
> +		return 0;
> +
> +	fence_size = i915_gem_fence_size(i915, obj->base.size,
> +					 i915_gem_object_get_tiling(obj),
> +					 i915_gem_object_get_stride(obj));
> +
> +	if (i915_vm_has_cache_coloring(&ggtt->vm))
> +		fence_size += 2 * I915_GTT_PAGE_SIZE;
> +
> +	fence_alignment = i915_gem_fence_alignment(i915, obj->base.size,
> +						   i915_gem_object_get_tiling(obj),
> +						   i915_gem_object_get_stride(obj));
> +	alignment = max_t(u64, alignment, fence_alignment);
> +
> +	/*
> +	 * Assuming this object is a large scanout buffer, we try to find
> +	 * out if there is room to map at-least two of them. There could
> +	 * be space available to map one but to be consistent, we try to
> +	 * avoid mapping/fencing any of them.
> +	 */
> +	drm_mm_for_each_suitable_hole(hole, &ggtt->vm.mm, 0, ggtt->mappable_end,
> +				      fence_size, DRM_MM_INSERT_LOW) {
> +		hole_start = drm_mm_hole_node_start(hole);
> +		hole_end = hole_start + hole->hole_size;
> +
> +		do {
> +			start = round_up(hole_start, alignment);
> +			end = min_t(u64, hole_end, ggtt->mappable_end);
> +
> +			if (range_overflows(start, fence_size, end))
> +				break;
> +
> +			if (++count >= 2)
> +				return 0;
> +
> +			hole_start = start + fence_size;
> +		} while (1);
> +	}
> +
> +	return -ENOSPC;
> +}
> +
>   struct i915_vma *
>   i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>   			    struct i915_gem_ww_ctx *ww,
> @@ -891,36 +978,9 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>   
>   	if (flags & PIN_MAPPABLE &&
>   	    (!view || view->type == I915_GGTT_VIEW_NORMAL)) {
> -		/*
> -		 * If the required space is larger than the available
> -		 * aperture, we will not able to find a slot for the
> -		 * object and unbinding the object now will be in
> -		 * vain. Worse, doing so may cause us to ping-pong
> -		 * the object in and out of the Global GTT and
> -		 * waste a lot of cycles under the mutex.
> -		 */
> -		if (obj->base.size > ggtt->mappable_end)
> -			return ERR_PTR(-E2BIG);
> -
> -		/*
> -		 * If NONBLOCK is set the caller is optimistically
> -		 * trying to cache the full object within the mappable
> -		 * aperture, and *must* have a fallback in place for
> -		 * situations where we cannot bind the object. We
> -		 * can be a little more lax here and use the fallback
> -		 * more often to avoid costly migrations of ourselves
> -		 * and other objects within the aperture.
> -		 *
> -		 * Half-the-aperture is used as a simple heuristic.
> -		 * More interesting would to do search for a free
> -		 * block prior to making the commitment to unbind.
> -		 * That caters for the self-harm case, and with a
> -		 * little more heuristics (e.g. NOFAULT, NOEVICT)
> -		 * we could try to minimise harm to others.
> -		 */
> -		if (flags & PIN_NONBLOCK &&
> -		    obj->base.size > ggtt->mappable_end / 2)
> -			return ERR_PTR(-ENOSPC);
> +		ret = i915_gem_object_fits_in_aperture(obj, alignment, flags);
> +		if (ret)
> +			return ERR_PTR(ret);
>   	}
>   
>   new_vma:

Reviewed-by: Tvrtko Ursulin <[e-mail]>

Regards,

Tvrtko

Ville Syrjälä Feb. 7, 2022, 10:58 a.m. UTC | #2

On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2 or
> more framebuffers/scanout buffers results in only one that is mappable/
> fenceable. Therefore, pageflipping between these 2 FBs where only one
> is mappable/fenceable creates latencies large enough to miss alternate
> vblanks thereby producing less optimal framerate.
> 
> This mainly happens because when i915_gem_object_pin_to_display_plane()
> is called to pin one of the FB objs, the associated vma is identified
> as misplaced and therefore i915_vma_unbind() is called which unbinds and
> evicts it. This misplaced vma gets subseqently pinned only when
> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> Therefore, to fix this issue, we try to see if there is space to map
> at-least two objects of a given size and return early if there isn't. This
> would ensure that we do not try with PIN_MAPPABLE for any objects that
> are too big to map thereby preventing unncessary unbind.
> 
> Testcase:
> Running Weston and weston-simple-egl on an Alderlake_S (ADLS) platform
> with a 8K@60 mode results in only ~40 FPS. Since upstream Weston submits
> a frame ~7ms before the next vblank, the latencies seen between atomic
> commit and flip event are 7, 24 (7 + 16.66), 7, 24..... suggesting that
> it misses the vblank every other frame.
> 
> Here is the ftrace snippet that shows the source of the ~10ms latency:
>               i915_gem_object_pin_to_display_plane() {
> 0.102 us   |    i915_gem_object_set_cache_level();
>                 i915_gem_object_ggtt_pin_ww() {
> 0.390 us   |      i915_vma_instance();
> 0.178 us   |      i915_vma_misplaced();
>                   i915_vma_unbind() {
>                   __i915_active_wait() {
> 0.082 us   |        i915_active_acquire_if_busy();
> 0.475 us   |      }
>                   intel_runtime_pm_get() {
> 0.087 us   |        intel_runtime_pm_acquire();
> 0.259 us   |      }
>                   __i915_active_wait() {
> 0.085 us   |        i915_active_acquire_if_busy();
> 0.240 us   |      }
>                   __i915_vma_evict() {
>                     ggtt_unbind_vma() {
>                       gen8_ggtt_clear_range() {
> 10507.255 us |        }
> 10507.689 us |      }
> 10508.516 us |   }
> 
> v2: Instead of using bigjoiner checks, determine whether a scanout
>     buffer is too big by checking to see if it is possible to map
>     two of them into the ggtt.
> 
> v3 (Ville):
> - Count how many fb objects can be fit into the available holes
>   instead of checking for a hole twice the object size.
> - Take alignment constraints into account.
> - Limit this large scanout buffer check to >= Gen 11 platforms.
> 
> v4:
> - Remove existing heuristic that checks just for size. (Ville)
> - Return early if we find space to map at-least two objects. (Tvrtko)
> - Slightly update the commit message.
> 
> v5: (Tvrtko)
> - Rename the function to indicate that the object may be too big to
>   map into the aperture.
> - Account for guard pages while calculating the total size required
>   for the object.
> - Do not subject all objects to the heuristic check and instead
>   consider objects only of a certain size.
> - Do the hole walk using the rbtree.
> - Preserve the existing PIN_NONBLOCK logic.
> - Drop the PIN_MAPPABLE check while pinning the VMA.
> 
> v6: (Tvrtko)
> - Return 0 on success and the specific error code on failure to
>   preserve the existing behavior.
> 
> Cc: Ville Syrjälä <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Tvrtko Ursulin <[e-mail]>
> Cc: Manasi Navare <[e-mail]>
> Signed-off-by: Vivek Kasireddy <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
>  1 file changed, 90 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e3a2c2a0e156..39f0d17550c3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -46,6 +46,7 @@
>  #include "gem/i915_gem_mman.h"
>  #include "gem/i915_gem_region.h"
>  #include "gem/i915_gem_userptr.h"
> +#include "gem/i915_gem_tiling.h"
>  #include "gt/intel_engine_user.h"
>  #include "gt/intel_gt.h"
>  #include "gt/intel_gt_pm.h"
> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
>  	spin_unlock(&obj->vma.lock);
>  }
>  
> +static int
> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> +				 u64 alignment, u64 flags)
> +{
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> +	struct drm_mm_node *hole;
> +	u64 hole_start, hole_end, start, end;
> +	u64 fence_size, fence_alignment;
> +	unsigned int count = 0;
> +
> +	/*
> +	 * If the required space is larger than the available
> +	 * aperture, we will not able to find a slot for the
> +	 * object and unbinding the object now will be in
> +	 * vain. Worse, doing so may cause us to ping-pong
> +	 * the object in and out of the Global GTT and
> +	 * waste a lot of cycles under the mutex.
> +	 */
> +	if (obj->base.size > ggtt->mappable_end)
> +		return -E2BIG;
> +
> +	/*
> +	 * If NONBLOCK is set the caller is optimistically
> +	 * trying to cache the full object within the mappable
> +	 * aperture, and *must* have a fallback in place for
> +	 * situations where we cannot bind the object. We
> +	 * can be a little more lax here and use the fallback
> +	 * more often to avoid costly migrations of ourselves
> +	 * and other objects within the aperture.
> +	 */
> +	if (!(flags & PIN_NONBLOCK))
> +		return 0;
> +
> +	/*
> +	 * We only consider objects whose size is at-least a quarter of
> +	 * the aperture to be too big and subject them to the new
> +	 * heuristic below.
> +	 */
> +	if (obj->base.size < ggtt->mappable_end / 4)
> +		return 0;

That seems a fairly arbitrary thing to put here. Maybe something the
caller should check/specify?

> +
> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> +	    !i915_gem_object_is_framebuffer(obj))
> +		return 0;

None of that seems appropriate for a generic gem function
with this name.

> +
> +	fence_size = i915_gem_fence_size(i915, obj->base.size,
> +					 i915_gem_object_get_tiling(obj),
> +					 i915_gem_object_get_stride(obj));
> +
> +	if (i915_vm_has_cache_coloring(&ggtt->vm))
> +		fence_size += 2 * I915_GTT_PAGE_SIZE;
> +
> +	fence_alignment = i915_gem_fence_alignment(i915, obj->base.size,
> +						   i915_gem_object_get_tiling(obj),
> +						   i915_gem_object_get_stride(obj));
> +	alignment = max_t(u64, alignment, fence_alignment);
> +
> +	/*
> +	 * Assuming this object is a large scanout buffer, we try to find
> +	 * out if there is room to map at-least two of them. There could
> +	 * be space available to map one but to be consistent, we try to
> +	 * avoid mapping/fencing any of them.
> +	 */
> +	drm_mm_for_each_suitable_hole(hole, &ggtt->vm.mm, 0, ggtt->mappable_end,
> +				      fence_size, DRM_MM_INSERT_LOW) {
> +		hole_start = drm_mm_hole_node_start(hole);
> +		hole_end = hole_start + hole->hole_size;
> +
> +		do {
> +			start = round_up(hole_start, alignment);
> +			end = min_t(u64, hole_end, ggtt->mappable_end);
> +
> +			if (range_overflows(start, fence_size, end))
> +				break;
> +
> +			if (++count >= 2)
> +				return 0;
> +
> +			hole_start = start + fence_size;
> +		} while (1);
> +	}
> +
> +	return -ENOSPC;
> +}
> +
>  struct i915_vma *
>  i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>  			    struct i915_gem_ww_ctx *ww,
> @@ -891,36 +978,9 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>  
>  	if (flags & PIN_MAPPABLE &&
>  	    (!view || view->type == I915_GGTT_VIEW_NORMAL)) {
> -		/*
> -		 * If the required space is larger than the available
> -		 * aperture, we will not able to find a slot for the
> -		 * object and unbinding the object now will be in
> -		 * vain. Worse, doing so may cause us to ping-pong
> -		 * the object in and out of the Global GTT and
> -		 * waste a lot of cycles under the mutex.
> -		 */
> -		if (obj->base.size > ggtt->mappable_end)
> -			return ERR_PTR(-E2BIG);
> -
> -		/*
> -		 * If NONBLOCK is set the caller is optimistically
> -		 * trying to cache the full object within the mappable
> -		 * aperture, and *must* have a fallback in place for
> -		 * situations where we cannot bind the object. We
> -		 * can be a little more lax here and use the fallback
> -		 * more often to avoid costly migrations of ourselves
> -		 * and other objects within the aperture.
> -		 *
> -		 * Half-the-aperture is used as a simple heuristic.
> -		 * More interesting would to do search for a free
> -		 * block prior to making the commitment to unbind.
> -		 * That caters for the self-harm case, and with a
> -		 * little more heuristics (e.g. NOFAULT, NOEVICT)
> -		 * we could try to minimise harm to others.
> -		 */
> -		if (flags & PIN_NONBLOCK &&
> -		    obj->base.size > ggtt->mappable_end / 2)
> -			return ERR_PTR(-ENOSPC);
> +		ret = i915_gem_object_fits_in_aperture(obj, alignment, flags);
> +		if (ret)
> +			return ERR_PTR(ret);
>  	}
>  
>  new_vma:
> -- 
> 2.34.1

Tvrtko Ursulin Feb. 7, 2022, 11:47 a.m. UTC | #3

On 07/02/2022 10:58, Ville Syrjälä wrote:
> On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
>> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2 or
>> more framebuffers/scanout buffers results in only one that is mappable/
>> fenceable. Therefore, pageflipping between these 2 FBs where only one
>> is mappable/fenceable creates latencies large enough to miss alternate
>> vblanks thereby producing less optimal framerate.
>>
>> This mainly happens because when i915_gem_object_pin_to_display_plane()
>> is called to pin one of the FB objs, the associated vma is identified
>> as misplaced and therefore i915_vma_unbind() is called which unbinds and
>> evicts it. This misplaced vma gets subseqently pinned only when
>> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
>> results in a latency of ~10ms and happens every other vblank/repaint cycle.
>> Therefore, to fix this issue, we try to see if there is space to map
>> at-least two objects of a given size and return early if there isn't. This
>> would ensure that we do not try with PIN_MAPPABLE for any objects that
>> are too big to map thereby preventing unncessary unbind.
>>
>> Testcase:
>> Running Weston and weston-simple-egl on an Alderlake_S (ADLS) platform
>> with a 8K@60 mode results in only ~40 FPS. Since upstream Weston submits
>> a frame ~7ms before the next vblank, the latencies seen between atomic
>> commit and flip event are 7, 24 (7 + 16.66), 7, 24..... suggesting that
>> it misses the vblank every other frame.
>>
>> Here is the ftrace snippet that shows the source of the ~10ms latency:
>>                i915_gem_object_pin_to_display_plane() {
>> 0.102 us   |    i915_gem_object_set_cache_level();
>>                  i915_gem_object_ggtt_pin_ww() {
>> 0.390 us   |      i915_vma_instance();
>> 0.178 us   |      i915_vma_misplaced();
>>                    i915_vma_unbind() {
>>                    __i915_active_wait() {
>> 0.082 us   |        i915_active_acquire_if_busy();
>> 0.475 us   |      }
>>                    intel_runtime_pm_get() {
>> 0.087 us   |        intel_runtime_pm_acquire();
>> 0.259 us   |      }
>>                    __i915_active_wait() {
>> 0.085 us   |        i915_active_acquire_if_busy();
>> 0.240 us   |      }
>>                    __i915_vma_evict() {
>>                      ggtt_unbind_vma() {
>>                        gen8_ggtt_clear_range() {
>> 10507.255 us |        }
>> 10507.689 us |      }
>> 10508.516 us |   }
>>
>> v2: Instead of using bigjoiner checks, determine whether a scanout
>>      buffer is too big by checking to see if it is possible to map
>>      two of them into the ggtt.
>>
>> v3 (Ville):
>> - Count how many fb objects can be fit into the available holes
>>    instead of checking for a hole twice the object size.
>> - Take alignment constraints into account.
>> - Limit this large scanout buffer check to >= Gen 11 platforms.
>>
>> v4:
>> - Remove existing heuristic that checks just for size. (Ville)
>> - Return early if we find space to map at-least two objects. (Tvrtko)
>> - Slightly update the commit message.
>>
>> v5: (Tvrtko)
>> - Rename the function to indicate that the object may be too big to
>>    map into the aperture.
>> - Account for guard pages while calculating the total size required
>>    for the object.
>> - Do not subject all objects to the heuristic check and instead
>>    consider objects only of a certain size.
>> - Do the hole walk using the rbtree.
>> - Preserve the existing PIN_NONBLOCK logic.
>> - Drop the PIN_MAPPABLE check while pinning the VMA.
>>
>> v6: (Tvrtko)
>> - Return 0 on success and the specific error code on failure to
>>    preserve the existing behavior.
>>
>> Cc: Ville Syrjälä <[e-mail]>
>> Cc: Maarten Lankhorst <[e-mail]>
>> Cc: Tvrtko Ursulin <[e-mail]>
>> Cc: Manasi Navare <[e-mail]>
>> Signed-off-by: Vivek Kasireddy <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
>>   1 file changed, 90 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index e3a2c2a0e156..39f0d17550c3 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -46,6 +46,7 @@
>>   #include "gem/i915_gem_mman.h"
>>   #include "gem/i915_gem_region.h"
>>   #include "gem/i915_gem_userptr.h"
>> +#include "gem/i915_gem_tiling.h"
>>   #include "gt/intel_engine_user.h"
>>   #include "gt/intel_gt.h"
>>   #include "gt/intel_gt_pm.h"
>> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
>>   	spin_unlock(&obj->vma.lock);
>>   }
>>   
>> +static int
>> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
>> +				 u64 alignment, u64 flags)
>> +{
>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
>> +	struct drm_mm_node *hole;
>> +	u64 hole_start, hole_end, start, end;
>> +	u64 fence_size, fence_alignment;
>> +	unsigned int count = 0;
>> +
>> +	/*
>> +	 * If the required space is larger than the available
>> +	 * aperture, we will not able to find a slot for the
>> +	 * object and unbinding the object now will be in
>> +	 * vain. Worse, doing so may cause us to ping-pong
>> +	 * the object in and out of the Global GTT and
>> +	 * waste a lot of cycles under the mutex.
>> +	 */
>> +	if (obj->base.size > ggtt->mappable_end)
>> +		return -E2BIG;
>> +
>> +	/*
>> +	 * If NONBLOCK is set the caller is optimistically
>> +	 * trying to cache the full object within the mappable
>> +	 * aperture, and *must* have a fallback in place for
>> +	 * situations where we cannot bind the object. We
>> +	 * can be a little more lax here and use the fallback
>> +	 * more often to avoid costly migrations of ourselves
>> +	 * and other objects within the aperture.
>> +	 */
>> +	if (!(flags & PIN_NONBLOCK))
>> +		return 0;
>> +
>> +	/*
>> +	 * We only consider objects whose size is at-least a quarter of
>> +	 * the aperture to be too big and subject them to the new
>> +	 * heuristic below.
>> +	 */
>> +	if (obj->base.size < ggtt->mappable_end / 4)
>> +		return 0;
> 
> That seems a fairly arbitrary thing to put here. Maybe something the
> caller should check/specify?

I have no strong opinion on this one. In my mind I categorised it under 
"is it a large framebuffer" heuristics. Previously it was less than one 
half of aperture always okay, now one quarter, plus 2x hole check if 
larger. Both are heuristics. I even mentioned earlier if 2x should be an 
input parameter as well, but again, given it's not an exported function 
couldn't really justify it.

> 
>> +
>> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
>> +	    !i915_gem_object_is_framebuffer(obj))
>> +		return 0;
> 
> None of that seems appropriate for a generic gem function
> with this name.

It's not exported though, maybe remove i915_gem prefix to avoid any 
ideas of it being generic?

Regards,

Tvrtko

> 
>> +
>> +	fence_size = i915_gem_fence_size(i915, obj->base.size,
>> +					 i915_gem_object_get_tiling(obj),
>> +					 i915_gem_object_get_stride(obj));
>> +
>> +	if (i915_vm_has_cache_coloring(&ggtt->vm))
>> +		fence_size += 2 * I915_GTT_PAGE_SIZE;
>> +
>> +	fence_alignment = i915_gem_fence_alignment(i915, obj->base.size,
>> +						   i915_gem_object_get_tiling(obj),
>> +						   i915_gem_object_get_stride(obj));
>> +	alignment = max_t(u64, alignment, fence_alignment);
>> +
>> +	/*
>> +	 * Assuming this object is a large scanout buffer, we try to find
>> +	 * out if there is room to map at-least two of them. There could
>> +	 * be space available to map one but to be consistent, we try to
>> +	 * avoid mapping/fencing any of them.
>> +	 */
>> +	drm_mm_for_each_suitable_hole(hole, &ggtt->vm.mm, 0, ggtt->mappable_end,
>> +				      fence_size, DRM_MM_INSERT_LOW) {
>> +		hole_start = drm_mm_hole_node_start(hole);
>> +		hole_end = hole_start + hole->hole_size;
>> +
>> +		do {
>> +			start = round_up(hole_start, alignment);
>> +			end = min_t(u64, hole_end, ggtt->mappable_end);
>> +
>> +			if (range_overflows(start, fence_size, end))
>> +				break;
>> +
>> +			if (++count >= 2)
>> +				return 0;
>> +
>> +			hole_start = start + fence_size;
>> +		} while (1);
>> +	}
>> +
>> +	return -ENOSPC;
>> +}
>> +
>>   struct i915_vma *
>>   i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>>   			    struct i915_gem_ww_ctx *ww,
>> @@ -891,36 +978,9 @@ i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
>>   
>>   	if (flags & PIN_MAPPABLE &&
>>   	    (!view || view->type == I915_GGTT_VIEW_NORMAL)) {
>> -		/*
>> -		 * If the required space is larger than the available
>> -		 * aperture, we will not able to find a slot for the
>> -		 * object and unbinding the object now will be in
>> -		 * vain. Worse, doing so may cause us to ping-pong
>> -		 * the object in and out of the Global GTT and
>> -		 * waste a lot of cycles under the mutex.
>> -		 */
>> -		if (obj->base.size > ggtt->mappable_end)
>> -			return ERR_PTR(-E2BIG);
>> -
>> -		/*
>> -		 * If NONBLOCK is set the caller is optimistically
>> -		 * trying to cache the full object within the mappable
>> -		 * aperture, and *must* have a fallback in place for
>> -		 * situations where we cannot bind the object. We
>> -		 * can be a little more lax here and use the fallback
>> -		 * more often to avoid costly migrations of ourselves
>> -		 * and other objects within the aperture.
>> -		 *
>> -		 * Half-the-aperture is used as a simple heuristic.
>> -		 * More interesting would to do search for a free
>> -		 * block prior to making the commitment to unbind.
>> -		 * That caters for the self-harm case, and with a
>> -		 * little more heuristics (e.g. NOFAULT, NOEVICT)
>> -		 * we could try to minimise harm to others.
>> -		 */
>> -		if (flags & PIN_NONBLOCK &&
>> -		    obj->base.size > ggtt->mappable_end / 2)
>> -			return ERR_PTR(-ENOSPC);
>> +		ret = i915_gem_object_fits_in_aperture(obj, alignment, flags);
>> +		if (ret)
>> +			return ERR_PTR(ret);
>>   	}
>>   
>>   new_vma:
>> -- 
>> 2.34.1
>

Ville Syrjälä Feb. 7, 2022, 1:24 p.m. UTC | #4

On Mon, Feb 07, 2022 at 11:47:16AM +0000, Tvrtko Ursulin wrote:
> 
> On 07/02/2022 10:58, Ville Syrjälä wrote:
> > On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
> >> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2 or
> >> more framebuffers/scanout buffers results in only one that is mappable/
> >> fenceable. Therefore, pageflipping between these 2 FBs where only one
> >> is mappable/fenceable creates latencies large enough to miss alternate
> >> vblanks thereby producing less optimal framerate.
> >>
> >> This mainly happens because when i915_gem_object_pin_to_display_plane()
> >> is called to pin one of the FB objs, the associated vma is identified
> >> as misplaced and therefore i915_vma_unbind() is called which unbinds and
> >> evicts it. This misplaced vma gets subseqently pinned only when
> >> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> >> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> >> Therefore, to fix this issue, we try to see if there is space to map
> >> at-least two objects of a given size and return early if there isn't. This
> >> would ensure that we do not try with PIN_MAPPABLE for any objects that
> >> are too big to map thereby preventing unncessary unbind.
> >>
> >> Testcase:
> >> Running Weston and weston-simple-egl on an Alderlake_S (ADLS) platform
> >> with a 8K@60 mode results in only ~40 FPS. Since upstream Weston submits
> >> a frame ~7ms before the next vblank, the latencies seen between atomic
> >> commit and flip event are 7, 24 (7 + 16.66), 7, 24..... suggesting that
> >> it misses the vblank every other frame.
> >>
> >> Here is the ftrace snippet that shows the source of the ~10ms latency:
> >>                i915_gem_object_pin_to_display_plane() {
> >> 0.102 us   |    i915_gem_object_set_cache_level();
> >>                  i915_gem_object_ggtt_pin_ww() {
> >> 0.390 us   |      i915_vma_instance();
> >> 0.178 us   |      i915_vma_misplaced();
> >>                    i915_vma_unbind() {
> >>                    __i915_active_wait() {
> >> 0.082 us   |        i915_active_acquire_if_busy();
> >> 0.475 us   |      }
> >>                    intel_runtime_pm_get() {
> >> 0.087 us   |        intel_runtime_pm_acquire();
> >> 0.259 us   |      }
> >>                    __i915_active_wait() {
> >> 0.085 us   |        i915_active_acquire_if_busy();
> >> 0.240 us   |      }
> >>                    __i915_vma_evict() {
> >>                      ggtt_unbind_vma() {
> >>                        gen8_ggtt_clear_range() {
> >> 10507.255 us |        }
> >> 10507.689 us |      }
> >> 10508.516 us |   }
> >>
> >> v2: Instead of using bigjoiner checks, determine whether a scanout
> >>      buffer is too big by checking to see if it is possible to map
> >>      two of them into the ggtt.
> >>
> >> v3 (Ville):
> >> - Count how many fb objects can be fit into the available holes
> >>    instead of checking for a hole twice the object size.
> >> - Take alignment constraints into account.
> >> - Limit this large scanout buffer check to >= Gen 11 platforms.
> >>
> >> v4:
> >> - Remove existing heuristic that checks just for size. (Ville)
> >> - Return early if we find space to map at-least two objects. (Tvrtko)
> >> - Slightly update the commit message.
> >>
> >> v5: (Tvrtko)
> >> - Rename the function to indicate that the object may be too big to
> >>    map into the aperture.
> >> - Account for guard pages while calculating the total size required
> >>    for the object.
> >> - Do not subject all objects to the heuristic check and instead
> >>    consider objects only of a certain size.
> >> - Do the hole walk using the rbtree.
> >> - Preserve the existing PIN_NONBLOCK logic.
> >> - Drop the PIN_MAPPABLE check while pinning the VMA.
> >>
> >> v6: (Tvrtko)
> >> - Return 0 on success and the specific error code on failure to
> >>    preserve the existing behavior.
> >>
> >> Cc: Ville Syrjälä <[e-mail]>
> >> Cc: Maarten Lankhorst <[e-mail]>
> >> Cc: Tvrtko Ursulin <[e-mail]>
> >> Cc: Manasi Navare <[e-mail]>
> >> Signed-off-by: Vivek Kasireddy <[e-mail]>
> >> ---
> >>   drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
> >>   1 file changed, 90 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index e3a2c2a0e156..39f0d17550c3 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -46,6 +46,7 @@
> >>   #include "gem/i915_gem_mman.h"
> >>   #include "gem/i915_gem_region.h"
> >>   #include "gem/i915_gem_userptr.h"
> >> +#include "gem/i915_gem_tiling.h"
> >>   #include "gt/intel_engine_user.h"
> >>   #include "gt/intel_gt.h"
> >>   #include "gt/intel_gt_pm.h"
> >> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
> >>   	spin_unlock(&obj->vma.lock);
> >>   }
> >>   
> >> +static int
> >> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> >> +				 u64 alignment, u64 flags)
> >> +{
> >> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> >> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> >> +	struct drm_mm_node *hole;
> >> +	u64 hole_start, hole_end, start, end;
> >> +	u64 fence_size, fence_alignment;
> >> +	unsigned int count = 0;
> >> +
> >> +	/*
> >> +	 * If the required space is larger than the available
> >> +	 * aperture, we will not able to find a slot for the
> >> +	 * object and unbinding the object now will be in
> >> +	 * vain. Worse, doing so may cause us to ping-pong
> >> +	 * the object in and out of the Global GTT and
> >> +	 * waste a lot of cycles under the mutex.
> >> +	 */
> >> +	if (obj->base.size > ggtt->mappable_end)
> >> +		return -E2BIG;
> >> +
> >> +	/*
> >> +	 * If NONBLOCK is set the caller is optimistically
> >> +	 * trying to cache the full object within the mappable
> >> +	 * aperture, and *must* have a fallback in place for
> >> +	 * situations where we cannot bind the object. We
> >> +	 * can be a little more lax here and use the fallback
> >> +	 * more often to avoid costly migrations of ourselves
> >> +	 * and other objects within the aperture.
> >> +	 */
> >> +	if (!(flags & PIN_NONBLOCK))
> >> +		return 0;
> >> +
> >> +	/*
> >> +	 * We only consider objects whose size is at-least a quarter of
> >> +	 * the aperture to be too big and subject them to the new
> >> +	 * heuristic below.
> >> +	 */
> >> +	if (obj->base.size < ggtt->mappable_end / 4)
> >> +		return 0;
> > 
> > That seems a fairly arbitrary thing to put here. Maybe something the
> > caller should check/specify?
> 
> I have no strong opinion on this one. In my mind I categorised it under 
> "is it a large framebuffer" heuristics. Previously it was less than one 
> half of aperture always okay, now one quarter, plus 2x hole check if 
> larger. Both are heuristics. I even mentioned earlier if 2x should be an 
> input parameter as well, but again, given it's not an exported function 
> couldn't really justify it.

Is there any point in even having this extra check? If we 
don't think checking this is worth the hassle then why call
the function at all?

> 
> > 
> >> +
> >> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> >> +	    !i915_gem_object_is_framebuffer(obj))
> >> +		return 0;
> > 
> > None of that seems appropriate for a generic gem function
> > with this name.
> 
> It's not exported though, maybe remove i915_gem prefix to avoid any 
> ideas of it being generic?

These checks don't even seem to doing anything useful. HAS_GMCH should
already be covered by always setting PIN_MAPPABLE and hence O_NONBLOCK
is never even tried, the pre-icl vs. icl+ check should not exist at all
IMO, and if this is only called for framebuffers then why does the code
pretend that is not the case?

So I would suggest just ditching all these checks, and then the function
even does what it says on the tin.

Tvrtko Ursulin Feb. 7, 2022, 1:36 p.m. UTC | #5

On 07/02/2022 13:24, Ville Syrjälä wrote:
> On Mon, Feb 07, 2022 at 11:47:16AM +0000, Tvrtko Ursulin wrote:
>>
>> On 07/02/2022 10:58, Ville Syrjälä wrote:
>>> On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
>>>> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2 or
>>>> more framebuffers/scanout buffers results in only one that is mappable/
>>>> fenceable. Therefore, pageflipping between these 2 FBs where only one
>>>> is mappable/fenceable creates latencies large enough to miss alternate
>>>> vblanks thereby producing less optimal framerate.
>>>>
>>>> This mainly happens because when i915_gem_object_pin_to_display_plane()
>>>> is called to pin one of the FB objs, the associated vma is identified
>>>> as misplaced and therefore i915_vma_unbind() is called which unbinds and
>>>> evicts it. This misplaced vma gets subseqently pinned only when
>>>> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
>>>> results in a latency of ~10ms and happens every other vblank/repaint cycle.
>>>> Therefore, to fix this issue, we try to see if there is space to map
>>>> at-least two objects of a given size and return early if there isn't. This
>>>> would ensure that we do not try with PIN_MAPPABLE for any objects that
>>>> are too big to map thereby preventing unncessary unbind.
>>>>
>>>> Testcase:
>>>> Running Weston and weston-simple-egl on an Alderlake_S (ADLS) platform
>>>> with a 8K@60 mode results in only ~40 FPS. Since upstream Weston submits
>>>> a frame ~7ms before the next vblank, the latencies seen between atomic
>>>> commit and flip event are 7, 24 (7 + 16.66), 7, 24..... suggesting that
>>>> it misses the vblank every other frame.
>>>>
>>>> Here is the ftrace snippet that shows the source of the ~10ms latency:
>>>>                 i915_gem_object_pin_to_display_plane() {
>>>> 0.102 us   |    i915_gem_object_set_cache_level();
>>>>                   i915_gem_object_ggtt_pin_ww() {
>>>> 0.390 us   |      i915_vma_instance();
>>>> 0.178 us   |      i915_vma_misplaced();
>>>>                     i915_vma_unbind() {
>>>>                     __i915_active_wait() {
>>>> 0.082 us   |        i915_active_acquire_if_busy();
>>>> 0.475 us   |      }
>>>>                     intel_runtime_pm_get() {
>>>> 0.087 us   |        intel_runtime_pm_acquire();
>>>> 0.259 us   |      }
>>>>                     __i915_active_wait() {
>>>> 0.085 us   |        i915_active_acquire_if_busy();
>>>> 0.240 us   |      }
>>>>                     __i915_vma_evict() {
>>>>                       ggtt_unbind_vma() {
>>>>                         gen8_ggtt_clear_range() {
>>>> 10507.255 us |        }
>>>> 10507.689 us |      }
>>>> 10508.516 us |   }
>>>>
>>>> v2: Instead of using bigjoiner checks, determine whether a scanout
>>>>       buffer is too big by checking to see if it is possible to map
>>>>       two of them into the ggtt.
>>>>
>>>> v3 (Ville):
>>>> - Count how many fb objects can be fit into the available holes
>>>>     instead of checking for a hole twice the object size.
>>>> - Take alignment constraints into account.
>>>> - Limit this large scanout buffer check to >= Gen 11 platforms.
>>>>
>>>> v4:
>>>> - Remove existing heuristic that checks just for size. (Ville)
>>>> - Return early if we find space to map at-least two objects. (Tvrtko)
>>>> - Slightly update the commit message.
>>>>
>>>> v5: (Tvrtko)
>>>> - Rename the function to indicate that the object may be too big to
>>>>     map into the aperture.
>>>> - Account for guard pages while calculating the total size required
>>>>     for the object.
>>>> - Do not subject all objects to the heuristic check and instead
>>>>     consider objects only of a certain size.
>>>> - Do the hole walk using the rbtree.
>>>> - Preserve the existing PIN_NONBLOCK logic.
>>>> - Drop the PIN_MAPPABLE check while pinning the VMA.
>>>>
>>>> v6: (Tvrtko)
>>>> - Return 0 on success and the specific error code on failure to
>>>>     preserve the existing behavior.
>>>>
>>>> Cc: Ville Syrjälä <[e-mail]>
>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>> Cc: Tvrtko Ursulin <[e-mail]>
>>>> Cc: Manasi Navare <[e-mail]>
>>>> Signed-off-by: Vivek Kasireddy <[e-mail]>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
>>>>    1 file changed, 90 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index e3a2c2a0e156..39f0d17550c3 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -46,6 +46,7 @@
>>>>    #include "gem/i915_gem_mman.h"
>>>>    #include "gem/i915_gem_region.h"
>>>>    #include "gem/i915_gem_userptr.h"
>>>> +#include "gem/i915_gem_tiling.h"
>>>>    #include "gt/intel_engine_user.h"
>>>>    #include "gt/intel_gt.h"
>>>>    #include "gt/intel_gt_pm.h"
>>>> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
>>>>    	spin_unlock(&obj->vma.lock);
>>>>    }
>>>>    
>>>> +static int
>>>> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
>>>> +				 u64 alignment, u64 flags)
>>>> +{
>>>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
>>>> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
>>>> +	struct drm_mm_node *hole;
>>>> +	u64 hole_start, hole_end, start, end;
>>>> +	u64 fence_size, fence_alignment;
>>>> +	unsigned int count = 0;
>>>> +
>>>> +	/*
>>>> +	 * If the required space is larger than the available
>>>> +	 * aperture, we will not able to find a slot for the
>>>> +	 * object and unbinding the object now will be in
>>>> +	 * vain. Worse, doing so may cause us to ping-pong
>>>> +	 * the object in and out of the Global GTT and
>>>> +	 * waste a lot of cycles under the mutex.
>>>> +	 */
>>>> +	if (obj->base.size > ggtt->mappable_end)
>>>> +		return -E2BIG;
>>>> +
>>>> +	/*
>>>> +	 * If NONBLOCK is set the caller is optimistically
>>>> +	 * trying to cache the full object within the mappable
>>>> +	 * aperture, and *must* have a fallback in place for
>>>> +	 * situations where we cannot bind the object. We
>>>> +	 * can be a little more lax here and use the fallback
>>>> +	 * more often to avoid costly migrations of ourselves
>>>> +	 * and other objects within the aperture.
>>>> +	 */
>>>> +	if (!(flags & PIN_NONBLOCK))
>>>> +		return 0;
>>>> +
>>>> +	/*
>>>> +	 * We only consider objects whose size is at-least a quarter of
>>>> +	 * the aperture to be too big and subject them to the new
>>>> +	 * heuristic below.
>>>> +	 */
>>>> +	if (obj->base.size < ggtt->mappable_end / 4)
>>>> +		return 0;
>>>
>>> That seems a fairly arbitrary thing to put here. Maybe something the
>>> caller should check/specify?
>>
>> I have no strong opinion on this one. In my mind I categorised it under
>> "is it a large framebuffer" heuristics. Previously it was less than one
>> half of aperture always okay, now one quarter, plus 2x hole check if
>> larger. Both are heuristics. I even mentioned earlier if 2x should be an
>> input parameter as well, but again, given it's not an exported function
>> couldn't really justify it.
> 
> Is there any point in even having this extra check? If we
> don't think checking this is worth the hassle then why call
> the function at all?

The "/4" one? It was my suggestion to avoid the hole search if we can 
know based on size it cannot be a frame buffer that would be affected by 
the ping-ping problem. Granted that was before the rbtree hole search, 
when it was traversing the un-ordered linked list of holes. What is the 
correct size threshold I don't know.

>>>> +
>>>> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
>>>> +	    !i915_gem_object_is_framebuffer(obj))
>>>> +		return 0;
>>>
>>> None of that seems appropriate for a generic gem function
>>> with this name.
>>
>> It's not exported though, maybe remove i915_gem prefix to avoid any
>> ideas of it being generic?
> 
> These checks don't even seem to doing anything useful. HAS_GMCH should
> already be covered by always setting PIN_MAPPABLE and hence O_NONBLOCK
> is never even tried, the pre-icl vs. icl+ check should not exist at all
> IMO, and if this is only called for framebuffers then why does the code
> pretend that is not the case?
> 
> So I would suggest just ditching all these checks, and then the function
> even does what it says on the tin.

Change log for v3 made me think at least some of this was your 
suggestion so I did not think about it further. :) No strong opinion either.

Regards,

Tvrtko

Kasireddy, Vivek Feb. 8, 2022, 5:10 a.m. UTC | #6

Hi Tvrtko, Ville,

> On 07/02/2022 13:24, Ville Syrjälä wrote:
> > On Mon, Feb 07, 2022 at 11:47:16AM +0000, Tvrtko Ursulin wrote:
> >>
> >> On 07/02/2022 10:58, Ville Syrjälä wrote:
> >>> On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
> >>>> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2
> >>>> or more framebuffers/scanout buffers results in only one that is
> >>>> mappable/ fenceable. Therefore, pageflipping between these 2 FBs
> >>>> where only one is mappable/fenceable creates latencies large enough
> >>>> to miss alternate vblanks thereby producing less optimal framerate.
> >>>>
> >>>> This mainly happens because when
> >>>> i915_gem_object_pin_to_display_plane()
> >>>> is called to pin one of the FB objs, the associated vma is
> >>>> identified as misplaced and therefore i915_vma_unbind() is called
> >>>> which unbinds and evicts it. This misplaced vma gets subseqently
> >>>> pinned only when
> >>>> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> >>>> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> >>>> Therefore, to fix this issue, we try to see if there is space to
> >>>> map at-least two objects of a given size and return early if there
> >>>> isn't. This would ensure that we do not try with PIN_MAPPABLE for
> >>>> any objects that are too big to map thereby preventing unncessary unbind.
> >>>>
> >>>> Testcase:
> >>>> Running Weston and weston-simple-egl on an Alderlake_S (ADLS)
> >>>> platform with a 8K@60 mode results in only ~40 FPS. Since upstream
> >>>> Weston submits a frame ~7ms before the next vblank, the latencies
> >>>> seen between atomic commit and flip event are 7, 24 (7 + 16.66), 7,
> >>>> 24..... suggesting that it misses the vblank every other frame.
> >>>>
> >>>> Here is the ftrace snippet that shows the source of the ~10ms latency:
> >>>>                 i915_gem_object_pin_to_display_plane() {
> >>>> 0.102 us   |    i915_gem_object_set_cache_level();
> >>>>                   i915_gem_object_ggtt_pin_ww() {
> >>>> 0.390 us   |      i915_vma_instance();
> >>>> 0.178 us   |      i915_vma_misplaced();
> >>>>                     i915_vma_unbind() {
> >>>>                     __i915_active_wait() {
> >>>> 0.082 us   |        i915_active_acquire_if_busy();
> >>>> 0.475 us   |      }
> >>>>                     intel_runtime_pm_get() {
> >>>> 0.087 us   |        intel_runtime_pm_acquire();
> >>>> 0.259 us   |      }
> >>>>                     __i915_active_wait() {
> >>>> 0.085 us   |        i915_active_acquire_if_busy();
> >>>> 0.240 us   |      }
> >>>>                     __i915_vma_evict() {
> >>>>                       ggtt_unbind_vma() {
> >>>>                         gen8_ggtt_clear_range() {
> >>>> 10507.255 us |        }
> >>>> 10507.689 us |      }
> >>>> 10508.516 us |   }
> >>>>
> >>>> v2: Instead of using bigjoiner checks, determine whether a scanout
> >>>>       buffer is too big by checking to see if it is possible to map
> >>>>       two of them into the ggtt.
> >>>>
> >>>> v3 (Ville):
> >>>> - Count how many fb objects can be fit into the available holes
> >>>>     instead of checking for a hole twice the object size.
> >>>> - Take alignment constraints into account.
> >>>> - Limit this large scanout buffer check to >= Gen 11 platforms.
> >>>>
> >>>> v4:
> >>>> - Remove existing heuristic that checks just for size. (Ville)
> >>>> - Return early if we find space to map at-least two objects.
> >>>> (Tvrtko)
> >>>> - Slightly update the commit message.
> >>>>
> >>>> v5: (Tvrtko)
> >>>> - Rename the function to indicate that the object may be too big to
> >>>>     map into the aperture.
> >>>> - Account for guard pages while calculating the total size required
> >>>>     for the object.
> >>>> - Do not subject all objects to the heuristic check and instead
> >>>>     consider objects only of a certain size.
> >>>> - Do the hole walk using the rbtree.
> >>>> - Preserve the existing PIN_NONBLOCK logic.
> >>>> - Drop the PIN_MAPPABLE check while pinning the VMA.
> >>>>
> >>>> v6: (Tvrtko)
> >>>> - Return 0 on success and the specific error code on failure to
> >>>>     preserve the existing behavior.
> >>>>
> >>>> Cc: Ville Syrjälä <[e-mail]>
> >>>> Cc: Maarten Lankhorst <[e-mail]>
> >>>> Cc: Tvrtko Ursulin <[e-mail]>
> >>>> Cc: Manasi Navare <[e-mail]>
> >>>> Signed-off-by: Vivek Kasireddy <[e-mail]>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
> >>>>    1 file changed, 90 insertions(+), 30 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> >>>> b/drivers/gpu/drm/i915/i915_gem.c index e3a2c2a0e156..39f0d17550c3
> >>>> 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_gem.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >>>> @@ -46,6 +46,7 @@
> >>>>    #include "gem/i915_gem_mman.h"
> >>>>    #include "gem/i915_gem_region.h"
> >>>>    #include "gem/i915_gem_userptr.h"
> >>>> +#include "gem/i915_gem_tiling.h"
> >>>>    #include "gt/intel_engine_user.h"
> >>>>    #include "gt/intel_gt.h"
> >>>>    #include "gt/intel_gt_pm.h"
> >>>> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
> >>>>    	spin_unlock(&obj->vma.lock);
> >>>>    }
> >>>>
> >>>> +static int
> >>>> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> >>>> +				 u64 alignment, u64 flags)
> >>>> +{
> >>>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> >>>> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> >>>> +	struct drm_mm_node *hole;
> >>>> +	u64 hole_start, hole_end, start, end;
> >>>> +	u64 fence_size, fence_alignment;
> >>>> +	unsigned int count = 0;
> >>>> +
> >>>> +	/*
> >>>> +	 * If the required space is larger than the available
> >>>> +	 * aperture, we will not able to find a slot for the
> >>>> +	 * object and unbinding the object now will be in
> >>>> +	 * vain. Worse, doing so may cause us to ping-pong
> >>>> +	 * the object in and out of the Global GTT and
> >>>> +	 * waste a lot of cycles under the mutex.
> >>>> +	 */
> >>>> +	if (obj->base.size > ggtt->mappable_end)
> >>>> +		return -E2BIG;
> >>>> +
> >>>> +	/*
> >>>> +	 * If NONBLOCK is set the caller is optimistically
> >>>> +	 * trying to cache the full object within the mappable
> >>>> +	 * aperture, and *must* have a fallback in place for
> >>>> +	 * situations where we cannot bind the object. We
> >>>> +	 * can be a little more lax here and use the fallback
> >>>> +	 * more often to avoid costly migrations of ourselves
> >>>> +	 * and other objects within the aperture.
> >>>> +	 */
> >>>> +	if (!(flags & PIN_NONBLOCK))
> >>>> +		return 0;
> >>>> +
> >>>> +	/*
> >>>> +	 * We only consider objects whose size is at-least a quarter of
> >>>> +	 * the aperture to be too big and subject them to the new
> >>>> +	 * heuristic below.
> >>>> +	 */
> >>>> +	if (obj->base.size < ggtt->mappable_end / 4)
> >>>> +		return 0;
> >>>
> >>> That seems a fairly arbitrary thing to put here. Maybe something the
> >>> caller should check/specify?
> >>
> >> I have no strong opinion on this one. In my mind I categorised it
> >> under "is it a large framebuffer" heuristics. Previously it was less
> >> than one half of aperture always okay, now one quarter, plus 2x hole
> >> check if larger. Both are heuristics. I even mentioned earlier if 2x
> >> should be an input parameter as well, but again, given it's not an
> >> exported function couldn't really justify it.
> >
> > Is there any point in even having this extra check? If we don't think
> > checking this is worth the hassle then why call the function at all?
> 
> The "/4" one? It was my suggestion to avoid the hole search if we can know based on size
> it cannot be a frame buffer that would be affected by the ping-ping problem. Granted that
> was before the rbtree hole search, when it was traversing the un-ordered linked list of
> holes. What is the correct size threshold I don't know.
> 
> >>>> +
> >>>> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> >>>> +	    !i915_gem_object_is_framebuffer(obj))
> >>>> +		return 0;
> >>>
> >>> None of that seems appropriate for a generic gem function with this
> >>> name.
> >>
> >> It's not exported though, maybe remove i915_gem prefix to avoid any
> >> ideas of it being generic?
> >
> > These checks don't even seem to doing anything useful. HAS_GMCH should
> > already be covered by always setting PIN_MAPPABLE and hence O_NONBLOCK
[Kasireddy, Vivek] I can drop the HAS_GMCH(i915) check given that it is redundant.

> > is never even tried, the pre-icl vs. icl+ check should not exist at
[Kasireddy, Vivek] My aim was to narrow down the list of situations in which the
ping-pong problem becomes more pronounced and may lead to performance 
issues. Therefore, I added the DISPLAY_VER(i915) check since 8K/bigjoiner is
feasible only on those newer platforms. 

> > all IMO, and if this is only called for framebuffers then why does the
> > code pretend that is not the case?
[Kasireddy, Vivek] Oh, I added the i915_gem_object_is_framebuffer() check after I 
found that there are other callers (for example, reloc_iomap() in i915_gem_execbuffer.c)
of i915_gem_object_ggtt_pin_ww() that may not be working on an fb.

Also, I figured size < ggtt->mappable_end / 4 or a similar check is needed as we do
not want to subject all FBs through this performance critical path. 

Thanks,
Vivek

> >
> > So I would suggest just ditching all these checks, and then the
> > function even does what it says on the tin.
> 
> Change log for v3 made me think at least some of this was your suggestion so I did not
> think about it further. :) No strong opinion either.
> 
> Regards,
> 
> Tvrtko

Ville Syrjälä Feb. 8, 2022, 5:43 a.m. UTC | #7

On Tue, Feb 08, 2022 at 05:10:42AM +0000, Kasireddy, Vivek wrote:
> Hi Tvrtko, Ville,
> 
> > On 07/02/2022 13:24, Ville Syrjälä wrote:
> > > On Mon, Feb 07, 2022 at 11:47:16AM +0000, Tvrtko Ursulin wrote:
> > >>
> > >> On 07/02/2022 10:58, Ville Syrjälä wrote:
> > >>> On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
> > >>>> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2
> > >>>> or more framebuffers/scanout buffers results in only one that is
> > >>>> mappable/ fenceable. Therefore, pageflipping between these 2 FBs
> > >>>> where only one is mappable/fenceable creates latencies large enough
> > >>>> to miss alternate vblanks thereby producing less optimal framerate.
> > >>>>
> > >>>> This mainly happens because when
> > >>>> i915_gem_object_pin_to_display_plane()
> > >>>> is called to pin one of the FB objs, the associated vma is
> > >>>> identified as misplaced and therefore i915_vma_unbind() is called
> > >>>> which unbinds and evicts it. This misplaced vma gets subseqently
> > >>>> pinned only when
> > >>>> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> > >>>> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> > >>>> Therefore, to fix this issue, we try to see if there is space to
> > >>>> map at-least two objects of a given size and return early if there
> > >>>> isn't. This would ensure that we do not try with PIN_MAPPABLE for
> > >>>> any objects that are too big to map thereby preventing unncessary unbind.
> > >>>>
> > >>>> Testcase:
> > >>>> Running Weston and weston-simple-egl on an Alderlake_S (ADLS)
> > >>>> platform with a 8K@60 mode results in only ~40 FPS. Since upstream
> > >>>> Weston submits a frame ~7ms before the next vblank, the latencies
> > >>>> seen between atomic commit and flip event are 7, 24 (7 + 16.66), 7,
> > >>>> 24..... suggesting that it misses the vblank every other frame.
> > >>>>
> > >>>> Here is the ftrace snippet that shows the source of the ~10ms latency:
> > >>>>                 i915_gem_object_pin_to_display_plane() {
> > >>>> 0.102 us   |    i915_gem_object_set_cache_level();
> > >>>>                   i915_gem_object_ggtt_pin_ww() {
> > >>>> 0.390 us   |      i915_vma_instance();
> > >>>> 0.178 us   |      i915_vma_misplaced();
> > >>>>                     i915_vma_unbind() {
> > >>>>                     __i915_active_wait() {
> > >>>> 0.082 us   |        i915_active_acquire_if_busy();
> > >>>> 0.475 us   |      }
> > >>>>                     intel_runtime_pm_get() {
> > >>>> 0.087 us   |        intel_runtime_pm_acquire();
> > >>>> 0.259 us   |      }
> > >>>>                     __i915_active_wait() {
> > >>>> 0.085 us   |        i915_active_acquire_if_busy();
> > >>>> 0.240 us   |      }
> > >>>>                     __i915_vma_evict() {
> > >>>>                       ggtt_unbind_vma() {
> > >>>>                         gen8_ggtt_clear_range() {
> > >>>> 10507.255 us |        }
> > >>>> 10507.689 us |      }
> > >>>> 10508.516 us |   }
> > >>>>
> > >>>> v2: Instead of using bigjoiner checks, determine whether a scanout
> > >>>>       buffer is too big by checking to see if it is possible to map
> > >>>>       two of them into the ggtt.
> > >>>>
> > >>>> v3 (Ville):
> > >>>> - Count how many fb objects can be fit into the available holes
> > >>>>     instead of checking for a hole twice the object size.
> > >>>> - Take alignment constraints into account.
> > >>>> - Limit this large scanout buffer check to >= Gen 11 platforms.
> > >>>>
> > >>>> v4:
> > >>>> - Remove existing heuristic that checks just for size. (Ville)
> > >>>> - Return early if we find space to map at-least two objects.
> > >>>> (Tvrtko)
> > >>>> - Slightly update the commit message.
> > >>>>
> > >>>> v5: (Tvrtko)
> > >>>> - Rename the function to indicate that the object may be too big to
> > >>>>     map into the aperture.
> > >>>> - Account for guard pages while calculating the total size required
> > >>>>     for the object.
> > >>>> - Do not subject all objects to the heuristic check and instead
> > >>>>     consider objects only of a certain size.
> > >>>> - Do the hole walk using the rbtree.
> > >>>> - Preserve the existing PIN_NONBLOCK logic.
> > >>>> - Drop the PIN_MAPPABLE check while pinning the VMA.
> > >>>>
> > >>>> v6: (Tvrtko)
> > >>>> - Return 0 on success and the specific error code on failure to
> > >>>>     preserve the existing behavior.
> > >>>>
> > >>>> Cc: Ville Syrjälä <[e-mail]>
> > >>>> Cc: Maarten Lankhorst <[e-mail]>
> > >>>> Cc: Tvrtko Ursulin <[e-mail]>
> > >>>> Cc: Manasi Navare <[e-mail]>
> > >>>> Signed-off-by: Vivek Kasireddy <[e-mail]>
> > >>>> ---
> > >>>>    drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++--------
> > >>>>    1 file changed, 90 insertions(+), 30 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > >>>> b/drivers/gpu/drm/i915/i915_gem.c index e3a2c2a0e156..39f0d17550c3
> > >>>> 100644
> > >>>> --- a/drivers/gpu/drm/i915/i915_gem.c
> > >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> > >>>> @@ -46,6 +46,7 @@
> > >>>>    #include "gem/i915_gem_mman.h"
> > >>>>    #include "gem/i915_gem_region.h"
> > >>>>    #include "gem/i915_gem_userptr.h"
> > >>>> +#include "gem/i915_gem_tiling.h"
> > >>>>    #include "gt/intel_engine_user.h"
> > >>>>    #include "gt/intel_gt.h"
> > >>>>    #include "gt/intel_gt_pm.h"
> > >>>> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
> > >>>>    	spin_unlock(&obj->vma.lock);
> > >>>>    }
> > >>>>
> > >>>> +static int
> > >>>> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> > >>>> +				 u64 alignment, u64 flags)
> > >>>> +{
> > >>>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > >>>> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> > >>>> +	struct drm_mm_node *hole;
> > >>>> +	u64 hole_start, hole_end, start, end;
> > >>>> +	u64 fence_size, fence_alignment;
> > >>>> +	unsigned int count = 0;
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * If the required space is larger than the available
> > >>>> +	 * aperture, we will not able to find a slot for the
> > >>>> +	 * object and unbinding the object now will be in
> > >>>> +	 * vain. Worse, doing so may cause us to ping-pong
> > >>>> +	 * the object in and out of the Global GTT and
> > >>>> +	 * waste a lot of cycles under the mutex.
> > >>>> +	 */
> > >>>> +	if (obj->base.size > ggtt->mappable_end)
> > >>>> +		return -E2BIG;
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * If NONBLOCK is set the caller is optimistically
> > >>>> +	 * trying to cache the full object within the mappable
> > >>>> +	 * aperture, and *must* have a fallback in place for
> > >>>> +	 * situations where we cannot bind the object. We
> > >>>> +	 * can be a little more lax here and use the fallback
> > >>>> +	 * more often to avoid costly migrations of ourselves
> > >>>> +	 * and other objects within the aperture.
> > >>>> +	 */
> > >>>> +	if (!(flags & PIN_NONBLOCK))
> > >>>> +		return 0;
> > >>>> +
> > >>>> +	/*
> > >>>> +	 * We only consider objects whose size is at-least a quarter of
> > >>>> +	 * the aperture to be too big and subject them to the new
> > >>>> +	 * heuristic below.
> > >>>> +	 */
> > >>>> +	if (obj->base.size < ggtt->mappable_end / 4)
> > >>>> +		return 0;
> > >>>
> > >>> That seems a fairly arbitrary thing to put here. Maybe something the
> > >>> caller should check/specify?
> > >>
> > >> I have no strong opinion on this one. In my mind I categorised it
> > >> under "is it a large framebuffer" heuristics. Previously it was less
> > >> than one half of aperture always okay, now one quarter, plus 2x hole
> > >> check if larger. Both are heuristics. I even mentioned earlier if 2x
> > >> should be an input parameter as well, but again, given it's not an
> > >> exported function couldn't really justify it.
> > >
> > > Is there any point in even having this extra check? If we don't think
> > > checking this is worth the hassle then why call the function at all?
> > 
> > The "/4" one? It was my suggestion to avoid the hole search if we can know based on size
> > it cannot be a frame buffer that would be affected by the ping-ping problem. Granted that
> > was before the rbtree hole search, when it was traversing the un-ordered linked list of
> > holes. What is the correct size threshold I don't know.
> > 
> > >>>> +
> > >>>> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> > >>>> +	    !i915_gem_object_is_framebuffer(obj))
> > >>>> +		return 0;
> > >>>
> > >>> None of that seems appropriate for a generic gem function with this
> > >>> name.
> > >>
> > >> It's not exported though, maybe remove i915_gem prefix to avoid any
> > >> ideas of it being generic?
> > >
> > > These checks don't even seem to doing anything useful. HAS_GMCH should
> > > already be covered by always setting PIN_MAPPABLE and hence O_NONBLOCK
> [Kasireddy, Vivek] I can drop the HAS_GMCH(i915) check given that it is redundant.
> 
> > > is never even tried, the pre-icl vs. icl+ check should not exist at
> [Kasireddy, Vivek] My aim was to narrow down the list of situations in which the
> ping-pong problem becomes more pronounced and may lead to performance 
> issues. Therefore, I added the DISPLAY_VER(i915) check since 8K/bigjoiner is
> feasible only on those newer platforms. 

Like I said before bigjoiner is irrelevant. The only thing that
matters is the size of the mapping vs. mappable aperture size.

> 
> > > all IMO, and if this is only called for framebuffers then why does the
> > > code pretend that is not the case?
> [Kasireddy, Vivek] Oh, I added the i915_gem_object_is_framebuffer() check after I 
> found that there are other callers (for example, reloc_iomap() in i915_gem_execbuffer.c)
> of i915_gem_object_ggtt_pin_ww() that may not be working on an fb.
> 
> Also, I figured size < ggtt->mappable_end / 4 or a similar check is needed as we do
> not want to subject all FBs through this performance critical path. 

Why not?

Kasireddy, Vivek Feb. 9, 2022, 1:47 a.m. UTC | #8

Hi Ville,

> 
> On Tue, Feb 08, 2022 at 05:10:42AM +0000, Kasireddy, Vivek wrote:
> > Hi Tvrtko, Ville,
> >
> > > On 07/02/2022 13:24, Ville Syrjälä wrote:
> > > > On Mon, Feb 07, 2022 at 11:47:16AM +0000, Tvrtko Ursulin wrote:
> > > >>
> > > >> On 07/02/2022 10:58, Ville Syrjälä wrote:
> > > >>> On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
> > > >>>> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2
> > > >>>> or more framebuffers/scanout buffers results in only one that is
> > > >>>> mappable/ fenceable. Therefore, pageflipping between these 2 FBs
> > > >>>> where only one is mappable/fenceable creates latencies large enough
> > > >>>> to miss alternate vblanks thereby producing less optimal framerate.
> > > >>>>
> > > >>>> This mainly happens because when
> > > >>>> i915_gem_object_pin_to_display_plane()
> > > >>>> is called to pin one of the FB objs, the associated vma is
> > > >>>> identified as misplaced and therefore i915_vma_unbind() is called
> > > >>>> which unbinds and evicts it. This misplaced vma gets subseqently
> > > >>>> pinned only when
> > > >>>> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> > > >>>> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> > > >>>> Therefore, to fix this issue, we try to see if there is space to
> > > >>>> map at-least two objects of a given size and return early if there
> > > >>>> isn't. This would ensure that we do not try with PIN_MAPPABLE for
> > > >>>> any objects that are too big to map thereby preventing unncessary unbind.
> > > >>>>
> > > >>>> Testcase:
> > > >>>> Running Weston and weston-simple-egl on an Alderlake_S (ADLS)
> > > >>>> platform with a 8K@60 mode results in only ~40 FPS. Since upstream
> > > >>>> Weston submits a frame ~7ms before the next vblank, the latencies
> > > >>>> seen between atomic commit and flip event are 7, 24 (7 + 16.66), 7,
> > > >>>> 24..... suggesting that it misses the vblank every other frame.
> > > >>>>
> > > >>>> Here is the ftrace snippet that shows the source of the ~10ms latency:
> > > >>>>                 i915_gem_object_pin_to_display_plane() {
> > > >>>> 0.102 us   |    i915_gem_object_set_cache_level();
> > > >>>>                   i915_gem_object_ggtt_pin_ww() {
> > > >>>> 0.390 us   |      i915_vma_instance();
> > > >>>> 0.178 us   |      i915_vma_misplaced();
> > > >>>>                     i915_vma_unbind() {
> > > >>>>                     __i915_active_wait() {
> > > >>>> 0.082 us   |        i915_active_acquire_if_busy();
> > > >>>> 0.475 us   |      }
> > > >>>>                     intel_runtime_pm_get() {
> > > >>>> 0.087 us   |        intel_runtime_pm_acquire();
> > > >>>> 0.259 us   |      }
> > > >>>>                     __i915_active_wait() {
> > > >>>> 0.085 us   |        i915_active_acquire_if_busy();
> > > >>>> 0.240 us   |      }
> > > >>>>                     __i915_vma_evict() {
> > > >>>>                       ggtt_unbind_vma() {
> > > >>>>                         gen8_ggtt_clear_range() {
> > > >>>> 10507.255 us |        }
> > > >>>> 10507.689 us |      }
> > > >>>> 10508.516 us |   }
> > > >>>>
> > > >>>> v2: Instead of using bigjoiner checks, determine whether a scanout
> > > >>>>       buffer is too big by checking to see if it is possible to map
> > > >>>>       two of them into the ggtt.
> > > >>>>
> > > >>>> v3 (Ville):
> > > >>>> - Count how many fb objects can be fit into the available holes
> > > >>>>     instead of checking for a hole twice the object size.
> > > >>>> - Take alignment constraints into account.
> > > >>>> - Limit this large scanout buffer check to >= Gen 11 platforms.
> > > >>>>
> > > >>>> v4:
> > > >>>> - Remove existing heuristic that checks just for size. (Ville)
> > > >>>> - Return early if we find space to map at-least two objects.
> > > >>>> (Tvrtko)
> > > >>>> - Slightly update the commit message.
> > > >>>>
> > > >>>> v5: (Tvrtko)
> > > >>>> - Rename the function to indicate that the object may be too big to
> > > >>>>     map into the aperture.
> > > >>>> - Account for guard pages while calculating the total size required
> > > >>>>     for the object.
> > > >>>> - Do not subject all objects to the heuristic check and instead
> > > >>>>     consider objects only of a certain size.
> > > >>>> - Do the hole walk using the rbtree.
> > > >>>> - Preserve the existing PIN_NONBLOCK logic.
> > > >>>> - Drop the PIN_MAPPABLE check while pinning the VMA.
> > > >>>>
> > > >>>> v6: (Tvrtko)
> > > >>>> - Return 0 on success and the specific error code on failure to
> > > >>>>     preserve the existing behavior.
> > > >>>>
> > > >>>> Cc: Ville Syrjälä <[e-mail]>
> > > >>>> Cc: Maarten Lankhorst <[e-mail]>
> > > >>>> Cc: Tvrtko Ursulin <[e-mail]>
> > > >>>> Cc: Manasi Navare <[e-mail]>
> > > >>>> Signed-off-by: Vivek Kasireddy <[e-mail]>
> > > >>>> ---
> > > >>>>    drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++-----
> ---
> > > >>>>    1 file changed, 90 insertions(+), 30 deletions(-)
> > > >>>>
> > > >>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > >>>> b/drivers/gpu/drm/i915/i915_gem.c index e3a2c2a0e156..39f0d17550c3
> > > >>>> 100644
> > > >>>> --- a/drivers/gpu/drm/i915/i915_gem.c
> > > >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > >>>> @@ -46,6 +46,7 @@
> > > >>>>    #include "gem/i915_gem_mman.h"
> > > >>>>    #include "gem/i915_gem_region.h"
> > > >>>>    #include "gem/i915_gem_userptr.h"
> > > >>>> +#include "gem/i915_gem_tiling.h"
> > > >>>>    #include "gt/intel_engine_user.h"
> > > >>>>    #include "gt/intel_gt.h"
> > > >>>>    #include "gt/intel_gt_pm.h"
> > > >>>> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
> > > >>>>    	spin_unlock(&obj->vma.lock);
> > > >>>>    }
> > > >>>>
> > > >>>> +static int
> > > >>>> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> > > >>>> +				 u64 alignment, u64 flags)
> > > >>>> +{
> > > >>>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > > >>>> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> > > >>>> +	struct drm_mm_node *hole;
> > > >>>> +	u64 hole_start, hole_end, start, end;
> > > >>>> +	u64 fence_size, fence_alignment;
> > > >>>> +	unsigned int count = 0;
> > > >>>> +
> > > >>>> +	/*
> > > >>>> +	 * If the required space is larger than the available
> > > >>>> +	 * aperture, we will not able to find a slot for the
> > > >>>> +	 * object and unbinding the object now will be in
> > > >>>> +	 * vain. Worse, doing so may cause us to ping-pong
> > > >>>> +	 * the object in and out of the Global GTT and
> > > >>>> +	 * waste a lot of cycles under the mutex.
> > > >>>> +	 */
> > > >>>> +	if (obj->base.size > ggtt->mappable_end)
> > > >>>> +		return -E2BIG;
> > > >>>> +
> > > >>>> +	/*
> > > >>>> +	 * If NONBLOCK is set the caller is optimistically
> > > >>>> +	 * trying to cache the full object within the mappable
> > > >>>> +	 * aperture, and *must* have a fallback in place for
> > > >>>> +	 * situations where we cannot bind the object. We
> > > >>>> +	 * can be a little more lax here and use the fallback
> > > >>>> +	 * more often to avoid costly migrations of ourselves
> > > >>>> +	 * and other objects within the aperture.
> > > >>>> +	 */
> > > >>>> +	if (!(flags & PIN_NONBLOCK))
> > > >>>> +		return 0;
> > > >>>> +
> > > >>>> +	/*
> > > >>>> +	 * We only consider objects whose size is at-least a quarter of
> > > >>>> +	 * the aperture to be too big and subject them to the new
> > > >>>> +	 * heuristic below.
> > > >>>> +	 */
> > > >>>> +	if (obj->base.size < ggtt->mappable_end / 4)
> > > >>>> +		return 0;
> > > >>>
> > > >>> That seems a fairly arbitrary thing to put here. Maybe something the
> > > >>> caller should check/specify?
> > > >>
> > > >> I have no strong opinion on this one. In my mind I categorised it
> > > >> under "is it a large framebuffer" heuristics. Previously it was less
> > > >> than one half of aperture always okay, now one quarter, plus 2x hole
> > > >> check if larger. Both are heuristics. I even mentioned earlier if 2x
> > > >> should be an input parameter as well, but again, given it's not an
> > > >> exported function couldn't really justify it.
> > > >
> > > > Is there any point in even having this extra check? If we don't think
> > > > checking this is worth the hassle then why call the function at all?
> > >
> > > The "/4" one? It was my suggestion to avoid the hole search if we can know based on
> size
> > > it cannot be a frame buffer that would be affected by the ping-ping problem. Granted
> that
> > > was before the rbtree hole search, when it was traversing the un-ordered linked list of
> > > holes. What is the correct size threshold I don't know.
> > >
> > > >>>> +
> > > >>>> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> > > >>>> +	    !i915_gem_object_is_framebuffer(obj))
> > > >>>> +		return 0;
> > > >>>
> > > >>> None of that seems appropriate for a generic gem function with this
> > > >>> name.
> > > >>
> > > >> It's not exported though, maybe remove i915_gem prefix to avoid any
> > > >> ideas of it being generic?
> > > >
> > > > These checks don't even seem to doing anything useful. HAS_GMCH should
> > > > already be covered by always setting PIN_MAPPABLE and hence O_NONBLOCK
> > [Kasireddy, Vivek] I can drop the HAS_GMCH(i915) check given that it is redundant.
> >
> > > > is never even tried, the pre-icl vs. icl+ check should not exist at
> > [Kasireddy, Vivek] My aim was to narrow down the list of situations in which the
> > ping-pong problem becomes more pronounced and may lead to performance
> > issues. Therefore, I added the DISPLAY_VER(i915) check since 8K/bigjoiner is
> > feasible only on those newer platforms.
> 
> Like I said before bigjoiner is irrelevant. The only thing that
> matters is the size of the mapping vs. mappable aperture size.
[Kasireddy, Vivek] Ok, got it.

> 
> >
> > > > all IMO, and if this is only called for framebuffers then why does the
> > > > code pretend that is not the case?
> > [Kasireddy, Vivek] Oh, I added the i915_gem_object_is_framebuffer() check after I
> > found that there are other callers (for example, reloc_iomap() in i915_gem_execbuffer.c)
> > of i915_gem_object_ggtt_pin_ww() that may not be working on an fb.
> >
> > Also, I figured size < ggtt->mappable_end / 4 or a similar check is needed as we do
> > not want to subject all FBs through this performance critical path.
> 
> Why not?
[Kasireddy, Vivek] Oh, I just thought that it makes sense to avoid the expensive hole search
for smaller FBs that are unlikely to exhaust the mappable aperture space. And, I also wanted
to preserve the behavior prior to this patch. However, I guess I could drop this check as well
given that the hole search via rbtree traversal may not be that bad in terms of time.

What about the i915_gem_object_is_framebuffer() check? Should I keep it given the smaller
size and transient nature of batchbuffers that go through this path?

Thanks,
Vivek

> 
> --
> Ville Syrjälä
> Intel

Ville Syrjälä Feb. 9, 2022, 7:42 a.m. UTC | #9

On Wed, Feb 09, 2022 at 01:47:01AM +0000, Kasireddy, Vivek wrote:
> Hi Ville,
> 
> > 
> > On Tue, Feb 08, 2022 at 05:10:42AM +0000, Kasireddy, Vivek wrote:
> > > Hi Tvrtko, Ville,
> > >
> > > > On 07/02/2022 13:24, Ville Syrjälä wrote:
> > > > > On Mon, Feb 07, 2022 at 11:47:16AM +0000, Tvrtko Ursulin wrote:
> > > > >>
> > > > >> On 07/02/2022 10:58, Ville Syrjälä wrote:
> > > > >>> On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
> > > > >>>> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2
> > > > >>>> or more framebuffers/scanout buffers results in only one that is
> > > > >>>> mappable/ fenceable. Therefore, pageflipping between these 2 FBs
> > > > >>>> where only one is mappable/fenceable creates latencies large enough
> > > > >>>> to miss alternate vblanks thereby producing less optimal framerate.
> > > > >>>>
> > > > >>>> This mainly happens because when
> > > > >>>> i915_gem_object_pin_to_display_plane()
> > > > >>>> is called to pin one of the FB objs, the associated vma is
> > > > >>>> identified as misplaced and therefore i915_vma_unbind() is called
> > > > >>>> which unbinds and evicts it. This misplaced vma gets subseqently
> > > > >>>> pinned only when
> > > > >>>> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> > > > >>>> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> > > > >>>> Therefore, to fix this issue, we try to see if there is space to
> > > > >>>> map at-least two objects of a given size and return early if there
> > > > >>>> isn't. This would ensure that we do not try with PIN_MAPPABLE for
> > > > >>>> any objects that are too big to map thereby preventing unncessary unbind.
> > > > >>>>
> > > > >>>> Testcase:
> > > > >>>> Running Weston and weston-simple-egl on an Alderlake_S (ADLS)
> > > > >>>> platform with a 8K@60 mode results in only ~40 FPS. Since upstream
> > > > >>>> Weston submits a frame ~7ms before the next vblank, the latencies
> > > > >>>> seen between atomic commit and flip event are 7, 24 (7 + 16.66), 7,
> > > > >>>> 24..... suggesting that it misses the vblank every other frame.
> > > > >>>>
> > > > >>>> Here is the ftrace snippet that shows the source of the ~10ms latency:
> > > > >>>>                 i915_gem_object_pin_to_display_plane() {
> > > > >>>> 0.102 us   |    i915_gem_object_set_cache_level();
> > > > >>>>                   i915_gem_object_ggtt_pin_ww() {
> > > > >>>> 0.390 us   |      i915_vma_instance();
> > > > >>>> 0.178 us   |      i915_vma_misplaced();
> > > > >>>>                     i915_vma_unbind() {
> > > > >>>>                     __i915_active_wait() {
> > > > >>>> 0.082 us   |        i915_active_acquire_if_busy();
> > > > >>>> 0.475 us   |      }
> > > > >>>>                     intel_runtime_pm_get() {
> > > > >>>> 0.087 us   |        intel_runtime_pm_acquire();
> > > > >>>> 0.259 us   |      }
> > > > >>>>                     __i915_active_wait() {
> > > > >>>> 0.085 us   |        i915_active_acquire_if_busy();
> > > > >>>> 0.240 us   |      }
> > > > >>>>                     __i915_vma_evict() {
> > > > >>>>                       ggtt_unbind_vma() {
> > > > >>>>                         gen8_ggtt_clear_range() {
> > > > >>>> 10507.255 us |        }
> > > > >>>> 10507.689 us |      }
> > > > >>>> 10508.516 us |   }
> > > > >>>>
> > > > >>>> v2: Instead of using bigjoiner checks, determine whether a scanout
> > > > >>>>       buffer is too big by checking to see if it is possible to map
> > > > >>>>       two of them into the ggtt.
> > > > >>>>
> > > > >>>> v3 (Ville):
> > > > >>>> - Count how many fb objects can be fit into the available holes
> > > > >>>>     instead of checking for a hole twice the object size.
> > > > >>>> - Take alignment constraints into account.
> > > > >>>> - Limit this large scanout buffer check to >= Gen 11 platforms.
> > > > >>>>
> > > > >>>> v4:
> > > > >>>> - Remove existing heuristic that checks just for size. (Ville)
> > > > >>>> - Return early if we find space to map at-least two objects.
> > > > >>>> (Tvrtko)
> > > > >>>> - Slightly update the commit message.
> > > > >>>>
> > > > >>>> v5: (Tvrtko)
> > > > >>>> - Rename the function to indicate that the object may be too big to
> > > > >>>>     map into the aperture.
> > > > >>>> - Account for guard pages while calculating the total size required
> > > > >>>>     for the object.
> > > > >>>> - Do not subject all objects to the heuristic check and instead
> > > > >>>>     consider objects only of a certain size.
> > > > >>>> - Do the hole walk using the rbtree.
> > > > >>>> - Preserve the existing PIN_NONBLOCK logic.
> > > > >>>> - Drop the PIN_MAPPABLE check while pinning the VMA.
> > > > >>>>
> > > > >>>> v6: (Tvrtko)
> > > > >>>> - Return 0 on success and the specific error code on failure to
> > > > >>>>     preserve the existing behavior.
> > > > >>>>
> > > > >>>> Cc: Ville Syrjälä <[e-mail]>
> > > > >>>> Cc: Maarten Lankhorst <[e-mail]>
> > > > >>>> Cc: Tvrtko Ursulin <[e-mail]>
> > > > >>>> Cc: Manasi Navare <[e-mail]>
> > > > >>>> Signed-off-by: Vivek Kasireddy <[e-mail]>
> > > > >>>> ---
> > > > >>>>    drivers/gpu/drm/i915/i915_gem.c | 120 ++++++++++++++++++++++++-----
> > ---
> > > > >>>>    1 file changed, 90 insertions(+), 30 deletions(-)
> > > > >>>>
> > > > >>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > > >>>> b/drivers/gpu/drm/i915/i915_gem.c index e3a2c2a0e156..39f0d17550c3
> > > > >>>> 100644
> > > > >>>> --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > >>>> @@ -46,6 +46,7 @@
> > > > >>>>    #include "gem/i915_gem_mman.h"
> > > > >>>>    #include "gem/i915_gem_region.h"
> > > > >>>>    #include "gem/i915_gem_userptr.h"
> > > > >>>> +#include "gem/i915_gem_tiling.h"
> > > > >>>>    #include "gt/intel_engine_user.h"
> > > > >>>>    #include "gt/intel_gt.h"
> > > > >>>>    #include "gt/intel_gt_pm.h"
> > > > >>>> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma *vma)
> > > > >>>>    	spin_unlock(&obj->vma.lock);
> > > > >>>>    }
> > > > >>>>
> > > > >>>> +static int
> > > > >>>> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> > > > >>>> +				 u64 alignment, u64 flags)
> > > > >>>> +{
> > > > >>>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > > > >>>> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> > > > >>>> +	struct drm_mm_node *hole;
> > > > >>>> +	u64 hole_start, hole_end, start, end;
> > > > >>>> +	u64 fence_size, fence_alignment;
> > > > >>>> +	unsigned int count = 0;
> > > > >>>> +
> > > > >>>> +	/*
> > > > >>>> +	 * If the required space is larger than the available
> > > > >>>> +	 * aperture, we will not able to find a slot for the
> > > > >>>> +	 * object and unbinding the object now will be in
> > > > >>>> +	 * vain. Worse, doing so may cause us to ping-pong
> > > > >>>> +	 * the object in and out of the Global GTT and
> > > > >>>> +	 * waste a lot of cycles under the mutex.
> > > > >>>> +	 */
> > > > >>>> +	if (obj->base.size > ggtt->mappable_end)
> > > > >>>> +		return -E2BIG;
> > > > >>>> +
> > > > >>>> +	/*
> > > > >>>> +	 * If NONBLOCK is set the caller is optimistically
> > > > >>>> +	 * trying to cache the full object within the mappable
> > > > >>>> +	 * aperture, and *must* have a fallback in place for
> > > > >>>> +	 * situations where we cannot bind the object. We
> > > > >>>> +	 * can be a little more lax here and use the fallback
> > > > >>>> +	 * more often to avoid costly migrations of ourselves
> > > > >>>> +	 * and other objects within the aperture.
> > > > >>>> +	 */
> > > > >>>> +	if (!(flags & PIN_NONBLOCK))
> > > > >>>> +		return 0;
> > > > >>>> +
> > > > >>>> +	/*
> > > > >>>> +	 * We only consider objects whose size is at-least a quarter of
> > > > >>>> +	 * the aperture to be too big and subject them to the new
> > > > >>>> +	 * heuristic below.
> > > > >>>> +	 */
> > > > >>>> +	if (obj->base.size < ggtt->mappable_end / 4)
> > > > >>>> +		return 0;
> > > > >>>
> > > > >>> That seems a fairly arbitrary thing to put here. Maybe something the
> > > > >>> caller should check/specify?
> > > > >>
> > > > >> I have no strong opinion on this one. In my mind I categorised it
> > > > >> under "is it a large framebuffer" heuristics. Previously it was less
> > > > >> than one half of aperture always okay, now one quarter, plus 2x hole
> > > > >> check if larger. Both are heuristics. I even mentioned earlier if 2x
> > > > >> should be an input parameter as well, but again, given it's not an
> > > > >> exported function couldn't really justify it.
> > > > >
> > > > > Is there any point in even having this extra check? If we don't think
> > > > > checking this is worth the hassle then why call the function at all?
> > > >
> > > > The "/4" one? It was my suggestion to avoid the hole search if we can know based on
> > size
> > > > it cannot be a frame buffer that would be affected by the ping-ping problem. Granted
> > that
> > > > was before the rbtree hole search, when it was traversing the un-ordered linked list of
> > > > holes. What is the correct size threshold I don't know.
> > > >
> > > > >>>> +
> > > > >>>> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> > > > >>>> +	    !i915_gem_object_is_framebuffer(obj))
> > > > >>>> +		return 0;
> > > > >>>
> > > > >>> None of that seems appropriate for a generic gem function with this
> > > > >>> name.
> > > > >>
> > > > >> It's not exported though, maybe remove i915_gem prefix to avoid any
> > > > >> ideas of it being generic?
> > > > >
> > > > > These checks don't even seem to doing anything useful. HAS_GMCH should
> > > > > already be covered by always setting PIN_MAPPABLE and hence O_NONBLOCK
> > > [Kasireddy, Vivek] I can drop the HAS_GMCH(i915) check given that it is redundant.
> > >
> > > > > is never even tried, the pre-icl vs. icl+ check should not exist at
> > > [Kasireddy, Vivek] My aim was to narrow down the list of situations in which the
> > > ping-pong problem becomes more pronounced and may lead to performance
> > > issues. Therefore, I added the DISPLAY_VER(i915) check since 8K/bigjoiner is
> > > feasible only on those newer platforms.
> > 
> > Like I said before bigjoiner is irrelevant. The only thing that
> > matters is the size of the mapping vs. mappable aperture size.
> [Kasireddy, Vivek] Ok, got it.
> 
> > 
> > >
> > > > > all IMO, and if this is only called for framebuffers then why does the
> > > > > code pretend that is not the case?
> > > [Kasireddy, Vivek] Oh, I added the i915_gem_object_is_framebuffer() check after I
> > > found that there are other callers (for example, reloc_iomap() in i915_gem_execbuffer.c)
> > > of i915_gem_object_ggtt_pin_ww() that may not be working on an fb.
> > >
> > > Also, I figured size < ggtt->mappable_end / 4 or a similar check is needed as we do
> > > not want to subject all FBs through this performance critical path.
> > 
> > Why not?
> [Kasireddy, Vivek] Oh, I just thought that it makes sense to avoid the expensive hole search
> for smaller FBs that are unlikely to exhaust the mappable aperture space. And, I also wanted
> to preserve the behavior prior to this patch. However, I guess I could drop this check as well
> given that the hole search via rbtree traversal may not be that bad in terms of time.
> 
> What about the i915_gem_object_is_framebuffer() check? Should I keep it given the smaller
> size and transient nature of batchbuffers that go through this path?

Hmm. I guess if we don't have any real idea on the cost of this then
maybe we should not do it there, for the moment at least. I'm just
pondering if this whole thing should just be a seaprate function
each current caller of i915_gem_object_ggtt_pin_ww() should call
explicitly... But maybe we want framebuffers in partcular to undergo
the same restrictions always whether it's execbuffer or display
activity that's trying to pin them.

However, now that I think about this I'm wondering if we should do
all this stuff a bit later. I mean why are we even bothering with
these checks if the vma is already bound and not misplaced? Seems
the vma_is_misplaced() case already has the same mappable_size/2
check so that should probably be replaced with this thing, and then
additionally we could do something like this:

  if (vma_is_miplaced()) {
  	...
  	if (flags & PIN_NONBLOCK)
  		already have a check here;
+ } else if (!vma_is_bound()) {
+ 	if (flags & PIN_NONBLOCK)
+ 		check here too;
+ }

Kasireddy, Vivek Feb. 10, 2022, 1:41 a.m. UTC | #10

Hi Ville,

> On Wed, Feb 09, 2022 at 01:47:01AM +0000, Kasireddy, Vivek wrote:
> > Hi Ville,
> >
> > >
> > > On Tue, Feb 08, 2022 at 05:10:42AM +0000, Kasireddy, Vivek wrote:
> > > > Hi Tvrtko, Ville,
> > > >
> > > > > On 07/02/2022 13:24, Ville Syrjälä wrote:
> > > > > > On Mon, Feb 07, 2022 at 11:47:16AM +0000, Tvrtko Ursulin wrote:
> > > > > >>
> > > > > >> On 07/02/2022 10:58, Ville Syrjälä wrote:
> > > > > >>> On Thu, Feb 03, 2022 at 05:22:10PM -0800, Vivek Kasireddy wrote:
> > > > > >>>> On platforms capable of allowing 8K (7680 x 4320) modes, pinning 2
> > > > > >>>> or more framebuffers/scanout buffers results in only one that is
> > > > > >>>> mappable/ fenceable. Therefore, pageflipping between these 2 FBs
> > > > > >>>> where only one is mappable/fenceable creates latencies large enough
> > > > > >>>> to miss alternate vblanks thereby producing less optimal framerate.
> > > > > >>>>
> > > > > >>>> This mainly happens because when
> > > > > >>>> i915_gem_object_pin_to_display_plane()
> > > > > >>>> is called to pin one of the FB objs, the associated vma is
> > > > > >>>> identified as misplaced and therefore i915_vma_unbind() is called
> > > > > >>>> which unbinds and evicts it. This misplaced vma gets subseqently
> > > > > >>>> pinned only when
> > > > > >>>> i915_gem_object_ggtt_pin_ww() is called without PIN_MAPPABLE. This
> > > > > >>>> results in a latency of ~10ms and happens every other vblank/repaint cycle.
> > > > > >>>> Therefore, to fix this issue, we try to see if there is space to
> > > > > >>>> map at-least two objects of a given size and return early if there
> > > > > >>>> isn't. This would ensure that we do not try with PIN_MAPPABLE for
> > > > > >>>> any objects that are too big to map thereby preventing unncessary unbind.
> > > > > >>>>
> > > > > >>>> Testcase:
> > > > > >>>> Running Weston and weston-simple-egl on an Alderlake_S (ADLS)
> > > > > >>>> platform with a 8K@60 mode results in only ~40 FPS. Since upstream
> > > > > >>>> Weston submits a frame ~7ms before the next vblank, the latencies
> > > > > >>>> seen between atomic commit and flip event are 7, 24 (7 + 16.66), 7,
> > > > > >>>> 24..... suggesting that it misses the vblank every other frame.
> > > > > >>>>
> > > > > >>>> Here is the ftrace snippet that shows the source of the ~10ms latency:
> > > > > >>>>                 i915_gem_object_pin_to_display_plane() {
> > > > > >>>> 0.102 us   |    i915_gem_object_set_cache_level();
> > > > > >>>>                   i915_gem_object_ggtt_pin_ww() {
> > > > > >>>> 0.390 us   |      i915_vma_instance();
> > > > > >>>> 0.178 us   |      i915_vma_misplaced();
> > > > > >>>>                     i915_vma_unbind() {
> > > > > >>>>                     __i915_active_wait() {
> > > > > >>>> 0.082 us   |        i915_active_acquire_if_busy();
> > > > > >>>> 0.475 us   |      }
> > > > > >>>>                     intel_runtime_pm_get() {
> > > > > >>>> 0.087 us   |        intel_runtime_pm_acquire();
> > > > > >>>> 0.259 us   |      }
> > > > > >>>>                     __i915_active_wait() {
> > > > > >>>> 0.085 us   |        i915_active_acquire_if_busy();
> > > > > >>>> 0.240 us   |      }
> > > > > >>>>                     __i915_vma_evict() {
> > > > > >>>>                       ggtt_unbind_vma() {
> > > > > >>>>                         gen8_ggtt_clear_range() {
> > > > > >>>> 10507.255 us |        }
> > > > > >>>> 10507.689 us |      }
> > > > > >>>> 10508.516 us |   }
> > > > > >>>>
> > > > > >>>> v2: Instead of using bigjoiner checks, determine whether a scanout
> > > > > >>>>       buffer is too big by checking to see if it is possible to map
> > > > > >>>>       two of them into the ggtt.
> > > > > >>>>
> > > > > >>>> v3 (Ville):
> > > > > >>>> - Count how many fb objects can be fit into the available holes
> > > > > >>>>     instead of checking for a hole twice the object size.
> > > > > >>>> - Take alignment constraints into account.
> > > > > >>>> - Limit this large scanout buffer check to >= Gen 11 platforms.
> > > > > >>>>
> > > > > >>>> v4:
> > > > > >>>> - Remove existing heuristic that checks just for size. (Ville)
> > > > > >>>> - Return early if we find space to map at-least two objects.
> > > > > >>>> (Tvrtko)
> > > > > >>>> - Slightly update the commit message.
> > > > > >>>>
> > > > > >>>> v5: (Tvrtko)
> > > > > >>>> - Rename the function to indicate that the object may be too big to
> > > > > >>>>     map into the aperture.
> > > > > >>>> - Account for guard pages while calculating the total size required
> > > > > >>>>     for the object.
> > > > > >>>> - Do not subject all objects to the heuristic check and instead
> > > > > >>>>     consider objects only of a certain size.
> > > > > >>>> - Do the hole walk using the rbtree.
> > > > > >>>> - Preserve the existing PIN_NONBLOCK logic.
> > > > > >>>> - Drop the PIN_MAPPABLE check while pinning the VMA.
> > > > > >>>>
> > > > > >>>> v6: (Tvrtko)
> > > > > >>>> - Return 0 on success and the specific error code on failure to
> > > > > >>>>     preserve the existing behavior.
> > > > > >>>>
> > > > > >>>> Cc: Ville Syrjälä <[e-mail]>
> > > > > >>>> Cc: Maarten Lankhorst <[e-mail]>
> > > > > >>>> Cc: Tvrtko Ursulin <[e-mail]>
> > > > > >>>> Cc: Manasi Navare <[e-mail]>
> > > > > >>>> Signed-off-by: Vivek Kasireddy <[e-mail]>
> > > > > >>>> ---
> > > > > >>>>    drivers/gpu/drm/i915/i915_gem.c | 120
> ++++++++++++++++++++++++-----
> > > ---
> > > > > >>>>    1 file changed, 90 insertions(+), 30 deletions(-)
> > > > > >>>>
> > > > > >>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > > > >>>> b/drivers/gpu/drm/i915/i915_gem.c index e3a2c2a0e156..39f0d17550c3
> > > > > >>>> 100644
> > > > > >>>> --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > > >>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > > >>>> @@ -46,6 +46,7 @@
> > > > > >>>>    #include "gem/i915_gem_mman.h"
> > > > > >>>>    #include "gem/i915_gem_region.h"
> > > > > >>>>    #include "gem/i915_gem_userptr.h"
> > > > > >>>> +#include "gem/i915_gem_tiling.h"
> > > > > >>>>    #include "gt/intel_engine_user.h"
> > > > > >>>>    #include "gt/intel_gt.h"
> > > > > >>>>    #include "gt/intel_gt_pm.h"
> > > > > >>>> @@ -876,6 +877,92 @@ static void discard_ggtt_vma(struct i915_vma
> *vma)
> > > > > >>>>    	spin_unlock(&obj->vma.lock);
> > > > > >>>>    }
> > > > > >>>>
> > > > > >>>> +static int
> > > > > >>>> +i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
> > > > > >>>> +				 u64 alignment, u64 flags)
> > > > > >>>> +{
> > > > > >>>> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> > > > > >>>> +	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> > > > > >>>> +	struct drm_mm_node *hole;
> > > > > >>>> +	u64 hole_start, hole_end, start, end;
> > > > > >>>> +	u64 fence_size, fence_alignment;
> > > > > >>>> +	unsigned int count = 0;
> > > > > >>>> +
> > > > > >>>> +	/*
> > > > > >>>> +	 * If the required space is larger than the available
> > > > > >>>> +	 * aperture, we will not able to find a slot for the
> > > > > >>>> +	 * object and unbinding the object now will be in
> > > > > >>>> +	 * vain. Worse, doing so may cause us to ping-pong
> > > > > >>>> +	 * the object in and out of the Global GTT and
> > > > > >>>> +	 * waste a lot of cycles under the mutex.
> > > > > >>>> +	 */
> > > > > >>>> +	if (obj->base.size > ggtt->mappable_end)
> > > > > >>>> +		return -E2BIG;
> > > > > >>>> +
> > > > > >>>> +	/*
> > > > > >>>> +	 * If NONBLOCK is set the caller is optimistically
> > > > > >>>> +	 * trying to cache the full object within the mappable
> > > > > >>>> +	 * aperture, and *must* have a fallback in place for
> > > > > >>>> +	 * situations where we cannot bind the object. We
> > > > > >>>> +	 * can be a little more lax here and use the fallback
> > > > > >>>> +	 * more often to avoid costly migrations of ourselves
> > > > > >>>> +	 * and other objects within the aperture.
> > > > > >>>> +	 */
> > > > > >>>> +	if (!(flags & PIN_NONBLOCK))
> > > > > >>>> +		return 0;
> > > > > >>>> +
> > > > > >>>> +	/*
> > > > > >>>> +	 * We only consider objects whose size is at-least a quarter of
> > > > > >>>> +	 * the aperture to be too big and subject them to the new
> > > > > >>>> +	 * heuristic below.
> > > > > >>>> +	 */
> > > > > >>>> +	if (obj->base.size < ggtt->mappable_end / 4)
> > > > > >>>> +		return 0;
> > > > > >>>
> > > > > >>> That seems a fairly arbitrary thing to put here. Maybe something the
> > > > > >>> caller should check/specify?
> > > > > >>
> > > > > >> I have no strong opinion on this one. In my mind I categorised it
> > > > > >> under "is it a large framebuffer" heuristics. Previously it was less
> > > > > >> than one half of aperture always okay, now one quarter, plus 2x hole
> > > > > >> check if larger. Both are heuristics. I even mentioned earlier if 2x
> > > > > >> should be an input parameter as well, but again, given it's not an
> > > > > >> exported function couldn't really justify it.
> > > > > >
> > > > > > Is there any point in even having this extra check? If we don't think
> > > > > > checking this is worth the hassle then why call the function at all?
> > > > >
> > > > > The "/4" one? It was my suggestion to avoid the hole search if we can know based
> on
> > > size
> > > > > it cannot be a frame buffer that would be affected by the ping-ping problem.
> Granted
> > > that
> > > > > was before the rbtree hole search, when it was traversing the un-ordered linked list
> of
> > > > > holes. What is the correct size threshold I don't know.
> > > > >
> > > > > >>>> +
> > > > > >>>> +	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
> > > > > >>>> +	    !i915_gem_object_is_framebuffer(obj))
> > > > > >>>> +		return 0;
> > > > > >>>
> > > > > >>> None of that seems appropriate for a generic gem function with this
> > > > > >>> name.
> > > > > >>
> > > > > >> It's not exported though, maybe remove i915_gem prefix to avoid any
> > > > > >> ideas of it being generic?
> > > > > >
> > > > > > These checks don't even seem to doing anything useful. HAS_GMCH should
> > > > > > already be covered by always setting PIN_MAPPABLE and hence
> O_NONBLOCK
> > > > [Kasireddy, Vivek] I can drop the HAS_GMCH(i915) check given that it is
> redundant.
> > > >
> > > > > > is never even tried, the pre-icl vs. icl+ check should not exist at
> > > > [Kasireddy, Vivek] My aim was to narrow down the list of situations in which the
> > > > ping-pong problem becomes more pronounced and may lead to performance
> > > > issues. Therefore, I added the DISPLAY_VER(i915) check since 8K/bigjoiner is
> > > > feasible only on those newer platforms.
> > >
> > > Like I said before bigjoiner is irrelevant. The only thing that
> > > matters is the size of the mapping vs. mappable aperture size.
> > [Kasireddy, Vivek] Ok, got it.
> >
> > >
> > > >
> > > > > > all IMO, and if this is only called for framebuffers then why does the
> > > > > > code pretend that is not the case?
> > > > [Kasireddy, Vivek] Oh, I added the i915_gem_object_is_framebuffer() check after I
> > > > found that there are other callers (for example, reloc_iomap() in
> i915_gem_execbuffer.c)
> > > > of i915_gem_object_ggtt_pin_ww() that may not be working on an fb.
> > > >
> > > > Also, I figured size < ggtt->mappable_end / 4 or a similar check is needed as we do
> > > > not want to subject all FBs through this performance critical path.
> > >
> > > Why not?
> > [Kasireddy, Vivek] Oh, I just thought that it makes sense to avoid the expensive hole
> search
> > for smaller FBs that are unlikely to exhaust the mappable aperture space. And, I also
> wanted
> > to preserve the behavior prior to this patch. However, I guess I could drop this check as
> well
> > given that the hole search via rbtree traversal may not be that bad in terms of time.
> >
> > What about the i915_gem_object_is_framebuffer() check? Should I keep it given the
> smaller
> > size and transient nature of batchbuffers that go through this path?
> 
> Hmm. I guess if we don't have any real idea on the cost of this then
> maybe we should not do it there, for the moment at least. I'm just
> pondering if this whole thing should just be a seaprate function
> each current caller of i915_gem_object_ggtt_pin_ww() should call
> explicitly... But maybe we want framebuffers in partcular to undergo
> the same restrictions always whether it's execbuffer or display
> activity that's trying to pin them.
> 
> However, now that I think about this I'm wondering if we should do
> all this stuff a bit later. I mean why are we even bothering with
> these checks if the vma is already bound and not misplaced?
[Kasireddy, Vivek] As I explain in the commit message, Weston flips
between two 8K FBs: FB1 and FB2 where
FB1 is mappable/fenceable and therefore not misplaced.
FB2 is NOT mappable and hence identified as misplaced.

And, when i915_gem_object_pin_to_display_plane() is called to pin
these FBs -- as part of pageflips -- this is what it does:
vma = ERR_PTR(-ENOSPC);
        if ((flags & PIN_MAPPABLE) == 0 &&
            (!view || view->type == I915_GGTT_VIEW_NORMAL))
                vma = i915_gem_object_ggtt_pin_ww(obj, ww, view, 0, alignment,
                                                  flags | PIN_MAPPABLE |
                                                  PIN_NONBLOCK);
        if (IS_ERR(vma) && vma != ERR_PTR(-EDEADLK))
                vma = i915_gem_object_ggtt_pin_ww(obj, ww, view, 0,
                                                  alignment, flags);

FB1 gets pinned in the first call to i915_gem_object_ggtt_pin_ww() (which
has PIN_MAPPABLE) and FB2 gets pinned in the second -- which does 
not have PIN_MAPPABLE as the first call fails because there is no 
space left in the aperture. However, during the next pageflips, FB1 
does not cause any issues but FB2 gets identified as misplaced
(because it fails the check
(flags & PIN_MAPPABLE && !i915_vma_is_map_and_fenceable(vma))
in the first call resulting in a costly unbind/eviction. It subsequently gets
pinned outside of mappable aperture in the second call though.

My solution -- with this patch -- is to just find out if there is enough space
in the mappable aperture for both FB1 and FB2 when either of them are getting
pinned -- with PIN_MAPPABLE. Neither of them are pinned into the aperture
if there is no room for both. One could argue that since there is room for FB1
in the aperture, we should let it be but I am not sure if this inconsistency
(i.e no FB2 but only FB1 in aperture) is OK with userspace apps.

> the vma_is_misplaced() case already has the same mappable_size/2
> check so that should probably be replaced with this thing, and then
[Kasireddy, Vivek] Right, this check seems redundant and can be removed.

> additionally we could do something like this:
> 
>   if (vma_is_miplaced()) {
>   	...
>   	if (flags & PIN_NONBLOCK)
>   		already have a check here;
> + } else if (!vma_is_bound()) {
> + 	if (flags & PIN_NONBLOCK)
> + 		check here too;
[Kasireddy, Vivek] Since it is the same check, I think doing it once
before if (i915_vma_misplaced()) should be ok as well.

Thanks,
Vivek

> + }
> 
> --
> Ville Syrjälä
> Intel

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e3a2c2a0e156..39f0d17550c3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -46,6 +46,7 @@ 
 #include "gem/i915_gem_mman.h"
 #include "gem/i915_gem_region.h"
 #include "gem/i915_gem_userptr.h"
+#include "gem/i915_gem_tiling.h"
 #include "gt/intel_engine_user.h"
 #include "gt/intel_gt.h"
 #include "gt/intel_gt_pm.h"
@@ -876,6 +877,92 @@  static void discard_ggtt_vma(struct i915_vma *vma)
 	spin_unlock(&obj->vma.lock);
 }
 
+static int
+i915_gem_object_fits_in_aperture(struct drm_i915_gem_object *obj,
+				 u64 alignment, u64 flags)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
+	struct drm_mm_node *hole;
+	u64 hole_start, hole_end, start, end;
+	u64 fence_size, fence_alignment;
+	unsigned int count = 0;
+
+	/*
+	 * If the required space is larger than the available
+	 * aperture, we will not able to find a slot for the
+	 * object and unbinding the object now will be in
+	 * vain. Worse, doing so may cause us to ping-pong
+	 * the object in and out of the Global GTT and
+	 * waste a lot of cycles under the mutex.
+	 */
+	if (obj->base.size > ggtt->mappable_end)
+		return -E2BIG;
+
+	/*
+	 * If NONBLOCK is set the caller is optimistically
+	 * trying to cache the full object within the mappable
+	 * aperture, and *must* have a fallback in place for
+	 * situations where we cannot bind the object. We
+	 * can be a little more lax here and use the fallback
+	 * more often to avoid costly migrations of ourselves
+	 * and other objects within the aperture.
+	 */
+	if (!(flags & PIN_NONBLOCK))
+		return 0;
+
+	/*
+	 * We only consider objects whose size is at-least a quarter of
+	 * the aperture to be too big and subject them to the new
+	 * heuristic below.
+	 */
+	if (obj->base.size < ggtt->mappable_end / 4)
+		return 0;
+
+	if (HAS_GMCH(i915) || DISPLAY_VER(i915) < 11 ||
+	    !i915_gem_object_is_framebuffer(obj))
+		return 0;
+
+	fence_size = i915_gem_fence_size(i915, obj->base.size,
+					 i915_gem_object_get_tiling(obj),
+					 i915_gem_object_get_stride(obj));
+
+	if (i915_vm_has_cache_coloring(&ggtt->vm))
+		fence_size += 2 * I915_GTT_PAGE_SIZE;
+
+	fence_alignment = i915_gem_fence_alignment(i915, obj->base.size,
+						   i915_gem_object_get_tiling(obj),
+						   i915_gem_object_get_stride(obj));
+	alignment = max_t(u64, alignment, fence_alignment);
+
+	/*
+	 * Assuming this object is a large scanout buffer, we try to find
+	 * out if there is room to map at-least two of them. There could
+	 * be space available to map one but to be consistent, we try to
+	 * avoid mapping/fencing any of them.
+	 */
+	drm_mm_for_each_suitable_hole(hole, &ggtt->vm.mm, 0, ggtt->mappable_end,
+				      fence_size, DRM_MM_INSERT_LOW) {
+		hole_start = drm_mm_hole_node_start(hole);
+		hole_end = hole_start + hole->hole_size;
+
+		do {
+			start = round_up(hole_start, alignment);
+			end = min_t(u64, hole_end, ggtt->mappable_end);
+
+			if (range_overflows(start, fence_size, end))
+				break;
+
+			if (++count >= 2)
+				return 0;
+
+			hole_start = start + fence_size;
+		} while (1);
+	}
+
+	return -ENOSPC;
+}
+
 struct i915_vma *
 i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
 			    struct i915_gem_ww_ctx *ww,
@@ -891,36 +978,9 @@  i915_gem_object_ggtt_pin_ww(struct drm_i915_gem_object *obj,
 
 	if (flags & PIN_MAPPABLE &&
 	    (!view || view->type == I915_GGTT_VIEW_NORMAL)) {
-		/*
-		 * If the required space is larger than the available
-		 * aperture, we will not able to find a slot for the
-		 * object and unbinding the object now will be in
-		 * vain. Worse, doing so may cause us to ping-pong
-		 * the object in and out of the Global GTT and
-		 * waste a lot of cycles under the mutex.
-		 */
-		if (obj->base.size > ggtt->mappable_end)
-			return ERR_PTR(-E2BIG);
-
-		/*
-		 * If NONBLOCK is set the caller is optimistically
-		 * trying to cache the full object within the mappable
-		 * aperture, and *must* have a fallback in place for
-		 * situations where we cannot bind the object. We
-		 * can be a little more lax here and use the fallback
-		 * more often to avoid costly migrations of ourselves
-		 * and other objects within the aperture.
-		 *
-		 * Half-the-aperture is used as a simple heuristic.
-		 * More interesting would to do search for a free
-		 * block prior to making the commitment to unbind.
-		 * That caters for the self-harm case, and with a
-		 * little more heuristics (e.g. NOFAULT, NOEVICT)
-		 * we could try to minimise harm to others.
-		 */
-		if (flags & PIN_NONBLOCK &&
-		    obj->base.size > ggtt->mappable_end / 2)
-			return ERR_PTR(-ENOSPC);
+		ret = i915_gem_object_fits_in_aperture(obj, alignment, flags);
+		if (ret)
+			return ERR_PTR(ret);
 	}
 
 new_vma: