Re: [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 4/4] drm/i915/execlists: Use coherent writes into the context image
Date: Fri, 14 Sep 2018 16:03:35 +0300	[thread overview]
Message-ID: <20180914130335.GB5565@intel.com> (raw)
In-Reply-To: <20180914094215.23649-4-chris@chris-wilson.co.uk>

On Fri, Sep 14, 2018 at 10:42:15AM +0100, Chris Wilson wrote:
> That we use a WB mapping for updating the RING_TAIL register inside the
> context image even on !llc machines has been a source of consternation
> for every reader. It appears to work on bsw+, but it may just have been
> that we have been incredibly bad at detecting the errors.

Presumably it's due to the "all ggtt accesses go through pat[0]" and
we make pat[0] snoop. So presumably the hw should snoop when loading
the context... maybe.

> 
> v2: With extra enthusiasm.
> v3: Drop force of map type for pinned default_state as by the time we
> pin it, the map type is always WB and doesn't conflict with the earlier
> use by ce->state.
> v4: Transfer engine->default_state from MAP_WC to MAP_WB on creation so
> we do not need the MAP_FORCE littered around the backends
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 6 ++++++
>  drivers/gpu/drm/i915/i915_gem.c  | 4 +++-
>  drivers/gpu/drm/i915/i915_perf.c | 3 ++-
>  drivers/gpu/drm/i915/intel_lrc.c | 6 ++++--
>  4 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bb43e56df197..7d4daa7412f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3097,6 +3097,12 @@ enum i915_map_type {
>  	I915_MAP_FORCE_WC = I915_MAP_WC | I915_MAP_OVERRIDE,
>  };
>  
> +static inline enum i915_map_type
> +i915_coherent_map_type(struct drm_i915_private *i915)
> +{
> +	return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
> +}
> +
>  /**
>   * i915_gem_object_pin_map - return a contiguous mapping of the entire object
>   * @obj: the object to map into kernel address space
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 37353afec66e..d9465bd1a00a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5426,6 +5426,8 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>  		struct i915_vma *state;
>  		void *vaddr;
>  
> +		GEM_BUG_ON(to_intel_context(ctx, engine)->pin_count);
> +
>  		state = to_intel_context(ctx, engine)->state;
>  		if (!state)
>  			continue;
> @@ -5450,7 +5452,7 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>  
>  		/* Check we can acquire the image of the context state */
>  		vaddr = i915_gem_object_pin_map(engine->default_state,
> -						I915_MAP_WB);
> +						I915_MAP_FORCE_WB);
>  		if (IS_ERR(vaddr)) {
>  			err = PTR_ERR(vaddr);
>  			goto err_active;
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 3d7a052b4cca..664b96bb65a3 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1707,6 +1707,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
>  				       const struct i915_oa_config *oa_config)
>  {
>  	struct intel_engine_cs *engine = dev_priv->engine[RCS];
> +	unsigned int map_type = i915_coherent_map_type(dev_priv);
>  	struct i915_gem_context *ctx;
>  	struct i915_request *rq;
>  	int ret;
> @@ -1741,7 +1742,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
>  		if (!ce->state)
>  			continue;
>  
> -		regs = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
> +		regs = i915_gem_object_pin_map(ce->state->obj, map_type);
>  		if (IS_ERR(regs))
>  			return PTR_ERR(regs);
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d7fcbba8e982..a51be16ddaac 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1294,7 +1294,7 @@ static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
>  	 * on an active context (which by nature is already on the GPU).
>  	 */
>  	if (!(vma->flags & I915_VMA_GLOBAL_BIND)) {
> -		err = i915_gem_object_set_to_gtt_domain(vma->obj, true);
> +		err = i915_gem_object_set_to_wc_domain(vma->obj, true);
>  		if (err)
>  			return err;
>  	}
> @@ -1322,7 +1322,9 @@ __execlists_context_pin(struct intel_engine_cs *engine,
>  	if (ret)
>  		goto err;
>  
> -	vaddr = i915_gem_object_pin_map(ce->state->obj, I915_MAP_WB);
> +	vaddr = i915_gem_object_pin_map(ce->state->obj,
> +					i915_coherent_map_type(ctx->i915) |
> +					I915_MAP_OVERRIDE);
>  	if (IS_ERR(vaddr)) {
>  		ret = PTR_ERR(vaddr);
>  		goto unpin_vma;
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx