[Mesa-dev] [PATCH 1/3] cso: don't release sampler states that are bound

[Michel Dänzer]

On 07/12/16 07:16 PM, Nicolai Hähnle wrote:
> On 07.12.2016 08:50, Michel Dänzer wrote:
>> On 06/12/16 10:24 PM, Marek Olšák wrote:
>>> On Mon, Dec 5, 2016 at 10:05 AM, Michel Dänzer <michel at daenzer.net>
>>> wrote:
>>>> On 03/12/16 05:38 AM, Marek Olšák wrote:
>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>
>>>>> This fixes random radeonsi GPU hangs in Batman Arkham: Origins
>>>>> (Wine) and
>>>>> probably many other games too.
>>>>>
>>>>> cso_cache deletes sampler states when the cache size is too big and
>>>>> doesn't
>>>>> check which sampler states are bound, causing use-after-free in
>>>>> drivers.
>>>>> Because of that, radeonsi uploaded garbage sampler states and the
>>>>> hardware
>>>>> went bananas. Other drivers may have experienced similar issues.
>>>>>
>>>>> Cc: 13.0 12.0 <mesa-stable at lists.freedesktop.org>
>>>>> ---
>>>>>  src/gallium/auxiliary/cso_cache/cso_cache.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/gallium/auxiliary/cso_cache/cso_cache.c
>>>>> b/src/gallium/auxiliary/cso_cache/cso_cache.c
>>>>> index b240c93..1f3be4b 100644
>>>>> --- a/src/gallium/auxiliary/cso_cache/cso_cache.c
>>>>> +++ b/src/gallium/auxiliary/cso_cache/cso_cache.c
>>>>> @@ -181,21 +181,23 @@ static inline void sanitize_cb(struct
>>>>> cso_hash *hash, enum cso_cache_type type,
>>>>>        --to_remove;
>>>>>     }
>>>>>  }
>>>>>
>>>>>  struct cso_hash_iter
>>>>>  cso_insert_state(struct cso_cache *sc,
>>>>>                   unsigned hash_key, enum cso_cache_type type,
>>>>>                   void *state)
>>>>>  {
>>>>>     struct cso_hash *hash = _cso_hash_for_type(sc, type);
>>>>> -   sanitize_hash(sc, hash, type, sc->max_size);
>>>>> +
>>>>> +   if (type != CSO_SAMPLER)
>>>>> +      sanitize_hash(sc, hash, type, sc->max_size);
>>>>>
>>>>>     return cso_hash_insert(hash, hash_key, state);
>>>>>  }
>>>>>
>>>>>  struct cso_hash_iter
>>>>>  cso_find_state(struct cso_cache *sc,
>>>>>                 unsigned hash_key, enum cso_cache_type type)
>>>>>  {
>>>>>     struct cso_hash *hash = _cso_hash_for_type(sc, type);
>>>>>
>>>>>
>>>>
>>>> If I understand correctly, this means that the maximum cache size
>>>> effectively isn't enforced for sampler states? Wouldn't it be better
>>>> instead to add code which prevents currently bound states from being
>>>> deleted from here?
>>>
>>> Not in this patch. Maybe later and if CPU profiling results show that
>>> it matters.
>>>
>>> Currently, if the cache size is exceeded, the GPU usually hangs. If
>>> pruning the cache was important, most apps would hang.
>>>
>>> Thus, I'd like to push this as-is.
>>
>> Fair enough, but this will require some follow-up work:
>>
>> Why are sampler states singled out? The same problem can at least
>> theoretically happen with other CSOs as well, right?
> 
> The other CSOs only have single bind points. So with other CSOs, what
> happens is that:
> 
> 1. State tracker wants to bind some state, creates the CSO.
> 2. cso_cache gets cleaned out; this might delete the currently bound CSO.
> 3. Currently bound CSO gets immediately overwritten with the new state
> anyway.

Actually, looking at cso_context.c, all delete_*_state functions except
delete_sampler_state check if the state to be deleted is the currently
bound one, and return FALSE in that case.


> Samplers are different because there are many bind points within a
> single context.

I suspect that was indeed why the above wasn't done for sampler states
as well. However, it seems like it should be easy to add, and should be
cheap in most cases.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer