On Tue, 14 Feb 2012 22:09:13 +0100, Ben Widawsky <ben at bwidawsk.net> wrote:
> This is the HW dependent context switch code.
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> drivers/gpu/drm/i915/intel_ringbuffer.c | 117 +++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_ringbuffer.h | 6 ++-
> 3 files changed, 125 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 34e6f4f..4175929 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -965,6 +965,9 @@ struct drm_i915_gem_context {
> bool is_initialized;
> };
>
> +#define I915_CONTEXT_NORMAL_SWITCH (1 << 0)
> +#define I915_CONTEXT_SAVE_ONLY (1 << 1)
> +#define I915_CONTEXT_FORCED_SWITCH (1 << 2)
> #define INTEL_INFO(dev) (((struct drm_i915_private *) (dev)->dev_private)->info)
>
> #define IS_I830(dev) ((dev)->pci_device == 0x3577)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e71e7fc..dcdc80e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -942,6 +942,122 @@ render_ring_dispatch_execbuffer(struct intel_ring_buffer *ring,
> return 0;
> }
>
> +static int do_ring_switch(struct intel_ring_buffer *ring,
> + struct drm_i915_gem_context *new_context,
> + u32 hw_flags)
Can we call this function do_mi_set_context()? It doesn't look like it
has to do with switching rings.
> +{
> + struct drm_device *dev = ring->dev;
> + int ret = 0;
> +
> + if (!new_context->is_initialized) {
> + ret = ring->flush(ring, 0, 0);
> + if (ret)
> + return ret;
> +
> + ret = intel_ring_begin(ring, 2);
> + if (ret)
> + return ret;
> +
> + intel_ring_emit(ring, MI_NOOP | (1 << 22) | new_context->id);
> + intel_ring_emit(ring, MI_NOOP);
> + intel_ring_advance(ring);
> + }
Not sure what this block is doing, nor have the docs enlightened me.
Comment?
> + if (IS_GEN6(dev) && new_context->is_initialized &&
> + ring->itlb_before_ctx_switch) {
> + /* w/a: If Flush TLB Invalidation Mode is enabled, driver must
> + * do a TLB invalidation prior to MI_SET_CONTEXT
> + */
> + gen6_render_ring_flush(ring, 0, 0);
> + }
> +
> + ret = intel_ring_begin(ring, 6);
> + if (ret)
> + return ret;
> +
> + intel_ring_emit(ring, MI_NOOP);
> +
> + switch (INTEL_INFO(dev)->gen) {
> + case 5:
> + intel_ring_emit(ring, MI_SUSPEND_FLUSH | MI_SUSPEND_FLUSH_EN);
> + break;
> + case 6:
> + intel_ring_emit(ring, MI_NOOP);
> + break;
> + case 7:
> + intel_ring_emit(ring, MI_ARB_ON_OFF | MI_ARB_DISABLE);
> + break;
> + case 4:
> + default:
> + BUG();
> + }
I can't see what this MI_ARB_ON_OFF is about. We don't use run lists,
so preemption can't occur, right? And if it's needed on gen7, why isn't
it needed on the previous chipsets, where the command apparently exists
as well?
Also, MI_SUSPEND_FLUSH? (also exists on all chispets) It "Blocks MMIO
sync flush or any flushes related to VT-d while enabled." We don't use
sync flushes, and presumably if we do VT-d related flushes, we still
want them to work, right? Why do hardware render contexts change that?
> + intel_ring_emit(ring, MI_SET_CONTEXT);
> + intel_ring_emit(ring, new_context->obj->gtt_offset |
> + MI_MM_SPACE_GTT |
> + MI_SAVE_EXT_STATE_EN |
> + MI_RESTORE_EXT_STATE_EN |
> + hw_flags);
> +static struct drm_i915_gem_context *
> +render_ring_context_switch(struct intel_ring_buffer *ring,
> + struct drm_i915_gem_context *new_context,
> + u32 flags)
> +{
> + struct drm_device *dev = ring->dev;
> + bool force = (flags & I915_CONTEXT_FORCED_SWITCH) ? true : false;
> + struct drm_i915_gem_context *last = NULL;
> + uint32_t hw_flags = 0;
> +
> + /* last_context is only protected by struct_mutex */
> + WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> +
> + BUG_ON(new_context->obj == NULL || new_context->obj->gtt_offset == 0);
> +
> + if (!force && (ring->last_context == new_context))
> + return new_context;
> +
> + if (flags & I915_CONTEXT_SAVE_ONLY)
> + hw_flags = MI_RESTORE_INHIBIT;
> +
> + if (do_ring_switch(ring, new_context, hw_flags))
> + return NULL;
> +
> + last = ring->last_context;
> + ring->last_context = new_context;
> +
> + /* The first context switch with default context is special */
> + if (last == NULL && new_context->is_default)
> + return new_context;
I'm concerned that by returning new_context here, we're about to
release_context() and unpin the context we just switched to, in
i915_switch_context(). I think we don't want to ever return new_context
here, and the caller wants to not do the request stuff in the case that
