> -----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
