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

Mon Jun 13 16:04:09 UTC 2016

On Mon, Jun 13, 2016 at 5:17 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 13.06.2016 um 14:53 schrieb Marek Olšák:
>> 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.
>
> I think the state tracker could indeed restrict the bind flags based on
> extensions supported, but not anything more. So, with most drivers it
> would always have to set everything always for buffers (which includes
> things as textures buffers).
> I am not sure how well that would work with all drivers (svga for
> instance has separate paths for buffers having or not CONSTANT bind
> flags (the rest matter little), not sure if or how it would be affected).
> Also, for some of the texture view / rt use cases for textures, the
> state tracker does try to set the possibly required bind flags. But last
> time I checked I think some of the issues were then coming from util
> code actually (blits etc.).
> Not saying it wouldn't be really nice to have it enforceable, but I've
> just given up all hope - this is a problem since just about forever...

We can also remove pipe_resource::bind and move the remaining useful
flags to pipe_resource::flags. Those are:
- PIPE_BIND_SCANOUT - determines whether a texture can be displayed on
a monitor, very important
- PIPE_BIND_LINEAR - forces the linear layout for multi-GPU texture
sharing, very important
- PIPE_BIND_CONSTANT_BUFFER - a driver that doesn't support UBOs can
malloc the storage
- maybe a few others, but not many

Some other BIND flags are useful for is_format_supported. The rest can
be removed.

Marek