[Mesa-dev] [PATCH 05/29] intel/isl: Add a helper for inverting swizzles

Sun Mar 4 13:10:01 UTC 2018

On March 3, 2018 10:50:43 "Pohjolainen, Topi" <topi.pohjolainen at gmail.com> 
wrote:

> On Thu, Mar 01, 2018 at 03:04:14PM -0800, Jason Ekstrand wrote:
>> On Thu, Mar 1, 2018 at 6:49 AM, Pohjolainen, Topi <
>> topi.pohjolainen at gmail.com> wrote:
>>
>> > On Mon, Feb 26, 2018 at 08:42:42AM -0800, Jason Ekstrand wrote:
>> > > On Mon, Feb 26, 2018 at 6:19 AM, Pohjolainen, Topi <
>> > > topi.pohjolainen at gmail.com> wrote:
>> > >
>> > > > On Fri, Jan 26, 2018 at 05:59:34PM -0800, Jason Ekstrand wrote:
>> > > > > ---
>> > > > >  src/intel/isl/isl.c | 30 ++++++++++++++++++++++++++++++
>> > > > >  src/intel/isl/isl.h |  2 ++
>> > > > >  2 files changed, 32 insertions(+)
>> > > > >
>> > > > > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
>> > > > > index a2d3ae6..420d387 100644
>> > > > > --- a/src/intel/isl/isl.c
>> > > > > +++ b/src/intel/isl/isl.c
>> > > > > @@ -2379,3 +2379,33 @@ isl_swizzle_compose(struct isl_swizzle first,
>> > > > struct isl_swizzle second)
>> > > > >        .a = swizzle_select(first.a, second),
>> > > > >     };
>> > > > >  }
>> > > > > +
>> > > > > +/**
>> > > > > + * Returns a swizzle that is the pseudo-inverse of this swizzle.
>> > > > > + */
>> > > > > +struct isl_swizzle
>> > > > > +isl_swizzle_invert(struct isl_swizzle swizzle)
>> > > > > +{
>> > > > > +   /* Default to zero for channels which do not show up in the
>> > swizzle
>> > > > */
>> > > > > +   enum isl_channel_select chans[4] = {
>> > > > > +      ISL_CHANNEL_SELECT_ZERO,
>> > > > > +      ISL_CHANNEL_SELECT_ZERO,
>> > > > > +      ISL_CHANNEL_SELECT_ZERO,
>> > > > > +      ISL_CHANNEL_SELECT_ZERO,
>> > > > > +   };
>> > > > > +
>> > > > > +   /* We go in ABGR order so that, if there are any duplicates, the
>> > > > first one
>> > > > > +    * is taken if you look at it in RGBA order.  This is what
>> > Haswell
>> > > > hardware
>> > > > > +    * does for render target swizzles.
>> > > > > +    */
>> > > > > +   if ((unsigned)(swizzle.a - ISL_CHANNEL_SELECT_RED) < 4)
>> > > > > +      chans[swizzle.a - ISL_CHANNEL_SELECT_RED] =
>> > > > ISL_CHANNEL_SELECT_ALPHA;
>> > > > > +   if ((unsigned)(swizzle.b - ISL_CHANNEL_SELECT_RED) < 4)
>> > > > > +      chans[swizzle.b - ISL_CHANNEL_SELECT_RED] =
>> > > > ISL_CHANNEL_SELECT_BLUE;
>> > > > > +   if ((unsigned)(swizzle.g - ISL_CHANNEL_SELECT_RED) < 4)
>> > > > > +      chans[swizzle.g - ISL_CHANNEL_SELECT_RED] =
>> > > > ISL_CHANNEL_SELECT_GREEN;
>> > > > > +   if ((unsigned)(swizzle.r - ISL_CHANNEL_SELECT_RED) < 4)
>> > > > > +      chans[swizzle.r - ISL_CHANNEL_SELECT_RED] =
>> > > > ISL_CHANNEL_SELECT_RED;
>> > > > > +
>> > > > > +   return (struct isl_swizzle) { chans[0], chans[1], chans[2],
>> > chans[3]
>> > > > };
>> > > >
>> > > > If given
>> > > >
>> > > >     swizzle == { ISL_CHANNEL_SELECT_RED,
>> > > >                  ISL_CHANNEL_SELECT_GREEN,
>> > > >                  ISL_CHANNEL_SELECT_BLUE,
>> > > >                  ISL_CHANNEL_SELECT_ALPHA },
>> > > >
>> > > > then
>> > > >     chans[ISL_CHANNEL_SELECT_ALPHA - ISL_CHANNEL_SELECT_RED] ==
>> > chans[3] ==
>> > > >     ISL_CHANNEL_SELECT_ALPHA
>> > > >
>> > > > and so on, and the function returns the same swizzle as given?
>> > >
>> > >
>> > > Yes, that is how the subtraction works.
>> >
>> > I was expecting it to "invert" that, i.e., to return ABGR. But okay, if
>> > given
>> > identity swizzle it returns identity.
>> >
>> > In order to understand how it works I thought I read further the series to
>> > find an example - there seems to be one in patch 12 and another in patch
>> > 16.
>> > In case of 16 and destination format B4G4R4A4 the swizzle looks to be BGRA
>> > (looking at anv_formats.c::main_formats).
>> >
>> > In that case we get:
>> >
>> >    chans[ALPHA - RED] = chans[3] = ALPHA
>> >    chans[RED   - RED] = chans[0] = BLUE
>> >    chans[GREEN - RED] = chans[1] = GREEN
>> >    chans[BLUE  - RED] = chans[2] = RED
>> >
>> > and as a swizzle BLUE, GREEN, RED, ALPHA. This is again the same as given.
>> > What am I not understanding?
>> >
>>
>> I think the confusion is what "invert" means.  It doesn't mean we reverse
>> the channels or anything like that.  It's an inverse in the sense that when
>> you compose a swizzle with it's inverse, you get the identity back out.
>> The inverse of BGRA is BGRA because if you apply the BGRA swizzle twice,
>> you get RGBA again.  If you start with ARGB, you get
>>
>> chans[BLUE - RED] = chans[2] = ALPHA
>> chans[GREEN - RED] = chans[1] = BLUE
>> chans[RED - RED] = chans[0] = GREEN
>> chans[ALPHA - RED] = chans[3] = RED
>>
>> This gives an inverse swizzle of GBAR which is certainly a weird swizzle.
>> However, if you apply ARGB and then GBAR, you get back to identity since
>> one is a left rotate and one is a right rotate.  Does that make a bit more
>> sense?
>
> Thanks again for taking the time to explain.

No worries.

> That does makes sense per se.
> But I'm still failing to understand its use in patch 16 - I guess that is
> partly why I had trouble here. I tried to understand the meaning of "inverse"
> via its use.
> Looking patches 6 and 16, I'm seeing anvil giving blorp destination format
> as ISL_FORMAT_A4B4G4R4_UNORM and swizzle as BGRA. In blorp blit I'm thinking
> that blorp considers the destination swizzle and decides that rt write can't
> handle it. Then blorp tells rt write not to try any swizzling, i.e., to
> simply just use the identity swizzle. And instead blorp tweaks the shader to
> swizzle, i.e., to place the color components in BGRA order to the rt write
> payload. Now, what I don't understand is why the destination swizzle needs to
> be inverted.

The "shader channel select" specifies, for each channel in the shader, what 
channel in the texture it corresponds to.  For sources, this does exactly 
what you think.  For destinations, it's a bit tricker because a swizzle 
with duplicates like RGGB would mean that both green and blue would write 
to the green channel and alpha writes to blue. Things are a bit ill-defined 
precisely because the swizzle is inverted.

Another way to think about this is by looking at patch 6.  There NIR 
function for swizzling applies the swizzle to the source to yield the 
destination.  For destinations swizzles, this is backwards because the 
swizzle specifies which channels in the destination get written by a given 
source channel.  We could have two separate NIR functions for source and 
destination swizzles but the destination swizzle function would be 
identical to the source swizzle function with an inverted swizzle.

> In this case it works regardless if the swizzle is inverted or
> not (both are the same as we discussed) and therefore at least this use case
> doesn't help me to understand. In other words, I don't get why the destination
> swizzle needs to be inverted when the swizzle operation itself is just moved
> from data port to the shader.
>
>
> Now, in patch 12 we actually get the example of yours, ARGB becomes GBAR:
>
>  } else if (params->dst.view.format == ISL_FORMAT_A4B4G4R4_UNORM) {
> +      params->dst.view.swizzle =
> +         isl_swizzle_compose(params->dst.view.swizzle,
> +                             ISL_SWIZZLE(ALPHA, RED, GREEN, BLUE));
> +      params->dst.view.format = ISL_FORMAT_B4G4R4A4_UNORM;
>
>
> Here the inverse actually seems to kick in, there are two swizzles back-2-back.
> First the shader does a re-order as GBAR and then hardware uses BGRA instead
> of ABGR.
> I started wondering why we actually do the inverse in blorp as here it would
> explain itself better I think - it is the change of destination format along
> with the swizzle that tells that there are actually two swizzle operations in
> play.
> I suppose I'm still not 100% there as I feel that in case of patch 16 the
> swizzle shouldn't be inverted.

I'm not sure what you mean about patch 16.  Mind elaborating?

> Comparing 12 and 16 actually brought another question: Anvil seems to avoid
> using ISL_FORMAT_B4G4R4A4_UNORM but prefers ISL_FORMAT_A4B4G4R4_UNORM and
> swizzle BGRA instead (table anv_formats.c::main_formats).

Yes, this is because A4B4G4R4 supports rendering on gen9+ and we can't 
blend when the swizzle moves the alpha channel.  In order to get the 
fullest format support, we use A4B4G4R4 at least on gen9 and above.

> In patch 12 we seem to do the exact opposite - avoid using
> ISL_FORMAT_A4B4G4R4_UNORM but use ISL_FORMAT_B4G4R4A4_UNORM and swizzle of
> ARGB. Could you explain this as well?

The A4B4G4R4 format is not supported at all I'm gen7 and is only supported 
for texturing on gen8.  In order to support blitting to it, we have to use 
another format and swizzle.  I suppose we could just disallow A4B4G4R4 on 
gen8 and then we wouldn't need this.  That would allow rendering but not 
blending on gen8.