[Mesa-dev] [PATCH] i965: Fix clear code for ignoring colormask for XRGB formats on Gen9+.

[Pohjolainen, Topi]

On Thu, Apr 21, 2016 at 02:09:41PM -0700, Kenneth Graunke wrote:
> On Thursday, April 21, 2016 10:00:57 AM PDT Iago Toral wrote:
> > On Wed, 2016-04-20 at 18:38 -0700, Kenneth Graunke wrote:
> > > In commit cda886a4851ab767fba40e8474d6fa8190347e4f, Neil made us stop
> > > advertising RGBX formats on Gen9+, as the hardware apparently no longer
> > > has working fast clear support for those formats.  Instead, we just
> > > fall back to RGBA formats, and use SCS to override alpha to 1.0.
> > > 
> > > This is fine, but had one unintended side effect: it made us fall back
> > > to slow clears when the color mask disables alpha.  Normally, we ignore
> > > the color mask for non-existent channels.  This includes alpha for XRGB
> > > formats as writing garbage to the X channel is harmless.  But, now that
> > > we use RGBA, we think there's a real alpha channel, and can't do the
> > > optimization.
> > > 
> > > To hack around this, check if _BaseFormat is GL_RGB and ignore alpha.
> > > 
> > > Improves WebGL Aquarium performance on Skylake GT3e by about 50%
> > > by letting it use repclears instead of slow clears.
> > > 
> > > Cc: Ben Widawsky <ben at bwidawsk.net>
> > > Cc: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > Cc: mesa-stable at lists.freedesktop.org
> > > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/
> drivers/dri/i965/brw_meta_fast_clear.c
> > > index 1fb5dc8..5d89294 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > > +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > > @@ -645,6 +645,7 @@ brw_meta_fast_clear(struct brw_context *brw, struct 
> gl_framebuffer *fb,
> > >        GLubyte *color_mask = ctx->Color.ColorMask[buf];
> > >        for (int i = 0; i < 4; i++) {
> > >           if (_mesa_format_has_color_component(irb->mt->format, i) &&
> > > +             !(i == 3 && irb->Base.Base._BaseFormat == GL_RGB) &&
> > 
> > I was wondering if we could generalize the fix so we just ignore any
> > channels that are not present in the base format. Something like this:
> > 
> > i <= _mesa_base_format_component_count(irb->Base.Base._BaseFormat)
> > 
> > Basically, we do the same check we do against the mesa format, but with
> > the base format.
> > 
> > What do you think?
> > 
> > Either way,
> > Reviewed-by: Iago Toral Quiroga <itoral at igalia.com>
> 
> That seems like a good idea - I figured there had to be a function like
> _mesa_base_format_component_count(), but I didn't find it when writing
> the patch.
> 
> We'd need a little more code - _mesa_base_format_component_count() can
> return -1 in some cases, so we'd need to override it to 4.  It's doable.
> 
> As far as I know, the only cases where this will be useful is XRGB and
> maybe luminance/intensity.  For luminance/intensity, I think the right
> solution is to use RED formats.  So, for all practical purposes, this
> should be good enough.
> 
> Also, Topi's just about on the verge of replacing the meta fast clear
> code with BLORP clears.  So this is all going away - I mostly wanted
> a quick fix that could be backported to stable and the ChromeOS tree.
> 
> So I think I'll go with the patch as is, as it's tested, and we can
> try and do this more generally in the new BLORP framework.

After discussing with Ken, I wrote something of this sort:

+static bool
+set_write_disables(const struct intel_renderbuffer *irb, 
+                   const GLubyte *color_mask, bool *color_write_disable)
+{
+   /* Format information in the renderbuffer represents the requirements
+    * given by the client. There are cases where the backing miptree uses,
+    * for example, RGBA to represent RGBX. Since the client is only expecting
+    * RGB we can treat alpha as not used and write whatever we like into it.
+    */
+   const GLenum base_format = irb->Base.Base._BaseFormat;
+   const mesa_format format = irb->mt->format;
+   const unsigned components = _mesa_base_format_component_count(base_format);
+   bool disables = false;
+
+   for (unsigned i = 0; i < components; i++) {
+     if (_mesa_format_has_color_component(format, i) && !color_mask[i]) {
+         color_write_disable[i] = true;
+         disables = true;
+      }
+   }
+
+   return disables;
+}
+