[Mesa-dev] [PATCH 2/3] i965/gen6/blorp: Set need_workaround_flush at top of blorp (v2)

[Chad Versace]

On Wed, Jan 08, 2014 at 07:59:38AM -0800, Paul Berry wrote:
> On 7 January 2014 16:58, Chad Versace <chad.versace at linux.intel.com> wrote:
> 
>     Unconditionally set brw->need_workaround_flush at the top of gen6 blorp
>     state emission.
> 
>     The art of emitting workaround flushes on Sandybridge is mysterious and
>     not fully understood. Ken and I believe that
>     intel_emit_post_sync_nonzero_flush() may be required when switching from
>     regular drawing to blorp.  This is an extra safety measure to prevent
>     undiscovered difficult-to-diagnose gpu hangs.
> 
>     I verified that on ChromeOS, pre-patch, need_workaround_flush was not
>     set at the top of blorp, as Paul expected. To verify, I inserted the
>     following debug code at the top of gen6_blorp_exec(), restarted the ui,
>     and inspected the logs in /var/log/ui. The abort gets triggered so early
>     that the browser never appears on the display.
> 
>         static void
>         gen6_blorp_exec(...)
>         {
>             if (!brw->need_workaround_flush) {
>                 fprintf(stderr, "chadv: %s:%d\n", __FILE__, __LINE__);
>                 abort();
>             }
>             ...
>         }
> 
>     v2: Explain how I determined that need_workaround_flush wasn't getting
>         set when expected.
> 
>     CC: Kenneth Graunke <kenneth at whitecape.org>
>     CC: Paul Berry <stereotype441 at gmail.com>
>     CC: Stéphane Marchesin <marcheu at chromium.org>
>     Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
>     ---
>      src/mesa/drivers/dri/i965/gen6_blorp.cpp | 4 +++-
>      1 file changed, 3 insertions(+), 1 deletion(-)
> 
> 
> Ok, I dug a little further and figured out what was wrong with my previous
> reasoning.  It turns out that we call intel_batchbuffer_emit_mi_flush() at the
> top of brw_blorp_exec().  intel_batchbuffer_emit_mi_flush() in turn calls
> intel_emit_post_sync_nonzero_flush() before emitting its pipe control.  As a
> result, brw->need_workaround_flush is always false on entry to gen6_blorp_exec
> ().  By resetting brw->need_workaround_flush to true at the top of
> gen6_blorp_exec(), we ensure that the pipe control emitted by
> intel_batchbuffer_emit_mi_flush() is also followed by a post-sync nonzero
> flush.
> 
> Now that I understand what's going on, I'm fine with the patch as is.  If you
> want to copy anything from my paragraph above into the commit message, feel
> free to do so, but whether you do or not, the series is:
> 
> Reviewed-by: Paul Berry <stereotype441 at gmail.com> 

The commit message is already quite verbose, so I'll leave it as-is.
Thanks for the thorough review.