[Intel-gfx] [PATCH] i965: use nonblocking maps MapRangeBuffer

[Ben Widawsky]

On Fri, 23 Sep 2011 10:15:02 -0700
Eric Anholt <eric at anholt.net> wrote:

> On Thu, 22 Sep 2011 16:27:12 -0700, Ben Widawsky <ben at bwidawsk.net>
> wrote:
> > This makes the code a lot cleaner, and theoretically faster (not
> > many real world tests use this GL extension).
> > 
> > Cc: Eric Anholt <eric at anholt.net>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Mesa Devs <mesa-dev at lists.freedesktop.org>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/intel/intel_buffer_objects.c |   48
> > ++------------------ 1 files changed, 5 insertions(+), 43
> > deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/intel/intel_buffer_objects.c
> > b/src/mesa/drivers/dri/intel/intel_buffer_objects.c index
> > d35a50e..91dddce 100644 ---
> > a/src/mesa/drivers/dri/intel/intel_buffer_objects.c +++
> > b/src/mesa/drivers/dri/intel/intel_buffer_objects.c @@ -296,8
> > +296,6 @@ intel_bufferobj_get_subdata(struct gl_context * ctx, }
> >  }
> >  
> > -
> > -
> >  /**
> >   * Called via glMapBufferRange and glMapBuffer
> >   *
> > @@ -363,50 +361,14 @@ intel_bufferobj_map_range(struct gl_context *
> > ctx, return NULL;
> >     }
> >  
> > -   /* If the user doesn't care about existing buffer contents and
> > mapping
> > -    * would cause us to block, then throw out the old buffer.
> > -    */
> > -   if (!(access & GL_MAP_UNSYNCHRONIZED_BIT) &&
> > -       (access & GL_MAP_INVALIDATE_BUFFER_BIT) &&
> > -       drm_intel_bo_busy(intel_obj->buffer)) {
> > -      drm_intel_bo_unreference(intel_obj->buffer);
> > -      intel_bufferobj_alloc_buffer(intel, intel_obj);
> > -   }
> 
> Why did you delete this code?  Applications rely on this happening.

Maybe I got confused by the order of operations, but I thought this
should be,
If synchronized and invalidate and busy, just create a new buffer. This
*should* be handled correctly by the nonblocking call below. Maybe I'm
mistaken.

> 
> > -
> > -   /* If the user is mapping a range of an active buffer object but
> > -    * doesn't require the current contents of that range, make a
> > new
> > -    * BO, and we'll copy what they put in there out at unmap or
> > -    * FlushRange time.
> > -    */
> >     if ((access & GL_MAP_INVALIDATE_RANGE_BIT) &&
> > -       drm_intel_bo_busy(intel_obj->buffer)) {
> > -      if (access & GL_MAP_FLUSH_EXPLICIT_BIT) {
> > +       drm_intel_bo_busy(intel_obj->buffer) &&
> > +       (access & GL_MAP_FLUSH_EXPLICIT_BIT)) {
> >  	 intel_obj->range_map_buffer = malloc(length);
> >  	 obj->Pointer = intel_obj->range_map_buffer;
> > -      } else {
> > -	 intel_obj->range_map_bo =
> > drm_intel_bo_alloc(intel->bufmgr,
> > -						      "range map",
> > -						      length, 64);
> > -	 if (!(access & GL_MAP_READ_BIT)) {
> > -	    drm_intel_gem_bo_map_gtt(intel_obj->range_map_bo);
> > -	    intel_obj->mapped_gtt = GL_TRUE;
> > -	 } else {
> > -	    drm_intel_bo_map(intel_obj->range_map_bo,
> > -			     (access & GL_MAP_WRITE_BIT) != 0);
> > -	    intel_obj->mapped_gtt = GL_FALSE;
> > -	 }
> > -	 obj->Pointer = intel_obj->range_map_bo->virtual;
> > -      }
> > -      return obj->Pointer;
> > -   }
> > -
> > -   if (!(access & GL_MAP_READ_BIT)) {
> > -      drm_intel_gem_bo_map_gtt(intel_obj->buffer);
> > -      intel_obj->mapped_gtt = GL_TRUE;
> > -   } else {
> > -      drm_intel_bo_map(intel_obj->buffer, (access &
> > GL_MAP_WRITE_BIT) != 0);
> > -      intel_obj->mapped_gtt = GL_FALSE;
> > -   }
> > +	 return obj->Pointer;
> > +   } else
> > +    drm_intel_gem_bo_map_nonblocking(intel_obj->buffer,
> > &intel_obj->mapped_gtt);
> 
> So, if either INVALIDATE or FLUSH_EXPLICIT is not set, or the buffer
> is idle, you use the nonblocking map instead of the normal blocking
> map? How is that supposed to be correct?
> 
> This patch should be adding one new case and an early return.  If it
> does something else, that's a bad sign.
> 
> >     obj->Pointer = intel_obj->buffer->virtual + offset;
> >     return obj->Pointer;

First of all, I realize this diff is pretty awful, so for the sake of
readability let me put the relevant part here:

   if ((access & GL_MAP_INVALIDATE_RANGE_BIT) &&
       drm_intel_bo_busy(intel_obj->buffer) &&
       (access & GL_MAP_FLUSH_EXPLICIT_BIT)) {
	 intel_obj->range_map_buffer = malloc(length);
	 obj->Pointer = intel_obj->range_map_buffer;
>>>>>>>> return obj->Pointer;
   } else
       drm_intel_gem_bo_map_nonblocking(intel_obj->buffer,
	        &intel_obj->mapped_gtt);

   obj->Pointer = intel_obj->buffer->virtual + offset;
   return obj->Pointer;

The reason why I think this works is drm_intel_gem_bo_map_nonblocking()
is supposed to do the right thing (so the name is misleading). Please
look at the libdrm patch in the series and see if you still agree.