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

[Ilia Mirkin]

On Fri, Jun 10, 2016 at 12:19 AM, Roland Scheidegger <sroland at vmware.com> wrote:
> 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...

FWIW nouveau has similar requirements around constbuf uploads. I stuck
a cb_bindings per-shader bitmap on each piipe_resource (well,
nv04_resource) to keep track of where a particular resource is bound.
(This information is necessary so that any data uploads are done
"correctly", with the same base address/offset/size as the original
cb, otherwise the hw gets unhappy.) Have a look at
https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/nvc0/nvc0_state_validate.c#n464
and https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/nouveau_buffer.h#n44
. In nouveau_buffer.c we call a special push_cb function which
determines whether data should be uploaded via constbuf or regularly.

  -ilia