Re: [PATCH] drm/i915: Initialize workarounds in logical ring mode too

[Michel Thierry <michel.thierry@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6374 bytes --]

On 11/10/2014 4:38 PM, Mika Kuoppala wrote:
> Arun Siluvery <arun.siluvery@linux.intel.com> writes:
>
>> From: Michel Thierry <michel.thierry@intel.com>
>>
>> Following the legacy ring submission example, update the
>> ring->init_context() hook to support the execlist submission mode.
>>
>> v2: update to use the new workaround macros and cleanup unused code.
>> This takes care of both bdw and chv workarounds.
>>
>> v2.1: Add missing call to init_context() during deferred context creation.
>>
>> Issue: VIZ-4092
>> Issue: GMIN-3475
>> Change-Id: Ie3d093b2542ab0e2a44b90460533e2f979788d6c
>> Cc: Deepak S <deepak.s@intel.com>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>>   drivers/gpu/drm/i915/intel_lrc.c        | 46 ++++++++++++++++++++++++++++++++-
>>   drivers/gpu/drm/i915/intel_ringbuffer.c |  5 ++--
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  4 ++-
>>   4 files changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>> index a5221d8..a37668f 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -629,7 +629,7 @@ done:
>>   
>>   	if (uninitialized) {
>>   		if (ring->init_context) {
>> -			ret = ring->init_context(ring);
>> +			ret = ring->init_context(ring->buffer);
>>   			if (ret)
>>   				DRM_ERROR("ring init context: %d\n", ret);
>>   		}
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index cd74e5c..f3efdbd 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -991,6 +991,43 @@ int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, int num_dwords)
>>   	return 0;
>>   }
>>   
>> +static int intel_logical_ring_workarounds_emit(struct intel_ringbuffer *ringbuf)
>> +{
>> +	int ret, i;
>> +	struct intel_engine_cs *ring = ringbuf->ring;
>> +	struct drm_device *dev = ring->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct i915_workarounds *w = &dev_priv->workarounds;
>> +
>> +	if (WARN_ON(w->count == 0))
>> +		return 0;
>> +
>> +	ring->gpu_caches_dirty = true;
>> +	ret = logical_ring_flush_all_caches(ringbuf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = intel_logical_ring_begin(ringbuf, w->count * 2 + 2);
>> +	if (ret)
>> +		return ret;
>> +
>> +	intel_logical_ring_emit(ringbuf, MI_LOAD_REGISTER_IMM(w->count));
>> +	for (i = 0; i < w->count; i++) {
>> +		intel_logical_ring_emit(ringbuf, w->reg[i].addr);
>> +		intel_logical_ring_emit(ringbuf, w->reg[i].value);
>> +	}
>> +	intel_logical_ring_emit(ringbuf, MI_NOOP);
>> +
>> +	intel_logical_ring_advance(ringbuf);
>> +
>> +	ring->gpu_caches_dirty = true;
>> +	ret = logical_ring_flush_all_caches(ringbuf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>>   static int gen8_init_common_ring(struct intel_engine_cs *ring)
>>   {
>>   	struct drm_device *dev = ring->dev;
>> @@ -1034,7 +1071,7 @@ static int gen8_init_render_ring(struct intel_engine_cs *ring)
>>   
>>   	I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
>>   
>> -	return ret;
>> +	return init_workarounds_ring(ring);
>>   }
>>   
>>   static int gen8_emit_bb_start(struct intel_ringbuffer *ringbuf,
>> @@ -1282,6 +1319,7 @@ static int logical_render_ring_init(struct drm_device *dev)
>>   		ring->irq_keep_mask |= GT_RENDER_L3_PARITY_ERROR_INTERRUPT;
>>   
>>   	ring->init = gen8_init_render_ring;
>> +	ring->init_context = intel_logical_ring_workarounds_emit;
>>   	ring->cleanup = intel_fini_pipe_control;
>>   	ring->get_seqno = gen8_get_seqno;
>>   	ring->set_seqno = gen8_set_seqno;
>> @@ -1745,6 +1783,12 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
>>   	}
>>   
>>   	if (ring->id == RCS && !ctx->rcs_initialized) {
>> +		if (ring->init_context) {
>> +			ret = ring->init_context(ringbuf);
>> +			if (ret)
>> +				DRM_ERROR("ring init context: %d\n", ret);
>> +		}
>> +
>>   		ret = intel_lr_context_render_state_init(ring, ctx);
>>   		if (ret) {
>>   			DRM_ERROR("Init render state failed: %d\n", ret);
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 368b20a..70442fa 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -665,9 +665,10 @@ err:
>>   	return ret;
>>   }
>>   
>> -static int intel_ring_workarounds_emit(struct intel_engine_cs *ring)
>> +static int intel_ring_workarounds_emit(struct intel_ringbuffer *ringbuf)
>>   {
>>   	int ret, i;
>> +	struct intel_engine_cs *ring = ringbuf->ring;
>>   	struct drm_device *dev = ring->dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	struct i915_workarounds *w = &dev_priv->workarounds;
>> @@ -806,7 +807,7 @@ static int chv_init_workarounds(struct intel_engine_cs *ring)
>>   	return 0;
>>   }
>>   
>> -static int init_workarounds_ring(struct intel_engine_cs *ring)
>> +int init_workarounds_ring(struct intel_engine_cs *ring)
>>   {
>>   	struct drm_device *dev = ring->dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 96479c8..a69ef8b 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -148,7 +148,7 @@ struct  intel_engine_cs {
>>   
>>   	int		(*init)(struct intel_engine_cs *ring);
>>   
>> -	int		(*init_context)(struct intel_engine_cs *ring);
>> +	int		(*init_context)(struct intel_ringbuffer *ringbuf);
>                                          
> What is the rationale of this? This seems odd.
>
> Why not:
>
> int             (*init_context)(struct intel_engine_cs *ring,
>                                         struct intel_context *ctx);
>
> -Mika
>   

We want to have the emit function for both legacy and execlist 
submission modes (intel_ring_workarounds_emit & 
intel_logical_ring_workarounds_emit).
It's easy to get the ring from a ringbuf, but not the other way around.

-Michel


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx