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

Mon Jun 13 12:53:15 UTC 2016

On Mon, Jun 13, 2016 at 1:14 PM, Jose Fonseca <jfonseca at vmware.com> wrote:
> On 10/06/16 15:40, Roland Scheidegger wrote:
>>
>> Am 10.06.2016 um 12:38 schrieb Marek Olšák:
>>>
>>> On Fri, Jun 10, 2016 at 6: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...
>>>
>>>
>>> Or you can assume bind == 0 means all flags are set.
>>
>>
>> Yes in this case, but I don't think that really helps. Even when using
>> the old gl api for setting this, the target (and hence the bind flag)
>> may be something completely different than the actual usage.
>
>
> I'd rather have GL renderer set all possible bind flags.
>
> And if the excess of bind flags leads certain drivers to significant
> inefficiencies, then those drivers can internally track exactly where those
> buffers/resources have been bound (similar to Roland's recent proposed patch
> for llvmpipe.)
>
> (And if it turns out that all drivers end up ignoring the state tracker bind
> flags and just track bindings internally, we could even elimiate bind
> altogether.  But my hunch is that bind flags are useful.  So one step at a
> time.)
>
> Honestly have anything except this unenforced contract would be better.

Setting PIPE_BIND_CONSTANT_BUFFER for all buffers will break r300g. We
should be careful and set only the possible flags that can occur based
on exposed extensions.

Marek