Test summary results are being parsed

Merged by 3 months ago (Apr 13, 2022 5:57pm UTC) 3 months ago

• Now, with timeline semaphores! I should stop typing patches and let you review. 😂

• If you want me to pull the top few that do timeline semaphores into a separate MR, I can do that.

• Well, I think that it is ok to keep them on this MR. But I think that now that we are on the situation of reviewing, testing, and finding some regressions, probably it is better to stop to add features and focus on stabilize the code.

  Really appreciated though that you added timeline semaphores though! Thanks again.

• That's awesome! I didn't know there was support for emulated timeline semaphores.

  I hope I can finish reviewing the patches tomorrow, but modulo a few things I found and commented here I think it is looking pretty great.

• I think I'll leave "real" timeline semaphores to someone else. As much fun as I've had hacking on v3d for a few days, the pain of kernel development decreases the motivation to go quite that far. It should be pretty easy to wire up in the v3d kernel driver, though. I'd be happy to give some pointers or maybe even throw you an untested patch or two.

• As much fun as I've had hacking on v3d for a few days, the pain of kernel development decreases the motivation to go quite that far.

  Hehehe, I feel you 😄

  It should be pretty easy to wire up in the v3d kernel driver, though. I'd be happy to give some pointers or maybe even throw you an untested patch or two.

  Thanks again Jason, we should really buy you a beer or something next time we have a chance to meet face to face ;)

Are Reviewed-by: Iago Toral Quiroga <[email protected]>

• Resolved by Iago Toral 3 months ago

  @jekstrand: a question about the submit thread model, is my understanding correct that we are spawning a single thread for this? So for example in this case:

  We would block on the CmdWaitEvents until the client signals the event and would not start processing the 2nd vkQueueSubmit until that happens?

• Last reply by Iago Toral 3 months ago

• I think this may not be handling correctly the scenario where our first job in a command buffer batch is a CPU job. For example, if the first command is vkCmdResetQueryPool, the first CPU job will wait for the relevant queries to finish before resetting but will not wait for semaphores, if any.

  We have been handling this scenario by emitting a noop job to consume the wait semaphores first and then waiting for idle before processing the CPU job. See https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/broadcom/vulkan/v3dv_queue.c#L1137

  AFAICS, it seems that we are not handling that case here, no?

• @jekstrand: I believe this has not been addressed yet?

• @jekstrand: you didn't include this patch in the branch yet, do you have any concerns related to it?

• In the multi-wait case, this is handled by the vk_sync_wait_many() in queue_wait_idle(). In the non-multi-wait case, we first use SYNC_FILE_MERGE to fold the wait semaphores into queue->last_job_syncs.syncs[V3DV_QUEUE_ANY] and then we wait on that sync in queue_wait_idle().

  In all the cases where queue_wait_idle() is called, the resource being touched is not a shared resource so there's no way that a different device will be touching it and v3dv_bo_wait() is sufficient. If you ever decide to allow multiple queues within a single v3dv_device (that should be possible now), we'll need to be a bit more clever. I typed the clever version at one point but decided it wasn't needed.

• Yes, that makes sense. Maybe it is a good idea to add a comment in v3dv_queue_driver_submit explaining this for future reference.

• Resolved by Jason Ekstrand 3 months ago

  I did a full run of dEQP-VK.synchronization.* with multisync (CI only runs a small fraction) and got a few failures:

• Last reply by Iago Toral 4 months ago

As mentioned by @jasuarez on the last full runs there were still some flake failures. But those runs didn't include last Jason's fixups pointed by Iago. Tomorrow I will ask him to do a new one, or I could try to do it manually myself on my rpi4.

Meanwhile tonight I will try to test the branch with real apps (just in case) and going on with the review.

Reviewed-by: Iago Toral Quiroga <[email protected]>

• It seems there is a problem with WSI, anything that tries to render to a window freezes and never actually seems to render anything. I tested this with different apps and it seems to happen in all of them. I think this happens because we end up waiting on the semaphore from a previous acquire image, which never gets signaled.

• 0001-FIXUP-Fix-WSI.patch

  This change fixes the problem, it basically removes the whole sync_for_memory stuff and puts back our previous version of AcquireNextImage2KHR that signals fence/semaphore directly after calling the common code. I noticed that Turnip does the same thing by the way.

  @jekstrand: is there any implicit requirement for the sync-for-memory stuff to work?

• It seems that the common framework signals with sync_for_memory only on QueuePresent by inserting a VK_STRUCTURE_TYPE_WSI_MEMORY_SIGNAL_SUBMIT_INFO_MESA struct into a QueueSubmit produced from there. Not sure how this is supposed to work though, since I think the cases I see are doing:

  So we end up waiting forever in step 2.

