On Thu, Mar 14, 2019 at 8:28 PM Roland Scheidegger <srol...@vmware.com> wrote: > > Am 14.03.19 um 22:06 schrieb Rob Clark: > > On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger <srol...@vmware.com> > > wrote: > >> > >> Am 14.03.19 um 14:13 schrieb Rob Clark: > >>> On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger <srol...@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 <robdcl...@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 > >>>>> > >>>> > >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
