| Message ID | 1365118914-15753-2-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote: > Only the very first switch doesn't take the path. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 94d873a..aa080ea 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -390,7 +390,7 @@ static int do_switch(struct i915_hw_context *to) > * is a bit suboptimal because the retiring can occur simply after the > * MI_SET_CONTEXT instead of when the next seqno has completed. > */ > - if (from_obj != NULL) { > + if (likely(from_obj)) { Looking at the function, is this, uh, likely to make a difference? Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's *two* patches 4/7 in the series! :o) I'm just generally wary of adding (un)likely annotations. I don't think it matters that the annotation itself is obviously correct; IMHO the performance impact should matter. </bikeshed> BR, Jani. > from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; > i915_gem_object_move_to_active(from_obj, ring); > /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the > -- > 1.8.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Apr 05, 2013 at 10:09:58AM +0300, Jani Nikula wrote: > On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote: > > Only the very first switch doesn't take the path. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index 94d873a..aa080ea 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -390,7 +390,7 @@ static int do_switch(struct i915_hw_context *to) > > * is a bit suboptimal because the retiring can occur simply after the > > * MI_SET_CONTEXT instead of when the next seqno has completed. > > */ > > - if (from_obj != NULL) { > > + if (likely(from_obj)) { > > Looking at the function, is this, uh, likely to make a difference? > Probably not. > Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's > *two* patches 4/7 in the series! :o) I don't see two, weird. > > I'm just generally wary of adding (un)likely annotations. I don't think > it matters that the annotation itself is obviously correct; IMHO the > performance impact should matter. > > </bikeshed> Right. I actually stuck it in by accident, and decided to keep it only because it serves as nice documentation. I suppose a comment would do the same thing, but this also had a chance (albeit slight) to improve things. > > BR, > Jani. > > > > from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; > > i915_gem_object_move_to_active(from_obj, ring); > > /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the > > -- > > 1.8.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Apr 05, 2013 at 09:44:54AM -0700, Ben Widawsky wrote: > On Fri, Apr 05, 2013 at 10:09:58AM +0300, Jani Nikula wrote: > > On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote: > > > Only the very first switch doesn't take the path. > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > --- > > > drivers/gpu/drm/i915/i915_gem_context.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > > index 94d873a..aa080ea 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > @@ -390,7 +390,7 @@ static int do_switch(struct i915_hw_context *to) > > > * is a bit suboptimal because the retiring can occur simply after the > > > * MI_SET_CONTEXT instead of when the next seqno has completed. > > > */ > > > - if (from_obj != NULL) { > > > + if (likely(from_obj)) { > > > > Looking at the function, is this, uh, likely to make a difference? > > > > Probably not. > > > Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's > > *two* patches 4/7 in the series! :o) > > I don't see two, weird. > > > > > I'm just generally wary of adding (un)likely annotations. I don't think > > it matters that the annotation itself is obviously correct; IMHO the > > performance impact should matter. > > > > </bikeshed> > > Right. I actually stuck it in by accident, and decided to keep it only > because it serves as nice documentation. I suppose a comment would do > the same thing, but this also had a chance (albeit slight) to improve > things. I have to admit that I like the documentation a likely/unlikely can provide for the extreme case (like here where it only happens on a fresh gpu iirc). But ofthen the same can be achieved with a goto slowpath or an appropriately called function for the slowpath (or naming the check in an obvious way like not_initiailized or so). But often the slowpath is already pretty clear, e.g. handling -EFAULT for copy_from|to_user_atomic. Definite bikeshed territory ;-) -Daniel
On Fri, 05 Apr 2013, Ben Widawsky <ben@bwidawsk.net> wrote: > On Fri, Apr 05, 2013 at 10:09:58AM +0300, Jani Nikula wrote: >> Same goes for the unlikely in patches 4/7. (Yes, patch_es_ 4/7 - there's >> *two* patches 4/7 in the series! :o) > > I don't see two, weird. Hmm, it's not only my mail setup: http://thread.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/20106/focus=20108 >> I'm just generally wary of adding (un)likely annotations. I don't think >> it matters that the annotation itself is obviously correct; IMHO the >> performance impact should matter. >> >> </bikeshed> > > Right. I actually stuck it in by accident, and decided to keep it only > because it serves as nice documentation. I suppose a comment would do > the same thing, but this also had a chance (albeit slight) to improve > things. That's true too, and I do like code that documents itself. No biggie. BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 94d873a..aa080ea 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -390,7 +390,7 @@ static int do_switch(struct i915_hw_context *to) * is a bit suboptimal because the retiring can occur simply after the * MI_SET_CONTEXT instead of when the next seqno has completed. */ - if (from_obj != NULL) { + if (likely(from_obj)) { from_obj->base.read_domains = I915_GEM_DOMAIN_INSTRUCTION; i915_gem_object_move_to_active(from_obj, ring); /* As long as MI_SET_CONTEXT is serializing, ie. it flushes the
Only the very first switch doesn't take the path. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)