[Mesa-dev] [PATCH 08/11] mesa: Implement the GL entry points defined by ARB_shader_image_load_store.

[Paul Berry]

On 7 December 2013 17:11, Francisco Jerez <currojerez at riseup.net> wrote:

> Paul Berry <stereotype441 at gmail.com> writes:
>
> > On 24 November 2013 21:00, Francisco Jerez <currojerez at riseup.net>
> wrote:
> >[...]
> >> +
> >> +   case GL_RGBA16UI:
> >> +      return MESA_FORMAT_RGBA_UINT16;
> >> +
> >> +   case GL_RGB10_A2UI:
> >> +      return MESA_FORMAT_ABGR2101010_UINT;
> >>
> >
> > I don't understand the naming conventions of the GL and MESA formats well
> > enough to be sure about this, but I'm troubled by the inconsistency
> between
> > the two case statements above.  In the GL_RGBA16UI case, the MESA format
> > lists the components in the same order as the GL format (RGBA).  But in
> the
> > GL_RGB10_A2UI case, the MESA format lists the components in the opposite
> > order as the GL format (ABGR instead of RGBA).  Unless there are some
> > really counterintuitive naming conventions at work here, it seems like
> > these cases can't both be right.
> >
>
> Yes.  Mesa's formats mix two naming conventions: MESA_FORMAT_XYZW_N
> specifies the XYZW components in memory order, each of N bits, while
> MESA_FORMAT_XYZWNMPQ specifies the XYZW components from MSB to LSB in an
> endianness-dependent manner.  The GL_RGB10_A2UI internal format refers
> to an RGBA_INTEGER format of type UNSIGNED_INT_2_10_10_10_REV, which
> matches the Mesa format above -- See table 8.8 in the GL4.4 spec.
>

Wow, that explains why I never managed to pick up the convention by
osmosis.  Thanks for explaining.

BTW, after reading through format_pack.c, I found some exceptions to what
you said above.  The following formats are always packed by format_pack.c
in reverse memory order, regardless of endianness:

- MESA_FORMAT_RGB888
- MESA_FORMAT_BGR888
- MESA_FORMAT_SRGB8
- MESA_FORMAT_XBGR16161616_UNORM
- MESA_FORMAT_XBGR16161616_SNORM
- MESA_FORMAT_XBGR16161616_FLOAT
- MESA_FORMAT_XBGR32323232_FLOAT

Which means they get packed correctly on little-endian systems but
backwards on big-endian systems.  This seems like it's a bug in
format_pack.c, so it's not relevant to your patch :)
>

> > This can't be right, because u->Layer can be much larger than 6, but the
> > size of the t->Image[] array is 6.  I believe what we need to do instead
> is:
> >
> > (a) for cubemaps, we need to validate that 0 <= u->Layer < 6 before
> > accessing t->Image, otherwise we will access garbage memory.
> >
> > (b) for non-cubemaps (e.g. 2D arrays), all the layers for each miplevel
> are
> > stored in a single gl_texture_image, so we need to access
> > t->Image[0][u->Level], and then check that 0 <= u->Layer < t->Depth.
> >
> > (c) I'm not sure how cubemap arrays are handled by the Mesa front end, so
> > I'm not sure what is correct to do for them.
> >
>
> I've addressed these issues here [1], I can send a v2 to the mailing
> list if you have more suggestions specific to the new version.
>

Personally, I don't need to see a v2.  I looked at what you currently have
on your branch (a801b805b3016eb905028726ea20a3338c4d7662), and it looks
good to me.  It's

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131209/0d804318/attachment.html>