[Mesa-dev] [PATCH 05/10] i965/meta/gen9: Individually fast clear color attachments

[Matt Turner]

On Tue, Nov 3, 2015 at 4:46 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Tue, Oct 13, 2015 at 09:14:29PM -0700, Matt Turner wrote:
>> On Tue, Oct 13, 2015 at 9:12 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> > On Tue, Oct 13, 2015 at 8:50 PM, Ben Widawsky
>> > <benjamin.widawsky at intel.com> wrote:
>> >> The impetus for this patch comes from a seemingly benign statement within the
>> >> spec (quoted within the patch). For me, this patch was at some point critical
>> >> for getting stable piglit results (though this did not seem to be the case on a
>> >> branch Chad was working on).
>> >>
>> >> It is very important for clearing multiple color buffer attachments and can be
>> >> observed in the following piglit tests:
>> >> spec/arb_framebuffer_object/fbo-drawbuffers-none glclear
>> >> spec/ext_framebuffer_multisample/blit-multiple-render-targets 0
>> >>
>> >> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> >> ---
>> >>  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c | 97 +++++++++++++++++++++----
>> >>  1 file changed, 84 insertions(+), 13 deletions(-)
>> >>
>> >> 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 7bf52f0..9e6711e 100644
>> >> --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
>> >> +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
>> >> @@ -427,6 +427,74 @@ use_rectlist(struct brw_context *brw, bool enable)
>> >>     brw->ctx.NewDriverState |= BRW_NEW_FRAGMENT_PROGRAM;
>> >>  }
>> >>
>> >> +/**
>> >> + * Individually fast clear each color buffer attachment. On previous gens this
>> >> + * isn't required. The motivation for this comes from one line (which seems to
>> >> + * be specific to SKL+). The list item is in section titled _MCS Buffer for
>> >> + * Render Target(s)_
>> >> + *
>> >> + *   "Since only one RT is bound with a clear pass, only one RT can be cleared
>> >> + *   at a time. To clear multiple RTs, multiple clear passes are required."
>> >> + *
>> >> + * The code follows the same idea as the resolve code which creates a fake FBO
>> >> + * to avoid interfering with too much of the GL state.
>> >> + */
>> >> +static void
>> >> +fast_clear_attachments(struct brw_context *brw,
>> >> +                       struct gl_framebuffer *fb,
>> >> +                       uint32_t fast_clear_buffers,
>> >> +                       struct rect fast_clear_rect)
>> >> +{
>> >> +   assert(brw->gen >= 9);
>> >> +   struct gl_context *ctx = &brw->ctx;
>> >> +
>> >> +   GLuint old_fb = ctx->DrawBuffer->Name;
>> >> +
>> >> +   for (unsigned buf = 0; buf < fb->_NumColorDrawBuffers; buf++) {
>> >> +      struct gl_renderbuffer *rb = fb->_ColorDrawBuffers[buf];
>> >> +      struct intel_renderbuffer *irb = intel_renderbuffer(rb);
>> >> +      GLuint fbo, rbo;
>> >> +      int index = fb->_ColorDrawBufferIndexes[buf];
>> >> +
>> >> +      if (!((1 << index) & fast_clear_buffers))
>> >
>> > Small suggestion: I think this would read better as
>> > ((fast_clear_buffers & (1 << index)) != 0.
>>
>> Oops, sorry -- of course I meant ((fast_clear_buffers & (1 << index)) == 0.
>
> I like this better too, but I do feel it's a bit contradictory to compare to 0
> here when you asked me to not compare to true in an earlier patch :P

It's not -- because ((fast_clear_buffers & (1 << index)) is not a
boolean. Testing it against 0 makes the otherwise implicit conversion
to boolean explicit.