Re: [PATCH v2] drm/i915: fix FORCEWAKE posting reads

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 17 Jan 2013, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 17 Jan 2013 10:24:09 +0200, Jani Nikula <jani.nikula@intel.com> wrote:
>> We stopped reading FORCEWAKE for posting reads in
>> 
>> commit 8dee3eea3ccd3b6c00a8d3a08dd715d6adf737dd
>> Author: Ben Widawsky <ben@bwidawsk.net>
>> Date:   Sat Sep 1 22:59:50 2012 -0700
>> 
>>     drm/i915: Never read FORCEWAKE
>> 
>> and started using something from the same cacheline instead. It turns out
>> reading ECOBUS as posting read worked fine, while GTFIFODBG did not,
>> preventing RC6 states after suspend/resume per the bug report referenced
>> below. It's not entirely clear why, but clearly GTFIFODBG was nowhere near
>> the same cacheline or address range as FORCEWAKE.
>> 
>> Trying out various registers for posting reads showed that all tested
>> registers for which NEEDS_FORCE_WAKE() (in i915_drv.c) returns true
>> work. Conversely, most (but not quite all) registers for which
>> NEEDS_FORCE_WAKE() returns false do not work. Details in the referenced
>> bug.
>> 
>> Based on the above, add posting reads on ECOBUS where GTFIFODBG was
>> previously relied on.
>> 
>> In true cargo cult spirit, add posting reads for FORCEWAKE_VLV writes as
>> well, but instead of ECOBUS, use FORCEWAKE_ACK_VLV which is in the same
>> address range as FORCEWAKE_VLV.
>> 
>> v2: Add more details to the commit message. No functional changes.
>> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=52411
>> Reported-and-tested-by: Alexander Bersenev <bay@hackerdom.ru>
>> CC: Ben Widawsky <ben@bwidawsk.net>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> You don't mention at all why you consider this to be important at all. A
> hint that the extra read makes rc6 after resume more stable on one
> machine would be useful.
>
> So a missing flush of the write in _put() makes sense for rc6 staying
> active. (If it happens once, it can happen everytime we do the forcewake
> dance and so stay asserted indefinitely). And I can quite believe that
> the hw has separate queues for the GT powerwell that require a read from
> within that powerwell to flush, so the magic pixie dust looks consistent.
>
> Ok, I'm happy that I can rationalise that this does indeed fix a genuine
> quirk of our hw. The cargo cult survives.

Right, I just didn't feel very confident making strong claims here based
on more or less anecdotal evidence. (And even the original "never read
FORCEWAKE" feels like folklore...) Making rc6 more stable on one machine
was implied, but I could have been more explict about it. Perhaps Daniel
can amend the commit message from the above as he sees fit?

Thanks for the review.

BR,
Jani.

>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre