[Mesa-dev] [PATCH 08/18] i965: Drop the global GEM name from regions.

[Chad Versace]

On Wed, Apr 30, 2014 at 11:25:27PM -0700, Chad Versace wrote:
> On Tue, Apr 29, 2014 at 04:34:33PM -0700, Eric Anholt wrote:
> > Once a buffer has been named, drm_intel_bo_flink() is just a getter.
> 
> Yep. Surprising but true. (Well, it surprised me).
> 
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> > index 28118b9..e35cc7e 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -1267,7 +1267,16 @@ intel_process_dri2_buffer(struct brw_context *brw,
> >     else
> >        last_mt = rb->singlesample_mt;
> >  
> > -   if (last_mt && last_mt->region->name == buffer->name)
> > +   /* Get the name for our previous RB mt.  We know it had a name already (and
> > +    * thus the DRM call is just a getter), because it could only have been
> > +    * allocated by a previous intel_process_dri2_buffer(), so
> > +    * drm_intel_bo_flink() is just a getter.
> > +    */
> > +   uint32_t old_name = 0;
> > +   if (last_mt)
> > +      drm_intel_bo_flink(last_mt->region->bo, &old_name);
> > +
> > +   if (old_name == buffer->name)
> >        return;
> 
> Without the context provided the commit message, I would not understand
> the above comment. It feels too circular and wordy. Also, it's making
> a claim that's only true *inside* the if-block, but the commit is
> outside. And it says "is just a getter" twice; another point of
> confusion.
> 
> How about something like this?
> 
>   uint32_t old_name = 0;
>   if (last_mt) {
>      /* The bo already has a name because the miptree was created by
>       * a previous call to intel_process_dri2_buffer(). If a bo already
>       * has a name, then drm_intel_bo_flink() is a lowcost getter.  It
>       * does not create a new name.
>       */
>      drm_intel_bo_flink(last_mt->region->bo, &old_name);
>   }
> 
> 
> > -bool
> > -intel_region_flink(struct intel_region *region, uint32_t *name)
> > -{
> > -   if (region->name == 0) {
> > -      if (drm_intel_bo_flink(region->bo, &region->name))
> > -	 return false;
> > -   }
> > -
> > -   *name = region->name;
> > -
> > -   return true;
> > -}
> > -
> 
> Functions like intel_region_flink() make me sad. Good to see it die.
> 
> Patches up to here are
> Reviewed-by: Chad Versace <chad.versace at linux.intel.com> 

To be clear, I prefer that you clarify the comment.

Regardless, the patch has my r-b because I don't like blocking patches
on non-essential comments.