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

Fri Mar 15 12:12:29 UTC 2019

On Fri, Mar 15, 2019 at 3:49 AM Axel Davy <davyaxel0 at gmail.com> wrote:
>
> On 15/03/2019 03:12, Rob Clark wrote:
> > On Thu, Mar 14, 2019 at 9:58 PM Roland Scheidegger <sroland at vmware.com> wrote:
> >> 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...)
> > yeah, the cso special handling for frag is something that would be
> > nice to kill.. judging by looking at some of the older gallium drivers
> > that have separate frag and vert sampler view paths I guess this
> > leaked one way or another.. not really sure if it started in drivers
> > or state-tracker/cso.
> >
> > one way or another, I'd kinda like to declare one way or another, that
> > for all these APIs, either NULL is ok or not, and then once we
> > establish what is "correct" we can go fix docs and implementation from
> > there.  (And maybe one day eventually cleanup cso.)
> >
> > BR,
> > -R
> >
> >
>
> To my knowledge, the semantic of set_sampler_views was changed two years
> ago, and that caused some issues:
> https://github.com/iXit/Mesa-3D/issues/308
>
> Making drivers consistent (and complete the doc) is a good initiative.
>
> My understanding though is that some drivers may want different
> behaviors in order to reduce overhead. If I understood correctly,
> radeonsi prefers to minimize view changes to reduce cpu overhead (and
> thus prefers not to unbind things outside of the set_sampler_views call
> range),
> while nouveau prefers to have only what's needed by the call bound for
> better gpu performance. I think both are happy with the current code,
> because the latter preferred behavior can be emulated in the driver,
> while it would be more difficult for the former.

Hmm, but this is at odds with Roland's statement that sampler views
outside the specified range are unmodified.

If we defined the API such that slots >= start+nr are unbound, then a
driver could choose not to touch them to minimize CPU overhead.  But
if defined that they are unmodified then the driver cannot silently
discard them.

BR,
-R

>
> As a personal opinion, please kill handling differences between vert and
> frag.
> As for the NULL behavior, I don't have any particular opinion on the matter.
>
> Axel
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev