[Mesa-dev] [PATCH 1/2] i965: add missing rollback of URB requirements

[Chris Wilson]

Quoting Kenneth Graunke (2019-01-08 20:17:01)
> On Tuesday, January 8, 2019 3:11:37 AM PST Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2019-01-08 11:03:26)
> > > Hi Andrii,
> > > 
> > > Although I think what these patches do makes sense, I think it's missing 
> > > the bigger picture.
> > > There is a lot more state that gets lost if we have to revert all of the 
> > > emitted commands.
> > > A quick look at brw_upload_pipeline_state() shows all of the programs 
> > > could be invalid as well, or the number of samples, etc...
> > > 
> > > To me it seems like we need a much larger state save/restore.
> > > 
> > > Ken: what do you think?
> > 
> > How about not rolling back? If we accept it as currently broken, and just
> > demand the kernel provide logical contexts for all i965 devices (just ack
> > some patches!), and then just flush the batch (possibly with a dummy 3D
> > prim if you want to be sure the 3D state is loaded) and rely on the
> > context preserving state across batches.
> > -Chris
> 
> I'm not sure precisely what you're proposing, but every scenario I can
> think of is breaking in some subtle way.
> 
> Option 1: Submit batch at last 3DPRIMITIVE, re-call emit in a new batch.
> 
> This is what we do today; it doesn't work since brw_upload_render_state
> implicitly sets CPU-side fields for "current state of the GPU", which
> become inaccurate at rollback, so the next state upload would do the
> wrong thing.  We'd need to roll all those back as well, or only commit
> them when we finish uploading state.  Atoms also flag other new atoms,
> and that's not getting rolled back, but extra flagging is harmless.
> 
> Fixing this means disentangling some of our code, saving and restoring
> more values, and so on.  Maybe dropping a few CPU micro-optimizations.
> 
> Option 2: Submit batch at last 3DPRIMITIVE (A), memcpy commands for
>           for failing primitive (B) into a new batch.
> 
> This doesn't work either.  Let's say (A) issued a 3DSTATE_INDEX_BUFFER
> command with a pointer to the index buffer.  (B) didn't change index
> buffer state, so it doesn't emit one, intending to reuse the old value.

This with all relocs with BRW_NEW_BATCH, and iirc the rule is that they
are already tied into that BRW_NEW_BATCH flag. Except you don't need to
memcpy, since you already have the 3DSTATE equivalent to the cpu state
tracker in the batch, emit that, and it will be recorded in the context
for the next batch. (You just don't tell the kernel about the
buffers/relocs that cause the aperture exhaustion and it won't do
anything about them. The GPU may load stale addresses, but never uses
them, exactly like the context state on the next batch anyway.)

It's not a great argument, but the argument is that there's less you
need to fixup, than having to remember to every single comparison in
order to rollback the cpu state tracker. 
-Chris