Re: [PATCH] drm/i915: Use GGTT batchbuffer selector

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 10, 2014 at 10:26:54AM +0100, Chris Wilson wrote:
> gen6 and earlier conflate address space selection (ppgtt vs ggtt) with
> the security bit (i.e. only privileged batches were allowed to run from
> ggtt). From Haswell onwards, 

Not onwards unfortunately. BDW went back to the single bit approach.

> you are able to select the security bit
> separate from the address space - and we always requested to use ppgtt.
> This breaks the golden render state batch execution as that is only
> present in the global GTT and so we need to disable the use of the ppgtt
> selector bit, or else we hang immediately upon boot and thence after
> every GPU reset...
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c   | 2 +-
>  drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +-
>  drivers/gpu/drm/i915/intel_ringbuffer.c      | 5 +++--
>  drivers/gpu/drm/i915/intel_ringbuffer.h      | 5 +++--
>  4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 1a0611bb576b..8ff448dc8be4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1260,7 +1260,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		if (!file->is_master || !capable(CAP_SYS_ADMIN))
>  		    return -EPERM;
>  
> -		flags |= I915_DISPATCH_SECURE;
> +		flags |= I915_DISPATCH_SECURE | I915_DISPATCH_GGTT;
>  	}
>  	if (args->flags & I915_EXEC_IS_PINNED)
>  		flags |= I915_DISPATCH_PINNED;
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index a9a62d75aa57..a158d610720b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -165,7 +165,7 @@ int i915_gem_render_state_init(struct intel_engine_cs *ring)
>  	ret = ring->dispatch_execbuffer(ring,
>  					so.ggtt_offset,
>  					so.rodata->batch_items * 4,
> -					I915_DISPATCH_SECURE);
> +					I915_DISPATCH_GGTT);
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 109de2eeb9a8..d053819407da 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2174,7 +2174,7 @@ gen8_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
>  			      u64 offset, u32 len,
>  			      unsigned flags)
>  {
> -	bool ppgtt = USES_PPGTT(ring->dev) && !(flags & I915_DISPATCH_SECURE);
> +	bool ppgtt = USES_PPGTT(ring->dev) && !(flags & I915_DISPATCH_GGTT);
>  	int ret;
>  
>  	ret = intel_ring_begin(ring, 4);
> @@ -2203,7 +2203,8 @@ hsw_ring_dispatch_execbuffer(struct intel_engine_cs *ring,
>  		return ret;
>  
>  	intel_ring_emit(ring,
> -			MI_BATCH_BUFFER_START | MI_BATCH_PPGTT_HSW |
> +			MI_BATCH_BUFFER_START |
> +			(flags & I915_DISPATCH_GGTT ? 0 : MI_BATCH_PPGTT_HSW) |
>  			(flags & I915_DISPATCH_SECURE ? 0 : MI_BATCH_NON_SECURE_HSW));

Do we actually want to make a distinction between GGTT and secure given
that HSW is the only one where it makes any difference? Why not just
set both GGGT and secure bits on HSW when I915_DISPATCH_SECURE is set?

>  	/* bit0-7 is the length on GEN6+ */
>  	intel_ring_emit(ring, offset);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 96479c89f4bd..755585a6fcc7 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -169,8 +169,9 @@ struct  intel_engine_cs {
>  	int		(*dispatch_execbuffer)(struct intel_engine_cs *ring,
>  					       u64 offset, u32 length,
>  					       unsigned flags);
> -#define I915_DISPATCH_SECURE 0x1
> -#define I915_DISPATCH_PINNED 0x2
> +#define I915_DISPATCH_GGTT	0x1
> +#define I915_DISPATCH_SECURE	0x2
> +#define I915_DISPATCH_PINNED	0x4
>  	void		(*cleanup)(struct intel_engine_cs *ring);
>  
>  	/* GEN8 signal/wait table - never trust comments!
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC