Re: [PATCH] tools/null_state_gen: generate null render state

[Damien Lespiau <damien.lespiau@intel.com>]

On Tue, May 06, 2014 at 02:47:40PM +0100, Chris Wilson wrote:
> Why does this work? It is neither the most minimal batch, nor the
> maximal. Which state is truly required? It looks like cargo-culted
> Chinese.

I'll have to echo this. It's really not obvious why this is needed.
If you look at the render engine power context for instance, it's just a
list of registers. So if we do:

  - init_clock_gating() (bad name!)
  - enable_rc6()

The render power context should contain the W/A we setup.

Would we do:

  - enable_rc6()
      -> enter rc6
      -> power context save
  - init_clock_gating()
      -> exit rc6
      -> power context restore

We'd end up restoring the reset value of the registers we touch in
init_clock_gating() (or the values after BIOS really), ie may not
contain all W/As and hang?

Note that init_clock_gating() is not the only place where we touch the
registers that are part of the power context(s), we do need to ensure
rc6 is only enabled after we setup those registers.

It could also be that something else than saving/restoring the power
contexts is happening at rc6 entry/exit, but the documentation is rather
sparse here and so we need to try talking to the hardware engineers
again.

So yes very much feels like cargo culting. I'd be nice to really
understand what's happening.

Now, a rather pragmatic approach would be to take those patches if they
actually paper over an issue, but the Tested-by: tags are not legion,
Mika didn't reproduce the issue on his BDW (IIRC) and Ben was saying
Kristen didn't confirm it was these exact patches that were solving
hangs for her (If I understood correctly on the call).

I do have to point out that it's a lot of code to review and rather full
of details, ie, we'll get it wrong-ish (but not enough to break
anything, hopefully).

-- 
Damien