On 15/03/2019 03:12, Rob Clark wrote:
On Thu, Mar 14, 2019 at 9:58 PM Roland Scheidegger <srol...@vmware.com> wrote:
Am 15.03.19 um 02:18 schrieb Rob Clark:
On Thu, Mar 14, 2019 at 8:28 PM Roland Scheidegger <srol...@vmware.com> wrote:
Am 14.03.19 um 22:06 schrieb Rob Clark:
On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger <srol...@vmware.com> wrote:
Am 14.03.19 um 14:13 schrieb Rob Clark:
On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger <srol...@vmware.com> wrote:
Am 12.03.19 um 16:16 schrieb Rob Clark:
This previously was not called out clearly, but based on a survey of the
code, it seems the expected behavior is to release the reference to any
sampler views beyond the new range being bound.
That isn't really true. This was designed to work like d3d10, where
other views are unmodified.
The cso code will actually unset all views which previously were set and
are above the num_views in the call (this wouldn't be necessary if the
pipe function itself would work like this).
However, it will only do this for fragment textures, and pass through
the parameters unmodified otherwise. Which means behavior might not be
very consistent for the different stages...
Any opinion about whether views==NULL should be allowed? Currently I
have something like:
----
diff --git a/src/gallium/docs/source/context.rst
b/src/gallium/docs/source/context.rst
index f89d9e1005e..06d30bfb38b 100644
--- a/src/gallium/docs/source/context.rst
+++ b/src/gallium/docs/source/context.rst
@@ -143,6 +143,11 @@ to the array index which is used for sampling.
to a respective sampler view and releases a reference to the previous
sampler view.
+ Sampler views outside of ``[start_slot, start_slot + num_views)`` are
+ unmodified. If ``views`` is NULL, the behavior is the same as if
+ ``views[n]`` was NULL for the entire range, ie. releasing the reference
+ for all the sampler views in the specified range.
+
* ``create_sampler_view`` creates a new sampler view. ``texture`` is
associated
with the sampler view which results in sampler view holding a reference
to the texture. Format specified in template must be compatible
----
But going thru the other drivers, a lot of them also don't handle the
views==NULL case. This case doesn't seem to come up with mesa/st, but
does with XA and nine, and some of the test code.
I think this should be illegal. As you've noted some drivers can't
handle it, and I don't see a particularly good reason to allow it. Well
I guess it trades some complexity in state trackers with some complexity
in drivers...
fwiw, going with the idea that it should be legal, I fixed that in the
drivers that didn't handle it in:
https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2Fmerge_requests%2F449&data=02%7C01%7Csroland%40vmware.com%7C2fe81dea2d9d4de1974f08d6a8e42caa%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636882095286989477&sdata=qd1z5iv8dvt2z16ZlT2OPngoDGofvCM%2F%2F0hsddqAbO4%3D&reserved=0
(planning to send to list, I just pushed a WIP MR to run it thru the CI system)
I'm pretty sure both softpipe and llvmpipe would crash too, they
dereference this without checking if it's null.
So effectively all drivers but one thought it was illegal?
I still see no point in allowing it (or rather, changing this to be
allowed - per se there's nothing really wrong with this to be allowed).
That said, it appears that set_shader_images and set_shader_buffers
allow it, so there's some precedence for this.
hmm, I'd assumed llvmpipe was used with xa somewhere so I didn't
really look at it and assumed it handled this..
xa only sets fragment sampler views, and those only through cso.
cso will turn this into a non-null views parameter.
(cso itself also won't tolerate null views parameter, unless the count
is zero, but that should be alright since its semantics are that it will
unbind all views above the count - well for fragment sampler views...)
nine also sets vertex sampler views through cso, which will get passed
through to drivers as-is. However, the NULL views used there is always
accompanied by a 0 count, so for drivers interpreting things as range to
change rather than unbind things outside it is a natural no-op, and
they'll never even look at views in their loop. (Of course, that's not
quite what nine actually wanted to do...)
And yes things are very inconsistent when passed through cso (for
drivers interpreting it as range to change), since cso will unbind the
views above count for fragment shader views explicitly, but won't do
anything for any other shader stage...
but as you mentioned below, if set_shader_buffers and
set_shader_images allow null, for consistency (and since I'm already
fixing up a bunch of set_shader_buffer implementations, so handling
the ==NULL case isn't a big deal), I'd lean towards allowing NULL. I
guess if we are going to do API cleanup, then consistency is a useful
thing.. I can check llvmpipe and softpipe and add patches to fix them
if needed.
Yes consistency is a nice goal. I'm just not sure it's really worth the
trouble of change for what I'd consider to be a rather minor inconsistency.
(Frankly the cso inconsistency wrt handling fragment and other stages
sampler views is imho way more worrying, even though that's technically
not core gallium interface...)
yeah, the cso special handling for frag is something that would be
nice to kill.. judging by looking at some of the older gallium drivers
that have separate frag and vert sampler view paths I guess this
leaked one way or another.. not really sure if it started in drivers
or state-tracker/cso.
one way or another, I'd kinda like to declare one way or another, that
for all these APIs, either NULL is ok or not, and then once we
establish what is "correct" we can go fix docs and implementation from
there. (And maybe one day eventually cleanup cso.)
BR,
-R
To my knowledge, the semantic of set_sampler_views was changed two years
ago, and that caused some issues:
https://github.com/iXit/Mesa-3D/issues/308
Making drivers consistent (and complete the doc) is a good initiative.
My understanding though is that some drivers may want different
behaviors in order to reduce overhead. If I understood correctly,
radeonsi prefers to minimize view changes to reduce cpu overhead (and
thus prefers not to unbind things outside of the set_sampler_views call
range),
while nouveau prefers to have only what's needed by the call bound for
better gpu performance. I think both are happy with the current code,
because the latter preferred behavior can be emulated in the driver,
while it would be more difficult for the former.
As a personal opinion, please kill handling differences between vert and
frag.
As for the NULL behavior, I don't have any particular opinion on the matter.
Axel
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
