[Mesa-dev] classic driver cleanup: ChooseTexFormat

[Eric Anholt]

On Sun, 26 Sep 2010 08:12:59 +1000, Dave Airlie <airlied at gmail.com> wrote:
> On Sun, Sep 26, 2010 at 5:36 AM, Brian Paul <brian.e.paul at gmail.com> wrote:
> > On Fri, Sep 24, 2010 at 6:08 PM, Eric Anholt <eric at anholt.net> wrote:
> >> One of the uglier bits of the classic drivers, in my opinion, is the
> >> ChooseTexFormat hook.  Most drivers are trying to get to a similar set
> >> of formats, and using similar fallbacks for unavailable formats.
> >> Worse, they need to cleverly choose formats based on the incoming
> >> type/format to avoid having to munge the data on the way in if
> >> possible, and everyone needs to work those out.  So, rather than
> >> having each driver have these smarts, move it to the core.
> >>
> >> There are still some rough edges.  There are not enough fallbacks yet
> >> to cover everything (see i810 for example), so drivers with a more
> >> restricted set of supported formats may need to add more lines to
> >> _mesa_choose_tex_format.  Also, the endianness-dependent choice of
> >> formats in texmem.c and radeon's texturing isn't merged -- I knew I'd
> >> mess it up somehow if I did it on my own, so I'm leaving radeon for
> >> someone with big-endian to finish off.
> >>
> >> Any complaints, or should I go ahead and merge this?
> >
> > In general this is a good clean-up.  A couple comments.
> >
> > I see that you're merging the various format selections that the
> > drivers do back into core Mesa one driver at a time.  I guess that
> > would eventually cover all cases but there'll still be quite a few
> > holes.  For example, if a driver requests a GL_LUMINANCE internal
> > format but the driver has no native luminance texture formats, any
> > RGBA format will do as a fallback.  So after trying MESA_FORMAT_L8,
> > MESA_FORMAT_AL88, _AL88_REV etc., the various MESA_FORMAT_RGBA
> > permutations should be tried.
> >
> > If you take a look at the st/mesa state tracker, a helper function
> > such as default_rgba_format() would help to fill in the holes.  Also
> > note how the Z/stencil formats are handled there.
> >
> > Next, I think it's still worthwhile to keep using the user's
> > format/type info to help choose texture formats to try to hit the fast
> > paths in texstore.c for texture copying.
> >
> > Lastly, I'd like to see ctx->texture_format_supported renamed
> > ctx->TextureFormatSupported to follow existing naming conventions.
> >
> 
> Another area we had problems with in classic radeon, is picking
> formats for texture that can't be rendered to, though they do hit
> texture fast paths.
> 
> So when something like clutter wants to render to a texture later, it
> gets an incomplete FBO state (or due to bugs in radeon, we just
> misrender), and it has no real way to avoid this happening.
> 
> I think reducing the number of texture formats the driver can pick
> might help, but could also create speed regressions in streaming
> texture apps perhaps.

Just came back to review the comments since I'm looking at merging this.
Yes, I think classic radeon should stop exposing those texture formats
not supported for rendering to by hardware (we're talking about the
alternatives to ARGB8888 here, I think, or at least that's what I'm
talking about).  It's not like people are uploading a lot of ABGR8888
data that it's so important to avoid a performance penalty for.

(Actually, I think we might be screwing up entirely by doing these
tricks -- while we're allowed to choose internal component sizes based
on any parameter of teximage but no other state, but texture
completeness only requires matching internalformat, so what happens when
someone uploads ARGB8888 type/format for level 0 and ARGB4444
type/format to level 1 both with internalformat of GL_RGBA, and we then
try to validate the texture.  Ouch.)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110104/25e0a646/attachment-0001.pgp>