On Tue, Apr 12, 2016 at 02:05:05PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> We can use the new pin/lazy unpin API for simplicity
> and more performance in the execlist submission paths.
>
> v2:
> * Fix error handling and convert more users.
> * Compact some names for readability.
>
> v3:
> * intel_lr_context_free was not unpinning.
> * Special case for GPU reset which otherwise unbalances
> the HWS object pages pin count by running the engine
> initialization only (not destructors).
Ah! Light dawns...
Should we not just separate out the hws setup and hws hw_init?
> -static void lrc_setup_hardware_status_page(struct intel_engine_cs *engine,
> - struct drm_i915_gem_object *default_ctx_obj)
> +static int
> +lrc_setup_hws(struct intel_engine_cs *engine,
> + struct drm_i915_gem_object *def_ctx_obj)
> {
> struct drm_i915_private *dev_priv = engine->dev->dev_private;
> - struct page *page;
> + void *hws;
>
> /* The HWSP is part of the default context object in LRC mode. */
> - engine->status_page.gfx_addr = i915_gem_obj_ggtt_offset(default_ctx_obj)
> - + LRC_PPHWSP_PN * PAGE_SIZE;
> - page = i915_gem_object_get_page(default_ctx_obj, LRC_PPHWSP_PN);
> - engine->status_page.page_addr = kmap(page);
> - engine->status_page.obj = default_ctx_obj;
> + engine->status_page.gfx_addr = i915_gem_obj_ggtt_offset(def_ctx_obj) +
> + LRC_PPHWSP_PN * PAGE_SIZE;
> + hws = i915_gem_object_pin_map(def_ctx_obj);
> + if (IS_ERR(hws))
> + return PTR_ERR(hws);
> + engine->status_page.page_addr = hws + LRC_PPHWSP_PN * PAGE_SIZE;
> + engine->status_page.obj = def_ctx_obj;
>
> I915_WRITE(RING_HWS_PGA(engine->mmio_base),
> - (u32)engine->status_page.gfx_addr);
> + (u32)engine->status_page.gfx_addr);
> POSTING_READ(RING_HWS_PGA(engine->mmio_base));
> +
> + return 0;
> }
>
> /**
> @@ -2688,10 +2700,9 @@ error_deref_obj:
> return ret;
> }
>
> -void intel_lr_context_reset(struct drm_device *dev,
> - struct intel_context *ctx)
> +void intel_lr_context_reset(struct drm_i915_private *dev_priv,
> + struct intel_context *ctx)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_engine_cs *engine;
>
> for_each_engine(engine, dev_priv) {
> @@ -2699,23 +2710,31 @@ void intel_lr_context_reset(struct drm_device *dev,
> ctx->engine[engine->id].state;
> struct intel_ringbuffer *ringbuf =
> ctx->engine[engine->id].ringbuf;
> + void *obj_addr;
> uint32_t *reg_state;
> - struct page *page;
>
> if (!ctx_obj)
> continue;
>
> - if (i915_gem_object_get_pages(ctx_obj)) {
> - WARN(1, "Failed get_pages for context obj\n");
> + obj_addr = i915_gem_object_pin_map(ctx_obj);
> + if (WARN_ON(IS_ERR(obj_addr)))
> continue;
> - }
> - page = i915_gem_object_get_dirty_page(ctx_obj, LRC_STATE_PN);
> - reg_state = kmap_atomic(page);
> +
> + reg_state = obj_addr + LRC_STATE_PN * PAGE_SIZE;
> + ctx_obj->dirty = true;
>
> reg_state[CTX_RING_HEAD+1] = 0;
> reg_state[CTX_RING_TAIL+1] = 0;
>
> - kunmap_atomic(reg_state);
> + i915_gem_object_unpin_map(ctx_obj);
> +
> + /*
> + * Kernel context will pin_map the HWS after reset so we
> + * have to drop the pin count here in order ->pages_pin_count
> + * remains balanced.
> + */
> + if (ctx == dev_priv->kernel_context)
> + i915_gem_object_unpin_map(engine->status_page.obj);
Then we wouldn't have this kludge. That's going to be worth the time and
we can then split out the bug fix for the current code.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
