| Message ID | 1342185256-16024-2-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 13 Jul 2012 14:14:04 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ben Widawsky <ben@bwidawsk.net> I guess setting the read_domains is now superfluous. Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 9ae3f2c..90857f8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -225,6 +225,13 @@ static int create_default_context(struct drm_i915_private *dev_priv) > return ret; > } > > + ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true); > + if (ret) { > + i915_gem_object_unpin(ctx->obj); > + do_destroy(ctx); > + return ret; > + } > + > ret = do_switch(NULL, ctx, 0); > if (ret) { > i915_gem_object_unpin(ctx->obj); > @@ -396,8 +403,6 @@ 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) { > - 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 > * whole damn pipeline, we don't need to explicitly mark the > * object dirty. The only exception is that the context must be > @@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj, > * able to defer doing this until we know the object would be > * swapped, but there is no way to do that yet. > */ > + from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION; > + from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; > + i915_gem_object_move_to_active(from_obj, ring, seqno); > from_obj->dirty = 1; > BUG_ON(from_obj->ring != to->ring); > i915_gem_object_unpin(from_obj);
On Fri, Jul 13, 2012 at 02:14:04PM +0100, Chris Wilson wrote: > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 9ae3f2c..90857f8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -225,6 +225,13 @@ static int create_default_context(struct drm_i915_private *dev_priv) > return ret; > } > > + ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true); > + if (ret) { > + i915_gem_object_unpin(ctx->obj); > + do_destroy(ctx); > + return ret; > + } > + > ret = do_switch(NULL, ctx, 0); > if (ret) { > i915_gem_object_unpin(ctx->obj); > @@ -396,8 +403,6 @@ 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) { > - 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 > * whole damn pipeline, we don't need to explicitly mark the > * object dirty. The only exception is that the context must be > @@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj, > * able to defer doing this until we know the object would be > * swapped, but there is no way to do that yet. > */ > + from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION; > + from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; > + i915_gem_object_move_to_active(from_obj, ring, seqno); > from_obj->dirty = 1; > BUG_ON(from_obj->ring != to->ring); > i915_gem_object_unpin(from_obj); I think only the first hunk should be part of this patch - the later two hunks make more sense squashed together with the last patch. At least that would avoid me going a bit wtf here and then again on the last patch where the from_obj->dirty=1 gets removed and smashed into move_to_active. Until I've realized what's going on here ;-) -Daniel
On Fri, 13 Jul 2012 17:54:54 +0200, Daniel Vetter <daniel@ffwll.ch> wrote: > I think only the first hunk should be part of this patch - the later two > hunks make more sense squashed together with the last patch. At least that > would avoid me going a bit wtf here and then again on the last patch where > the from_obj->dirty=1 gets removed and smashed into move_to_active. Until > I've realized what's going on here ;-) When I've rewritten this patch to be more correct (handle the swap-in case as well), it will make more sense the way it is in this patch. Honestly. :) -Chris
On Fri, Jul 13, 2012 at 02:14:04PM +0100, Chris Wilson wrote: > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 9ae3f2c..90857f8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -225,6 +225,13 @@ static int create_default_context(struct drm_i915_private *dev_priv) > return ret; > } > > + ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true); > + if (ret) { > + i915_gem_object_unpin(ctx->obj); > + do_destroy(ctx); > + return ret; > + } > + > ret = do_switch(NULL, ctx, 0); > if (ret) { > i915_gem_object_unpin(ctx->obj); > @@ -396,8 +403,6 @@ 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) { > - 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 > * whole damn pipeline, we don't need to explicitly mark the > * object dirty. The only exception is that the context must be > @@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj, > * able to defer doing this until we know the object would be > * swapped, but there is no way to do that yet. > */ > + from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION; > + from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; Presuming I understand things correctly, setting write_domain to non-zero will result in the ctx object landing on the flushing list when we retire it from the active list. But it isn't being added to the ring's gpu_write_list, so it won't ever get off that flushing list and eventually result in the BUG_ON(seqno == 0) when we try to wait for it after a flush. So afact this first patch here seems to add another instance of the very bug this patch series tries squash ... Additionally I'm still hunting for that other failure case, which can't be fixed by adding the flush in execbuffer if ring->gpu_caches_dirty is set. /me is still lost -Daniel
On Sat, 14 Jul 2012 13:58:58 +0200, Daniel Vetter <daniel@ffwll.ch> wrote: > So afact this first patch here seems to add another instance of the very > bug this patch series tries squash ... Additionally I'm still hunting for > that other failure case, which can't be fixed by adding the flush in > execbuffer if ring->gpu_caches_dirty is set. > > /me is still lost Now all that you are missing is that we only flush GPU_DOMAINS when on the gpu_write_list. -Chris
On Sat, Jul 14, 2012 at 2:48 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Sat, 14 Jul 2012 13:58:58 +0200, Daniel Vetter <daniel@ffwll.ch> wrote: >> So afact this first patch here seems to add another instance of the very >> bug this patch series tries squash ... Additionally I'm still hunting for >> that other failure case, which can't be fixed by adding the flush in >> execbuffer if ring->gpu_caches_dirty is set. >> >> /me is still lost > > Now all that you are missing is that we only flush GPU_DOMAINS when on > the gpu_write_list. Hm, I guess we've seen that one before ;-) But that would require that we add an object with a non-gpu write domain to the active list, but both with or without this patch I don't see how that should happen ... I guess a WARN in move_to_active won't hurt (and we should have done that when fixing the GTT_DOMAIN relocation hole in execbuf), but besides that I'm still as dense as ever, it seems. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 9ae3f2c..90857f8 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -225,6 +225,13 @@ static int create_default_context(struct drm_i915_private *dev_priv) return ret; } + ret = i915_gem_object_set_to_gtt_domain(ctx->obj, true); + if (ret) { + i915_gem_object_unpin(ctx->obj); + do_destroy(ctx); + return ret; + } + ret = do_switch(NULL, ctx, 0); if (ret) { i915_gem_object_unpin(ctx->obj); @@ -396,8 +403,6 @@ 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) { - 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 * whole damn pipeline, we don't need to explicitly mark the * object dirty. The only exception is that the context must be @@ -405,6 +410,9 @@ static int do_switch(struct drm_i915_gem_object *from_obj, * able to defer doing this until we know the object would be * swapped, but there is no way to do that yet. */ + from_obj->base.write_domain = I915_GEM_DOMAIN_INSTRUCTION; + from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; + i915_gem_object_move_to_active(from_obj, ring, seqno); from_obj->dirty = 1; BUG_ON(from_obj->ring != to->ring); i915_gem_object_unpin(from_obj);
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)