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

[Paul Berry]

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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140108/aa58d90d/attachment.html>