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

Jason Ekstrand jason at jlekstrand.net
Mon Mar 5 15:15:35 UTC 2018


On March 5, 2018 00:50:26 "Pohjolainen, Topi" <topi.pohjolainen at gmail.com> 
wrote:

> On Sun, Mar 04, 2018 at 07:10:01AM -0600, Jason Ekstrand wrote:
>> 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.
>
> I need to digest this a little more...
>
>>
>> >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?
>
> I just mean the aforementioned case of format,swizzle pair of A4B4G4R4,BGRA.
> Patch 16 enables blorp blits for it.
>
>>
>> >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.
>
> Ok, that makes more sense. However, in anv_formats.c::anv_get_format_plane()
> there is:
>
>    /* The B4G4R4A4 format isn't available prior to Broadwell so we have to fall
>     * back to a format with a more complex swizzle.
>     */
>    if (vk_format == VK_FORMAT_B4G4R4A4_UNORM_PACK16 && devinfo->gen < 8) {
>       plane_format.isl_format = ISL_FORMAT_B4G4R4A4_UNORM;
>       plane_format.swizzle = ISL_SWIZZLE(GREEN, RED, ALPHA, BLUE);
>    }
>
> Shouldn't that rather say that "...A4B4G4R4 format isn't available..."? The
> code block specifically removes the workaround of using A4B4G4R4 instead of
> B4G4R4A4 introduced in anv_formats::main_formats:
>
> swiz_fmt1(VK_FORMAT_B4G4R4A4_UNORM_PACK16, ISL_FORMAT_A4B4G4R4_UNORM, BGRA),

I think what I said in the comment is exactly correct of you prepend 
VK_FORMAT_ but backwards of you prepend ISL_FORMAT_. That's horrifically 
confusing and I should fix it.




More information about the mesa-dev mailing list