[Mesa-dev] [PATCH 8/9] i965: Check after malloc success in intel_miptree_alloc_hiz()

[Juha-Pekka Heikkila]

On 04.07.2014 00:21, Kenneth Graunke wrote:
> On Thursday, July 03, 2014 11:13:18 AM Juha-Pekka Heikkila wrote:
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c 
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> index 2ab0faa..30030d1 100644
>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> @@ -1399,6 +1399,10 @@ intel_miptree_alloc_hiz(struct brw_context *brw,
>>  
>>        for (int layer = 0; layer < mt->level[level].depth; ++layer) {
>>           struct intel_resolve_map *m = malloc(sizeof(struct 
> intel_resolve_map));
>> +         if (!m) {
>> +            _mesa_error_no_memory(__func__);
>> +            return false;
>> +         }
>>           exec_node_init(&m->link);
>>           m->level = level;
>>           m->layer = layer;
>>
> 
> NAK.  This might shut up the tool, but it's almost certainly not correct.  
> You're leaving HiZ enabled for a level, but in a broken state.
> 
> At the very least, I would expect to see mt->level[level].has_hiz = false.  I 
> don't know if that's sufficient.
> 
> On Gen7+, HiZ is just an optional optimization, so you should be able to just 
> turn it off, and have everything continue working, without needing to report 
> an GL_OUT_OF_MEMORY error.
> 
> In particular, you should be able to replace the malloc call here with "m = 
> NULL", simulating a 100% malloc failure rate, and run Piglit.  If you've 
> solved this problem correctly, everything will continue working - there will 
> be no crashes, no extra GL errors, and all the tests should continue passing.

Just out of curiosity, should such behavior be favored? There are other
places also  where "out of memory" could be temporarily dodged but
should it be dodged? I'm thinking if allocation fails on these
structures it is already here quite fatal since if I don't report error
from this failure it probably will be very next allocation failing
unavoidably.

As is I see new crashes on Piglit when I force error here. There are two
types of crashes, ones where inside piglit asserting glError fails and
others where is SIGSEGV result. Did not yet check where SIGSEGV is
coming from.

> 
> On Sandybridge we're probably just screwed, since failing to alloc HiZ (at 
> least for level 0) means we need to disable separate stencil as well.  I 
> highly recommend ignoring that problem for now.
> 
> --Ken
>