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

IPC-HLE event handling improvements #432

Merged
merged 2 commits into from Jun 15, 2014

Conversation

magumagu
Copy link
Contributor

See commits for detailed description. Makes the System Menu work much better: installed channels can now be launched from the System Menu.

This is making changes to historically sensitive timing code, but testing hasn't found any regressions involving Wiimote handling (or anything else).

@lioncash
Copy link
Member

As far as I know, this looks good to me.

@JMC47
Copy link
Contributor

JMC47 commented May 28, 2014

I had to revise this comment because the disc channel stuff was removed.

The non-disc channel stuff works perfectly.

@magumagu
Copy link
Contributor Author

Taking out the commit for the disk channel; getting it right requires more changes, and those fixes will be easier to review separately.

@magumagu
Copy link
Contributor Author

Updated to be properly threadsafe.

@@ -65,7 +65,7 @@ void CWII_IPC_HLE_Device_hid::handleUsbUpdates(struct libusb_transfer *transfer)
// Return value
Memory::Write_U32(ret, replyAddress + 4);

WII_IPC_HLE_Interface::EnqueReplyCallback(replyAddress);
WII_IPC_HLE_Interface::EnqReply_Threadsafe(replyAddress);

This comment was marked as off-topic.

@neobrain
Copy link
Member

How extensively was this tested by @JMC47 ?

@magumagu: If we were to ship a release next week, would you think it's a good idea to merge this now anyway?

@delroth
Copy link
Member

delroth commented Jun 12, 2014

@neobrain if we were to ship a release next week this branch wouldn't be cherry-picked/merged to our stable branch. I don't see the point of that comment. We're not cutting a release next week as far as I know.

@JMC47
Copy link
Contributor

JMC47 commented Jun 12, 2014

I very extensively tested this and ipc-disc-channel2 + dvdlowopenpartition. It's a great set of features that I really like, and the other two rely on this merge. I'm biased because it lets me do really dumb stuff in Dolphin, like use the system menu proficiently.

This alone doesn't do much, I don't remember what it fixes alone and will need to retest it.

@@ -76,9 +76,9 @@ IWII_IPC_HLE_Device* es_handles[ES_MAX_COUNT];
typedef std::deque<u32> ipc_msg_queue;
static ipc_msg_queue request_queue; // ppc -> arm
static ipc_msg_queue reply_queue; // arm -> ppc
static std::mutex s_reply_queue;

static int enque_reply;

This comment was marked as off-topic.

@JMC47
Copy link
Contributor

JMC47 commented Jun 12, 2014

Okay, so after retesting it, this PR basically fixes pretty fundamental problems with loading/exiting games from the system menu. With this installed, I can load any channel that runs (except the disc channel, which is part of another branch,) play around in it, exit the channel back to the system menu. This includes Virtual Console games, Mii Channel, Internet Channel, etc.

After playing through with this alone, I really don't think it will cause issues. I'm about 95% sure that there are no regressions; whatever regressions there could be are too tricky for me to catch or so obvious I didn't notice them. Considering I had been testing the more complex builds that fixes a lot more and noticed nothing, I can say that this could be merged without issues.

@neobrain
Copy link
Member

Given that we won't be having a release for another while and that this change seems to be stable, this can be merged soonish. There are still some open PR comments though.

@magumagu
Copy link
Contributor Author

I think I've addressed all the review comments.

@neobrain
Copy link
Member

Is the second commit a correction to the first one? If so, please squash them together. Otherwise I'll merge this later today.

This change attempts to make calls to WII_IPC_HLE_Interface::Update()
more consistent.  We should be calling it whenever there is something we
can pop from a queue, and this patch does that.

DeviceUpdate() now works independently of Update(): it's now just a periodic
timer for devices which need to regularly update state.

With this change, we need a non-zero default delay for IPC replies because
replies are popped off the queue much more aggressively.

This fixes launching channels from the the System Menu.
neobrain added a commit that referenced this pull request Jun 15, 2014
IPC-HLE event handling improvements
@neobrain neobrain merged commit a6395ae into dolphin-emu:master Jun 15, 2014
@magumagu magumagu deleted the ipchle-timing branch June 16, 2014 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants