Re: [PATCH] drm/i915: Split alloc from init for lrc

[Nick Hoath <nicholas.hoath@intel.com>]

On 19/08/2015 13:37, Chris Wilson wrote:
> On Wed, Aug 19, 2015 at 01:24:28PM +0100, Nick Hoath wrote:
>> Extend init/init_hw split to context init.
>>     - Move context initialisation in to i915_gem_init_hw
>>     - Move one off initialisation for render ring to
>>          i915_gem_validate_context
>>     - Move default context initialisation to logical_ring_init
>>
>> Rename intel_lr_context_deferred_create to
>> intel_lr_context_deferred_alloc, to reflect reduced functionality &
>> alloc/init split.
>>
>> This patch is intended to split out the allocation of resources & initialisation
>> to allow easier reuse of code for resume/gpu reset.
>>
>> v2: Removed function ptr wrapping of do_switch_context (Daniel Vetter)
>>      Left ->init_context int intel_lr_context_deferred_alloc (Daniel Vetter)
>>      Remove unnecessary init flag & ring type test. (Daniel Vetter)
>>      Improve commit message (Daniel Vetter)
>> v3: On init/reinit, set the hw next sequence number to the sw next sequence
>>      number. This is set to 1 at driver load time. This prevents the seqno
>>      being reset on reinit (Chris Wilson)
>>
>> Issue: VIZ-4798
>> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h            |   1 -
>>   drivers/gpu/drm/i915/i915_gem.c            |  18 ++--
>>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |   3 +-
>>   drivers/gpu/drm/i915/intel_lrc.c           | 147 ++++++++++++++---------------
>>   drivers/gpu/drm/i915/intel_lrc.h           |   4 +-
>>   5 files changed, 86 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index f7fd519..844ccf0 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -880,7 +880,6 @@ struct intel_context {
>>   	} legacy_hw_ctx;
>>
>>   	/* Execlists */
>> -	bool rcs_initialized;
>>   	struct {
>>   		struct drm_i915_gem_object *state;
>>   		struct intel_ringbuffer *ringbuf;
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 73293b4..eb7c1f2 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4603,14 +4603,8 @@ int i915_gem_init_rings(struct drm_device *dev)
>>   			goto cleanup_vebox_ring;
>>   	}
>>
>> -	ret = i915_gem_set_seqno(dev, ((u32)~0 - 0x1000));
>> -	if (ret)
>> -		goto cleanup_bsd2_ring;
>> -
>>   	return 0;
>>
>> -cleanup_bsd2_ring:
>> -	intel_cleanup_ring_buffer(&dev_priv->ring[VCS2]);
>>   cleanup_vebox_ring:
>>   	intel_cleanup_ring_buffer(&dev_priv->ring[VECS]);
>>   cleanup_blt_ring:
>> @@ -4629,6 +4623,7 @@ i915_gem_init_hw(struct drm_device *dev)
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	struct intel_engine_cs *ring;
>>   	int ret, i, j;
>> +	struct drm_i915_gem_request *req;
>>
>>   	if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
>>   		return -EIO;
>> @@ -4680,9 +4675,12 @@ i915_gem_init_hw(struct drm_device *dev)
>>   			goto out;
>>   	}
>>
>> +	ret = i915_gem_set_seqno(dev, dev_priv->next_seqno);
>> +	if (ret)
>> +		goto out;
>
> The only reason to do this would be to ensure that the contents of the
> registers are valid (assuming we take over from ourselves). The right
> value to use then is last_seqno.
i915_gem_set_seqno uses the following code:
	ret = i915_gem_init_seqno(dev, seqno - 1);
......
	dev_priv->next_seqno = seqno;
	dev_priv->last_seqno = seqno - 1;

So using last_seqno would rewind the seqno by one...
>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index 923a3c4..95f1a0d 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -994,6 +994,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>>   {
>>   	struct intel_context *ctx = NULL;
>>   	struct i915_ctx_hang_stats *hs;
>> +	int ret;
>>
>>   	if (ring->id != RCS && ctx_id != DEFAULT_CONTEXT_HANDLE)
>>   		return ERR_PTR(-EINVAL);
>> @@ -1009,7 +1010,7 @@ i915_gem_validate_context(struct drm_device *dev, struct drm_file *file,
>>   	}
>>
>>   	if (i915.enable_execlists && !ctx->engine[ring->id].state) {
>> -		int ret = intel_lr_context_deferred_create(ctx, ring);
>> +		ret = intel_lr_context_deferred_alloc(ctx, ring);
>>   		if (ret) {
>>   			DRM_DEBUG("Could not create LRC %u: %d\n", ctx_id, ret);
>>   			return ERR_PTR(ret);
>
> Still modifying this for no reason, and you still haven't realised this
> call is redundant (hint there is already a hook in alloc_request).
>
>  From last year:
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?id=37fbd370152211688bc5bce3d28d13233cfe7d8b
>
> More recent (i.e a couple of months ago):
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=ba4950a8f489d54ec4898f94dad44f9ec13301d2
> -Chris
>

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