On 14/10/2012, Ben Hutchings <[email protected]> wrote: > On Sun, 2012-10-14 at 13:20 +0200, Daniel Vetter wrote: >> On Sun, Oct 14, 2012 at 12:14 PM, Ben Hutchings <[email protected]> >> wrote: >> > Please don't drop the context of which commit we are talking about. >> > >> > For further reference, it is 7dd490658627 ('drm/i915: Mark untiled BLT >> > commands as fenced on gen2/3') >> > >> > On Mon, 2012-10-08 at 10:49 +0100, Luís Picciochi Oliveira wrote: >> >> Hi, >> >> >> >> Here is the actual diff to the 3.2 kernel: >> >> >> >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 2012-07-12 >> >> 04:32:21.000000000 +0100 >> >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c 2012-10-02 >> >> 22:17:56.079302807 +0100 >> >> @@ -573,8 +573,8 @@ i915_gem_execbuffer_reserve(struct intel >> >> ret = >> >> i915_gem_object_put_fence(obj); >> >> if (ret) >> >> break; >> >> + obj->pending_fenced_gpu_access = true; >> >> } >> >> - obj->pending_fenced_gpu_access = >> >> need_fence; >> >> } >> >> >> >> entry->offset = obj->gtt_offset; > [...] >> > Maybe it should be: >> > >> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> > @@ -574,7 +574,7 @@ >> > if (ret) >> > break; >> > } >> > - obj->pending_fenced_gpu_access = >> > need_fence; >> > + obj->pending_fenced_gpu_access = true; >> > } >> > >> > entry->offset = obj->gtt_offset; >> > } >> >> This should work, although for safety I think I'd be better to keep >> the 2nd conditional, i.e. >> >> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c >> @@ -574,7 +574,7 @@ >> if (ret) >> break; >> } >> - obj->pending_fenced_gpu_access = >> need_fence; >> + obj->pending_fenced_gpu_access >> =entry->flags & EXEC_OBJECT_NEEDS_FENCE ; >> } >> >> entry->offset = obj->gtt_offset; >> } >> >> In any case, intel-gpu-tools >> (http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/) has a neat >> testcase to exercise this corner case. If >> i-g-t/tests/gem_set_tiling_vs_blt is still happy, the fix should be >> correct. > > I don't have anything with gen2/gen3 chips in (I think that's i830 up to > i945) so I'll put your version in the queue now and let Luís or others > test that.
Hi, Daniel's patch also seems more reasonable to me. I ran the intel-gpu-tools tests on this card. Here's some input on the multiple options we have: At first impression, the last patch proposed by Daniel works well and also fixes the reported problems. It also passes more tests on the mentioned testbench than the "blind backport" I suggested previously. Here's a summary of the obtained results: With the original debian 3.2 kernel: $ grep FAIL 3.2.0-3-686-pae-all_tests | wc -l 13 With the "blind" patch I suggested previously: $ grep FAIL 3.2.0-3-686-pae-patched-all_tests | wc -l 20 These tests don't even finish. A kernel BUG is spit to the console and the system has to be rebooted. With Daniel Vetter's patch: $ grep FAIL 3.2.23-vetterpatch-all_tests | wc -l 10 Although we have less tests failing, some new FAILs appeared with this patch. Here's the diff between the obtained "FAIL"s: $ diff fails-debian-unpatched fails-debian-with-vetters-patch 0a1 > FAIL: gem_cs_tlb 4,6c5 < FAIL: gem_set_tiling_vs_blt < FAIL: gem_tiled_pread < FAIL: gem_tiled_pread_pwrite --- > FAIL: gem_set_tiling_vs_pwrite 8d6 < FAIL: gem_tiled_swapping 10d7 < FAIL: gem_tiled_fence_blits I'm not sure if those 2 new FAILs are grave enough to hold back this fix. You can find all the test logs at [1]. Please tell me if you need more testing or information. Thanks and best regards, Luís Picciochi 1 - http://dl.dropbox.com/u/15755099/intel_gpu_tests.tar.bz2 P.S.: As a final note, here's the number of failed tests on the current 3.5 kernel on Debian's experimental repositories: $ grep FAIL 3.5-trunk-686-pae-all_tests | wc -l 3 Great job! :-) -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
