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

[Daniel Vetter]

On Mon, Jul 06, 2015 at 04:15:25PM +0100, Damien Lespiau wrote:
> 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.

Yeah I think I checked a biased sample for this case. Specically I ended
up looking at GEN6_UCGCTL2 where all pre-gen9 functions don't do a RMW.
But reviewing a lot more of the more modern clock gating code we seem to
be simply inconsistent all across the place. I guess if someone would be
really bored we could go through all modern-ish w/a and check that all
platforms apply them in a uniform way, instead of the current MO where we
walk the wa db mostly on a per-platform query. I'm just really grumpy
about w/a in general since we have only shitty options to validate that we
have them all, so the best we can aim for is consistency, which we don't
have either.

I guess everyone just move on and hope nothing breaks is the right option
:(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch