| Message ID | 1342352064-6749-3-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 15 Jul 2012 12:34:24 +0100 Chris Wilson <chris@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@chris-wilson.co.uk> > Cc: Ben Widawsky <ben@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,
On Sun, Jul 15, 2012 at 12:34:24PM +0100, Chris Wilson 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. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ben Widawsky <ben@bwidawsk.net> I've picked up patches 2&3 with Ben's irc r-b on patch 3, thanks. -Daniel
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,
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. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 57 ++++++++++++++----------------- 1 file changed, 26 insertions(+), 31 deletions(-)