[Mesa-dev] [PATCH] mesa: fix tex format selection for GL_R32F and other R/G float formats

[Brian Paul]

On 01/16/2012 09:40 AM, Jose Fonseca wrote:
> ----- Original Message -----
>> The i965 driver advertises GL_ARB_texture_float and GL_ARB_texture_rg
>> support but the ctx->TextureFormatSupported[] table entries for
>> MESA_FORMAT_R_FLOAT32 and MESA_FORMAT_RGBA_FLOAT32 are false on gen 4
>> hardware.  So the case for GL_R32F would fail and we'd print an
>> implementation error.
>>
>> This patch adds more Mesa tex format options for GL_R32F and other
>> R/G
>> formats so we fall back to 16-bit formats when 32-bit formats aren't
>> available.
>>
>> Eric made the same fix in commit 6216a5b4 for the non R/G formats.
>>
>> This should fix https://bugs.freedesktop.org/show_bug.cgi?id=44039
>>
>> NOTE: This is a candidate for the 8.0 branch.
>> ---
>>   src/mesa/main/texformat.c |    6 ++++++
>>   1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/mesa/main/texformat.c b/src/mesa/main/texformat.c
>> index 7e60541..2f680d9 100644
>> --- a/src/mesa/main/texformat.c
>> +++ b/src/mesa/main/texformat.c
>> @@ -788,19 +788,25 @@ _mesa_choose_tex_format( struct gl_context
>> *ctx, GLint internalFormat,
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_R_FLOAT16);
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_R_FLOAT32);
>
> Since we're touching this code, I think that MESA_FORMAT_RG_FLOAT(16|32) should be attempted here before MESA_FORMAT_RGBA_FLOAT(16|32), as it takes half the memory / bandwidth.
>
>>   	RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT32);
>> +	 RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT16);
>
> I think that MESA_FORMAT_RGBA_FLOAT16 should be attempted before MESA_FORMAT_RGBA_FLOAT32 for GL_*16F formats, and not after, as it is both more accurate&  efficient.
>
>>   	break;
>>         case GL_R32F:
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_R_FLOAT32);
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT32);
>
> Ditto (try RG before RGBA)

Is it likely that a driver will support RG formats but not R formats? 
  In any case, it's trivial to add those cases.


>> +	 RETURN_IF_SUPPORTED(MESA_FORMAT_R_FLOAT16);
>> +	 RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT16);
>>   	 break;
>>         case GL_RG16F:
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_RG_FLOAT16);
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_RG_FLOAT32);
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT32);
>> +	 RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT16);
>
> Ditto (insert FLOAT16 before  FLOAT32).
>
>>   	break;
>>         case GL_RG32F:
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_RG_FLOAT32);
>>   	 RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT32);
>> +	 RETURN_IF_SUPPORTED(MESA_FORMAT_RG_FLOAT16);
>> +	 RETURN_IF_SUPPORTED(MESA_FORMAT_RGBA_FLOAT16);
>>   	 break;
>>
>>         default:
>
> Otherwise looks good. Good find.

Thanks.  I'll post a v2.

-Brian