[Mesa-dev] [PATCH v3 27/43] anv/pipeline: Use 32-bit surface formats for 16-bit formats

[Chema Casanova]

El 16/10/17 a las 08:57, Alejandro Piñeiro escribió:
> On 15/10/17 12:14, Pohjolainen, Topi wrote:
>> On Thu, Oct 12, 2017 at 08:38:16PM +0200, Jose Maria Casanova Crespo wrote:
>>> From: Alejandro Piñeiro <apinheiro at igalia.com>
>>>
>>> From Vulkan 1.0.50 spec, Section 3.30.1. Format Definition:
>>>     VK_FORMAT_R16G16_SFLOAT
>>>
>>>     A two-component, 32-bit signed floating-point format that has a
>>>     16-bit R component in bytes 0..1, and a 16-bit G component in
>>>     bytes 2..3.
>>>
>>> So this format expects those 16-bit floats to be passed without any
>>> conversion (applies too using 2/3/4 components, and with int formats)
>>>
>>> But from skl PRM, vol 07, section FormatConversion, page 445 there is
>>> a table that points that *16*FLOAT formats are converted to FLOAT,
>>> that in that context, is a 32-bit float. This is similar to the
>>> *64*FLOAT formats, that converts 64-bit floats to 32-bit floats.
>>>
>>> Unfortunately, while with 64-bit floats we have the alternative to use
>>> *64*PASSTHRU formats, it is not the case with 16-bits.
>>>
>>> This issue happens too with 16-bit int surface formats.
>>>
>>> As a workaround, if we are using a 16-bit location at the shader, we
>>> use 32-bit formats to avoid the conversion, and will fix getting the
>>> proper content later. Note that as we are using 32-bit formats, we can
>>> use formats with less components (example: use *R32* for *R16G16*).
>>>
>>> Signed-off-by: Jose Maria Casanova Crespo <jmcasanova at igalia.com>
>>> Signed-off-by: Alejandro Piñeiro <apinheiro at igalia.com>
>>> ---
>>>  src/intel/vulkan/genX_pipeline.c | 47 ++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 47 insertions(+)
>>>
>>> diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
>>> index c2fa9c0ff7..8b2d472787 100644
>>> --- a/src/intel/vulkan/genX_pipeline.c
>>> +++ b/src/intel/vulkan/genX_pipeline.c
>>> @@ -83,6 +83,44 @@ vertex_element_comp_control(enum isl_format format, unsigned comp)
>>>     }
>>>  }
>>>  
>>> +#if GEN_GEN >= 8
>>> +static enum isl_format
>>> +adjust_16bit_format(enum isl_format format)
>>> +{
>>> +   switch(format) {
>>> +   case ISL_FORMAT_R16_FLOAT:
>>> +      return ISL_FORMAT_R32_FLOAT;
>>> +   case ISL_FORMAT_R16G16_FLOAT:
>>> +      return ISL_FORMAT_R32_FLOAT;
>>> +   case ISL_FORMAT_R16G16B16_FLOAT:
>>> +      return ISL_FORMAT_R32G32_FLOAT;
>>> +   case ISL_FORMAT_R16G16B16A16_FLOAT:
>>> +      return ISL_FORMAT_R32G32_FLOAT;
>>> +
>>> +   case ISL_FORMAT_R16_SINT:
>>> +      return ISL_FORMAT_R32_SINT;
>>> +   case ISL_FORMAT_R16G16_SINT:
>>> +      return ISL_FORMAT_R32_SINT;
>>> +   case ISL_FORMAT_R16G16B16_SINT:
>>> +      return ISL_FORMAT_R32G32_SINT;
>>> +   case ISL_FORMAT_R16G16B16A16_SINT:
>>> +      return ISL_FORMAT_R32G32_SINT;
>>> +
>>> +   case ISL_FORMAT_R16_UINT:
>>> +      return ISL_FORMAT_R32_UINT;
>>> +   case ISL_FORMAT_R16G16_UINT:
>>> +      return ISL_FORMAT_R32_UINT;
>>> +   case ISL_FORMAT_R16G16B16_UINT:
>>> +      return ISL_FORMAT_R32G32_UINT;
>>> +   case ISL_FORMAT_R16G16B16A16_UINT:
>>> +      return ISL_FORMAT_R32G32_UINT;
>>> +
>>> +   default:
>>> +      return format;
>>> +   }
>>> +}
>> Just wondering aloud. As we are going to reinterpret the data in any case 
>> we could simply use _UINT variants even for FLOAT and SINT. It doesn't really
>> make any difference only that here someone might think it is somehow relevant
>> to keep the base type. 
> FWIW, I also don't mind too much. So ok, we could try that change too.
I've just sent a v2 of this patch. The code is simplified and doesn't
introduce regressions on 16-bit tests.

Chema