[Intel-gfx] [PATCH] drm/i915: Split alloc from init for lrc

[Chris Wilson]

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 at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Chris Wilson <chris at 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.

> 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

-- 
Chris Wilson, Intel Open Source Technology Centre