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

Fri Mar 15 01:58:44 UTC 2019

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...)

Roland

> 
> BR,
> -R
> 
>>
>>>
>>> I was on the fence between making st handle it vs making driver handle
>>> it, but it is trivial for the driver to handle it if it knows it is
>>> supposed to.  And I had to fixup the drivers for various things
>>> already (most hadn't been updated to handle the `start_slot` param,
>>> for ex).
>> Yes, I think in particular because when going through cso things will
>> always start at slot 0, so some drivers got sloppy...
>> But well for views not being allowed to be null that's also pretty
>> trivial for state trackers to handle...
>>
>>>
>>> Eric suggested (on the MR) introducing a helper for this, which might
>>> be a better approach to cut down on the boilerplate..  I'll play with
>>> that idea.
>>>
>>> (btw, from a quick look, set_sampler_views isn't the only problem
>>> slot.. I noticed set_shader_buffers has the same issue.. but maybe
>>> I'll try to fix one thing at a time and worry more about that when
>>> panfrost or etnaviv gets closer to the point of supporting SSBO and
>>> compute shaders..)
>> Both set_shader_buffers and set_shader_images say it's allowed to be
>> null (as per their function description in p_context.h). They are much
>> newer functions though and not everybody implements them, so there might
>> not be much of an issue there (though it doesn't actually look mesa/st
>> would make use of that neither, so maybe it isn't all that useful...).
>>
>> Roland
>>
>>
>>>
>>> BR,
>>> -R
>>>
>>>>
>>>> Roland
>>>>
>>>>
>>>>
>>>>> BR,
>>>>> -R
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> I think radeonsi and freedreno were the only ones not doing this.  Which
>>>>>>> could probably temporarily leak a bit of memory by holding on to the
>>>>>>> sampler view reference.
>>>>>> Not sure about other drivers, but llvmpipe will not do this neither.
>>>>>>
>>>>>> Roland
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>>>>>> ---
>>>>>>>  src/gallium/docs/source/context.rst | 3 +++
>>>>>>>  1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/gallium/docs/source/context.rst b/src/gallium/docs/source/context.rst
>>>>>>> index f89d9e1005e..199d335f8f4 100644
>>>>>>> --- a/src/gallium/docs/source/context.rst
>>>>>>> +++ b/src/gallium/docs/source/context.rst
>>>>>>> @@ -143,6 +143,9 @@ to the array index which is used for sampling.
>>>>>>>    to a respective sampler view and releases a reference to the previous
>>>>>>>    sampler view.
>>>>>>>
>>>>>>> +  Previously bound samplers with index ``>= num_views`` are unbound rather
>>>>>>> +  than unmodified.
>>>>>>> +
>>>>>>>  * ``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
>>>>>>>
>>>>>>
>>>>
>>