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

[Chad Versace]

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>