<div dir="ltr">On 7 January 2014 16:58, Chad Versace <span dir="ltr"><<a href="mailto:chad.versace@linux.intel.com" target="_blank">chad.versace@linux.intel.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Unconditionally set brw->need_workaround_flush at the top of gen6 blorp<br>
state emission.<br>
<br>
The art of emitting workaround flushes on Sandybridge is mysterious and<br>
not fully understood. Ken and I believe that<br>
intel_emit_post_sync_nonzero_flush() may be required when switching from<br>
regular drawing to blorp.  This is an extra safety measure to prevent<br>
undiscovered difficult-to-diagnose gpu hangs.<br>
<br>
I verified that on ChromeOS, pre-patch, need_workaround_flush was not<br>
set at the top of blorp, as Paul expected. To verify, I inserted the<br>
following debug code at the top of gen6_blorp_exec(), restarted the ui,<br>
and inspected the logs in /var/log/ui. The abort gets triggered so early<br>
that the browser never appears on the display.<br>
<br>
    static void<br>
    gen6_blorp_exec(...)<br>
    {<br>
        if (!brw->need_workaround_flush) {<br>
            fprintf(stderr, "chadv: %s:%d\n", __FILE__, __LINE__);<br>
            abort();<br>
        }<br>
        ...<br>
    }<br>
<br>
v2: Explain how I determined that need_workaround_flush wasn't getting<br>
    set when expected.<br>
<br>
CC: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>><br>
CC: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
CC: Stéphane Marchesin <<a href="mailto:marcheu@chromium.org">marcheu@chromium.org</a>><br>
Signed-off-by: Chad Versace <<a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a>><br>
---<br>
 src/mesa/drivers/dri/i965/gen6_blorp.cpp | 4 +++-<br>
 1 file changed, 3 insertions(+), 1 deletion(-)<br></blockquote><div><br></div><div>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.<br>
<br></div><div>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:<br>
<br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>>  <br></div></div></div></div>