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

[Jose Fonseca]

----- Original Message -----
> On Mon, Jun 24, 2013 at 6:12 PM, 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.
> 
> I hestiate to say this as no doubt I'll be wrong, but if they are all
> inline helpers, the compiler should figure it out and collapse the
> lookups. Of course I believe compilers should do lots of things...

It's not totally wrong what you say.

Ordinarily, a C compiler has no way to know that each call util_format_description() with same arguments the same result.

That is, it has no way to know that that util_format_description doesn't refer and modify other global state like:

  const struct util_format_description *
  util_format_description(format) {
    static const struct util_format_description * last_format_description = &my_table;
    return ++last_format_desciption;
  }

Unless,

 - unless the compiler is using whole-program-optimzation / link-time-optimization
 - the implementation of util_format_description() is a header -- but that will make compilation time slow.

 - there is some attribute to tell the compiler that a function has no side effects.

And I checked  http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html and apparently there is actually some gcc attributes that could help here:

-- __attribute__((const))

-- __attribute__((pure))

But support with other compilers varies -- http://stackoverflow.com/questions/2798188/pure-const-function-attributes-in-different-compilers . declspec(noalias) comes close,  http://msdn.microsoft.com/en-us/library/k649tyc7.aspx , but not sure if it works. It might be worth a try though.


Jose