On Tue, Mar 25, 2014 at 10:42:09AM +0100, Daniel Vetter wrote: > On Mon, Mar 24, 2014 at 03:37:12PM -0700, Ben Widawsky wrote: > > On Sat, Mar 22, 2014 at 06:52:25PM +0100, Daniel Vetter wrote: > > > On Fri, Mar 21, 2014 at 07:33:59PM +0200, Mika Kuoppala wrote: > > > > Hi, > > > > > > > > Daniel Vetter <daniel.vet...@ffwll.ch> writes: > > > > > > > > > There's an entire pile of issues in here: > > > > > > > > > > - Use the main RING_HEAD register, not ACTHD. ACTHD points at the gtt > > > > > offset of the batch buffer when a batch is executed. Semaphores are > > > > > always emitted to the main ring, so we always want to look at that. > > > > > > > > The ipehr check should make sure that we are at the ring and > > > > acthd should not be at batch. > > > > > > > > Regardless I agree that RING_HEAD is much more safer. When I have > > > > tried to get bottom of the snb reset hang, I have noticed that > > > > after reset the acthd is sometimes 0x0c even tho head is 0x00, > > > > on snb. > > > > > > Hm, should we maybe mask out the lower bits, too? If you can confirm this, > > > can you please add a follow-up patch? > > > > > > > > - Mask the obtained HEAD pointer with the actual ring size, which is > > > > > much smaller. Together with the above issue this resulted us in > > > > > trying to dereference a pointer way outside of the ring mmio > > > > > mapping. The resulting invalid access in interrupt context > > > > > (hangcheck is executed from timers) lead to a full blown kernel > > > > > panic. The fbcon panic handler then tried to frob our driver harder, > > > > > resulting in a full machine hang at least on my snb here where I've > > > > > stumbled over this. > > > > > > > > > > - Handle ring wrapping correctly and be a bit more explicit about how > > > > > many dwords we're scanning. We probably should also scan more than > > > > > just 4 ... > > > > > > > > ipehr dont update on MI_NOOPS and the head should point to > > > > the extra MI_NOOP after semaphore sequence. So 4 should be > > > > enough. Perhaps revisit this when bdw gains semaphores. > > > > > > Yeah, Chris also mentioned that the HEAD should point at one dword past > > > the end of the ring, so should even work when there are no MI_NOOPs. In > > > any case this code is more robust in case hw engineers suddenly change the > > > behaviour. > > > > > > > > - Space out some of teh computations for readability. > > > > > > > > > > This prevents hard-hangs on my snb here. Verdict from QA is still > > > > > pending, but the symptoms match. > > > > > > > > > > Cc: Mika Kuoppala <mika.kuopp...@intel.com> > > > > > Cc: Ben Widawsky <b...@bwidawsk.net> > > > > > Cc: Chris Wilson <ch...@chris-wilson.co.uk> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74100 > > > > > > > > The hard hangs are not so frequent with this patch but > > > > they are still there. This patch should take care of > > > > the oops we have been seeing, but there is still > > > > something else to be found until #74100 can be marked as > > > > fixed. > > > > > > > > Very often after reset, when igt has pushed the quietence > > > > batches into rings, blitter and video ring heads gets > > > > moved properly but all updates to hardware status page and to > > > > the sync registers are missing. And result is hang by hangcheck. > > > > Repeat enough times and it is a hard hang. > > > > > > > > > Signed-off-by: Daniel Vetter <daniel.vet...@ffwll.ch> > > > > > > > > Please remove the blowup comment and also update the > > > > bugzilla tag. > > > > > > Yeah, QA also says that it doesn't fix the hard hangs, only seems to > > > reduce them a bit on certain setups. I've updated the commit message. > > > > > > btw are you testing with FBDEV=n? The lack of a fbcon panic handler should > > > greatly increase the chances that the last few message get correctly > > > transmitted through other means like netconsole. > > > > > > > Patches 1-2 and the followup one are > > > > > > > > Reviewed-by: Mika Kuoppala <mika.kuopp...@intel.com> > > > > > > Thanks for the review, patches merged. > > > -Daniel > > > > Patch 2 was trivial to implement for gen8. This functionality is a lot > > less trivial. I volunteered to do patch 2, are you going to port this to > > gen8? > > If you fill out the bdw pieces for patch 2&3 the only thing you need to > change here is to adjsut the number of dwords we walk backwards to make > sure that the bdw semaphore cmd still fits. Or at least that's been my > idea behind all this, maybe I've overlooked something. > -Daniel
I don't think it's quite that easy, but I took it as a, "yes." Which one is patch 3? -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
