On Fri, Jan 29, 2016 at 07:19:30PM +0000, Dave Gordon wrote:
> In LRC mode, the HWSP is part of the default context object, and
> therefore does not exist independently. Worse, it doesn't contribute
> to the refcount on the default context object either.
>
> Currently, the default context is deallocated in intel_lr_context_free(),
> but the HWSP kmapping is not torn down until the subsequent call to
> intel_logical_ring_cleanup(). Between these calls, ring->status_page.obj
> continues to point to the (now non-existent) default context object,
> and the kernel mapping likewise points to a page which is now free.
>
> The solution is to dispose of the HWSP kmapping and pointer before the
> object itself is freed, so this patch moves the call to the teardown
> code from intel_lr_context_free() to intel_logical_ring_cleanup().
>
> This code was introduced in
>
> 48d8238 drm/i915/bdw: Generic logical ring init and cleanup
>
> i.e. it has been there ever since LRC mode was first implemented.
>
> v3:
> Rebased.
>
> Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index ff38e57..947ef15 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -2003,7 +2003,9 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
> i915_cmd_parser_fini_ring(ring);
> i915_gem_batch_pool_fini(&ring->batch_pool);
>
> - lrc_teardown_hardware_status_page(ring);
> + /* Status page should have gone already */
> + WARN_ON(ring->status_page.page_addr);
> + WARN_ON(ring->status_page.obj);
>
> ring->disable_lite_restore_wa = false;
> ring->ctx_desc_template = 0;
> @@ -2447,6 +2449,11 @@ void intel_lr_context_free(struct intel_context *ctx)
> continue;
>
> if (ctx == ctx->i915->kernel_context) {
> + /*
> + * The HWSP is part of the kernel context
> + * object in LRC mode, so tear it down now.
> + */
> + lrc_teardown_hardware_status_page(ringbuf->ring);
No. This is tieing the engine specific detail into the already present
layering violation in the context that I insist be removed.
All you have to do is to correctly pin/unpin the default context during
engine setup/teardown to fix the code and elimiate all the silly checks
against the kernel_context that execlists does.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
