Re: [Mesa-dev] [PATCH v3] mesa: handle a bunch of formats in IMPLEMENTATION_COLOR_READ_*

2018-06-21 Thread Tomeu Vizoso 
On 31 May 2018 at 00:25, Eric Anholt  wrote:
> Tomeu Vizoso  writes:
>
>> Virgl could save a lot of work converting buffers in the host side
>> between formats if Mesa supported a bunch of other formats when reading
>> pixels.
>>
>> This commit adds cases to handle specific formats so that the values
>> reported by the two calls match more closely the underlying native
>> formats.
>>
>> In GLES is important that IMPLEMENTATION_COLOR_READ_* return the native
>> format and data type because the spec only allows reading with those,
>> besides GL_RGBA or GL_RGBA_INTEGER.
>>
>> Additionally, because virgl currently doesn't implement such
>> conversions, this commit fixes several tests in
>> dEQP-GLES3.functional.fbo.color.clear.*, when using virgl in the guest
>> side.
>>
>> The logic is based on knowledge that is shared with
>> _mesa_format_matches_format_and_type() but we cannot assert that the
>> results match as we don't have all the starting information at both
>> points. So leave the assert out and hope CI comes soon to save us all.
>>
>> v2: * Let R10G10B10A2_UINT fall back to GL_RGBA_INTEGER (Eric Anholt)
>> * Assert with _mesa_format_matches_format_and_type (Eric Anholt)
>>
>> v3: * Remove the assert, as it won't be reliable (Eric Anholt)
>>
>> Signed-off-by: Tomeu Vizoso 
>> ---
>>  src/mesa/main/framebuffer.c | 68 +++--
>>  1 file changed, 43 insertions(+), 25 deletions(-)
>>
>> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
>> index 8e751b453b75..b1a544b5646d 100644
>> --- a/src/mesa/main/framebuffer.c
>> +++ b/src/mesa/main/framebuffer.c
>> @@ -834,18 +834,52 @@ _mesa_get_color_read_format(struct gl_context *ctx,
>> }
>> else {
>>const mesa_format format = fb->_ColorReadBuffer->Format;
>> -  const GLenum data_type = _mesa_get_format_datatype(format);
>> +  GLenum data_type;
>> +  GLuint comps;
>>
>> -  if (format == MESA_FORMAT_B8G8R8A8_UNORM)
>> +  _mesa_uncompressed_format_to_type_and_comps(format, _type, 
>> );
>> +
>> +  switch (format) {
>> +  case MESA_FORMAT_RGBA_UINT8:
>> + return GL_RGBA_INTEGER;
>> +  case MESA_FORMAT_B8G8R8A8_UNORM:
>>   return GL_BGRA;
>> -  else if (format == MESA_FORMAT_B5G6R5_UNORM)
>> +  case MESA_FORMAT_B5G6R5_UNORM:
>> +  case MESA_FORMAT_R11G11B10_FLOAT:
>>   return GL_RGB;
>> -  else if (format == MESA_FORMAT_R_UNORM8)
>> +  case MESA_FORMAT_RG_FLOAT32:
>> +  case MESA_FORMAT_RG_FLOAT16:
>> +  case MESA_FORMAT_R8G8_UNORM:
>> + return GL_RG;
>> +  case MESA_FORMAT_RG_SINT32:
>> +  case MESA_FORMAT_RG_UINT32:
>> +  case MESA_FORMAT_RG_SINT16:
>> +  case MESA_FORMAT_RG_UINT16:
>> +  case MESA_FORMAT_RG_SINT8:
>> +  case MESA_FORMAT_RG_UINT8:
>> + return GL_RG_INTEGER;
>> +  case MESA_FORMAT_R_FLOAT32:
>> +  case MESA_FORMAT_R_FLOAT16:
>> +  case MESA_FORMAT_R_UNORM8:
>>   return GL_RED;
>> +  case MESA_FORMAT_R_SINT32:
>> +  case MESA_FORMAT_R_UINT32:
>> +  case MESA_FORMAT_R_SINT16:
>> +  case MESA_FORMAT_R_UINT16:
>> +  case MESA_FORMAT_R_SINT8:
>> +  case MESA_FORMAT_R_UINT8:
>> + return GL_RED_INTEGER;
>> +  default:
>> + break;
>> +  }
>>
>>switch (data_type) {
>>case GL_UNSIGNED_INT:
>> +  case GL_UNSIGNED_INT_2_10_10_10_REV:
>> +  case GL_UNSIGNED_SHORT:
>
> Formats like MESA_FORMAT_R_UNORM16 return GL_UNSIGNED_SHORT, but they
> should be returning GL_RGBA, right?  Similar for SNORM, and similar for BYTE
> cases.  Maybe _mesa_format_is_integer() could help you here?

You are right that it shouldn't be GL_RGBA_INTEGER as per the current
code, but I think it should be GL_RED and not GL_RGBA.

I will add those Mesa formats to the switch case, as
_mesa_format_is_integer() isn't enough to tell.

Thanks,

Tomeu

>
>>case GL_INT:
>> +  case GL_SHORT:
>> +  case GL_BYTE:
>>   return GL_RGBA_INTEGER;
>>default:
>>   return GL_RGBA;
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] mesa: handle a bunch of formats in IMPLEMENTATION_COLOR_READ_*

2018-05-30 Thread Eric Anholt 
Tomeu Vizoso  writes:

> Virgl could save a lot of work converting buffers in the host side
> between formats if Mesa supported a bunch of other formats when reading
> pixels.
>
> This commit adds cases to handle specific formats so that the values
> reported by the two calls match more closely the underlying native
> formats.
>
> In GLES is important that IMPLEMENTATION_COLOR_READ_* return the native
> format and data type because the spec only allows reading with those,
> besides GL_RGBA or GL_RGBA_INTEGER.
>
> Additionally, because virgl currently doesn't implement such
> conversions, this commit fixes several tests in
> dEQP-GLES3.functional.fbo.color.clear.*, when using virgl in the guest
> side.
>
> The logic is based on knowledge that is shared with
> _mesa_format_matches_format_and_type() but we cannot assert that the
> results match as we don't have all the starting information at both
> points. So leave the assert out and hope CI comes soon to save us all.
>
> v2: * Let R10G10B10A2_UINT fall back to GL_RGBA_INTEGER (Eric Anholt)
> * Assert with _mesa_format_matches_format_and_type (Eric Anholt)
>
> v3: * Remove the assert, as it won't be reliable (Eric Anholt)
>
> Signed-off-by: Tomeu Vizoso 
> ---
>  src/mesa/main/framebuffer.c | 68 +++--
>  1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
> index 8e751b453b75..b1a544b5646d 100644
> --- a/src/mesa/main/framebuffer.c
> +++ b/src/mesa/main/framebuffer.c
> @@ -834,18 +834,52 @@ _mesa_get_color_read_format(struct gl_context *ctx,
> }
> else {
>const mesa_format format = fb->_ColorReadBuffer->Format;
> -  const GLenum data_type = _mesa_get_format_datatype(format);
> +  GLenum data_type;
> +  GLuint comps;
>  
> -  if (format == MESA_FORMAT_B8G8R8A8_UNORM)
> +  _mesa_uncompressed_format_to_type_and_comps(format, _type, 
> );
> +
> +  switch (format) {
> +  case MESA_FORMAT_RGBA_UINT8:
> + return GL_RGBA_INTEGER;
> +  case MESA_FORMAT_B8G8R8A8_UNORM:
>   return GL_BGRA;
> -  else if (format == MESA_FORMAT_B5G6R5_UNORM)
> +  case MESA_FORMAT_B5G6R5_UNORM:
> +  case MESA_FORMAT_R11G11B10_FLOAT:
>   return GL_RGB;
> -  else if (format == MESA_FORMAT_R_UNORM8)
> +  case MESA_FORMAT_RG_FLOAT32:
> +  case MESA_FORMAT_RG_FLOAT16:
> +  case MESA_FORMAT_R8G8_UNORM:
> + return GL_RG;
> +  case MESA_FORMAT_RG_SINT32:
> +  case MESA_FORMAT_RG_UINT32:
> +  case MESA_FORMAT_RG_SINT16:
> +  case MESA_FORMAT_RG_UINT16:
> +  case MESA_FORMAT_RG_SINT8:
> +  case MESA_FORMAT_RG_UINT8:
> + return GL_RG_INTEGER;
> +  case MESA_FORMAT_R_FLOAT32:
> +  case MESA_FORMAT_R_FLOAT16:
> +  case MESA_FORMAT_R_UNORM8:
>   return GL_RED;
> +  case MESA_FORMAT_R_SINT32:
> +  case MESA_FORMAT_R_UINT32:
> +  case MESA_FORMAT_R_SINT16:
> +  case MESA_FORMAT_R_UINT16:
> +  case MESA_FORMAT_R_SINT8:
> +  case MESA_FORMAT_R_UINT8:
> + return GL_RED_INTEGER;
> +  default:
> + break;
> +  }
>  
>switch (data_type) {
>case GL_UNSIGNED_INT:
> +  case GL_UNSIGNED_INT_2_10_10_10_REV:
> +  case GL_UNSIGNED_SHORT:

Formats like MESA_FORMAT_R_UNORM16 return GL_UNSIGNED_SHORT, but they
should be returning GL_RGBA, right?  Similar for SNORM, and similar for BYTE
cases.  Maybe _mesa_format_is_integer() could help you here?

>case GL_INT:
> +  case GL_SHORT:
> +  case GL_BYTE:
>   return GL_RGBA_INTEGER;
>default:
>   return GL_RGBA;



signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] mesa: handle a bunch of formats in IMPLEMENTATION_COLOR_READ_*

2018-05-24 Thread Gurchetan Singh 
Reviewed-by: Gurchetan Singh 



On Wed, May 23, 2018 at 1:36 AM Tomeu Vizoso 
wrote:

> Virgl could save a lot of work converting buffers in the host side
> between formats if Mesa supported a bunch of other formats when reading
> pixels.
>
> This commit adds cases to handle specific formats so that the values
> reported by the two calls match more closely the underlying native
> formats.
>
> In GLES is important that IMPLEMENTATION_COLOR_READ_* return the native
> format and data type because the spec only allows reading with those,
> besides GL_RGBA or GL_RGBA_INTEGER.
>
> Additionally, because virgl currently doesn't implement such
> conversions, this commit fixes several tests in
> dEQP-GLES3.functional.fbo.color.clear.*, when using virgl in the guest
> side.
>
> The logic is based on knowledge that is shared with
> _mesa_format_matches_format_and_type() but we cannot assert that the
> results match as we don't have all the starting information at both
> points. So leave the assert out and hope CI comes soon to save us all.
>
> v2: * Let R10G10B10A2_UINT fall back to GL_RGBA_INTEGER (Eric Anholt)
> * Assert with _mesa_format_matches_format_and_type (Eric Anholt)
>
> v3: * Remove the assert, as it won't be reliable (Eric Anholt)
>
> Signed-off-by: Tomeu Vizoso 
> ---
>  src/mesa/main/framebuffer.c | 68 +++--
>  1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
> index 8e751b453b75..b1a544b5646d 100644
> --- a/src/mesa/main/framebuffer.c
> +++ b/src/mesa/main/framebuffer.c
> @@ -834,18 +834,52 @@ _mesa_get_color_read_format(struct gl_context *ctx,
> }
> else {
>const mesa_format format = fb->_ColorReadBuffer->Format;
> -  const GLenum data_type = _mesa_get_format_datatype(format);
> +  GLenum data_type;
> +  GLuint comps;
>
> -  if (format == MESA_FORMAT_B8G8R8A8_UNORM)
> +  _mesa_uncompressed_format_to_type_and_comps(format, _type,
> );
> +
> +  switch (format) {
> +  case MESA_FORMAT_RGBA_UINT8:
> + return GL_RGBA_INTEGER;
> +  case MESA_FORMAT_B8G8R8A8_UNORM:
>   return GL_BGRA;
> -  else if (format == MESA_FORMAT_B5G6R5_UNORM)
> +  case MESA_FORMAT_B5G6R5_UNORM:
> +  case MESA_FORMAT_R11G11B10_FLOAT:
>   return GL_RGB;
> -  else if (format == MESA_FORMAT_R_UNORM8)
> +  case MESA_FORMAT_RG_FLOAT32:
> +  case MESA_FORMAT_RG_FLOAT16:
> +  case MESA_FORMAT_R8G8_UNORM:
> + return GL_RG;
> +  case MESA_FORMAT_RG_SINT32:
> +  case MESA_FORMAT_RG_UINT32:
> +  case MESA_FORMAT_RG_SINT16:
> +  case MESA_FORMAT_RG_UINT16:
> +  case MESA_FORMAT_RG_SINT8:
> +  case MESA_FORMAT_RG_UINT8:
> + return GL_RG_INTEGER;
> +  case MESA_FORMAT_R_FLOAT32:
> +  case MESA_FORMAT_R_FLOAT16:
> +  case MESA_FORMAT_R_UNORM8:
>   return GL_RED;
> +  case MESA_FORMAT_R_SINT32:
> +  case MESA_FORMAT_R_UINT32:
> +  case MESA_FORMAT_R_SINT16:
> +  case MESA_FORMAT_R_UINT16:
> +  case MESA_FORMAT_R_SINT8:
> +  case MESA_FORMAT_R_UINT8:
> + return GL_RED_INTEGER;
> +  default:
> + break;
> +  }
>
>switch (data_type) {
>case GL_UNSIGNED_INT:
> +  case GL_UNSIGNED_INT_2_10_10_10_REV:
> +  case GL_UNSIGNED_SHORT:
>case GL_INT:
> +  case GL_SHORT:
> +  case GL_BYTE:
>   return GL_RGBA_INTEGER;
>default:
>   return GL_RGBA;
> @@ -882,29 +916,13 @@ _mesa_get_color_read_type(struct gl_context *ctx,
>return GL_NONE;
> }
> else {
> -  const GLenum format = fb->_ColorReadBuffer->Format;
> -  const GLenum data_type = _mesa_get_format_datatype(format);
> -
> -  if (format == MESA_FORMAT_B5G6R5_UNORM)
> - return GL_UNSIGNED_SHORT_5_6_5;
> +  const mesa_format format = fb->_ColorReadBuffer->Format;
> +  GLenum data_type;
> +  GLuint comps;
>
> -  if (format == MESA_FORMAT_B10G10R10A2_UNORM ||
> -  format == MESA_FORMAT_B10G10R10X2_UNORM ||
> -  format == MESA_FORMAT_R10G10B10A2_UNORM ||
> -  format == MESA_FORMAT_R10G10B10X2_UNORM)
> - return GL_UNSIGNED_INT_2_10_10_10_REV;
> +  _mesa_uncompressed_format_to_type_and_comps(format, _type,
> );
>
> -  switch (data_type) {
> -  case GL_SIGNED_NORMALIZED:
> - return GL_BYTE;
> -  case GL_UNSIGNED_INT:
> -  case GL_INT:
> -  case GL_FLOAT:
> - return data_type;
> -  case GL_UNSIGNED_NORMALIZED:
> -  default:
> - return GL_UNSIGNED_BYTE;
> -  }
> +  return data_type;
> }
>  }
>
> --
> 2.17.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>