Re: [PATCH] drm/i915/bdw: Render state init for Execlists

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Aug 21, 2014 at 11:40:54AM +0100, Thomas Daniel wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> The batchbuffer that sets the render context state is submitted
> in a different way, and from different places.
> 
> We needed to make both the render state preparation and free functions
> outside accesible, and namespace accordingly. This mess is so that all
> LR, LRC and Execlists functionality can go together in intel_lrc.c: we
> can fix all of this later on, once the interfaces are clear.
> 
> v2: Create a separate ctx->rcs_initialized for the Execlists case, as
> suggested by Chris Wilson.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> 
> v3: Setup ring status page in lr_context_deferred_create when the
> default context is being created. This means that the render state
> init for the default context is no longer a special case.  Execute
> deferred creation of the default context at the end of
> logical_ring_init to allow the render state commands to be submitted.
> Fix style errors reported by checkpatch. Rebased.
> 
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>

Queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.h              |    4 +-
>  drivers/gpu/drm/i915/i915_gem_render_state.c |   40 ++++++++------
>  drivers/gpu/drm/i915/i915_gem_render_state.h |   47 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c             |   73 ++++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_lrc.h             |    2 +
>  drivers/gpu/drm/i915/intel_renderstate.h     |    8 +--
>  6 files changed, 135 insertions(+), 39 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/i915_gem_render_state.h
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e449f81..f416e341 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -37,6 +37,7 @@
>  #include "intel_ringbuffer.h"
>  #include "intel_lrc.h"
>  #include "i915_gem_gtt.h"
> +#include "i915_gem_render_state.h"
>  #include <linux/io-mapping.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-algo-bit.h>
> @@ -635,6 +636,7 @@ struct intel_context {
>  	} legacy_hw_ctx;
>  
>  	/* Execlists */
> +	bool rcs_initialized;
>  	struct {
>  		struct drm_i915_gem_object *state;
>  		struct intel_ringbuffer *ringbuf;
> @@ -2596,8 +2598,6 @@ 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);
>  
> -/* i915_gem_render_state.c */
> -int i915_gem_render_state_init(struct intel_engine_cs *ring);
>  /* 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_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index e60be3f..a9a62d7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -28,13 +28,6 @@
>  #include "i915_drv.h"
>  #include "intel_renderstate.h"
>  
> -struct render_state {
> -	const struct intel_renderstate_rodata *rodata;
> -	struct drm_i915_gem_object *obj;
> -	u64 ggtt_offset;
> -	int gen;
> -};
> -
>  static const struct intel_renderstate_rodata *
>  render_state_get_rodata(struct drm_device *dev, const int gen)
>  {
> @@ -127,30 +120,47 @@ static int render_state_setup(struct render_state *so)
>  	return 0;
>  }
>  
> -static void render_state_fini(struct render_state *so)
> +void i915_gem_render_state_fini(struct render_state *so)
>  {
>  	i915_gem_object_ggtt_unpin(so->obj);
>  	drm_gem_object_unreference(&so->obj->base);
>  }
>  
> -int i915_gem_render_state_init(struct intel_engine_cs *ring)
> +int i915_gem_render_state_prepare(struct intel_engine_cs *ring,
> +				  struct render_state *so)
>  {
> -	struct render_state so;
>  	int ret;
>  
>  	if (WARN_ON(ring->id != RCS))
>  		return -ENOENT;
>  
> -	ret = render_state_init(&so, ring->dev);
> +	ret = render_state_init(so, ring->dev);
>  	if (ret)
>  		return ret;
>  
> -	if (so.rodata == NULL)
> +	if (so->rodata == NULL)
>  		return 0;
>  
> -	ret = render_state_setup(&so);
> +	ret = render_state_setup(so);
> +	if (ret) {
> +		i915_gem_render_state_fini(so);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int i915_gem_render_state_init(struct intel_engine_cs *ring)
> +{
> +	struct render_state so;
> +	int ret;
> +
> +	ret = i915_gem_render_state_prepare(ring, &so);
>  	if (ret)
> -		goto out;
> +		return ret;
> +
> +	if (so.rodata == NULL)
> +		return 0;
>  
>  	ret = ring->dispatch_execbuffer(ring,
>  					so.ggtt_offset,
> @@ -164,6 +174,6 @@ int i915_gem_render_state_init(struct intel_engine_cs *ring)
>  	ret = __i915_add_request(ring, NULL, so.obj, NULL);
>  	/* __i915_add_request moves object to inactive if it fails */
>  out:
> -	render_state_fini(&so);
> +	i915_gem_render_state_fini(&so);
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.h b/drivers/gpu/drm/i915/i915_gem_render_state.h
> new file mode 100644
> index 0000000..c44961e
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.h
> @@ -0,0 +1,47 @@
> +/*
> + * Copyright © 2014 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef _I915_GEM_RENDER_STATE_H_
> +#define _I915_GEM_RENDER_STATE_H_
> +
> +#include <linux/types.h>
> +
> +struct intel_renderstate_rodata {
> +	const u32 *reloc;
> +	const u32 *batch;
> +	const u32 batch_items;
> +};
> +
> +struct render_state {
> +	const struct intel_renderstate_rodata *rodata;
> +	struct drm_i915_gem_object *obj;
> +	u64 ggtt_offset;
> +	int gen;
> +};
> +
> +int i915_gem_render_state_init(struct intel_engine_cs *ring);
> +void i915_gem_render_state_fini(struct render_state *so);
> +int i915_gem_render_state_prepare(struct intel_engine_cs *ring,
> +				  struct render_state *so);
> +
> +#endif /* _I915_GEM_RENDER_STATE_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index c096b9b..8e51fd0 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1217,8 +1217,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
>  static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
>  {
>  	int ret;
> -	struct intel_context *dctx = ring->default_context;
> -	struct drm_i915_gem_object *dctx_obj;
>  
>  	/* Intentionally left blank. */
>  	ring->buffer = NULL;
> @@ -1232,18 +1230,6 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
>  	spin_lock_init(&ring->execlist_lock);
>  	ring->next_context_status_buffer = 0;
>  
> -	ret = intel_lr_context_deferred_create(dctx, ring);
> -	if (ret)
> -		return ret;
> -
> -	/* The status page is offset 0 from the context object in LRCs. */
> -	dctx_obj = dctx->engine[ring->id].state;
> -	ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(dctx_obj);
> -	ring->status_page.page_addr = kmap(sg_page(dctx_obj->pages->sgl));
> -	if (ring->status_page.page_addr == NULL)
> -		return -ENOMEM;
> -	ring->status_page.obj = dctx_obj;
> -
>  	ret = i915_cmd_parser_init_ring(ring);
>  	if (ret)
>  		return ret;
> @@ -1254,7 +1240,9 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
>  			return ret;
>  	}
>  
> -	return 0;
> +	ret = intel_lr_context_deferred_create(ring->default_context, ring);
> +
> +	return ret;
>  }
>  
>  static int logical_render_ring_init(struct drm_device *dev)
> @@ -1448,6 +1436,38 @@ cleanup_render_ring:
>  	return ret;
>  }
>  
> +int intel_lr_context_render_state_init(struct intel_engine_cs *ring,
> +				       struct intel_context *ctx)
> +{
> +	struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf;
> +	struct render_state so;
> +	struct drm_i915_file_private *file_priv = ctx->file_priv;
> +	struct drm_file *file = file_priv ? file_priv->file : NULL;
> +	int ret;
> +
> +	ret = i915_gem_render_state_prepare(ring, &so);
> +	if (ret)
> +		return ret;
> +
> +	if (so.rodata == NULL)
> +		return 0;
> +
> +	ret = ring->emit_bb_start(ringbuf,
> +			so.ggtt_offset,
> +			I915_DISPATCH_SECURE);
> +	if (ret)
> +		goto out;
> +
> +	i915_vma_move_to_active(i915_gem_obj_to_ggtt(so.obj), ring);
> +
> +	ret = __i915_add_request(ring, file, so.obj, NULL);
> +	/* intel_logical_ring_add_request moves object to inactive if it
> +	 * fails */
> +out:
> +	i915_gem_render_state_fini(&so);
> +	return ret;
> +}
> +
>  static int
>  populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj,
>  		    struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf)
> @@ -1687,6 +1707,29 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
>  	ctx->engine[ring->id].ringbuf = ringbuf;
>  	ctx->engine[ring->id].state = ctx_obj;
>  
> +	if (ctx == ring->default_context) {
> +		/* The status page is offset 0 from the default context object
> +		 * in LRC mode. */
> +		ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(ctx_obj);
> +		ring->status_page.page_addr =
> +				kmap(sg_page(ctx_obj->pages->sgl));
> +		if (ring->status_page.page_addr == NULL)
> +			return -ENOMEM;
> +		ring->status_page.obj = ctx_obj;
> +	}
> +
> +	if (ring->id == RCS && !ctx->rcs_initialized) {
> +		ret = intel_lr_context_render_state_init(ring, ctx);
> +		if (ret) {
> +			DRM_ERROR("Init render state failed: %d\n", ret);
> +			ctx->engine[ring->id].ringbuf = NULL;
> +			ctx->engine[ring->id].state = NULL;
> +			intel_destroy_ringbuffer_obj(ringbuf);
> +			goto error;
> +		}
> +		ctx->rcs_initialized = true;
> +	}
> +
>  	return 0;
>  
>  error:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 991d449..33c3b4b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -62,6 +62,8 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf,
>  int intel_logical_ring_begin(struct intel_ringbuffer *ringbuf, int num_dwords);
>  
>  /* Logical Ring Contexts */
> +int intel_lr_context_render_state_init(struct intel_engine_cs *ring,
> +				       struct intel_context *ctx);
>  void intel_lr_context_free(struct intel_context *ctx);
>  int intel_lr_context_deferred_create(struct intel_context *ctx,
>  				     struct intel_engine_cs *ring);
> diff --git a/drivers/gpu/drm/i915/intel_renderstate.h b/drivers/gpu/drm/i915/intel_renderstate.h
> index fd4f662..6c792d3 100644
> --- a/drivers/gpu/drm/i915/intel_renderstate.h
> +++ b/drivers/gpu/drm/i915/intel_renderstate.h
> @@ -24,13 +24,7 @@
>  #ifndef _INTEL_RENDERSTATE_H
>  #define _INTEL_RENDERSTATE_H
>  
> -#include <linux/types.h>
> -
> -struct intel_renderstate_rodata {
> -	const u32 *reloc;
> -	const u32 *batch;
> -	const u32 batch_items;
> -};
> +#include "i915_drv.h"
>  
>  extern const struct intel_renderstate_rodata gen6_null_state;
>  extern const struct intel_renderstate_rodata gen7_null_state;
> -- 
> 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