[Intel-gfx] [RFC PATCH 3/4] drm/i915: add api to pin/unpin context state

[Daniel Vetter]

On Mon, Nov 10, 2014 at 02:56:50PM +0000, Robert Bragg wrote:
> This adds i915_gem_context_pin/unpin_state functions so that code
> outside i915_gem_context.c can pin/unpin a context without duplicating
> knowledge about the alignment constraints.
> 
> Signed-off-by: Robert Bragg <robert at sixbynine.org>

At first I've thought this is ok to get going, but then I pondered a bit
more. We unpin ctx objects from the shrinker, and this here torpedos that
a bit. And I think the proper implementation isn't more fuzz that this
here:

When pinning the context we call an OA function to update OA state. When
OA notices that this is the buffer it cares about (for filtered mode) and
that it has moved, it updates OA hw/sw state. I don't think this can race
since a ctx can only be moved when it's not actively used by the gpu, so I
think we should be able to do this. Also unbinding ctxs is a fairly
last-resort kind of thing, so even when this requires a stall (maybe OA
needs to be stopped/restarted) it should still be ok.

Or do I miss something and this is too much fuzz?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  4 ++++
>  drivers/gpu/drm/i915/i915_gem_context.c | 30 +++++++++++++++++++++---------
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3212d62..acaf76c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2675,6 +2675,10 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  				   struct drm_file *file);
>  
> +int i915_gem_context_pin_state(struct drm_device *dev,
> +			       struct intel_context *ctx);
> +void i915_gem_context_unpin_state(struct intel_context *ctx);
> +
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct drm_device *dev,
>  					  struct i915_address_space *vm,
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a5221d8..feb1a23 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -132,6 +132,20 @@ static int get_context_size(struct drm_device *dev)
>  	return ret;
>  }
>  
> +int i915_gem_context_pin_state(struct drm_device *dev,
> +			       struct intel_context *ctx)
> +{
> +	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> +
> +	return i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state,
> +				     get_context_alignment(dev), 0);
> +}
> +
> +void i915_gem_context_unpin_state(struct intel_context *ctx)
> +{
> +	i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
> +}
> +
>  void i915_gem_context_free(struct kref *ctx_ref)
>  {
>  	struct intel_context *ctx = container_of(ctx_ref,
> @@ -253,8 +267,7 @@ i915_gem_create_context(struct drm_device *dev,
>  		 * be available. To avoid this we always pin the default
>  		 * context.
>  		 */
> -		ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state,
> -					    get_context_alignment(dev), 0);
> +		ret = i915_gem_context_pin_state(dev, ctx);
>  		if (ret) {
>  			DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret);
>  			goto err_destroy;
> @@ -278,7 +291,7 @@ i915_gem_create_context(struct drm_device *dev,
>  
>  err_unpin:
>  	if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state)
> -		i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state);
> +		i915_gem_context_unpin_state(ctx);
>  err_destroy:
>  	i915_gem_context_unreference(ctx);
>  	return ERR_PTR(ret);
> @@ -375,12 +388,12 @@ void i915_gem_context_fini(struct drm_device *dev)
>  		if (dev_priv->ring[RCS].last_context == dctx) {
>  			/* Fake switch to NULL context */
>  			WARN_ON(dctx->legacy_hw_ctx.rcs_state->active);
> -			i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> +			i915_gem_context_unpin_state(dctx);
>  			i915_gem_context_unreference(dctx);
>  			dev_priv->ring[RCS].last_context = NULL;
>  		}
>  
> -		i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
> +		i915_gem_context_unpin_state(dctx);
>  	}
>  
>  	for (i = 0; i < I915_NUM_RINGS; i++) {
> @@ -534,8 +547,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  
>  	/* Trying to pin first makes error handling easier. */
>  	if (ring == &dev_priv->ring[RCS]) {
> -		ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state,
> -					    get_context_alignment(ring->dev), 0);
> +		ret = i915_gem_context_pin_state(ring->dev, to);
>  		if (ret)
>  			return ret;
>  	}
> @@ -616,7 +628,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  		BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring);
>  
>  		/* obj is kept alive until the next request by its active ref */
> -		i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
> +		i915_gem_context_unpin_state(from);
>  		i915_gem_context_unreference(from);
>  	}
>  
> @@ -643,7 +655,7 @@ done:
>  
>  unpin_out:
>  	if (ring->id == RCS)
> -		i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state);
> +		i915_gem_context_unpin_state(to);
>  	return ret;
>  }
>  
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at 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