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

[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%7C503a661358114ccf08d208d6a8c0eadc%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636881943862256444&sdata=4c6ehFiS676ZwbneR6T0CBhBHHq7zoL5efQ7E9e%2Fd9E%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..

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.

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