[Mesa-dev] [PATCH 1/2] i965: Massively simplify the intel_upload implementation.

[Kenneth Graunke]

On 03/26/2014 11:14 AM, Eric Anholt wrote:
> Kenneth Graunke <kenneth at whitecape.org> writes:
> 
>> On 03/25/2014 11:27 AM, Eric Anholt wrote:
>>> The implementation kept a page-sized area for uploading data, and
>>> uploaded chunks from that to a 64kb-sized streamed buffer.  This wasted
>>> cache footprint (and extra state tracking to do so) when we want to just
>>> write our data into the buffer immediately.
>>>
>>> Instead, build it around an interface like brw_state_batch() that just
>>> gets you a pointer to BO memory to upload your stuff immediately.
>>>
>>> Improves OpenArena on HSW by 1.62209% +/- 0.355299% (n=61) and on BYT by
>>> 1.7916% +/- 0.415743% (n=31).
>>>
>>> v2: Rebase on Mesa master, drop old prototypes.  Re-do performance
>>>     comparison on a kernel that doesn't punish CPU efficiency
>>>     improvements.
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_context.h          |   5 +-
>>>  src/mesa/drivers/dri/i965/brw_draw_upload.c      |  10 +-
>>>  src/mesa/drivers/dri/i965/intel_buffer_objects.h |  21 +--
>>>  src/mesa/drivers/dri/i965/intel_upload.c         | 167 +++++++++--------------
>>>  4 files changed, 77 insertions(+), 126 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
>>> index 04af5d0..e119682 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>>> @@ -1041,10 +1041,7 @@ struct brw_context
>>>  
>>>     struct {
>>>        drm_intel_bo *bo;
>>> -      GLuint offset;
>>> -      uint32_t buffer_len;
>>> -      uint32_t buffer_offset;
>>> -      char buffer[4096];
>>> +      uint32_t next_offset;
>>>     } upload;
>>>  
>>>     /**
>>> diff --git a/src/mesa/drivers/dri/i965/brw_draw_upload.c b/src/mesa/drivers/dri/i965/brw_draw_upload.c
>>> index e261163..a579025 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_draw_upload.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_draw_upload.c
>>> @@ -381,21 +381,17 @@ copy_array_to_vbo_array(struct brw_context *brw,
>>>     const unsigned char *src = element->glarray->Ptr + min * src_stride;
>>>     int count = max - min + 1;
>>>     GLuint size = count * dst_stride;
>>> +   uint8_t *dst = intel_upload_space(brw, size, dst_stride,
>>> +                                     &buffer->bo, &buffer->offset);
>>>  
>>
>> It sure seems like these references will exist across batchbuffers,
>> meaning we may hold on to the upload buffer containing our vertex data
>> until we do copy_array_to_vbo_array on it again.  That seems
>> unfortunate.  (Or, maybe I'm wrong?)
> 
> copy_array_to_vbo_array() happens per draw call, so it won't persist
> across batchbuffers.

Technically, I think it will persist across batches after the last draw
call in a batch, and before the first in the second.  But that's
fine...we just need to avoid long-standing references which will keep it
alive too long.

I guess it's okay because brw_merge_inputs is called on every draw call,
and does:

   for (i = 0; i < brw->vb.nr_buffers; i++) {
      drm_intel_bo_unreference(brw->vb.buffers[i].bo);
      brw->vb.buffers[i].bo = NULL;
   }

and this is before switching out brw->vb.nr_buffers and brw->vb.buffers
for the new ones...so we always free our old data.

I think we're fine here after all.  Thanks.

--Ken

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140326/da5038d4/attachment.sig>