[Mesa-dev] Mesa (master): st/mesa: use buffer usage history to set dirty flags for revalidation

[Roland Scheidegger]

Am 10.06.2016 um 05:14 schrieb Ilia Mirkin:
> On Thu, Jun 9, 2016 at 11:13 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 10.06.2016 um 04:58 schrieb Roland Scheidegger:
>>> Am 10.06.2016 um 03:11 schrieb Ilia Mirkin:
>>>> On Thu, Jun 9, 2016 at 9:07 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>> On Wed, Jun 8, 2016 at 5:48 PM, Fredrik Höglund <fredrik at kde.org> wrote:
>>>>>> On Wednesday 08 June 2016, Ilia Mirkin wrote:
>>>>>>> Glancing at the code (I don't even have a piglit checkout here):
>>>>>>>
>>>>>>> static void
>>>>>>> set_ubo_binding(struct gl_context *ctx, ...)
>>>>>>> ...
>>>>>>>    /* If this is a real buffer object, mark it has having been used
>>>>>>>     * at some point as a UBO.
>>>>>>>     */
>>>>>>>    if (size >= 0)
>>>>>>>       bufObj->UsageHistory |= USAGE_UNIFORM_BUFFER;
>>>>>>>
>>>>>>> That seems bogus - what if the current size is 0 (unallocated), the
>>>>>>> buffer object gets bound to a UBO endpoint, and then someone goes in
>>>>>>> and does glBufferData()? Same for set_ssbo_binding.
>>>>>>>
>>>>>>>   -ilia
>>>>>>
>>>>>> The test is greater than or equal to zero, so the UsageHistory should
>>>>>> be set even when the buffer is unallocated.
>>>>>
>>>>> Right, duh.
>>>>>
>>>>>>
>>>>>> But the piglit test doesn't bind the buffer as a uniform buffer before
>>>>>> it allocates it.  It allocates the buffer first with glNamedBufferData(),
>>>>>> and then binds it.  The UsageHistory is still set to the default value in
>>>>>> the glNamedBufferData() call, since the buffer has never been bound
>>>>>> at that point.  But the uniform buffer state should still be marked as
>>>>>> dirty in the glBindBufferRange() call.  I think this failure suggests
>>>>>> that that doesn't happen for some reason.
>>>>>
>>>>> I haven't looked in GREAT detail, but the test does pass on nv50,
>>>>> nvc0, and softpipe. It only fails on llvmpipe.
>>>>>
>>>>> Brian, this might be out of my comfort area to figure out... Given
>>>>> that it's working on the other drivers, that seems more likely to be a
>>>>> failing of llvmpipe somehow.
>>>>
>>>> Another observation is that the square sizes/shapes are all correct.
>>>> However the colors are all of the first square (red). So it seems like
>>>> some issue is happening for the fragment shader, but not vertex.
>>>>
>>>
>>> I've looked at this briefly and I'm not sure who's to blame.
>>> It goes something like this:
>>> - we don't get any set_constant_buffer calls anymore when the contents
>>> of the buffer change. I don't think that's ok, looks like a bug to me?
>>> Granted it's still the same buffer, just modfying a bound one.
>>>
>>> - when the contents are updated, it ends up in some transfer stuff as
>>> expected, in the end llvmpipe_transfer_map(). This checks if the
>>> resource is referenced in the current scene, if so it would flush -
>>> however this is only done for textures and rts (because UBO contents are
>>> indeed copied to the scene, not referenced, this is quite ok here, we
>>> don't want to flush).
>>>
>>> - on the next draw, we'd check the dirty bits - we never got
>>> set_constant_buffer (which would set LP_NEW_CONSTANTS and in turn
>>> LP_SETUP_NEW_CONSTANTS), and llvmpipe_transfer_map didn't do anything,
>>> so we don't pick up the changed contents, and continue to use the old
>>> copied ones.
>>>
>>> We could check if buffers are referenced in the scene and set the
>>> LP_NEW_CONSTANTS bit accordingly, but I'm not sure what the st is doing
>>> is really ok? (For vs, it doesn't matter that we miss the update - as
>>> the offsets etc. are all the same the vs will just pick up the changes
>>> automatically as we don't copy anything since this stuff all runs
>>> synchronously.)
>>>
>>
>>
>> Err actually this analysis is flawed.
>> llvmpipe would indeed check in llvmpipe_transfer_map() if a currently
>> bound constant buffer is changed and hence set LP_NEW_CONSTANTS accordingly.
>> But it doesn't because the buffer doesn't have the
>> PIPE_BIND_CONSTANT_BUFFER usage flag set (so, strictly speaking, it
>> would be illegal to bind it as such, but this is never enforced). In
>> fact it doesn't have _any_ bind flag set.
>> So I blame the GL api, but I don't know how to fix this mess cleanly (we
>> don't really want to ignore bind flags completely).
> 
> Ah yeah, rookie mistake. pipe_resource->bind is only there to confuse
> you. If you use it for anything after resource creation, that's a bug.
> 

This was designed in gallium with dx10 in mind, which has a proper api
for this. But I suppose the only way to fix it in the mesa state tracker
would be to just set ALL possible bind flags for buffers always...

Roland