[Mesa-dev] [PATCH 1/2] anv: allow blue in alpha component in swizzle for render

[Nanley Chery]

On Thu, Feb 09, 2017 at 09:41:11AM +0100, Juan A. Suarez Romero wrote:
> On Wed, 2017-02-08 at 10:53 -0800, Nanley Chery wrote:
> > On Wed, Feb 08, 2017 at 06:42:44PM +0100, Juan A. Suarez Romero wrote:
> > > On Wed, 2017-02-08 at 09:27 -0800, Nanley Chery wrote:
> > > > On Wed, Feb 08, 2017 at 01:31:54PM +0100, Juan A. Suarez Romero wrote:
> > > > > In pre-Broadwell devices, as B4G4R4A4 is not supported natively, we
> > > > > workaround it by using a format with a more complex swizzle, that uses
> > > > > blue in alpha component.
> > > > > 
> > > > > Signed-off-by: Juan A. Suarez Romero <jasuarez at igalia.com>
> > > > > ---
> > > > >  src/intel/vulkan/anv_private.h | 14 ++++++++++----
> > > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> > > > > index 51e85c7..e9bf13c 100644
> > > > > --- a/src/intel/vulkan/anv_private.h
> > > > > +++ b/src/intel/vulkan/anv_private.h
> > > > > @@ -1559,13 +1559,19 @@ static inline struct isl_swizzle
> > > > >  anv_swizzle_for_render(struct isl_swizzle swizzle)
> > > > >  {
> > > > >     /* Sometimes the swizzle will have alpha map to one.  We do this to fake
> > > > > -    * RGB as RGBA for texturing
> > > > > +    * RGB as RGBA for texturing.
> > > > > +    *
> > > > > +    * It can have also alpha to blue. This happens to workaround support for
> > > > > +    * B4G4R4A4 in gen < 8 devices, as this not supported natively.
> > > > >      */
> > > > >     assert(swizzle.a == ISL_CHANNEL_SELECT_ONE ||
> > > > > -          swizzle.a == ISL_CHANNEL_SELECT_ALPHA);
> > > > > +          swizzle.a == ISL_CHANNEL_SELECT_ALPHA ||
> > > > > +          swizzle.a == ISL_CHANNEL_SELECT_BLUE);
> > > > >  
> > > > > -   /* But it doesn't matter what we render to that channel */
> > > > > -   swizzle.a = ISL_CHANNEL_SELECT_ALPHA;
> > > > > +   /* But it doesn't matter what we render to that channel, except for the
> > > > > +    * B4G4R4A4 workaround */
> > > > 
> > > > I'm finding this commit message a tad confusing. Would you agree that 
> > > > replacing it with the following would be better?
> > > > 
> > > > For RGB formats that have alpha mapped to one, we must remap the channel
> > > > to alpha to satisfy the BDW+ surface state restriction.
> > > > 
> > > 
> > > Uhm... I don't think so. That is what the code was doing so far.
> > > 
> > > The commit just takes in account that alpha can be mapped to blue too.
> > > And in this case, we keep the blue.
> > > 
> > 
> > Oh, sorry, I meant code comment, not commit message. That would
> > definitely not work as a commit message. Thoughts?
> 
> 
> Yes, it works for me as code comment. Thanks!
> 

Great!

> 
> With that change, can I consider you grant R-b?
> 

Yes, with that change, this patch is
Reviewed-by: Nanley Chery <nanley.g.chery at intel.com>

> 	J.A.
> 
> > > 
> > > > Aside from the comment, this patch looks good to me.
> > > > 
> > > > > +   if (swizzle.a == ISL_CHANNEL_SELECT_ONE)
> > > > > +      swizzle.a = ISL_CHANNEL_SELECT_ALPHA;
> > > > >  
> > > > >     return swizzle;
> > > > >  }
> > > > > -- 
> > > > > 2.9.3
> > > > > 
> > > > > _______________________________________________
> > > > > mesa-dev mailing list
> > > > > mesa-dev at lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > > > 
> > > > 
> > 
> >