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

[Kenneth Graunke]

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.

The kernel will process the batch containing (A), and relocate the index
buffer somewhere to some address.  Context save will store that address.
Context load will happen before executing (B)'s batch, and restore the
old pointer.  But batch (B) won't have any relocations for the index
buffer pointer (because there's no command to patch up).  So, we'd
either need relocations to patch the GEM context image...or we'd have
to softpin all inherited buffers.

Maybe softpinning inherited buffers in an otherwise relocation-centric
world is viable?  Does softpin even exist on Gen4?  This doesn't seem
easy.

Option 3: Submit batch as soon as we overflow aperture checks, with
          state for a primitive ending up in two batches.

This is not viable either - has all the problems of option 2, but also
means checking aperture everywhere...and our binding table upload code
means essentially pulling in all surfaces as one big group, which is
dodgy...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190108/7404ed29/attachment.sig>