[Mesa-dev] [PATCH] mesa: Match MESA_FORMAT_B5G6R5 for a shallow pixel format of GL_RGB

[Chris Wilson]

On Wed, Sep 09, 2015 at 12:09:40PM +0200, Erik Faye-Lund wrote:
> On Wed, Sep 9, 2015 at 11:25 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > On Wed, Sep 09, 2015 at 11:11:59AM +0200, Erik Faye-Lund wrote:
> >> On Thu, Sep 3, 2015 at 6:05 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >> > If the user supplies a pixel format of GL_RGB + GL_UNSIGNED_SHORT_5_6_5
> >> > and specifies a generic unsized GL_RGB internal format, match that to a
> >> > texture format of MESA_FORMAT_B5G6R5 if supported by the hardware.
> >> >
> >> > Noticed while playing with mesa-demos/teximage:
> >> >
> >> >   TexImage(RGB/565 256 x 256): 79.8 images/sec, 10.0 MB/sec
> >> >   TexSubImage(RGB/565 256 x 256): 3804.9 images/sec, 475.6 MB/sec
> >> >   GetTexImage(RGB/565 256 x 256): 99.5 images/sec, 12.4 MB/sec
> >> >
> >> > becomes
> >> >
> >> >   TexImage(RGB/565 256 x 256): 3439.1 images/sec, 429.9 MB/sec
> >> >   TexSubImage(RGB/565 256 x 256): 3744.1 images/sec, 468.0 MB/sec
> >> >   GetTexImage(RGB/565 256 x 256): 4713.5 images/sec, 589.2 MB/sec
> >> >
> >> > on a puny Baytrail which is still far from what it is capable of. The
> >> > reason for the disparity is that the teximage demo uses a busy texture
> >> > which is performs an accelerated pixel conversion from the user's B5G6R5
> >> > into the native X8B8G8R8. After the patch, no conversion is required
> >> > allowing use of the blitter and memcpy fast paths.
> >> >
> >> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >> > ---
> >> >  src/mesa/main/texformat.c | 2 ++
> >> >  1 file changed, 2 insertions(+)
> >> >
> >> > diff --git a/src/mesa/main/texformat.c b/src/mesa/main/texformat.c
> >> > index fd9f335..866c7b3 100644
> >> > --- a/src/mesa/main/texformat.c
> >> > +++ b/src/mesa/main/texformat.c
> >> > @@ -114,6 +114,8 @@ _mesa_choose_tex_format(struct gl_context *ctx, GLenum target,
> >> >     case GL_RGB:
> >> >        if (type == GL_UNSIGNED_INT_2_10_10_10_REV) {
> >> >           RETURN_IF_SUPPORTED(MESA_FORMAT_B10G10R10A2_UNORM);
> >> > +      } else if (type == GL_UNSIGNED_SHORT_5_6_5) {
> >> > +         RETURN_IF_SUPPORTED(MESA_FORMAT_B5G6R5_UNORM);
> >>
> >> Shouldn't this be MESA_FORMAT_R5G6B5_UNORM? AFAICT, the reason for
> >> using BGR above, is the _REV suffix on the type...
> >
> > No, it's the first line that's "wrong" since the B10G10R10A2 matches
> > GL_BGRA + GL_UNSIGNED_INT_2_10_10_10_REV, GL_RGB implies that the
> > incoming data only has 3 channels (no alpha at all).
> 
> Good point about the alpha channel. It sounds like it should be
> changed to MESA_FORMAT_B10G10R10X2_UNORM instead.
> 
> But according to src/mesa/main/format_pack.c's
> pack_ubyte_b10g10r10a2_unorm(), mesa's B10G10R10A2 corresponds to
> OpenGL's UNSIGNED_INT_2_10_10_10_REV. So I think it matches GL_RGBA,
> not GL_BGRA. The latter would mean another swizzle, AFAICT.

The mapping is (_mesa_format_from_format_and_type):

   case GL_UNSIGNED_INT_2_10_10_10_REV:
      if (format == GL_RGB)
         return MESA_FORMAT_R10G10B10X2_UNORM;
      if (format == GL_RGBA)
         return MESA_FORMAT_R10G10B10A2_UNORM;
      else if (format == GL_RGBA_INTEGER)
         return MESA_FORMAT_R10G10B10A2_UINT;
      else if (format == GL_BGRA)
         return MESA_FORMAT_B10G10R10A2_UNORM;
      else if (format == GL_BGRA_INTEGER)
         return MESA_FORMAT_B10G10R10A2_UINT;
      break;

The trick is that the packed formats are written as lsb first (or it may
just be native and my lsb bias is showing).

> > i965 know how to do B5G6R5 and not R5G6B5, but for completeness we could
> > also add RETURN_IF_SUPPORTED(MESA_FORMAT_R5G6B5_UNORM);
> 
> I think intel-specific hacks (like preferring B5G6R5 over R5G6B5)
> shouldn't leak into _mesa_choose_tex_format(),

Hah, did you look at _mesa_choose_tex_format()? :)
I sent an another patch to do the hardware agnostic unswizzled conversions.

> So I think it'd be a
> good move to add "RETURN_IF_SUPPORTED(MESA_FORMAT_R5G6B5_UNORM);"
> before the latter return.

Not quite, because the mapping for 565 is:

   case GL_UNSIGNED_SHORT_5_6_5:
      if (format == GL_RGB)
         return MESA_FORMAT_B5G6R5_UNORM;
      else if (format == GL_BGR)
         return MESA_FORMAT_R5G6B5_UNORM;
      else if (format == GL_RGB_INTEGER)
         return MESA_FORMAT_B5G6R5_UINT;
      break;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre