[Mesa-dev] [PATCH 6/7] i965: Don't use PREAD for glGetBufferSubData().

[Chris Wilson]

Quoting Kenneth Graunke (2017-07-07 06:19:07)
> On Thursday, July 6, 2017 4:21:28 AM PDT Chris Wilson wrote:
> > Quoting Kenneth Graunke (2017-07-05 21:56:53)
> > > diff --git a/src/mesa/drivers/dri/i965/intel_buffer_objects.c b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
> > > index a9ac29a6a81..2b0f7b9a698 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_buffer_objects.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
> > > @@ -289,7 +289,10 @@ brw_get_buffer_subdata(struct gl_context *ctx,
> > >     if (brw_batch_references(&brw->batch, intel_obj->buffer)) {
> > >        intel_batchbuffer_flush(brw);
> > >     }
> > > -   brw_bo_get_subdata(intel_obj->buffer, offset, size, data);
> > > +
> > > +   void *map = brw_bo_map(brw, intel_obj->buffer, MAP_READ);
> > 
> > Be paranoid and wrap this in a if (map). Data pointer is provided by the
> > user? otherwise you probably want to memset it on failure.
> > 
> > > +   memcpy(data, map + offset, size);
> > > +   brw_bo_unmap(intel_obj->buffer);
> > >  
> > >     mark_buffer_inactive(intel_obj);
> > >  }
> 
> I suppose the paranoia is reasonable, but I'm not sure why I'd memset
> it on failure...unless you're suggesting filling it with 0xd0d0d0d0 or
> something to aid in debugging.  The only reasonably legitimate error
> handling I can think of is to raise GL_OUT_OF_MEMORY, at which point
> we may as well not write anything...

Depends on the level of paranoia. If you created the pointer and hand it
back to the client with unknown contents that's your fault for leaking
whatever was previously in the buffer. If the client passed you the
pointer, then don't touch it on error. GL is in the latter, I was just
thinking aloud about the error path in the patch.
-Chris