Optimise in-game atad driver for size #1050
Conversation
We know that the PS2 only has 32MiB of ram, so we cannot read more than 65535 anyway. Therefore we can get away with just 2 bytes to store the sector count. Also adjust the type and dir fields to use an appropriately sized type.
This function has been re-written for simpler and faster codegen, but with similar-ish timings to the original. Checked that less assembly instructions were generated with godbolt gcc mips 5.4 and 13.1.0.
the table is small enough that this optimization doesn't matter, and now we get to save a bit of code since we don't need to check the exit condition and jump out of the loop.
You can enable SMART, but it can't be disabled, nor can any read/write commands be issued, so it's not useful anyway.
…this condition: If the command _was_ ATA_C_SMART it willl no longer be found in the table, and we enter this check and still return ATA_RES_ERR_NOTREADY. If it was any other command we would have returned an error, but in that case the caller would need to error out anyway, so that can be assumed to never happen in practice or games would likely not function properly due to missing ATA commands.
This code is unreachable as there is a return on line 3, which appears to have been there since b30ce3e
… false: However we probably do want the harddisk to shutdown properly when this function is called.
We can never reach the if() with res != 0, except from where we add the goto in this commit.
Even some none-data commands set an alarm on ata_alarm_cb(), but in the case of a time-out the alarm would not be properly clean up nor would the LED be turned off. As a happy side-effect this saves 56 bytes.
|
For formatting changes, could you please do it in accordance with clang-format? If you really need to change how clang is formatting your code you can either: change |
|
In the current messy situation we have, these changes look good. However, I think the builtin ATA driver should be deleted, and replaced with the BDM enabled ATA driver from ps2sdk: This makes cdvdman more modular, plus the GUI can use the same drivers as the ingame cdvdman. So if your HDD works in the GUI, it will also work ingame. This change increases the number of iopcores, just like the changes from @grimdoomer to add BDM/exFat support to the HDD did. If we are going down this road, the following different iopcore versions will all have to be compiled into the OPL ELF:
Preferably I would like this to become:
Note that in the ps2sdk drivers there are also compile options for the ata driver. I think your size optimizations would be very welcome there, to create a new "mini" driver version (ata_bd_mini.irx). The compatibility with "gamestar" adapters should not be in a separate file if you ask me. There also seem to be many different versions of gamestar adapters. As I understand from grimdoomer's research there are:
If you merge |
Done.
I don't have enough context of the project's goals and direction to really comment on this, but personally it is my understanding that for maximum compatibility the in-game driver should be as small as possible. Using the full
I can't comment on this.
This sounds like a nice benefit, but please consider the trade-off in compatibility.
I was actually thinking of splitting-up
Sorry, I don't have the resources to spend on this. Also it seems quite out-of-scope for this PR. Even if the If you do want to make a
The only differences are the ordering of As to why it should be in a separate file, this is a run-time cost (few bytes) that everyone needs to pay otherwise. By splitting it off we gain a few bytes in both cases, so both sides even have a small advantage. |
daf0x
force-pushed
the
optimize-ingame-atad-driver
branch
from
9128ca4
to
b746f49
Compare
The large ps2sdk ATA driver works ingame just fine. I know becouse I'm using it in neutrino. Yes, it will use more memory, and that will reduce game compatibility. But in comparison to USB, the ATA driver will still use a lot less memory.
There are no official goals and directions. However this PR is a move in the opposite direction I would personally like it to go, and it's one of the reasons for creating neutrino.
That's a good idea, but I wish this too could be done in a modular way. Not doing this modular will lead to many different iopcores for each compile option. The new ATA options will increase it to 8 iopcores. Separating zso will double the cores to 16. The next option will increase to 32... modularity is the only solution if you ask me, but opinions differ on this. If other developers think differently about this, then please feel free to discuss and/or pull this PR. PS: The real memory optimizations are found elsewhere. For instance here: Open-PS2-Loader/modules/iopcore/common/cdvdman_opl.h Lines 65 to 67 in ea72571 Reducing this to 2 will free 12KiB of IOP RAM. It will also make the game a little slower. If you ask me this value should be configurable per game. So we can reduce it when needed. And have more speed when the game allows it.
|
Pull Request checklist
Note: these are not necessarily requirements
▷ I tried a few games and they all still seem to work the same.
Pull Request description
I've played around with the in-game ATAD driver code a bit and cleaned it up a bit and optimised it a tad, focusing on making it smaller. This probably looks like a rather big change, but I've tried to keep the changes small in commits that can be assessed individually.
Overview of the biggest changes
Move GameStar support to a browser-time choice
I.e. there are now three in-game drivers:
hdpro_atad,gamestar_atad,ps2atad, and OPL will select the appropriate one when launching a game.Drop support for SMART commands.
So the way it seems to work is you send
ATA_C_SMARTas a command, and then you put the actual SMART command as sub command. But the only supported sub command isATA_C_SMART_ENABLE, so you can enable SMART, but then you can't actually do anything with it like read the status? This seems unlikely to be needed.Remove code for commands that cannot occur.
There was some code checking for command types that are no longer in the list of supported commands (
ata_cmd_table).Re-use the upper bits of the type field to encode time-out and direction
This saves on checking conditions in code, as we can encode them in the table at compile time.
Re-write the busy-wait to use binary operations.
Result
All-in-all optimising just the atad-driver file alone saves 688 bytes:
Open-PS2-Loader/modules/iopcore/cdvdman/hdd_cdvdman.irx:Size before: 39.084 bytes
Size after: 38.396 bytes