Bug 91314

For those of us without the game, could you figure out what internalFormat is?

in gdb, do

p internalFormat
p *img->TexObject
bt

that should provide some useful info. And/or if you could provide a trace, which when replayed triggers this issue, that'd be most helpful.

So from the backtrace on IRC we see:

#5  0xb6ab9a64 in init_teximage_fields_ms (ctx=<optimized out>, img=0x204c7420, width=<optimized out>, height=0, depth=0, border=0, 
    internalFormat=0, format=MESA_FORMAT_NONE, numSamples=0, fixedSampleLocations=1 '\001') at main/teximage.c:1320
#6  0xb6ab9d4a in _mesa_init_teximage_fields (ctx=0x2055bed0, img=0x204c7420, width=0, height=0, depth=0, border=0, internalFormat=0, 
    format=MESA_FORMAT_NONE) at main/teximage.c:1420
#7  0xb6abee31 in _mesa_texture_image_multisample (ctx=0x2055bed0, dims=2, texObj=0x204a55d8, target=37120, samples=5, internalformat=34836, 
    width=32, height=32, depth=1, fixedsamplelocations=0 '\000', immutable=0 '\000', func=0xb6ec6f81 "glTexImage2DMultisample")
    at main/teximage.c:5703
#8  0xb6abeecc in _mesa_TexImage2DMultisample (target=37120, samples=5, internalformat=34836, width=32, height=32, 
    fixedsamplelocations=0 '\000') at main/teximage.c:5731

So the culprit is:

      if (width > 0 && height > 0 && depth > 0) {
         if (!ctx->Driver.AllocTextureStorage(ctx, texObj, 1,
                  width, height, depth)) {
            /* tidy up the texture image state. strictly speaking,
             * we're allowed to just leave this in whatever state we
             * like, but being tidy is good.
             */
            _mesa_init_teximage_fields(ctx, texImage,
                  0, 0, 0, 0, GL_NONE, MESA_FORMAT_NONE);
         }
      }

The texture storage allocation fails because nv50 doesn't support GL_RGBA32F with > 4 samples.

Of course the sample count check above (samplesOK) passes because in st/mesa format choosing, GL_RGBA32F will happily fall back to RGBA16F, which is perfectly allowed to have 8 samples on nv50.

However st_AllocTextureStorage is blissfully unaware of such a restriction, and fails when it can't create a RGBA32F MS8 texture.

So the bug is 2-fold:

(a) st_AllocateTextureStorage should use st_choose_format (or equivalent) so that it deals with any sample count limitations

(b) _mesa_init_teximage_fields with MESA_FORMAT_NONE appears to trigger the assert no matter what -- this should be handled better. Iago, should we just let MESA_FORMAT_NONE through the assert, as was done before? (By the way, GLenum, which is the type of baseFormat, appears to be unsigned, so a check against -1 is a bit dangerous.)

Created attachment 117078 [details] [review]
make ChooseTextureFormat look at samples

Untested beyond compilation. [and not even for radeon]

(In reply to Ilia Mirkin from comment #2)
> So from the backtrace on IRC we see:
> 
> #5  0xb6ab9a64 in init_teximage_fields_ms (ctx=<optimized out>,
> img=0x204c7420, width=<optimized out>, height=0, depth=0, border=0, 
>     internalFormat=0, format=MESA_FORMAT_NONE, numSamples=0,
> fixedSampleLocations=1 '\001') at main/teximage.c:1320
> #6  0xb6ab9d4a in _mesa_init_teximage_fields (ctx=0x2055bed0,
> img=0x204c7420, width=0, height=0, depth=0, border=0, internalFormat=0, 
>     format=MESA_FORMAT_NONE) at main/teximage.c:1420
> #7  0xb6abee31 in _mesa_texture_image_multisample (ctx=0x2055bed0, dims=2,
> texObj=0x204a55d8, target=37120, samples=5, internalformat=34836, 
>     width=32, height=32, depth=1, fixedsamplelocations=0 '\000', immutable=0
> '\000', func=0xb6ec6f81 "glTexImage2DMultisample")
>     at main/teximage.c:5703
> #8  0xb6abeecc in _mesa_TexImage2DMultisample (target=37120, samples=5,
> internalformat=34836, width=32, height=32, 
>     fixedsamplelocations=0 '\000') at main/teximage.c:5731
> 
> So the culprit is:
> 
>       if (width > 0 && height > 0 && depth > 0) {
>          if (!ctx->Driver.AllocTextureStorage(ctx, texObj, 1,
>                   width, height, depth)) {
>             /* tidy up the texture image state. strictly speaking,
>              * we're allowed to just leave this in whatever state we
>              * like, but being tidy is good.
>              */
>             _mesa_init_teximage_fields(ctx, texImage,
>                   0, 0, 0, 0, GL_NONE, MESA_FORMAT_NONE);
>          }
>       }
> 
> The texture storage allocation fails because nv50 doesn't support GL_RGBA32F
> with > 4 samples.
> 
> Of course the sample count check above (samplesOK) passes because in st/mesa
> format choosing, GL_RGBA32F will happily fall back to RGBA16F, which is
> perfectly allowed to have 8 samples on nv50.
> 
> However st_AllocTextureStorage is blissfully unaware of such a restriction,
> and fails when it can't create a RGBA32F MS8 texture.
> 
> So the bug is 2-fold:
> 
> (a) st_AllocateTextureStorage should use st_choose_format (or equivalent) so
> that it deals with any sample count limitations
> 
> (b) _mesa_init_teximage_fields with MESA_FORMAT_NONE appears to trigger the
> assert no matter what -- this should be handled better. Iago, should we just
> let MESA_FORMAT_NONE through the assert, as was done before? (By the way,
> GLenum, which is the type of baseFormat, appears to be unsigned, so a check
> against -1 is a bit dangerous.)

Well, if we are going to be calling this with MESA_FORMAT_NONE then it does not make sense to have an assertion to check that _mesa_base_tex_format() on that does not return -1 of course. That said, maybe this code is abusing _mesa_init_teximage_fields() a bit too much?

I am not sure about the purpose of this call in the code above but the comment seems to suggest that it is not necessary anyway and it is just part of a bail out process, so in this particular case maybe it would make more sense to just remove the call to _mesa_init_teximage_fields since we are calling it in a way that is not expected only to reset texImage, and instead,  use something else to reset that variable (maybe memset with zeroes would suffice here?) or not do anything at all if it is not necessary like the comment suggests.

Personally, I'd only remove the assert if we expect that _mesa_base_tex_format() returning -1 is possible in normal scenarios, but looking at other uses of that function even in that same file it does not look like that is the case since I see plenty of errors being generated in such situations, even stating that such situations should never happen, which backs the idea that the assertion is legit.

What do you think?

I can't reproduce this in current Mesa git master, the game starts properly.
Marking fixed.

Fedora 22 32-bit
Mesa 11.0-branchpoint-344-ga778831
OpenGL renderer string: Gallium 0.4 on NV92
Kernel 4.2
xf86-video-nouveau-1.0.11-27-g6296145
libdrm-2.4.64-40-g7faedc9