Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cleaner RdramChanged algorithm (not biased to only 4 vs. 8 MB) #544

Merged
merged 4 commits into from
Jul 27, 2015
Merged

cleaner RdramChanged algorithm (not biased to only 4 vs. 8 MB) #544

merged 4 commits into from
Jul 27, 2015

Conversation

cxd4
Copy link
Contributor

@cxd4 cxd4 commented Jul 18, 2015

It's just confusing to see arbitrary values like 0x400000 or 0x800000 scattered through if/else.

Should it be if (old_size == 0x400000), if (old_size != 0x800000), if (old_size > 0x400000) ... none of those are clear to what's meant for the function to do.

To be more intelligent the function should really only be caring to VirtualFree/VirtualAlloc based on this:

if (old < new) {
    /* re-allocate RDRAM[old] extended up to RDRAM[new] */
} else if (old == new) {
    return;
} else {
    /* re-allocate RDRAM[old] freed down to RDRAM[new] */
}

project64 added a commit that referenced this pull request Jul 27, 2015
cleaner RdramChanged algorithm (not biased to only 4 vs. 8 MB)
@project64 project64 merged commit 1a481c5 into project64:master Jul 27, 2015
@cxd4 cxd4 deleted the dedotated_wam_4_mah_serv0r branch July 27, 2015 15:20
@Azimer
Copy link
Contributor

Azimer commented Jul 28, 2015

The only thing that makes me curious about this is what if there are bad rdb values? Should you allow the end user to hang themselves with the bad value or have it corrected? Perhaps that is somewhere else in the code.

@cxd4
Copy link
Contributor Author

cxd4 commented Jul 28, 2015

Well actually @Azimer that was part of why I felt like making these changes.

In my 64-bit fork of Project64 1.4 (and I know this will sound foolishly naive) I added options to use more than just 4 or 8 MB of RDRAM. A couple games were accessing the limiters for up to 64 or even (in Aleck64 cases) 512 MB of RDRAM. Since the 32-bit Windows code for MemoryFilter was not portable to 64-bit, my options were

  • a) let the ROMs crash on reading past 8 MB of RAM
  • b) implement address range checking in software for LW, LB, LH ...
  • c) add arbitrary options to allocate more than 8 MB of RDRAM.

Out of curiosity and personal interest in what some games did, I felt like playing with c) for the sake of learning. But yes, ideally you shouldn't be able to allocate more than 16 MB of RDRAM.

And according to AIO/RPGMaster, it already is possible to edit the CFG file (but not the RDB file--only the CFG file lets you do this) to use more than 8 MB of RDRAM. He noticed a corruption of the allocated RDRAM while loading saved states or switching ROMs, and that was the other thing this pull request was meant to fix.

@LegendOfDragoon
Copy link
Contributor

@Azimer, currently the only valid sizes for RDRAM in the RDB are 4 MB and 8 MB. Here's where it's located if you want to look at & possibly change. https://github.com/project64/project64/blob/master/Source/Project64/Settings/SettingType/SettingsType-RDBRamSize.cpp#L37

@cxd4
Copy link
Contributor Author

cxd4 commented Jul 28, 2015

Nah, not going to change. I would agree mostly with Azimer that settings besides 4 or 8 MB are usually "bad values" and don't care to implement them in the RDB file either. Only in my 1.4 fork.

What RPGMaster was talking about, was the CFG file in what I said about it letting you set values besides 4 or 8 MB. So the CFG has the bug already; the aim of this pull request was to dissolve its corruption.

@LegendOfDragoon
Copy link
Contributor

I was thinking of maybe adding a 16MB option for the RDB.

@DerekTurtleRoe DerekTurtleRoe mentioned this pull request Oct 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants