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

[Brian Paul]

On 06/13/2016 10:04 AM, Marek Olšák wrote:
> 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.

Let's not do this immediately.  It would take some effort to update our 
VMware driver.

-Brian