[Mesa-dev] gallium scaled types

[Jose Fonseca]

----- Original Message -----
> On Fri, Sep 16, 2011 at 5:40 PM, Jose Fonseca <jfonseca at vmware.com>
> wrote:
> > ----- Original Message -----
> >> José,
> >>
> >> I understand what you are trying to tell me, but it's not good
> >> enough
> >> to convince me. I am mainly advocating what maps nicely to my
> >> hardware
> >> and what appears to be a sane long-term solution in my opinion.
> >
> > Marek,
> >
> > I understand that you, like me, you're just trying to advocate what
> > appears to be best.  I think we're both smart people and neither
> > in disagreement just for the sake of it.  I think ultimately that
> > we'll just have to agree on disagreeing and move one with
> > something.  Preferably before this becomes an emotional argument.
> 
> I agree. :)
> 
> >
> >> IMO, the only two good arguments for not adding new formats have
> >> been:
> >> - It would be less work. (can't argue with that)
> >> - Let's not pollute pipe formats with formats that are only used
> >> by a
> >> vertex fetcher. (heh, good one! but then you can no longer claim
> >> that
> >> vertex and texture formats are unified)
> >>
> >> The Radeon drivers don't really have any switch(pipe_format)
> >> statements for vertices and textures. They just generate something
> >> like a "hardware fetch description" directly from
> >> util_format_description. The is_format_supported query is
> >> implemented
> >> in the exact same way - we just examine whether the hardware can
> >> do
> >> what util_format_description describes. There is no table of
> >> supported
> >> formats. You can easily see why I'd like to have the pure ints
> >> somehow
> >> expressed through util_format_description. We still have
> >> switch(pipe_format) statements for colorbuffers though.
> >
> > I confess I'm confused now: until now I thought you wanted to add
> > new pipe_format enums -- precisely to put in switch/case
> > statements --, but it seems that what you really want is extending
> > the struct util_format_description.  Which is a substantially
> > different conversation.
> >
> > Before I comment any further, could you succinctly describe,
> > exactly how you envision to extend util_format_description to
> > express what you need to know?
> >
> > If you could point these functions in the radeon code it would be
> > good too.
> >
> > Jose
> >
> >
> 
> This change would be best to describe a vertex and texture fetch as
> implemented by Radeons:
> 
> diff --git a/src/gallium/auxiliary/util/u_format.h
> b/src/gallium/auxiliary/util/u_format.h
> index 2eb3e1b..8c4d67f 100644
> --- a/src/gallium/auxiliary/util/u_format.h
> +++ b/src/gallium/auxiliary/util/u_format.h
> @@ -91,10 +91,11 @@ struct util_format_block
> 
>  enum util_format_type {
>     UTIL_FORMAT_TYPE_VOID = 0,
> -   UTIL_FORMAT_TYPE_UNSIGNED = 1,
> -   UTIL_FORMAT_TYPE_SIGNED = 2,
> -   UTIL_FORMAT_TYPE_FIXED = 3,
> -   UTIL_FORMAT_TYPE_FLOAT = 4
> +   UTIL_FORMAT_TYPE_FLOAT = 1,
> +   UTIL_FORMAT_TYPE_INT = 2,     /* signed/unsigned */
> +   UTIL_FORMAT_TYPE_NORM = 3,    /* signed/unsigned */
> +   UTIL_FORMAT_TYPE_SCALED = 4,  /* signed/unsigned */
> +   UTIL_FORMAT_TYPE_FIXED = 5,
>  };
>
> @@ -121,7 +122,7 @@ enum util_format_colorspace {
>  struct util_format_channel_description
>  {
>     unsigned type:6;        /**< UTIL_FORMAT_TYPE_x */
> -   unsigned normalized:1;
> +   unsigned is_signed:1;
>     unsigned size:9;        /**< bits per channel */
>  };

Moving the sign out of util_format_type into util_format_channel_description is fine.  Like NORM/FIXED can be done either way.  (For example, llvmpipe's struct lp_type has them all in 1 bit flags).

But fundamentally you're proposing introducing a new type, SCALED, into util_format_type.

> Of course, Translate doesn't need any of this. Not only does
> Translate
> know the input format, it also knows the output format. So it
> actually
> knows 2 pipe formats for one fetch-and-store operation. If you have 2
> pipe formats, it's easy to figure out the best mapping between them.
> 
> On the other hand, the vertex (or texture) fetch instruction, which
> is
> also a fetch-and-store operation, only knows the input format. The
> output is the untyped shader register. And we need to know whether
> it's float or int. I think this matters to softpipe and llvmpipe as
> well.

Agreed.
 
> Now you can take a look at the function r600_vertex_data_type in
> r600_asm.c:2020. It's a good example of what is going on. At the end
> of the function, there is this piece of code:
> 
> 	if (desc->channel[i].normalized) {
> 		*num_format = 0;
> 	} else {
> 		*num_format = 2;
> 	}
> 
> 0 means NORM, 2 means SCALED. And guess what... 1 means INT. I think
> "num_format" stands for "numeric format". The function which is
> called
> in is_format_supported is r600_is_vertex_format_supported in
> r600_formats.h:84.

I got the picture.

I understand that having the INT vs SCALED flag in desc would be nice. On the other hand, it doesn't look like the as_float flag to r600_vertex_data_type as an additional parameter would force a big rewrite.

> Note that r600 might be able to support SCALED textures too (I know I
> had said the opposite, but I am not sure now after I read the docs
> again). We only know SCALED doesn't work for 32 bits per channel,
> same
> as with vertices. 8 bits and 16 bits per-channel SCALED textures
> might
> work, but nobody tried that yet.
> 
> So now we are back to the discussion about the convert-to-float flag
> in pipe_vertex_element and is_format_supported. I am not saying it's
> wrong, I am just saying it's not clean. Either way, I will respect
> the
> final decision of the implementers of this functionality.

As I was playing out the changes that you suggest to u_format and llvmpipe in my head, and realized one issue I hadn't till now.


Imagine format conversion use cases like translate, u_blit, stretch blitters, llvmpipe, where we generate complex state (x86 assembly, blit shaders, LLVM, IR) that deal with , and we put the source format in a key to a hash table to avoid regenerating code.

  struct {
      enum pipe_format src_format;
      enum pipe_format dst_format;
      
      boolean stretch;
      int filtering_mode;
      ...
  };

Having different enums for PIPE_xxx_INT and PIPE_xxx_SCALED will cause unnecessary cache misses, because for these use cases INT/SCALED that are one and only.

And the only solution to avoid this is to have a canonizing mapping function, such as:

   enum pipe_format
   get_canonical_format(enum pipe_format) {
   case PIPE_FORMAT_FOO_SINT:
   case PIPE_FORMAT_FOO_SSCALED:
       return PIPE_FORMAT_FOO_SINT
   case PIPE_FORMAT_FOO_UINT:
   case PIPE_FORMAT_FOO_USCALED:
       return PIPE_FORMAT_FOO_UINT
   ....
   }

which is a bit painful.

Sure, GL, D3D, and a lot of hardware does not support integer -> float conversion in several places on the graphics pipeline, but modules such as translate and u_format , and software renderers are not bound by those limitations, and these cases can & do happen.


So, while I agree that distinguishing SCALED vs INT indeed simplify vertex translation, I think it would impair the usefulness of pipe_format as derived state cache lookup key, one of its important uses today.




But ignore all I wrote and said till now.  If I simply imagine the total work necessary to plumb a flag into the vertex fetch paths, vs the total work necessary to update for the INT/SCALED split of pipe_formats and u_format, the latter seems much more grunt work to me.



Anyway, you guys are doing it, and it doesn't seem like Keith or Brian have an opinion, so as far as I'm concerned, feel free to decide among yourselves and start coding away in a branch.

I only have two requirements:

- there must be no regression on llvmpipe upon merging.  (svga is equally important, but its capabilities in terms of formats are quite constrained so I doubt there will be any issues. llvmpipe, on the other hand, supports pretty most of the formats out there, most of them through LLVM IR, and the remainder trhough u_format's code).

- u_format's current functionality must not degrade -- i.e., don't remove the ability to pack/unpack/fetch _any_ integer format from/to 4 x floats or 4 x unorm8, just because GL/D3D doesn't allow it.

That's all!


And this is all I have to say until you guys decide what you wanna do.  After you decide, if you happen to have any implementation doubt on one of the modules I co-authored, I'll be glad to comment again.

I'm not writing arguments any more (*).



Jose

(*) Hopefully it won't be like the last time I said this.  I guess last time was what we could call a "floating" promise.  But this time is a "pure" promise.  Pun intended. :D