[Intel-gfx] [PATCH v3] drm/i915: Use new i915_gem_object_pin_map for LRC

[Chris Wilson]

On Tue, Apr 12, 2016 at 02:05:05PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> 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