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

[Ilia Mirkin]

On Thu, Jun 9, 2016 at 10:58 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> 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.

That's very much intentional, at least as I understand it. The buffer
hasn't changed, neither have the start/size. The fact that you were
getting a dirty bit set before was an accident of implementation - had
the test not been calling piglit_draw_rect(), glBufferData() would not
have been called, and you would not have seen the new bindings, which
would not have triggered the unnecessary dirtying of the UBO. It's
really unfortunate that piglit_draw_rect() ends up dirtying so much
state - this can end up papering over other issues.

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

I believe that your transfer_unmap/flush logic needs to become
smarter. Especially in the situation of persistently and
coherently-mapped buffers, you really need to keep track of this
stuff. (And you appear to support ARB_buffer_storage).

Cheers,

  -ilia