[Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

Fri Mar 15 12:21:15 UTC 2019

On 15/03/2019 13:12, Rob Clark wrote:
> On Fri, Mar 15, 2019 at 3:49 AM Axel Davy <davyaxel0 at gmail.com> wrote:
>> On 15/03/2019 03:12, Rob Clark wrote:
>>> On Thu, Mar 14, 2019 at 9:58 PM Roland Scheidegger <sroland at vmware.com> wrote:
>>>> Am 15.03.19 um 02:18 schrieb Rob Clark:
>>>>> On Thu, Mar 14, 2019 at 8:28 PM Roland Scheidegger <sroland at vmware.com> wrote:
>>>>>> Am 14.03.19 um 22:06 schrieb Rob Clark:
>>>>>>> On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger <sroland at vmware.com> wrote:
>>>>>>>> Am 14.03.19 um 14:13 schrieb Rob Clark:
>>>>>>>>> On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger <sroland at 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.
> Hmm, but this is at odds with Roland's statement that sampler views
> outside the specified range are unmodified.
>
> If we defined the API such that slots >= start+nr are unbound, then a
> driver could choose not to touch them to minimize CPU overhead.  But
> if defined that they are unmodified then the driver cannot silently
> discard them.
>
> BR,
> -R

The sampler views outside the specified range are unmodified.
This is compatible with what I said. Nouveau determines the minimum 
number of samplers actually needed
by the current shader, and internally unbinds those above (and rebinds 
if needed by new shader).

Axel

>
>> 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev