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

[Roland Scheidegger]

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

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