Closed PatrickvL closed 6 years ago
It's quite strange how an improvement in memory querying seems to lead to not booting these XDK samples anymore. Are you sure it's that exact commit that broke it?
I suspect these regressions are actually caused by one of the OOVPA changes @jarupxx made. I'm sure this will be corrected soon enough.
From @Voxel9 on April 12, 2017 19:40
I tested jarupxx's last commit before your MemoryManager improvement, and can confirm that the XDK Samples could boot just fine, so yeah, I am certain that the exact commit was ecded47.
Okay, thanks for verifying that. To be absolutely clear - is this https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/commit/ad659ca408a99870d0b1d6e4e6e3e1c2e8d5b535 the commit in which the XDK samples still ran?
From @Voxel9 on April 12, 2017 19:49
Yes, that's the one :)
From @LukeUsher on April 12, 2017 19:53
I'll do some hardware testing of the memory manager behavior next week, see if the results from Cxbx and real hardware differ.
Perhaps QueryAllocationSize shouldn't return remaining size in bytes, but something else, like remaining size in pages, or the size of the original allocation (either in bytes or in pages), or the base address of the allocation, or even something more obscure?
From @LukeUsher on April 12, 2017 20:1
My plan is to allocate some memory via various methods, one being MmAllocateContiguousMemory, run QueryAllocationSize on them, dump the results to a text file on the HDD then compare the text files generated by Cxbx-Reloaded and real hardware, that should provide some pointers on what we're doing wrong
Btw, thanks @Voxel9, for bringing this under our attention, as it's exactly the little things like this, that will truly get us accurate emulation - it's much appreciated!
From @LukeUsher on April 12, 2017 20:13
I believe that MmQueryAllocationSize returns the total size (in bytes) of the pages used by the memory region pointed to by the input. Rather than calculate the remaining size of the buffer and return that, I believe that it finds the base address of the allocation.
Eg. MmAllocateContiguousMemory(0x1000) returns 0x8001000.
MmQueryAllocationSize(0x8001050) would look through a page table (or our memory block info), find the page that contains this address, and return the total size of the pages the allocation uses (in this case 0x1000). Currently we return the remaining space in the page.
Just something to play with until I find time for the HW testing
From @LukeUsher on April 12, 2017 20:16
Update: changing
ret = info->block.size - ((size_t)addr - (size_t)info->block.addr);
to
ret = info->block.size;
Fixed the GamePad sample for me.
@Voxel9 Can you verify that PR #363 fixed these samples' regressions for you too?
@LukeUsher I'm still interested to see what would be returned if the call mentioned an address from a page after the starting page
From @LukeUsher on April 12, 2017 21:25
That's what the hardware testing will hopefully discover
From @ObiKKa on April 12, 2017 22:24
I've tested it. I was able to boot it up with the latest master build. But original poster above should do likewise. The D-Pad controls didn't work on the main menu. So I entered the Gamepad screen and saw this shot below. I think graphics seem to be missing in the center - I can only see the ring which moves when I use the left analog stick buttons on my keyboard. Right analog stick's button presses worked also but seemed to be slow to register on the screen's text lines perhaps.
https://i.imgur.com/TyTdeXi.png
Also tried the Hardware Video mode. The analog nub is emulated a bit better, more surfaces showing. But broken in two pieces in the middle. https://i.imgur.com/dQu8bsL.png
From @Voxel9 on April 13, 2017 6:22
Yep, can confirm the majority of the regressions have been fixed. But a lot of the graphics seem to have broken since they were last working, especially in the 4627 samples (probably side-effects of previous commits); the 5849 samples seem to have remained the same though.
Dolphin (4627) Before fix: https://i.imgur.com/mUhRU9B.png
Dolphin (4627) After fix: https://i.imgur.com/sxke12M.png
FuzzyTeapot (4627) Before fix: https://i.imgur.com/sSk02j4.png
FuzzyTeapot (4627) After fix: https://i.imgur.com/mQhxGKe.png
As for Gamepad, it seems that both 4627 and 4134 end up displaying none of the gamepad model at all anymore for me:
Gamepad (4627/4134) Before fix: https://i.imgur.com/gZLEvle.png
Gamepad (4627/4134) After fix: https://i.imgur.com/BBG7vWd.png
Only thing I have noticed is that Fire (5849) still seems to be in regression, hence why I said the majority of the samples no longer regress.
From @LukeUsher on April 13, 2017 7:16
Still investigating, but the graphical corruption in the XDK samples seems to have started when we introduced the Memory Manager. It would seem there is still something we don't have quite right yet. Perhaps we really do need to implement emulated page tables.
@LukeUsher The most important change the introduction of the new memory manager brought, is that since then addresses in contiguous memory (0x80000000 and up) are returned - this could lead to new code-paths being taken, perhaps even conversion back to non-GPU addresses (by anding with 0x7FFFFFFF) for some operations. If that's the case, we should devise a solution that would map these addresses to the same data. For that, memory-mapped files could be a solution, as those can be mapped onto multiple virtual memory addresses. For this we do need to use an address-range that's both available with and without the high-bit set.
From @LukeUsher on April 13, 2017 7:42
Mapping these same addresses is not really possible, eg: 0x80010000 would be 0x00010000, which resides within our XBE, also I don't think this is the issues (although it could be related, we possibly still have some code laying around in Cxbx that makes the < 0x80000000 assumption.
But we could raise the addresses in the 0x80000000 range up with 128MB, which would circumvent this problem.
From @BenNottelling on August 1, 2017 7:19
Is this still a valid issue?
XDK Samples and Tutorials are (and will stay) a very important "first-line-of-defense" test-set, as these are small (so can be ran through quickly) and use various methods of rendering. Our focus should be to get them all running flawlessly, as that will significantly raise the quality of larger executables too.
From @BenNottelling on August 1, 2017 8:29
@PatrickvL Haha I'm just asking if these still have regressed I know these are important :)
@BenNottelling As far as I know, these are still regressed. I've added the "high-priority" label to this issue, as for all issues marked with "regression", this is the most important one.
Since only this page shows earlier Dolphin screens unlike two other open pages with similar headings I'll post the video here.
This video from Literalmente{Game} (Oct 17, 2017) shows a nice running slide-by-slide comparison video for the Dolphin application on two different versions [Master 853eea7b and Patrick's WIP LessVertexPatching c12ac57].
Speed is good.
Dolphin is perfect, the only issue is that the ingame fps counter reports 15.75 - 15.80 FPS, whereas ours says it's 60 Version https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/commit/b5a3d23fc863b2ba52a3e02033983958209d4083 Dolphin Demos-10576a01.txt KrnlDebug.txt Xbe.txt
Great to hear 5849 Dolphin has improved! Unfortunately the 4627 version seems to be having some trouble though... 😕
Xbe.txt Dolphin-39abbe85.txt KrnlDebug.txt
---------------------------
Cxbx-Reloaded
---------------------------
Received Exception Code 0xC0000005 @ EIP := 0x5890C57C
Press "OK" to terminate emulation.
Press "Cancel" to debug.
---------------------------
OK Cancel
---------------------------
It appears that almost none of the XDK samples from 4627, 5933, or 4361 are working anymore. The only sample I tested which still works, but with broken shaders, is the gamepad sample from 4531. I provided some XBE dumps and KrnlDebug logs if it can help at all.
DolphinClassic (4627) KrnlDebug.txt Xbe.txt
DolphinClassic (5933) KrnlDebug.txt Xbe.txt
VertexShader (4627) KrnlDebug.txt Xbe.txt
Fire (4361) KrnlDebug.txt Xbe.txt
Gamepad (4531) (Doesn't Crash, but shaders seem to be broken) KrnlDebug.txt Xbe.txt
And yet at the same time, we're showing better progress than ever in many titles, rather odd. I'll look into it.
By the way, you should link up one of these gif's!
https://getyarn.io/yarn-clip/12dd3ef3-936d-478f-803e-fd0ac4459cf5/gif https://tenor.com/view/batman-begins-ill-look-into-it-batman-investigate-bug-report-gif-7338154
@PatrickvL et al.
Is the MmQueryAllocationSize
fix currently not causing regressions in the XDK samples? Still works?:
changing
ret = info->block.size - ((size_t)addr - (size_t)info->block.addr);
toret = info->block.size;
Fixed the GamePad sample for me.
I came to the same conclusion when finding out what MmQueryAllocationSize
returns, when passed a valid BaseAddress
that may be offset. Need a test case.
BTW, POSIX doesn't have an interface for this Windows call. The caller should keep the allocation size, as it's best programming practice.
@haxar Well, we can't really change the fact that the Xbox kernel contains a MmQueryAllocationSize
API which is actively being used by Xbox software titles. So our HLE code does nothing out of the ordinary when calling it too. But, now the new memory manager is in, this is one of the API's that will have to be reconsidered. We're still not entirely clear as to how it should really work. And there's the issue of GPU resource buffers that are accessible to the CPU simply by setting the high bit - that means those pages need to be "identity-mapped" and I'm not sure this already works reliably right now...
I've checked. Identity-mapping on contiguous memory allocations should work.
Tested with https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/commit/0709152531e62622d2033b3efb5e34b10a2fd4e2 Gamepad works again!
That's great news, but I haven't a clue why Gamepad 4531 still isn't working for me (in fact, it's regressed further and doesn't work at all)... I deleted my entire Cxbx folder from Appdata and tried again, but still no luck. I'm also using Cxbx-Reloaded/Cxbx-Reloaded@0709152
---------------------------
Cxbx-Reloaded
---------------------------
Received Exception Code 0xC0000005 @ EIP := 0x040594B3
Press "OK" to terminate emulation.
Press "Cancel" to debug.
---------------------------
OK Cancel
---------------------------
Another good test case is the cartoon sample, as it gets very slow or corrupted or it even crashes if the query didn't return correct sizes
Samples are working again! \o/
MirrorClip 4627
Cartoon 4627
TechCertGame 4631
And so on and so forth...
Sorry for the double post, but I only just realized that the Minnaert sample also boots now, with broken graphics:
(On a small side-note, I noticed that there are an awful lot of images in this issue now, so I will start to hotlink to images now instead of just displaying them.
I think it would be better to close this issue and create a different issue for each individual XDK sample, they are different pieces of software afterall. We don't/won't have a generic "homebrew games" or "emulator" issue, so why should XDK samples be any different?
This would be easier to manage too, as we could discuss issues that effect individual XDK samples much more cleanly.
There are so many XDK samples to cover though. I mean, I'd be happy to go through making issues for many of them right now, but wouldn't it clutter up the issue tracker?
At the end of the day, they are still unique pieces of software. There are a lot of them, but they're not part of a compilation or anything, they're entirely separate. But I understand what you mean, it would make a lot of issues...
I just think the current way of discussing them all in this issue doesn't work too good: There's too much information and things get lost, and when you read a comment it's not always clear which XDK sample it applies to.
Completely understand and couldn't agree more with you.
Alright, so I'll close this issue now, and any issues or regressions with any specific samples in due course I'll make an issue for.
From @Voxel9 on April 2, 2017 8:59
All XDK Samples listed in this issue's title (and probably more) will boot without any issues, albeit broken graphics, in cdc5d5cd and older commits. However, as of ecded47 these samples seem to no longer boot at all; only exceptions occur.
Example of the Gamepad sample working on the former, then being unable to boot on the latter:
KrnlDebug.txt (KrnlDebug log for ecded47 build)
Copied from original issue: Cxbx-Reloaded/Cxbx-Reloaded#316