• I have just confirmed that Iago's patch fix some tests that were failing yesterday like this:

  dEQP-VK.wsi.xlib.incremental_present.scale_none.immediate.identity.inherit.incremental_present

  and got the gfxreconstruct traces working again.

  I will keep doing tests, but meanwhile. @jasuarez could you do a new full cts run using this branch (that is just Jason's branch plus Iago's fix)?:

  https://gitlab.freedesktop.org/apinheiro/mesa/-/commits/wip/apinheiro/vk-submit

• @jekstrand: this is the stacktrace for the case where we hang in step 2, in case it is helpful:

  Edited by Iago Toral 3 months ago

• @jasuarez pointed to some building warnings when using Iago's fix (consideded errors when the CI builds). Have a fix here:

  apinheiro/mesa@c39b785f

• Ugh... Yeah, I just blindly applied the thing from RADV because that's what I did for lavapipe too. I think what you've done in your patch is mostly ok. The only problem is that the temporary/permanent semantics are a bit off. We should just copy+paste from panvk.

• Ok, I've pushed with updates from all the comments, including this one. I pulled in Iago's patch but modified it with the code from panvk.

• Hrm... Actually, the panvk code should be equivalent to what's there today. I need to do some digging. Fortunately, lavapipe also explodes and that one's easy to debug.

• Ok, I'm not not clear at all what we should be doing here. Question: How does implicit sync work on v3d? I don't see anything in the kernel interface that lets you wait/signal on BOs. Do you always do a full CPU stall before handing off to X11?

• Reading the v3d kernel code, it seems to always sync everything all the time. If that's the case, how have we had any sync problems ever?!? How is vkcube tearing? Maybe I'm reading it wrong? Anyway, I'm going to put it down for now until one of you can help me figure out how this stuff works. Once I know how implicit sync works, fixing WSI should be a snap.

• If everything syncs all the time like I suspect from my reading of the v3d kernel code then I think the WSI fixup patch as it exists in this MR right now should be correct. I've tested it a bit with vkcube and some sascha demos.

• If everything syncs all the time like I suspect from my reading of the v3d kernel code

  AFAIR, that's the case, but I will let Iago to confirm it.

  I've tested it a bit with vkcube and some sascha demos.

  I have been testing with vkQuake2, Quake3e (that have a Vulkan renderer), and RBDoom3-BFG, and seems to work fine. Pending to try with some ue4 demos that we have around, that we use as reference as intensive vulkan app.

  I also tested with gfxreconstruct, and they work in general, but got some new asserts now. Having said so, those are old traces, and it is not the first time that I get weird behaviour when using a recent gfxreconstruct with old traces, so I think that it would be better to capture new ones and try again.

  Having said so, before going too much on those apps, when doing full CTS runs we found some new flake tests. @jasuarez runs (so using the CI environment for that) got crashes now and then on the following tests:

  and I got the following failures using one of my rpi4:

  But as mentioned they are flake tests. In fact, we were not able to reproduce them manually. Individually they work fine, and also if run a bigger subset with a similar pattern (like dEQP-VK.binding_model.descriptor_copy.*). Tomorrow we will try to reproduce those failures without needing to do a full CTS run.

• Ok, I'm not not clear at all what we should be doing here. Question: How does implicit sync work on v3d? I don't see anything in the kernel interface that lets you wait/signal on BOs. Do you always do a full CPU stall before handing off to X11?

  Reading the v3d kernel code, it seems to always sync everything all the time. If that's the case, how have we had any sync problems ever?!? How is vkcube tearing? Maybe I'm reading it wrong? Anyway, I'm going to put it down for now until one of you can help me figure out how this stuff works. Once I know how implicit sync works, fixing WSI should be a snap.

  I think @mwen might be able to help here since she is the one familiar with the kernel bits. Melissa, some context about what we are looking at here: Jason has been rewriting how we do synchronization in v3dv driver to use the common sync code in Mesa and now we're specifically looking at how we send images to the display engine, particularly when it comes to the synchronization aspect so we can be certain that we don't start rendering or presenting too early.

I think we have another issue with signaling where the last job may complete before some other job we executed previously in the command buffer in a different queue. For example, if we have a command buffer like this:

TFU job -> CSD job -> CL job

We would signal with the CL job, but it may be possible that the TFU job is still running since that uses a totally different hw module. Currently, we have been handling this by only signaling on the last job if we have not submitted any jobs in the same command buffer batch to any of the other hw queues, and if that is not the case, we instead emit a serialized noop job at the end that handles signaling. See https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/broadcom/vulkan/v3dv_queue.c#L1306.

