DEV9: Add support for 8/16 bit dev9 rxfifo reads#14474
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for submitting a contribution to PCSX2
As this is your first pull request, please be aware of the contributing guidelines.
Additionally, as per recent changes in GitHub Actions, your pull request will need to be approved by a maintainer before GitHub Actions can run against it. You can find more information about this change here.
Please be patient until this happens. In the meantime if you'd like to confirm the builds are passing, you have the option of opening a PR on your own fork, just make sure your fork's master branch is up to date!
| default: | ||
| DevCon.WriteLn("DEV9: SMAP : Unknown 8 bit read @ %X,v=%X", addr, dev9Ru8(addr)); | ||
| return dev9Ru8(addr); | ||
| DevCon.WriteLn("DEV9: SMAP: Unknown 8 bit read"); |
There was a problem hiding this comment.
Modified this to drop into the below return dev9Ru8(addr); as the above statements do.
| } | ||
|
|
||
| DevCon.WriteLn("DEV9: SMAP : error , 8 bit read @ %X,v=%X", addr, dev9Ru8(addr)); | ||
| DevCon.WriteLn("DEV9: SMAP: 8 bit read @ %X,v=%X", addr, dev9Ru8(addr)); |
There was a problem hiding this comment.
This line was a bit confusing to me - 2 (now 3) of the possible cases end up in this state and execute this - are they error states? I've edited it here but am of course happy to revert this if it being an error state is indeed correct.
| //DevCon.WriteLn("DEV9: SMAP_R_RXFIFO_DATA 32bit read %x", rv); | ||
| return rv; | ||
| } | ||
| return smap_readRxfifo(4); |
There was a problem hiding this comment.
It's true that the original code here is more efficient - one 32 bit read rather than four eight bit reads. My understanding is that this is relatively insignificant, but I am happy to revert this to the original code if desired.
Pritchy96
changed the title
Pritchy96
force-pushed
the
rx_fifo_16_8_read
branch
from
40035b2 to
cb81d12
Compare
|
If possible, I'd prefer a minimal homebrew for the PS2 than can be used to test this, rather than assuming it's better because a homebrew suddenly works. Maybe @TheLastRar has other thoughts. I'm not familiar with this code. |
Yeah, normally I'd agree that the reproducer I've given isn't really enough - I just figured this might be more of a cut-and-dried case as it appears to be missing functionality rather than a true bug. I've been meaning to try writing some PS2 homebrew at some point, though.. |
3 participants
Description of Changes
Added a function to implement 8 and 16 bit reads from the SMAP RX FIFO.
Rationale behind Changes
When testing out Neutrino in PCSX2, I noticed booting a game over UDPFS would fail due to it attempting to do a six byte read of the SMAP RX FIFO - the last two bytes would come back blanked out to zeroes. Digging down, I've figured out this is because it's split into a four byte read, with smap_read32, then an attempted two byte read with smap_read16. However, smap_read16 has no case for reading from the rx_fifo, leading it to drop into the default case which just reads from dev9R.
To resolve this, I've added a function which can read 1, 2 or 4 bytes from the rxfifo and used it for the corresponding smap_read8, smap_read16, smap_read32 functions.
I should note - I'm new to PCSX2 and the PS2 architecture in general - it's very possible that Neutrino is doing something wrong in doing that six byte read and that PCSX2 is more correct to not implementing this. I just figured that this solution does get Neutrino to boot, and that raising a PR would be a good way to get visibility on this potential issue.
Suggested Testing Steps
Booting neutrino with
<full_path_to_pcsx2> -elf <full_path_to_neutrino.elf> -gameargs "-bsd=udpfs -dvd=udpfs:.<path_to_game_on_udpfs>will give the error "File is not a valid ISO" without this fix - the header mismatches due to the last two bytes being zero.Did you use AI to help find, test, or implement this issue or feature?
No.