On Sun, 15 Jul 2012 12:34:24 +0100
Chris Wilson <chris at chris-wilson.co.uk> wrote:
> When bug hunting, I found the interface to do_switch() overly
> complicated and I believe festered the earlier bug. This aims to make
> the code a little clearer.
Would you be willing to split this up into 2 patches? One which
reorganizes create_default, and one which changes do_switch?
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_gem_context.c | 57 ++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 0cfc9d2..90a9c9e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -97,8 +97,7 @@
>
> static struct i915_hw_context *
> i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id);
> -static int do_switch(struct drm_i915_gem_object *from_obj,
> - struct i915_hw_context *to, u32 seqno);
> +static int do_switch(struct i915_hw_context *to);
>
> static int get_context_size(struct drm_device *dev)
> {
> @@ -220,19 +219,20 @@ static int create_default_context(struct drm_i915_private *dev_priv)
> */
> dev_priv->ring[RCS].default_context = ctx;
> ret = i915_gem_object_pin(ctx->obj, CONTEXT_ALIGN, false);
> - if (ret) {
> - do_destroy(ctx);
> - return ret;
> - }
> + if (ret)
> + goto err_destroy;
>
> - ret = do_switch(NULL, ctx, 0);
> - if (ret) {
> - i915_gem_object_unpin(ctx->obj);
> - do_destroy(ctx);
> - } else {
> - DRM_DEBUG_DRIVER("Default HW context loaded\n");
> - }
> + ret = do_switch(ctx);
> + if (ret)
> + goto err_unpin;
>
> + DRM_DEBUG_DRIVER("Default HW context loaded\n");
> + return 0;
> +
> +err_unpin:
> + i915_gem_object_unpin(ctx->obj);
> +err_destroy:
> + do_destroy(ctx);
> return ret;
> }
>
> @@ -359,17 +359,18 @@ mi_set_context(struct intel_ring_buffer *ring,
> return ret;
> }
>
> -static int do_switch(struct drm_i915_gem_object *from_obj,
> - struct i915_hw_context *to,
> - u32 seqno)
> +static int do_switch(struct i915_hw_context *to)
> {
> - struct intel_ring_buffer *ring = NULL;
> + struct intel_ring_buffer *ring = to->ring;
> + struct drm_i915_gem_object *from_obj = ring->last_context_obj;
> u32 hw_flags = 0;
> int ret;
>
> - BUG_ON(to == NULL);
> BUG_ON(from_obj != NULL && from_obj->pin_count == 0);
>
> + if (from_obj == to->obj)
> + return 0;
> +
> ret = i915_gem_object_pin(to->obj, CONTEXT_ALIGN, false);
> if (ret)
> return ret;
> @@ -389,7 +390,6 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
> else if (WARN_ON_ONCE(from_obj == to->obj)) /* not yet expected */
> hw_flags |= MI_FORCE_RESTORE;
>
> - ring = to->ring;
> ret = mi_set_context(ring, to, hw_flags);
> if (ret) {
> i915_gem_object_unpin(to->obj);
> @@ -403,6 +403,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
> * MI_SET_CONTEXT instead of when the next seqno has completed.
> */
> if (from_obj != NULL) {
> + u32 seqno = i915_gem_next_request_seqno(ring);
> from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> i915_gem_object_move_to_active(from_obj, ring, seqno);
> /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
> @@ -413,7 +414,7 @@ static int do_switch(struct drm_i915_gem_object *from_obj,
> * swapped, but there is no way to do that yet.
> */
> from_obj->dirty = 1;
> - BUG_ON(from_obj->ring != to->ring);
> + BUG_ON(from_obj->ring != ring);
> i915_gem_object_unpin(from_obj);
>
> drm_gem_object_unreference(&from_obj->base);
> @@ -444,10 +445,7 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> int to_id)
> {
> struct drm_i915_private *dev_priv = ring->dev->dev_private;
> - struct drm_i915_file_private *file_priv = NULL;
> struct i915_hw_context *to;
> - struct drm_i915_gem_object *from_obj = ring->last_context_obj;
> - int ret;
>
> if (dev_priv->hw_contexts_disabled)
> return 0;
> @@ -455,21 +453,18 @@ int i915_switch_context(struct intel_ring_buffer *ring,
> if (ring != &dev_priv->ring[RCS])
> return 0;
>
> - if (file)
> - file_priv = file->driver_priv;
> -
> if (to_id == DEFAULT_CONTEXT_ID) {
> to = ring->default_context;
> } else {
> - to = i915_gem_context_get(file_priv, to_id);
> + if (file == NULL)
> + return -EINVAL;
> +
> + to = i915_gem_context_get(file->driver_priv, to_id);
> if (to == NULL)
> return -ENOENT;
> }
>
> - if (from_obj == to->obj)
> - return 0;
> -
> - return do_switch(from_obj, to, i915_gem_next_request_seqno(to->ring));
> + return do_switch(to);
> }
>
> int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
--
Ben Widawsky, Intel Open Source Technology Center
