[Intel-gfx] [PATCH] drm/i915: Move frontbuffer CS write tracking from ggtt vma to object
Paulo Zanoni
paulo.r.zanoni at intel.com
Wed Nov 16 19:52:43 UTC 2016
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 at 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 at chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni at 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;
> }
>
> /**
More information about the Intel-gfx
mailing list