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

[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.

  -ilia