[Intel-gfx] [PATCH 1/7] drm/i915: Mark context switch likely

[Daniel Vetter]

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 at bwidawsk.net> wrote:
> > > Only the very first switch doesn't take the path.
> > >
> > > Signed-off-by: Ben Widawsky <ben at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch