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

Fri Mar 15 07:49:46 UTC 2019

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.

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