Re: [PATCH 04/43] drm/i915/bdw: Initialization for Logical Ring Contexts

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 24, 2014 at 05:04:12PM +0100, Thomas Daniel wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> For the moment this is just a placeholder, but it shows one of the
> main differences between the good ol' HW contexts and the shiny
> new Logical Ring Contexts: LR contexts allocate  and free their
> own backing objects. Another difference is that the allocation is
> deferred (as the create function name suggests), but that does not
> happen in this patch yet, because for the moment we are only dealing
> with the default context.
> 
> Early in the series we had our own gen8_gem_context_init/fini
> functions, but the truth is they now look almost the same as the
> legacy hw context init/fini functions. We can always split them
> later if this ceases to be the case.
> 
> Also, we do not fall back to legacy ringbuffers when logical ring
> context initialization fails (not very likely to happen and, even
> if it does, hw contexts would probably fail as well).
> 
> v2: Daniel says "explain, do not showcase".
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c |   29 +++++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_lrc.c        |   15 +++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.h        |    5 +++++
>  3 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index de72a28..718150e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -182,7 +182,10 @@ void i915_gem_context_free(struct kref *ctx_ref)
>  						   typeof(*ctx), ref);
>  	struct i915_hw_ppgtt *ppgtt = NULL;
>  
> -	if (ctx->legacy_hw_ctx.rcs_state) {
> +	if (i915.enable_execlists) {
> +		ppgtt = ctx_to_ppgtt(ctx);
> +		intel_lr_context_free(ctx);
> +	} else if (ctx->legacy_hw_ctx.rcs_state) {
>  		/* We refcount even the aliasing PPGTT to keep the code symmetric */
>  		if (USES_PPGTT(ctx->legacy_hw_ctx.rcs_state->base.dev))
>  			ppgtt = ctx_to_ppgtt(ctx);
> @@ -419,7 +422,11 @@ int i915_gem_context_init(struct drm_device *dev)
>  	if (WARN_ON(dev_priv->ring[RCS].default_context))
>  		return 0;
>  
> -	if (HAS_HW_CONTEXTS(dev)) {
> +	if (i915.enable_execlists) {
> +		/* NB: intentionally left blank. We will allocate our own
> +		 * backing objects as we need them, thank you very much */
> +		dev_priv->hw_context_size = 0;
> +	} else if (HAS_HW_CONTEXTS(dev)) {
>  		dev_priv->hw_context_size = round_up(get_context_size(dev), 4096);
>  		if (dev_priv->hw_context_size > (1<<20)) {
>  			DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
> @@ -435,11 +442,20 @@ int i915_gem_context_init(struct drm_device *dev)
>  		return PTR_ERR(ctx);
>  	}
>  
> -	/* NB: RCS will hold a ref for all rings */
> -	for (i = 0; i < I915_NUM_RINGS; i++)
> -		dev_priv->ring[i].default_context = ctx;
> +	for (i = 0; i < I915_NUM_RINGS; i++) {
> +		struct intel_engine_cs *ring = &dev_priv->ring[i];
> +
> +		/* NB: RCS will hold a ref for all rings */
> +		ring->default_context = ctx;
> +
> +		/* FIXME: we really only want to do this for initialized rings */
> +		if (i915.enable_execlists)
> +			intel_lr_context_deferred_create(ctx, ring);
> +	}
>  
> -	DRM_DEBUG_DRIVER("%s context support initialized\n", dev_priv->hw_context_size ? "HW" : "fake");
> +	DRM_DEBUG_DRIVER("%s context support initialized\n",
> +			i915.enable_execlists ? "LR" :
> +			dev_priv->hw_context_size ? "HW" : "fake");
>  	return 0;
>  }
>  
> @@ -781,6 +797,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  	struct intel_context *ctx;
>  	int ret;
>  
> +	/* FIXME: allow user-created LR contexts as well */
>  	if (!hw_context_enabled(dev))
>  		return -ENODEV;
>  
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 21f7f1c..8cc6b55 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -51,3 +51,18 @@ int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists
>  
>  	return 0;
>  }
> +
> +void intel_lr_context_free(struct intel_context *ctx)
> +{
> +	/* TODO */
> +}
> +
> +int intel_lr_context_deferred_create(struct intel_context *ctx,
> +				     struct intel_engine_cs *ring)
> +{
> +	BUG_ON(ctx->legacy_hw_ctx.rcs_state != NULL);

I command thy:

Thou shalt not use BUG_ON except in especially dire circumstances!

Fixed while applying.
-Daniel

> +
> +	/* TODO */
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 75ee9c3..3b93572 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -24,6 +24,11 @@
>  #ifndef _INTEL_LRC_H_
>  #define _INTEL_LRC_H_
>  
> +/* Logical Ring Contexts */
> +void intel_lr_context_free(struct intel_context *ctx);
> +int intel_lr_context_deferred_create(struct intel_context *ctx,
> +				     struct intel_engine_cs *ring);
> +
>  /* Execlists */
>  int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists);
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch