On Fri, Jun 10, 2016 at 12:19 AM, Roland Scheidegger <[email protected]> wrote: > Am 10.06.2016 um 05:14 schrieb Ilia Mirkin: >> On Thu, Jun 9, 2016 at 11:13 PM, Roland Scheidegger <[email protected]> >> 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 <[email protected]> wrote: >>>>>> On Wed, Jun 8, 2016 at 5:48 PM, Fredrik Höglund <[email protected]> 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...
FWIW nouveau has similar requirements around constbuf uploads. I stuck a cb_bindings per-shader bitmap on each piipe_resource (well, nv04_resource) to keep track of where a particular resource is bound. (This information is necessary so that any data uploads are done "correctly", with the same base address/offset/size as the original cb, otherwise the hw gets unhappy.) Have a look at https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/nvc0/nvc0_state_validate.c#n464 and https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/drivers/nouveau/nouveau_buffer.h#n44 . In nouveau_buffer.c we call a special push_cb function which determines whether data should be uploaded via constbuf or regularly. -ilia _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
