[Intel-gfx] [PATCH 2/3] drm/i915: Do only one posting read on forcewake put sequence

[Chris Wilson]

On Wed, Jan 28, 2015 at 05:54:14PM +0200, Mika Kuoppala wrote:
> Ville Syrjälä <ville.syrjala at linux.intel.com> writes:
> 
> > On Wed, Jan 28, 2015 at 03:28:56PM +0200, Mika Kuoppala wrote:
> >> Chris Wilson <chris at chris-wilson.co.uk> writes:
> >> 
> >> > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote:
> >> >> commit 05a2fb157e44a53c79133805d30eaada43911941
> >> >> Author: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> >> >> Date:   Mon Jan 19 16:20:43 2015 +0200
> >> >> 
> >> >>     drm/i915: Consolidate forcewake code
> >> >> 
> >> >> introduced domain handling where each domain has it's own posting
> >> >> read registers. This changed the forcewake sequence on 'put' side when
> >> >> there is multiple domains as there would be extra read between the domain
> >> >> puts. Any posting read should be enough to flush all the changes.
> >> >> 
> >> >> Do a posting read only once, at the end of the sequence and for
> >> >> the first domain. Like it was before.
> >> >> 
> >> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala at intel.com>
> >> >
> >> > fwiw, I would argue that the posting read in _get() is superfluous as we
> >> > will serialise the fw with not only the ack, but any subsequent mmio.
> >> >
> >> > On the _put() side we do want to flush the write so that the hw can
> >> > power down as early as possible. So just kill the posting read from _get
> >> > and otherwise drop the patch. :)
> >> 
> >> Yes, both put/get patches should be dropped. I posted a patch removing
> >> the posting read on get side and with your explanations in commit message.
> >> 
> >> This all starts to make so much sense that some gen is bound to break ;)
> >
> > IIRC the posting read from same cache line actually fixed real bugs. So
> > I'm a bit worried about dropping them. But I suppose it's possible only
> > the _put side was important for those bugs.
> 
> I found these:
> 
> commit 6af2d180f82151cf3d58952e35a4f96e45bc453a
> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date:   Thu Jul 26 16:24:50 2012 +0200
> 
>     drm/i915: fix forcewake related hangs on snb
> 
> commit 8dee3eea3ccd3b6c00a8d3a08dd715d6adf737dd
> Author: Ben Widawsky <ben at bwidawsk.net>
> Date:   Sat Sep 1 22:59:50 2012 -0700
> 
>     drm/i915: Never read FORCEWAKE
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=51738
> https://bugs.freedesktop.org/show_bug.cgi?id=52424
> 
> The snb here seems to survive gem_dummy_reloc_loop and
> gem_ring_sync_loop in here with the get side posting removed.

Note that we kept the once associated with #52424, but judging by my
comments in #51738 the posting read is just a band aid anyway as a full
mb() itself was not adequate.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre