On Mon, Jun 13, 2016 at 5:17 PM, Roland Scheidegger <srol...@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 <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. > > 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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
