On Wed, Jan 08, 2014 at 07:59:38AM -0800, Paul Berry wrote:
> On 7 January 2014 16:58, Chad Versace <chad.vers...@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 <kenn...@whitecape.org>
> CC: Paul Berry <stereotype...@gmail.com>
> CC: Stéphane Marchesin <marc...@chromium.org>
> Signed-off-by: Chad Versace <chad.vers...@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 <stereotype...@gmail.com>
The commit message is already quite verbose, so I'll leave it as-is.
Thanks for the thorough review.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
