[Mesa-dev] [PATCH 1/2] i965: Fix reference counting in new basevertex upload code.

[Ian Romanick]

On 09/12/2014 09:05 AM, Kenneth Graunke wrote:
> On Friday, September 12, 2014 08:35:03 AM Ian Romanick wrote:
>> On 09/10/2014 03:41 PM, Kenneth Graunke wrote:
>>> In the non-indirect draw case, we call intel_upload_data to upload
>>> gl_BaseVertex.  It makes brw->draw.draw_params_bo point to the upload
>>> buffer, and increments the upload BO reference count.
>>>
>>> So, we need to unreference it when making brw->draw.draw_params_bo point
>>> at something else, or else we'll retain a reference to stale upload
>>> buffers and hold on to them forever.
>>>
>>> This also means that the indirect case should increment the reference
>>> count on the indirect draw buffer when making brw->draw.draw_params_bo
>>> point at it.  That way, both paths increment the reference count, so
>>> we can safely unreference it every time.
>>>
>>> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
>>> Cc: "10.3" <mesa-stable at lists.freedesktop.org>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_draw.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c
>>> index efa85de..b28eaf2 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_draw.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
>>> @@ -434,10 +434,13 @@ static bool brw_try_draw_prims( struct gl_context *ctx,
>>>        brw->draw.start_vertex_location = prims[i].start;
>>>        brw->draw.base_vertex_location = prims[i].basevertex;
>>>  
>>> +      drm_intel_bo_unreference(brw->draw.draw_params_bo);
>>> +
>>>        if (prims[i].is_indirect) {
>>>           /* Point draw_params_bo at the indirect buffer. */
>>>           brw->draw.draw_params_bo =
>>>              intel_buffer_object(ctx->DrawIndirectBuffer)->buffer;
>>> +         drm_intel_bo_reference(brw->draw.draw_params_bo);
>>
>> Is there are a race here, or do we know that the reference count is at
>> least two when we unreference above?
> 
> The idea is to unreference the buffer used for the previous draw call - which may be either a DrawIndirect buffer, or the upload buffer.  Then, we'll decide what buffer we want for the current draw call.
> 
> For background:
> 
> Legitimate GL-facing buffers hold a reference count until DeleteBuffer.
> brw->upload.bo holds a reference count on the upload buffer until batch flush, at which point we toss it and get a new (non-busy) upload buffer.
> 
> I think it works out in all cases.  Here's my logic:
> 
> Case: Non-indirect -> Non-indirect
> 
> If we haven't flushed the batch buffer, then brw->upload.bo still holds a
> reference to the upload buffer.  We drop our reference, then pick it back up.
> 
> If we've flushed the batch buffer, then brw->upload.bo will have been released,
> and will now point to a fresh buffer.  We may hold the last reference on the
> old upload buffer - but releasing it is okay, because we don't want it anymore.
> 
> We drop our reference to the old buffer, and take a reference on the new one.
> 
> Case: Indirect -> Indirect
> 
> The application may have switched out the indirect buffer between calls.
> 
> If not, it's still bound as ctx->DrawIndirectBuffer, which holds a gl_buffer_object::RefCount on the gl_buffer_object, keeping it alive.  So we can drop our reference and re-take it.
> 
> If they did, we drop our reference on the old one.  This could allow it to be
> freed, if the API-facing buffer was deleted.  But we don't care.  We take a
> reference to the new ctx->DrawIndirectBuffer's BO, which definitely exists.
> 
> Case: Non-indirect -> Indirect
> 
> We stop referencing the upload BO, and instead reference DrawIndirectBuffer.
> The upload BO will live or die based on other people holding refcounts,
> which is the desired behavior.
> 
> Case: Indirect -> Non-indirect
> 
> We stop referencing the BO which was bound as ctx->DrawIndirectBuffer when
> we prepared the previous draw call.  It will probably live, unless the API-
> facing buffer object was deleted and this was the last use...and that's okay.
> 
> We take a reference on the upload buffer, which is referenced by brw->upload.bo.

Okay.  That's convincing enough. :)

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>



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