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

[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://gitlab.freedesktop.org/mesa/mesa/merge_requests/449

(planning to send to list, I just pushed a WIP MR to run it thru the CI system)

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

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

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