Nouveau - Aug 2014 - [Mesa-stable] [PATCH 2/2] nv50: zero out unbound samplers

On 30/08/14 23:02, Ilia Mirkin wrote:
> Samplers are only defined up to num_samplers, so set all samplers above
> nr to NULL so that we don't try to read them again later.
> 
Would it be worth doing a similar thing with the unlocked samplers below the
nr mark ? It seems to me that we might be leaking nv50->samplers[s][i], or
perhaps I'm missing something ?

-Emil
> Tested-by: Christian Ruppert <idl0r at qasl.de>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> Cc: "10.2 10.3" <mesa-stable at lists.freedesktop.org>
> ---
>  src/gallium/drivers/nouveau/nv50/nv50_state.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_state.c
b/src/gallium/drivers/nouveau/nv50/nv50_state.c
> index 48bc079..cf84f88 100644
> --- a/src/gallium/drivers/nouveau/nv50/nv50_state.c
> +++ b/src/gallium/drivers/nouveau/nv50/nv50_state.c
> @@ -585,9 +585,12 @@ nv50_stage_sampler_states_bind(struct nv50_context
*nv50, int s,
>           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])
> +   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;
> +      }
> +   }
>  
>     nv50->num_samplers[s] = nr;
>  
>

On Sat, Aug 30, 2014 at 7:30 PM, Emil Velikov <emil.l.velikov at
gmail.com> wrote:
> On 30/08/14 23:02, Ilia Mirkin wrote:
>> Samplers are only defined up to num_samplers, so set all samplers above
>> nr to NULL so that we don't try to read them again later.
>>
> Would it be worth doing a similar thing with the unlocked samplers below
the
> nr mark ? It seems to me that we might be leaking nv50->samplers[s][i],
or
> perhaps I'm missing something ?
Can you elaborate? sampler_state_create/delete deal with allocation
and deallocation. samplers starts out as NULL. I'm just making sure
that a subsequent call with a larger number of samplers doesn't try to
unlock potentially-deleted samplers.

  -ilia
>
> -Emil
>
>> Tested-by: Christian Ruppert <idl0r at qasl.de>
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> Cc: "10.2 10.3" <mesa-stable at lists.freedesktop.org>
>> ---
>>  src/gallium/drivers/nouveau/nv50/nv50_state.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_state.c
b/src/gallium/drivers/nouveau/nv50/nv50_state.c
>> index 48bc079..cf84f88 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_state.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_state.c
>> @@ -585,9 +585,12 @@ nv50_stage_sampler_states_bind(struct nv50_context
*nv50, int s,
>>           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])
>> +   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;
>> +      }
>> +   }
>>
>>     nv50->num_samplers[s] = nr;
>>
>>
>

On 31/08/14 00:34, Ilia Mirkin wrote:
> On Sat, Aug 30, 2014 at 7:30 PM, Emil Velikov <emil.l.velikov at
gmail.com> wrote:
>> On 30/08/14 23:02, Ilia Mirkin wrote:
>>> Samplers are only defined up to num_samplers, so set all samplers
above
>>> nr to NULL so that we don't try to read them again later.
>>>
>> Would it be worth doing a similar thing with the unlocked samplers
below the
>> nr mark ? It seems to me that we might be leaking
nv50->samplers[s][i], or
>> perhaps I'm missing something ?
> 
> Can you elaborate? sampler_state_create/delete deal with allocation
> and deallocation. samplers starts out as NULL. I'm just making sure
> that a subsequent call with a larger number of samplers doesn't try to
> unlock potentially-deleted samplers.
> 
   for (i = 0; i < nr; ++i) {
      struct nv50_tsc_entry *old = nv50->samplers[s][i];

      nv50->samplers[s][i] = nv50_tsc_entry(hwcso[i]);
      if (old)
         nv50_screen_tsc_unlock(nv50->screen, old);
   }

In the above hunk we get the old/current tsc, drop in on the floor and assign
the new one in it's place. Does where does the ST keep track of the old one
in
order to nuke it via sampler_state_delete, or is it already deleted by the
time we get here ?

-Emil
>   -ilia
> 
>>
>> -Emil
>>
>>> Tested-by: Christian Ruppert <idl0r at qasl.de>
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> Cc: "10.2 10.3" <mesa-stable at
lists.freedesktop.org>
>>> ---
>>>  src/gallium/drivers/nouveau/nv50/nv50_state.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_state.c
b/src/gallium/drivers/nouveau/nv50/nv50_state.c
>>> index 48bc079..cf84f88 100644
>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_state.c
>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_state.c
>>> @@ -585,9 +585,12 @@ nv50_stage_sampler_states_bind(struct
nv50_context *nv50, int s,
>>>           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])
>>> +   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;
>>> +      }
>>> +   }
>>>
>>>     nv50->num_samplers[s] = nr;
>>>
>>>
>>

On 31/08/14 00:34, Ilia Mirkin wrote:
> On Sat, Aug 30, 2014 at 7:30 PM, Emil Velikov <emil.l.velikov at
gmail.com> wrote:
>> On 30/08/14 23:02, Ilia Mirkin wrote:
>>> Samplers are only defined up to num_samplers, so set all samplers
above
>>> nr to NULL so that we don't try to read them again later.
>>>
>> Would it be worth doing a similar thing with the unlocked samplers
below the
>> nr mark ? It seems to me that we might be leaking
nv50->samplers[s][i], or
>> perhaps I'm missing something ?
> 
> Can you elaborate? sampler_state_create/delete deal with allocation
> and deallocation. samplers starts out as NULL. I'm just making sure
> that a subsequent call with a larger number of samplers doesn't try to
> unlock potentially-deleted samplers.
> 
   for (i = 0; i < nr; ++i) {
      struct nv50_tsc_entry *old = nv50->samplers[s][i];

      nv50->samplers[s][i] = nv50_tsc_entry(hwcso[i]);
      if (old)
         nv50_screen_tsc_unlock(nv50->screen, old);
   }

In the above hunk we get the old/current tsc, drop in on the floor and assign
the new one in it's place. Does where does the ST keep track of the old one
in
order to nuke it via sampler_state_delete, or is it already deleted by the
time we get here ?

-Emil
>   -ilia
> 
>>
>> -Emil
>>
>>> Tested-by: Christian Ruppert <idl0r at qasl.de>
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> Cc: "10.2 10.3" <mesa-stable at
lists.freedesktop.org>
>>> ---
>>>  src/gallium/drivers/nouveau/nv50/nv50_state.c | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_state.c
b/src/gallium/drivers/nouveau/nv50/nv50_state.c
>>> index 48bc079..cf84f88 100644
>>> --- a/src/gallium/drivers/nouveau/nv50/nv50_state.c
>>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_state.c
>>> @@ -585,9 +585,12 @@ nv50_stage_sampler_states_bind(struct
nv50_context *nv50, int s,
>>>           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])
>>> +   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;
>>> +      }
>>> +   }
>>>
>>>     nv50->num_samplers[s] = nr;
>>>
>>>
>>