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

[Jose Fonseca]

Yes, that's a very good idea.  The implementation would be fast, furthermore the implementation body would be small, so it could be an inline function in u_format.h, therefore allowing the compiler to coalesce multiple calls too.

It implies that the u_format_table.py script needs to parse src/gallium/include/pipe/p_format.h to obtain the actual values of PIPE_FORMAT enums, so it knows which format goes on which entry of the table, but that's certainly doable.

Fair enough. Ignore my comment about util_format_description() for now, and we'll do this in a follow on change.

Jose

----- Original Message -----
> Wouldn't it be easier to just make util_format_description table-based
> and inline instead of switch-based? Something like:
> 
> return format < PIPE_FORMAT_COUNT ? table[format] : NULL;
> 
> Marek
> 
> On Mon, Jun 24, 2013 at 10:12 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
> >
> >
> > ----- 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,
>