Re: [PATCH 10/12] drm/i915: TLB invalidation with MI_FLUSH_SW requires a post-sync op

From: Daniel Vetter <daniel@ffwll.ch>
To: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ben Widawsky <ben@bwidawsk.net>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 10/12] drm/i915: TLB invalidation with MI_FLUSH_SW requires a post-sync op
Date: Thu, 4 Oct 2012 16:49:42 +0200	[thread overview]
Message-ID: <CAKMK7uFR5orWCXiWmsi+xNCZ83SPdbr_gZjHsayY1FngAXUxdA@mail.gmail.com> (raw)
In-Reply-To: <20121004093931.73bdac1a@jbarnes-t420>

On Thu, Oct 4, 2012 at 4:39 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
>> 2. w/a patches need testcases, too. Either a register check added to i-g-t
>> or if it's a runtime thing, a runtime assert at a nice place (where
>> feasible, ofc).
>
> A register check isn't that useful imo.  A real test case would be
> ideal, but given how hard some of these issues are to hit, it's
> unrealistic to spend weeks writing a test case for a workaround that's
> already been documented to fix a specific issue.

That's pretty because of Ben's w/a patch to remove the w/a from the
part of the init sequence where the write sticks, and keep it at the
place where the write doesn't stick. Similarly, we've sometimes
managed to apply w/a not correctly after gpu reset or resume.

Exactly since for many of these we won't ever have a good test-case,
we should at least make sure that we actually succeed in setting the
right values everywhere. Shockingly, we've failed even at that :(

>> 3. I'll randomly stall patches to bring 2. up to par for existing
>> workarounds.
>
> Btw if you want to take this to its logical conclusion, we also
> shouldn't be "fixing" issues that are obvious from code review but
> people haven't hit in practice (this goes for a good chunk of the code
> churn in our driver involving cleanups and fixes for potential
> non-issues).  And that's not even including test case development for
> any patch claiming it fixes anything.

And most of those actually go through dinq, at least if the exact
impact is unclear and there's no testcase or bug to demonstrate the
issue. My gripes here are purely for pushing too much w/a patches to
-fixes, since both your patches and Ben's had regressions that hang
machines. Hence my grumpiness.

> That said, I definitely agree we want to add more test cases.  Just
> don't block applying known workaround fixes or other stuff on those
> test cases.

Sure, I'll usually try to come up with a random distribution not
biased against stuff that fixes real bugs ;-) But even for bugfixes I
want testcases first (as always, if feasible ofc), since ppl are so
easily distracted (and writing testcase is a royal pain).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch