On Mon, Jun 13, 2016 at 1:14 PM, Jose Fonseca <jfons...@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 <srol...@vmware.com> >>> wrote: >>>> >>>> Am 10.06.2016 um 05:14 schrieb Ilia Mirkin: >>>>> >>>>> On Thu, Jun 9, 2016 at 11:13 PM, Roland Scheidegger >>>>> <srol...@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 <imir...@alum.mit.edu> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> On Wed, Jun 8, 2016 at 5:48 PM, Fredrik Höglund <fred...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
