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

[Roland Scheidegger]

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

Roland