On March 3, 2018 10:50:43 "Pohjolainen, Topi" <topi.pohjolai...@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.pohjolai...@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.pohjolai...@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.


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to