On Fri, 23 Sep 2011 10:15:02 -0700
Eric Anholt <e...@anholt.net> wrote:
> On Thu, 22 Sep 2011 16:27:12 -0700, Ben Widawsky <b...@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 <e...@anholt.net>
> > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > Cc: Mesa Devs <mesa-dev@lists.freedesktop.org>
> > Signed-off-by: Ben Widawsky <b...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