Yeah, this is a problem and it seems we actually have 2 bugs: I did a quick change where I we never signal on the last job and instead always signal with a noop job at the end, the idea being that we should be serializing that job against all queues and prevent the issue I describe above. And yes, this gets rid of the flaky crashes but then I found that some tests started to fail more permanently because the noop jobs are only really serialized if we don't have wait semaphores. This is because set_in_syncs assumes that if we are the first job in any queue we just need to wait on the semaphores and there is no need to serialize, however, this is not valid when we are trying to use a noop job for signaling at the end. In the particular case I was looking at, the test was doing a bunch of CSD jobs, so emitting a noop job (which is a CL) at the end to signal would see that it was the first CL job and decide that it only had to wait on the semaphores and not on the last job sycns, so we would be signaling immediately without waiting for the CSD jobs to finish. I fixed this by manually setting sync_info.wait_count = 0 when dispatching the noop job and we had a last job (meaning that have already waited on the input semaphores), and with that it seems that we were able to do a full CI run without issues.

So I think we have 2 ways to fix this properly:

1. Restore the logic I linked to above where we only signal on the last job of the command buffer if we had only submitted jobs to that one queue (since we know they are serialized in that case) and otherwise, signal with a noop job at the end (but making sure that it waits for the last syncs so it is properly serialized).

2. Always signal with the last job if it is a GPU job, but force proper serialization against last job syncs too, independently of whether we need to wait for semaphores or not too.

Personally, I think 2 makes more sense.

Tomorrow I have to run some errands mid-morning but I might be able to write that patch before I leave, unless @jekstrand beats me to it :)

• I like option 2 as well. Throwing noop jobs at the kernel seems a bit wasteful but, given how many jobs per command buffer you end up with, it really doesn't seem that bad.

• I've implemented 2. There is a fixup commit at the end and also one I snuck into the middle called v3dv: Always wait on last_job_syncs if job->serialize.

• Thanks Jason, the fixes look good to me.

  @jasuarez: could you test the branch in CI?

• So I've tested this MR using stock kernel (both 32bits and 64bits), as well as using a multisync-enabled kernel (only in 64bits, I don't have one for 32bits), and everything works fine.

• Thanks a bunch! @itoral Is that a review? I've not seen one for the big mega-patch yet.

  Edited by Jason Ekstrand 3 months ago

• The other thing we were waiting on was the question about implicit sync for WSI. It seems the display driver is supposed to wait on a fence for v3d to be done with rendering, but currently this is messed up and it is not working properly, it is a known issue and they are working on fixing this.

  I've just made a comment that we should be signaling immediately the semaphore/fence in vkAcquireNextImage2 by initializing their value to 1 which is what we have been doing before (instead of 0 like you currently do in this branch), however, since the kernel has this bug, I wonder if that is the right thing to do right now.

• But in any case, it is exactly what we have been doing all this time anyway, so I don't think we need to figure this out right now.

"v3dv: Add emulated timeline semaphore support" is:

Reviewed-by: Iago Toral Quiroga <[email protected]>

"v3dv: Always wait on last_job_syncs if job->serialize" is

Reviewed-by: Iago Toral Quiroga <[email protected]>

As @jasuarez has been focused on doing full CTS runs (and @itoral fixing the issues he found) I has been testing a little with real apps, specifically if there were any performance changes, even if it was just looking for FPS.

So I tested with the gfxreconstruct traces that I had an hand: ue4 shooter demo, ue4 vehicle demo, quake3e, vkQuake.

For the ue4 demos, there are not significant changes, although a slight improvement with the vehicle ones (slight as 11.5 fps vs 12.0 fps on average).

But on the case of Quake3e and vkQuake there is a significant FPS average drop. Around 10fps, and in some traces even more. I also briefly checked on Quake3e directly, just in case it was a issue with gfxreconstruct, and seems to confirm that performance drop.

But taking into account that it is not affecting all games, and the CTS is passing, I don't think that we should block this MR due that, and investigate and fix it later.

• I've squashed in all the fixups. It should be basically equivalent to the last version only the big patch is now all one patch again.

• It seems you didn't actually push the update, but in any case the big patch is:

  Reviewed-by: Iago Toral Quiroga <[email protected]>

  I think we are ready to land this. Although most of us will be off until Monday due to local holidays, since we have done full CI runs I don't expect there to be any unexpected issues, but in anyc ase @apinheiro will be around a few hours until Friday should anything unexpected happen.

  I'll try to figure out why Quake and Quake3 take a performance hit with this next week.

  Edited by Iago Toral 3 months ago

• Marge Bot @marge-bot merged 2 hours ago

  Just want to say again thanks @jekstrand for working on this MR.

• You're more than welcome! One of the things I'm excited about with my new role at Collabora is being able to dip my toes into all the different drivers. This seemed like as good an excuse as any to get to know v3dv a bit.