[Intel-gfx] [PATCH 27/43] drm/i915/bdw: Render state init for Execlists
Daniel, Thomas
thomas.daniel at intel.com
Wed Aug 13 17:07:29 CEST 2014
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, August 11, 2014 10:25 PM
> To: Daniel, Thomas
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [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 at 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 at intel.com>
> > ---
> > 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 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
More information about the Intel-gfx
mailing list