[Mesa-dev] [PATCH v2 06/23] mesa: Fix incorrect assertion in init_teximage_fields_ms

[Ilia Mirkin]

On Tue, Dec 2, 2014 at 8:24 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Tue, Dec 2, 2014 at 3:05 AM, Iago Toral <itoral at igalia.com> wrote:
>> On Mon, 2014-12-01 at 13:25 -0500, Ilia Mirkin wrote:
>>> On Mon, Dec 1, 2014 at 6:04 AM, Iago Toral Quiroga <itoral at igalia.com> wrote:
>>> > _BaseFormat is a GLenum (unsigned int) so testing if its value is
>>> > greater than 0 to detect the cases where _mesa_base_tex_format
>>> > returns -1 doesn't work.
>>> >
>>> > Fixing the assertion breaks the arb_texture_view-lifetime-format
>>> > piglit test on nouveau, since that test calls
>>> > _mesa_base_tex_format with GL_R16F with a context that does not
>>> > have ARB_texture_float, so it returns -1 for the BaseFormat, which
>>> > was not being catched properly by the ASSERT in init_teximage_fields_ms
>>> > until now.
>>>
>>> Sorry to hijack the thread, but... can you elaborate why this would
>>> fail on nouveau but not on i965? Does st/mesa do something differently
>>> wrt exposing ARB_texture_float vs i965? Does nouveau do something
>>> wrong?
>>
>> Hi Illia, I didn't investigate the source of the problem in Nouveau or
>> why this is different than the other drivers, however I can give more
>> details:
>>
>> We are running piglit against i965, llvmpipe, classic swrast, nouveu and
>> radeon, but we only hit the problem with nouveau. The code that triggers
>> the assertion is _mesa_base_tex_format (teximage.c), which is called
>> with internalFormat = GL_R16F, and then, in that function there is this
>> code:
>>
>> if (ctx->Extensions.ARB_texture_rg) {
>>    switch (internalFormat) {
>>    case GL_R16F:
>>    case GL_R32F:
>>       if (!ctx->Extensions.ARB_texture_float)
>>          break;
>>       return GL_RED;
>> ...
>>
>> On i965, the code returns GL_RED, but in Nouveau it hits the break
>> because ctx->Extensions.ARB_texture_float is false in this case (and
>> later will return -1 after being unable to find a proper base format).
>>
>> In the case of Intel, ARB_texture_float is always enabled. In the case
>> of Gallium I see this in st_extensions.c:
>>
>> static const struct st_extension_format_mapping rendertarget_mapping[] =
>> {
>>       { { o(ARB_texture_float) },
>>         { PIPE_FORMAT_R32G32B32A32_FLOAT,
>>           PIPE_FORMAT_R16G16B16A16_FLOAT } },
>> ...
>>
>> so if I understand that right, for ARB_texture_float to be activated I
>> need PIPE_FORMAT_R32G32B32A32_FLOAT and PIPE_FORMAT_R16G16B16A16_FLOAT
>> to be supported, so I guess that at least one of these is not supported
>> in the nVidia hardware I am using (NVIDIA GT218 [ION]). If that is not
>> the problem, then I guess it would need further investigation, but I
>> don't see any other places where Gallium or nouveau set that extension.
>
> The only way I can think of that it wouldn't have ARB_texture_float is
> if you didn't build with --enable-texture-float. This would lose you
> GL3 support, among other things... Perhaps you're only seeing the
> issue on nouveau because it's the only driver other than i965 that
> implements ARB_texture_view.
>
> You can see the set of extensions that should be enabled...
> http://people.freedesktop.org/~imirkin/glxinfo/glxinfo.html (look at
> the GT21x column).
>
>   -ilia

I believe the issue is that _mesa_TextureView should be checking the
target internal format against _mesa_base_tex_format() and returning a
GL_INVALID_VALUE error (? the spec conveniently lists errors as
'TODO') if it returns -1. Chris -- thoughts? Maybe stick it in
lookup_view_class?

  -ilia