[Mesa-dev] [PATCH] nv50, nvc0: Fix gallium nine regression regarding sampler bindings

[Karol Herbst]

On Sun, Nov 25, 2018 at 5:48 PM Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>
> Would it make sense to instead keep track of a samplers_bound[] like
> we do for textures? It's only ever used in a context of
>
> for (i = 0; i < num_samplers; i++)
>   if (samplers[i])
>      do stuff
>

I already had the idea to just remove that num_samplers thing
alltogether and maybe keep a bitmask of set samplers thorugh
sampler_states_bind.

I was looking into how that array looks like and we get something like that:
{ 0x7faab4041cd0, (nil), (nil), (nil), (nil), (nil), (nil), (nil),
(nil), 0x7e3e99b0, 0x7faab4041cd0, 0x7faab4041cd0, (nil), 0x7e3e99b0,
0x7e3e99b0, (nil), (nil), (nil), (nil), (nil), (nil), (nil), (nil),
(nil), (nil), (ni
l), (nil), (nil), (nil), (nil), (nil), (nil) }

I also noticed that with the trace I've got, num_samplers never goes
below 0xf, actually it once increases to 0xf and never changes again
(probably because those upper samplers never get unbound by cso. I am
just not sure if that's a gallium nine issue or if we really have to
deal with that. Will play around some more with all that.

> So having a mask would actually optimize that, and make this logic much simpler.
> On Sun, Nov 25, 2018 at 10:47 AM Karol Herbst <kherbst at redhat.com> wrote:
> >
> > The new approach is that samplers don't get unbound even if they won't be used
> > in a draw and we should just leave them be as well.
> >
> > Fixes a regression in multiple windows games using gallium nine and nouveau.
> >
> > v2: adjust num_samplers to keep track of the highest sampler bound
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106577
> > Fixes: 4d6fab245eec3880e2a59424a579851f44857ce8
> >        "cso: don't track the number of sampler states bound"
> > Signed-off-by: Karol Herbst <kherbst at redhat.com>
> > ---
> >  src/gallium/drivers/nouveau/nv50/nv50_state.c | 19 ++++++++++---------
> >  src/gallium/drivers/nouveau/nvc0/nvc0_state.c | 16 ++++++++++------
> >  2 files changed, 20 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/gallium/drivers/nouveau/nv50/nv50_state.c b/src/gallium/drivers/nouveau/nv50/nv50_state.c
> > index fb4a259ce16..286509d5fcd 100644
> > --- a/src/gallium/drivers/nouveau/nv50/nv50_state.c
> > +++ b/src/gallium/drivers/nouveau/nv50/nv50_state.c
> > @@ -589,6 +589,7 @@ nv50_sampler_state_delete(struct pipe_context *pipe, void *hwcso)
> >        for (i = 0; i < nv50_context(pipe)->num_samplers[s]; ++i)
> >           if (nv50_context(pipe)->samplers[s][i] == hwcso)
> >              nv50_context(pipe)->samplers[s][i] = NULL;
> > +      nv50_context(pipe)->num_samplers[s] = 0;
> >     }
> >
> >     nv50_screen_tsc_free(nv50_context(pipe)->screen, nv50_tsc_entry(hwcso));
> > @@ -600,26 +601,26 @@ static inline void
> >  nv50_stage_sampler_states_bind(struct nv50_context *nv50, int s,
> >                                 unsigned nr, void **hwcso)
> >  {
> > +   assert(nr <= PIPE_MAX_SAMPLERS);
> > +   unsigned highest_found = 0;
> >     unsigned i;
> >
> > -   assert(nr <= PIPE_MAX_SAMPLERS);
> >     for (i = 0; i < nr; ++i) {
> >        struct nv50_tsc_entry *old = nv50->samplers[s][i];
> >
> > +      if (hwcso[i])
> > +         highest_found = i;
> > +
> >        nv50->samplers[s][i] = nv50_tsc_entry(hwcso[i]);
> >        if (old)
> >           nv50_screen_tsc_unlock(nv50->screen, old);
> >     }
> > -   assert(nv50->num_samplers[s] <= PIPE_MAX_SAMPLERS);
> > -   for (; i < nv50->num_samplers[s]; ++i) {
> > -      if (nv50->samplers[s][i]) {
> > -         nv50_screen_tsc_unlock(nv50->screen, nv50->samplers[s][i]);
> > -         nv50->samplers[s][i] = NULL;
> > -      }
> > +   for (;i < nv50->num_samplers[s]; ++i) {
> > +      if (nv50->samplers[s][i])
> > +         highest_found = i;
> >     }
> >
> > -   nv50->num_samplers[s] = nr;
> > -
> > +   nv50->num_samplers[s] = highest_found + 1;
> >     nv50->dirty_3d |= NV50_NEW_3D_SAMPLERS;
> >  }
> >
> > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_state.c b/src/gallium/drivers/nouveau/nvc0/nvc0_state.c
> > index f2393cb27b5..756b828238e 100644
> > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_state.c
> > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_state.c
> > @@ -449,10 +449,12 @@ nvc0_sampler_state_delete(struct pipe_context *pipe, void *hwcso)
> >  {
> >     unsigned s, i;
> >
> > -   for (s = 0; s < 6; ++s)
> > +   for (s = 0; s < 6; ++s) {
> >        for (i = 0; i < nvc0_context(pipe)->num_samplers[s]; ++i)
> >           if (nvc0_context(pipe)->samplers[s][i] == hwcso)
> >              nvc0_context(pipe)->samplers[s][i] = NULL;
> > +      nvc0_context(pipe)->num_samplers[s] = 0;
> > +   }
> >
> >     nvc0_screen_tsc_free(nvc0_context(pipe)->screen, nv50_tsc_entry(hwcso));
> >
> > @@ -464,11 +466,15 @@ nvc0_stage_sampler_states_bind(struct nvc0_context *nvc0,
> >                                 unsigned s,
> >                                 unsigned nr, void **hwcso)
> >  {
> > +   unsigned highest_found = 0;
> >     unsigned i;
> >
> >     for (i = 0; i < nr; ++i) {
> >        struct nv50_tsc_entry *old = nvc0->samplers[s][i];
> >
> > +      if (hwcso[i])
> > +         highest_found = i;
> > +
> >        if (hwcso[i] == old)
> >           continue;
> >        nvc0->samplers_dirty[s] |= 1 << i;
> > @@ -478,13 +484,11 @@ nvc0_stage_sampler_states_bind(struct nvc0_context *nvc0,
> >           nvc0_screen_tsc_unlock(nvc0->screen, old);
> >     }
> >     for (; i < nvc0->num_samplers[s]; ++i) {
> > -      if (nvc0->samplers[s][i]) {
> > -         nvc0_screen_tsc_unlock(nvc0->screen, nvc0->samplers[s][i]);
> > -         nvc0->samplers[s][i] = NULL;
> > -      }
> > +      if (nvc0->samplers[s][i])
> > +         highest_found = i;
> >     }
> >
> > -   nvc0->num_samplers[s] = nr;
> > +   nvc0->num_samplers[s] = highest_found + 1;
> >  }
> >
> >  static void
> > --
> > 2.19.1
> >