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, ®ion->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.vers...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
