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

[Rob Clark]

On Wed, Mar 13, 2019 at 11:37 AM Roland Scheidegger <sroland at vmware.com> wrote:
>
> Am 12.03.19 um 22:48 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...
> >
> > hmm, I did notice w/ deqp tests (which aren't so good at
> > resetting/clearing state between tests), that I ended up w/ different
> > # of sampler views bound (without changing freedreno to match the
> > behavior of most of the other drivers).. I didn't really dig in that
> > closely but it seemed like mesa/st wasn't clearing the additional
> > previously bound textures.  Maybe I overlooked something, but that
> > seemed wrong.
> >
> > One way or another, I guess we should clarify and change the various
> > drivers to have a common behavior because right now there two
> > different behaviors and I guess it is at least confusing for new
> > gallium driver writers (as it was for me and I've been at it for a
> > while)
>
> Yes, I agree with that, the current state there doesn't help anyone.

I guess the best thing is that I should put together a patchset that
documents the opposite behavior of what this patch suggests, followed
by patches for the other drivers to change them to match the docs.

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