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

Schedule the interrupt signaling the end of an Aica DMA transfer #1412

Merged
merged 2 commits into from Oct 5, 2018

Conversation

Projects
None yet
5 participants
@flyinghead
Collaborator

flyinghead commented Sep 26, 2018

Schedule the interrupt signaling the end of an Aica DMA transfer instead of raising it immediately.

Fixes Street Fighter Alpha 3 and Bomberman Online

Schedule the interrupt signaling the end of an Aica DMA transfer instead
of raising it immediately.

Fixes Street Fighter Alpha 3 and Bomberman Online
@MrPsyMan

I'd suggest to comment that this is a hack, to avoid negative side-effects if it's handled properly in the future.

@skmp skmp referenced this pull request Sep 27, 2018

Open

More accurately model DMA behavior (non-instant dma) #1414

0 of 6 tasks complete
@skmp

This comment has been minimized.

Member

skmp commented Sep 27, 2018

This is definitely a step in the right direction. There's various nuances that we should implement, or at least assert about, like dma pauses, g1/g2 locks, and dma aborts.

This is much cleaner than other diffs I've seen trying to make DMAs take time.

@flyinghead do you want to add detection for pauses / aborts before merging this in? I'm fine with us handling it at a later time. Created a general follow up ticket, #1414.

@MrPsyMan do you think this should be included in the 18.10 release? I'd be wary to push it out without a thorough pass of 60/70 games myself, because of how easily timing can break games, and how much unimplemented functionality is exposed by making the DMA not be instant. I'm also wary that without asserts for unhandled cases we'll get 'silent breaks' that are only found out months later. Am I too cautious?

edit
I'd say this is more of a partial implementation than a hack.

edit 2
Ahh, I see the more dubious looking code was reverted as part of #1394

edit 3
@flyinghead how about adding an option for it, "AsyncAICADma", and enable it with some per-game configs? Then even if it causes issues users will have a workaround, and they can turn it if they want to test for other games.

@AbandonedCart

This comment has been minimized.

Contributor

AbandonedCart commented Sep 27, 2018

how about adding an option for it, "AsyncAICADma", and enable it with some per-game configs? Then even if it causes issues users will have a workaround, and they can turn it if they want to test for other games.

Isn't the purpose of this to do away with needing to configure it at all and actually fix the functionality, though? It seems like that would be 2 steps forward, one step back.

@MrPsyMan

This comment has been minimized.

Contributor

MrPsyMan commented Sep 27, 2018

I'd say to add it as an option enabled by default, until more related functionality is implemented.

If it turns out it breaks something, it'd be far easier to pinpoint if this is the cause when the users can turn it off.

Show resolved Hide resolved core/nullDC.cpp Outdated

@skmp skmp force-pushed the fh/aica-int-timing branch from d2edf84 to 09f8a50 Oct 5, 2018

@skmp skmp merged commit 4ceac65 into master Oct 5, 2018

4 of 9 checks passed

LGTM analysis: C/C++ Running analyses for revisions
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
license/cla Contributor License Agreement is signed.
Details
wercker/build Wercker pipeline passed
Details

@flyinghead flyinghead deleted the fh/aica-int-timing branch Oct 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment