Re: [PATCH] drm/i915: Move frontbuffer CS write tracking from ggtt vma to object

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qua, 2016-11-16 às 19:07 +0000, Chris Wilson escreveu:
> I tried to avoid having to track the write for every VMA by only
> tracking writes to the ggtt. However, for the purposes of frontbuffer
> tracking this is insufficient as we need to invalidate around writes
> not
> just to the the ggtt but all aliased ppgtt views of the framebuffer.
> By
> moving the critical section to the object and only doing so for
> framebuffer writes we can reduce the tracking even further by only
> watching framebuffers and not vma.

That fixes the test failures I was seeing. Thanks!

Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Testcase: igt/kms_frontbuffer_tracking/fbc-1p-primscrn-pri-indfb-draw-
blt (and a few others)

I didn't try bisecting, but maybe we could add:
Fixes: d07f0e59b2c7 ("drm/i915: Move GEM activity tracking into a
common struct reservation_object")

But while running kms_frontbuffer_tracking I'm still seeing a few dmesg
WARNs that were not present a few weeks ago:

WARNING: CPU: 3 PID: 56 at drivers/gpu/drm/i915/intel_lrc.c:706
execlists_schedule+0x32d/0x350 [i915]

WARN_ON(debug_locks && !lock_is_held(&(&request->i915-
>drm.struct_mutex)->dep_map))

[drm:drm_framebuffer_remove [drm]] *ERROR* failed to reset crtc
ffff88016385e7e8 when fb was deleted

(the lockdep ones are during page flip subtests)

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c            | 11 +++++++++++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 ++---
>  drivers/gpu/drm/i915/i915_gem_object.h     |  1 +
>  drivers/gpu/drm/i915/i915_gpu_error.c      |  4 ++--
>  drivers/gpu/drm/i915/i915_vma.c            | 12 ------------
>  drivers/gpu/drm/i915/i915_vma.h            |  1 -
>  drivers/gpu/drm/i915/intel_frontbuffer.h   |  5 +++--
>  7 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c
> index 7c57ba9ed2ea..20d402e75943 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3963,6 +3963,16 @@ i915_gem_madvise_ioctl(struct drm_device *dev,
> void *data,
>  	return err;
>  }
>  
> +static void
> +frontbuffer_retire(struct i915_gem_active *active,
> +		   struct drm_i915_gem_request *request)
> +{
> +	struct drm_i915_gem_object *obj =
> +		container_of(active, typeof(*obj),
> frontbuffer_write);
> +
> +	intel_fb_obj_flush(obj, true, ORIGIN_CS);
> +}
> +
>  void i915_gem_object_init(struct drm_i915_gem_object *obj,
>  			  const struct drm_i915_gem_object_ops *ops)
>  {
> @@ -3980,6 +3990,7 @@ void i915_gem_object_init(struct
> drm_i915_gem_object *obj,
>  	obj->resv = &obj->__builtin_resv;
>  
>  	obj->frontbuffer_ggtt_origin = ORIGIN_GTT;
> +	init_request_active(&obj->frontbuffer_write,
> frontbuffer_retire);
>  
>  	obj->mm.madv = I915_MADV_WILLNEED;
>  	INIT_RADIX_TREE(&obj->mm.get_page.radix, GFP_KERNEL |
> __GFP_NOWARN);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d845a54360a7..3ca12e6cbfa4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1282,9 +1282,8 @@ void i915_vma_move_to_active(struct i915_vma
> *vma,
>  	list_move_tail(&vma->vm_link, &vma->vm->active_list);
>  
>  	if (flags & EXEC_OBJECT_WRITE) {
> -		i915_gem_active_set(&vma->last_write, req);
> -
> -		intel_fb_obj_invalidate(obj, ORIGIN_CS);
> +		if (intel_fb_obj_invalidate(obj, ORIGIN_CS))
> +			i915_gem_active_set(&obj->frontbuffer_write, 
> req);
>  
>  		/* update for the implicit flush after a batch */
>  		obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
> diff --git a/drivers/gpu/drm/i915/i915_gem_object.h
> b/drivers/gpu/drm/i915/i915_gem_object.h
> index 440530b20ffa..15d96594bb7e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_object.h
> +++ b/drivers/gpu/drm/i915/i915_gem_object.h
> @@ -103,6 +103,7 @@ struct drm_i915_gem_object {
>  
>  	atomic_t frontbuffer_bits;
>  	unsigned int frontbuffer_ggtt_origin; /* write once */
> +	struct i915_gem_active frontbuffer_write;
>  
>  	/** Current tiling stride for the object, if it's tiled. */
>  	unsigned int tiling_and_stride;
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
> b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 5d620bd5dd22..2ca718868b37 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -887,8 +887,8 @@ static void capture_bo(struct
> drm_i915_error_buffer *err,
>  
>  	for (i = 0; i < I915_NUM_ENGINES; i++)
>  		err->rseqno[i] = __active_get_seqno(&vma-
> >last_read[i]);
> -	err->wseqno = __active_get_seqno(&vma->last_write);
> -	err->engine = __active_get_engine_id(&vma->last_write);
> +	err->wseqno = __active_get_seqno(&obj->frontbuffer_write);
> +	err->engine = __active_get_engine_id(&obj-
> >frontbuffer_write);
>  
>  	err->gtt_offset = vma->node.start;
>  	err->read_domains = obj->base.read_domains;
> diff --git a/drivers/gpu/drm/i915/i915_vma.c
> b/drivers/gpu/drm/i915/i915_vma.c
> index 44585300fdff..fd55bc8f1c61 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -74,16 +74,6 @@ i915_vma_retire(struct i915_gem_active *active,
>  	}
>  }
>  
> -static void
> -i915_ggtt_retire__write(struct i915_gem_active *active,
> -			struct drm_i915_gem_request *request)
> -{
> -	struct i915_vma *vma =
> -		container_of(active, struct i915_vma, last_write);
> -
> -	intel_fb_obj_flush(vma->obj, true, ORIGIN_CS);
> -}
> -
>  static struct i915_vma *
>  __i915_vma_create(struct drm_i915_gem_object *obj,
>  		  struct i915_address_space *vm,
> @@ -102,8 +92,6 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
>  	INIT_LIST_HEAD(&vma->exec_list);
>  	for (i = 0; i < ARRAY_SIZE(vma->last_read); i++)
>  		init_request_active(&vma->last_read[i],
> i915_vma_retire);
> -	init_request_active(&vma->last_write,
> -			    i915_is_ggtt(vm) ?
> i915_ggtt_retire__write : NULL);
>  	init_request_active(&vma->last_fence, NULL);
>  	list_add(&vma->vm_link, &vm->unbound_list);
>  	vma->vm = vm;
> diff --git a/drivers/gpu/drm/i915/i915_vma.h
> b/drivers/gpu/drm/i915/i915_vma.h
> index 2e49f5dd6107..85446f0b0b3f 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -80,7 +80,6 @@ struct i915_vma {
>  
>  	unsigned int active;
>  	struct i915_gem_active last_read[I915_NUM_ENGINES];
> -	struct i915_gem_active last_write;
>  	struct i915_gem_active last_fence;
>  
>  	/**
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.h
> b/drivers/gpu/drm/i915/intel_frontbuffer.h
> index 76ceb539f9f0..7bab41218cf7 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.h
> @@ -53,16 +53,17 @@ void __intel_fb_obj_flush(struct
> drm_i915_gem_object *obj,
>   * until the rendering completes or a flip on this frontbuffer plane
> is
>   * scheduled.
>   */
> -static inline void intel_fb_obj_invalidate(struct
> drm_i915_gem_object *obj,
> +static inline bool intel_fb_obj_invalidate(struct
> drm_i915_gem_object *obj,
>  					   enum fb_op_origin origin)
>  {
>  	unsigned int frontbuffer_bits;
>  
>  	frontbuffer_bits = atomic_read(&obj->frontbuffer_bits);
>  	if (!frontbuffer_bits)
> -		return;
> +		return false;
>  
>  	__intel_fb_obj_invalidate(obj, origin, frontbuffer_bits);
> +	return true;
>  }
>  
>  /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx