[Intel-gfx] [PATCH] drm/i915: RMW register cycles considered evil

[Damien Lespiau]

On Mon, Jul 06, 2015 at 04:58:19PM +0200, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 01:46:19PM +0100, Damien Lespiau wrote:
> > On Mon, Jul 06, 2015 at 02:42:02PM +0200, Daniel Vetter wrote:
> > > Especially for workarounds which is stuff that's almost impossible to
> > > verify: The initial state from the firmware on boot-up and after
> > > resume could be different, which will hide bugs when we do an RMW
> > > cycle.
> > > 
> > > Hence never do them, and if it's required we need a special mask.
> > > 
> > > Cc: Damien Lespiau <damien.lespiau at intel.com>
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > Cc: Nick Hoath <nicholas.hoath at intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > 
> > Eeek. Let's take the problem the other way around: have you verified
> > it's OK to zero all those other fields?
> 
> Nope, but it's what we do for other workarounds (e.g. the ones we load
> through the rings except for one case in the cxt switch wa) and on other
> platforms. And in general we've moved away from RMW wherever we can since
> it had too much surprises.

I don't think that's really fair. Most W/As through the rings are
touching masked registers, just setting/clearing specific bits.

I just looked at the *_init_clock_gating() function and it's full
of RMW cycles as well. We roughly have no idea of most of the early init
from the firmware and really want to reuse those when we're missing the
info about those bits.

> It's really just something I spotted while stumbling over a w/a patch for
> hsw that we never merged - I don't like the inconsistency. And it has
> bitten us in the past.
> 
> And yes I haven't done the audit here, but the fact that you suggest we
> need one kind proves my point ;-)

I don't mind the spririt of this, but it requires a massive amount of
lore that is currently done in the firmware. Not at all practical with
the amount of knowledge we have on low level units and early init
sequences and W/As.

-- 
Damien