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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel
> Vetter
> Sent: Monday, August 11, 2014 10:25 PM
> To: Daniel, Thomas
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re:  [PATCH 27/43] drm/i915/bdw: Render state init for
> Execlists
> 
> On Thu, Jul 24, 2014 at 05:04:35PM +0100, Thomas Daniel wrote:
> > From: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h              |    4 +--
> >  drivers/gpu/drm/i915/i915_gem_context.c      |   17 +++++++++-
> >  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             |   46
> +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_lrc.h             |    2 ++
> >  drivers/gpu/drm/i915/intel_renderstate.h     |    8 +----
> >  7 files changed, 139 insertions(+), 25 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 4303e2c..b7cf0ec 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>
> > @@ -623,6 +624,7 @@ struct intel_context {
> >  	} legacy_hw_ctx;
> >
> >  	/* Execlists */
> > +	bool rcs_initialized;
> >  	struct {
> >  		struct drm_i915_gem_object *state;
> >  		struct intel_ringbuffer *ringbuf;
> > @@ -2553,8 +2555,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_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 9085ff1..0dc6992 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -513,8 +513,23 @@ int i915_gem_context_enable(struct
> drm_i915_private *dev_priv)
> >  		ppgtt->enable(ppgtt);
> >  	}
> >
> > -	if (i915.enable_execlists)
> > +	if (i915.enable_execlists) {
> > +		struct intel_context *dctx;
> > +
> > +		ring = &dev_priv->ring[RCS];
> > +		dctx = ring->default_context;
> > +
> > +		if (!dctx->rcs_initialized) {
> > +			ret = intel_lr_context_render_state_init(ring, dctx);
> > +			if (ret) {
> > +				DRM_ERROR("Init render state failed: %d\n",
> ret);
> > +				return ret;
> > +			}
> > +			dctx->rcs_initialized = true;
> > +		}
> > +
> >  		return 0;
> > +	}
> 
> This looks very much like the wrong place. We should init the render state
> when we create the context, or when we switch to it for the first time.
> The later is what the legacy contexts currently do in do_switch.
> 
> But ctx_enable should do the switch to the default context and that's about
Well, a side-effect of switching to the default context in legacy mode is that
the render state gets initialized.  I could move the lr render state init call
into an enable_execlists branch in i915_switch_context() but that doen't
seem like the right place.

How about in i915_gem_init() after calling i915_gem_init_hw()?

> it. If there's some depency then I guess we should stall the creation of the
> default context a bit, maybe.
> 
> In any case someone needs to explain this better and if there's not other
> wey this at least needs a bit comment. So I'll punt for now.
When the default context is created the driver is not ready to execute a
batch.  That is why the render state init can't be done then.

Thomas.

> -Daniel
> 
> >
> >  	/* FIXME: We should make this work, even in reset */
> >  	if (i915_reset_in_progress(&dev_priv->gpu_error))
> > 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 (c) 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 0a04c03..4549eec 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -925,6 +925,37 @@ 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)
> > @@ -1142,6 +1173,21 @@ int intel_lr_context_deferred_create(struct
> intel_context *ctx,
> >  	ctx->engine[ring->id].ringbuf = ringbuf;
> >  	ctx->engine[ring->id].state = ctx_obj;
> >
> > +	/* The default context will have to wait, because we are not yet
> > +	 * ready to send a batchbuffer at this point */
> > +	if (ring->id == RCS && !ctx->rcs_initialized &&
> > +			ctx != ring->default_context) {
> > +		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 696e09e..f20c3d2 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.h
> > +++ b/drivers/gpu/drm/i915/intel_lrc.h
> > @@ -43,6 +43,8 @@ static inline void intel_logical_ring_emit(struct
> > intel_ringbuffer *ringbuf, u32  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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux