On Tue, Jan 8, 2019 at 11:01 PM Chris Wilson <ch...@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@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
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
