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

[andrey simiklit]

On Tue, Jan 8, 2019 at 11:01 PM Chris Wilson <chris at chris-wilson.co.uk>
wrote:

> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

So, have we a plan as to it?
This patch was sent 1 year ago and I almost forgot it.
Maybe it was fixed by somebody and this patch can be just rejected/closed)
- Andrii
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20191119/50590e62/attachment.html>