[Mesa-dev] [PATCH] st/mesa: handle SNORM formats in generic CopyPixels path

[Jose Fonseca]

----- Original Message -----
> ---
>  src/gallium/auxiliary/util/u_format.c     | 14 ++++++++++++++
>  src/gallium/auxiliary/util/u_format.h     |  3 +++
>  src/mesa/state_tracker/st_cb_drawpixels.c |  6 ++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/src/gallium/auxiliary/util/u_format.c
> b/src/gallium/auxiliary/util/u_format.c
> index 9bdc2ea..b70c108 100644
> --- a/src/gallium/auxiliary/util/u_format.c
> +++ b/src/gallium/auxiliary/util/u_format.c
> @@ -131,6 +131,20 @@ util_format_is_pure_uint(enum pipe_format format)
>     return (desc->channel[i].type == UTIL_FORMAT_TYPE_UNSIGNED &&
>     desc->channel[i].pure_integer) ? TRUE : FALSE;
>  }
>  
> +boolean
> +util_format_is_snorm(enum pipe_format format)
> +{
> +   const struct util_format_description *desc =
> util_format_description(format);
> +   int i;
> +
> +   i = util_format_get_first_non_void_channel(format);
> +   if (i == -1)
> +      return FALSE;
> +
> +   return desc->channel[i].type == UTIL_FORMAT_TYPE_SIGNED &&
> +          !desc->channel[i].pure_integer &&
> +          desc->channel[i].normalized;
> +}

This will give wrong results for mixed formats -- containing a mixture of SIGNED and UNSIGNED normalized types. You can avoid that by adding this statement

  if (desc->is_mixed) return FALSE at the start.

I think a comment would be good too -- something like "Returns true if all non-void channels are normalized signed."

This is not your fault, but it is disappointing to see the proliferation of redundant util_format_description() calls in these helpers. util_format_description is not cost free, and st_CopyPixels and friends keep calling it over and over again. I don't think it is hard to call util_format_description() once, and then pass the format_desc pointers around, but once we start on the other route is hard to back out. But as they say -- when you're in a hole, the first thing to do is stop digging --, so I think we should just stop adding new helper functions to util u_format that take enum format instead of util_format_description.

Jose

>  
>  boolean
>  util_format_is_luminance_alpha(enum pipe_format format)
> diff --git a/src/gallium/auxiliary/util/u_format.h
> b/src/gallium/auxiliary/util/u_format.h
> index 4cace6a..ccb7f92 100644
> --- a/src/gallium/auxiliary/util/u_format.h
> +++ b/src/gallium/auxiliary/util/u_format.h
> @@ -622,6 +622,9 @@ util_format_is_pure_sint(enum pipe_format format);
>  boolean
>  util_format_is_pure_uint(enum pipe_format format);
>  
> +boolean
> +util_format_is_snorm(enum pipe_format format);
> +
>  /**
>   * Check if the src format can be blitted to the destination format with
>   * a simple memcpy.  For example, blitting from RGBA to RGBx is OK, but not
> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c
> b/src/mesa/state_tracker/st_cb_drawpixels.c
> index 38563d5..0b5198a 100644
> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
> @@ -1549,6 +1549,7 @@ st_CopyPixels(struct gl_context *ctx, GLint srcx, GLint
> srcy,
>  
>     if (!screen->is_format_supported(screen, srcFormat, st->internal_target,
>     0,
>                                      srcBind)) {
> +      /* srcFormat is non-renderable. Find a compatible renderable format.
> */
>        if (type == GL_DEPTH) {
>           srcFormat = st_choose_format(st, GL_DEPTH_COMPONENT, GL_NONE,
>                                        GL_NONE, st->internal_target, 0,
> @@ -1572,6 +1573,11 @@ st_CopyPixels(struct gl_context *ctx, GLint srcx,
> GLint srcy,
>                                           GL_NONE, st->internal_target, 0,
>                                           srcBind, FALSE);
>           }
> +         else if (util_format_is_snorm(srcFormat)) {
> +            srcFormat = st_choose_format(st, GL_RGBA16_SNORM, GL_NONE,
> +                                         GL_NONE, st->internal_target, 0,
> +                                         srcBind, FALSE);
> +         }
>           else {
>              srcFormat = st_choose_format(st, GL_RGBA, GL_NONE,
>                                           GL_NONE, st->internal_target, 0,