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.

Sign up for GitHub

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

Make sure to run callback on AdhocThread (extracted from #13132). #13173

Merged
merged 2 commits into from
Jul 20, 2020
Merged

Make sure to run callback on AdhocThread (extracted from #13132). #13173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f3b473
Making sure adhoc callbacks runs on AdhocThread
anr2me Nov 11, 2019
da5b54e
Separate each AdhocMatching callback's buffer, since callback aren't …
anr2me Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 21 additions & 15 deletions Core/HLE/proAdhoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,11 +935,13 @@ void AfterMatchingMipsCall::run(MipsCall &call) {
}
context->eventlock->unlock(); //peerlock.unlock();
//call.setReturnValue(v0);
if (Memory::IsValidAddress(bufAddr)) userMemory.Free(bufAddr);
DEBUG_LOG(SCENET, "Leaving AfterMatchingMipsCall::run [ID=%i][Event=%d] [retV0: %08x]", context->id, EventID, currentMIPS->r[MIPS_REG_V0]);
}

void AfterMatchingMipsCall::SetContextID(u32 ContextID, u32 eventId) {
void AfterMatchingMipsCall::SetContextID(u32 ContextID, u32 eventId, u32_le BufAddr) {
EventID = eventId;
bufAddr = BufAddr;
peerlock.lock();
context = findMatchingContext(ContextID);
peerlock.unlock();
Expand All @@ -949,41 +951,45 @@ void AfterMatchingMipsCall::SetContextID(u32 ContextID, u32 eventId) {
void notifyAdhocctlHandlers(u32 flag, u32 error) {
__UpdateAdhocctlHandlers(flag, error);
// TODO: We should use after action instead of guessing the time like this
sleep_ms(20); // Ugly workaround to give time for the mips callback to fully executed, usually only need <16ms
//sleep_ms(20); // Ugly workaround to give time for the mips callback to fully executed, usually only need <16ms
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anr2me Why was this commented out?

Copy link
Collaborator

@anr2me anr2me Jul 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it was delayed to make sure previous callback is already fully executed before calling another one, but how much time a callback required to be done is just a guess, so i decide to use After action instead to be exact, but later i decide to give the delay (hleDelayResult) within __NetTriggerCallbacks where it process a single callback once per HLE call instead of multiple callbacks.
The delay time is still a guess and the after action ended doing nothing LOL but apparently that delay need to be adjusted based on I/O Timing method, there were multiplayer issue caused by different I/O Timing method (Fast was the best one for multiplayer).
I think after action is a better way to make sure a mipscall is fully executed, right?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no. I wouldn't have guessed that I/O timing would matter :(

But I guess when it comes to networking it's all a very inexact science...

After action definitely seems like a better way.

}

// Matching callback is void function: typedef void(*SceNetAdhocMatchingHandler)(int id, int event, SceNetEtherAddr * peer, int optlen, void * opt);
// Important! The MIPS call need to be fully executed before the next MIPS call invoked, as the game (ie. DBZ Tag Team) may need to prepare something for the next callback event to use
// Note: Must not lock peerlock within this function to prevent race-condition with other thread whos owning peerlock and trying to lock context->eventlock owned by this thread
void notifyMatchingHandler(SceNetAdhocMatchingContext * context, ThreadMessage * msg, void * opt, u32 &bufAddr, u32 &bufLen, u32_le * args) {
//u32_le args[5] = { 0, 0, 0, 0, 0 };
if ((s32)bufLen < (msg->optlen + 8)) {
/*if ((s32)bufLen < (msg->optlen + 8)) {
bufLen = msg->optlen + 8;
if (Memory::IsValidAddress(bufAddr)) userMemory.Free(bufAddr);
bufAddr = userMemory.Alloc(bufLen);
bufAddr = userMemory.Alloc(bufLen); // Max bufLen should be context->rxbuflen
INFO_LOG(SCENET, "MatchingHandler: Alloc(%i -> %i) = %08x", msg->optlen + 8, bufLen, bufAddr);
}
}*/
MatchingArgs argsNew;
bufAddr = userMemory.Alloc(bufLen); // We will free this after returning from mipscall
u8 * optPtr = Memory::GetPointer(bufAddr);
memcpy(optPtr, &msg->mac, sizeof(msg->mac));
if (msg->optlen > 0) memcpy(optPtr + 8, opt, msg->optlen);
args[0] = context->id;
args[1] = msg->opcode;
args[2] = bufAddr; // PSP_GetScratchpadMemoryBase() + 0x6000;
args[3] = msg->optlen;
args[4] = args[2] + 8;
args[5] = context->handler.entryPoint; //not part of callback argument, just borrowing a space to store callback address so i don't need to search the context first later
argsNew.data[0] = context->id;
argsNew.data[1] = msg->opcode;
argsNew.data[2] = bufAddr; // PSP_GetScratchpadMemoryBase() + 0x6000;
argsNew.data[3] = msg->optlen;
argsNew.data[4] = argsNew.data[2] + 8; // OptData Addr
argsNew.data[5] = context->handler.entryPoint; //not part of callback argument, just borrowing a space to store callback address so i don't need to search the context first later

context->eventlock->lock();
context->IsMatchingInCB = true;
context->eventlock->unlock();
// ScheduleEvent_Threadsafe_Immediate seems to get mixed up with interrupt (returning from mipscall inside an interrupt) and getting invalid address before returning from interrupt
__UpdateMatchingHandler((u64) args);
__UpdateMatchingHandler(argsNew);

// Make sure MIPS call have been fully executed before the next notifyMatchingHandler
int count = 0;
while (/*(after != NULL) &&*/ IsMatchingInCallback(context) && (count < 250)) {
/*int count = 0;
while ( IsMatchingInCallback(context) && (count < 250)) {
sleep_ms(1);
count++;
}
if (count >= 250) ERROR_LOG(SCENET, "MatchingHandler: Callback Failed to Return within %dms!", count);
if (count >= 250) ERROR_LOG(SCENET, "MatchingHandler: Callback Failed to Return within %dms! [ID=%i][Opcode=%d][OptSize=%d][MAC=%012X]", count, context->id, msg->opcode, msg->optlen, htonl(*(u_long*)&msg->mac));*/
//sleep_ms(20); // Wait a little more (for context switching may be?) to prevent DBZ Tag Team from getting connection lost, but this will cause lags on Lord of Arcana
}

Expand Down
6 changes: 4 additions & 2 deletions Core/HLE/proAdhoc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ typedef struct SceNetAdhocMatchingContext {
SceNetAdhocMatchingHandler handler;

// Event Handler Args
u32_le handlerArgs[6]; // actual arguments only 5, the 6th one is just for borrowing a space to store the callback address to use later
u32_le handlerArgs[6]; //MatchingArgs handlerArgs; // actual arguments only 5, the 6th one is just for borrowing a space to store the callback address to use later
//SceNetAdhocMatchingHandlerArgs handlerArgs;

// Hello Data Length
Expand Down Expand Up @@ -776,11 +776,13 @@ class AfterMatchingMipsCall : public PSPAction {
//context = NULL;
}
void run(MipsCall &call) override;
void SetContextID(u32 ContextID, u32 eventId);
void SetContextID(u32 ContextID, u32 eventId, u32_le BufAddr);
void SetContext(SceNetAdhocMatchingContext* Context, u32 eventId, u32_le BufAddr) { context = Context; EventID = eventId; bufAddr = BufAddr; }

private:
u32 EventID = 0;
SceNetAdhocMatchingContext *context = nullptr;
u32_le bufAddr = 0;
};

extern int actionAfterMatchingMipsCall;
Expand Down
66 changes: 36 additions & 30 deletions Core/HLE/sceNetAdhoc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ SceUID threadAdhocID;

std::mutex adhocEvtMtx;
std::vector<std::pair<u32, u32>> adhocctlEvents;
std::vector<u64> matchingEvents;
std::vector<MatchingArgs> matchingEvents;
u32 dummyThreadHackAddr = 0;
u32_le dummyThreadCode[3];

Expand Down Expand Up @@ -124,7 +124,7 @@ void __UpdateAdhocctlHandlers(u32 flag, u32 error) {
}

// TODO: MipsCall needs to be called from it's own PSP Thread instead of from any random PSP Thread
void __UpdateMatchingHandler(u64 ArgsPtr) {
void __UpdateMatchingHandler(MatchingArgs ArgsPtr) {
std::lock_guard<std::mutex> adhocGuard(adhocEvtMtx);
matchingEvents.push_back(ArgsPtr);
}
Expand Down Expand Up @@ -168,15 +168,6 @@ u32 sceNetAdhocInit() {
// Library initialized
netAdhocInited = true;

// Create fake PSP Thread for callback
// TODO: Should use a separated threads for friendFinder, matchingEvent, and matchingInput and created on AdhocctlInit & AdhocMatchingStart instead of here
#define PSP_THREAD_ATTR_KERNEL 0x00001000 // PSP_THREAD_ATTR_KERNEL is located in sceKernelThread.cpp instead of sceKernelThread.h :(
// TODO: This should probably be a user thread, but maybe from sceNetAdhocctlInit?
threadAdhocID = __KernelCreateThread("AdhocThread", __KernelGetCurThreadModuleId(), dummyThreadHackAddr, 0x10, 0x1000, PSP_THREAD_ATTR_KERNEL, 0, true);
if (threadAdhocID > 0) {
__KernelStartThread(threadAdhocID, 0, 0);
}

// Return Success
return hleLogSuccessInfoI(SCENET, 0, "at %08x", currentMIPS->pc);
}
Expand All @@ -189,9 +180,20 @@ static u32 sceNetAdhocctlInit(int stackSize, int prio, u32 productAddr) {

if (netAdhocctlInited)
return ERROR_NET_ADHOCCTL_ALREADY_INITIALIZED;

// Create fake PSP Thread for callback
// TODO: Should use a separated threads for friendFinder, matchingEvent, and matchingInput and created on AdhocctlInit & AdhocMatchingStart instead of here
#define PSP_THREAD_ATTR_KERNEL 0x00001000 // Using constants instead of numbers for readability reason, since PSP_THREAD_ATTR_KERNEL/USER is located in sceKernelThread.cpp instead of sceKernelThread.h
#define PSP_THREAD_ATTR_USER 0x80000000

threadAdhocID = __KernelCreateThread("AdhocThread", __KernelGetCurThreadModuleId(), dummyThreadHackAddr, prio, stackSize, PSP_THREAD_ATTR_USER, 0, false);
if (threadAdhocID > 0) {
__KernelStartThread(threadAdhocID, 0, 0);
}

if(g_Config.bEnableWlan) {
if (initNetwork((SceNetAdhocctlAdhocId *)Memory::GetPointer(productAddr)) == 0) {
// TODO: Merging friendFinder (real) thread to AdhocThread (fake) thread on PSP side
if (!friendFinderRunning) {
friendFinderRunning = true;
friendFinderThread = std::thread(friendFinder);
Expand Down Expand Up @@ -1138,6 +1140,12 @@ int sceNetAdhocctlTerm() {
// Free stuff here
closesocket(metasocket);
metasocket = (int)INVALID_SOCKET;
// Delete fake PSP Thread
if (threadAdhocID != 0) {
__KernelStopThread(threadAdhocID, SCE_KERNEL_ERROR_THREAD_TERMINATED, "AdhocThread stopped");
__KernelDeleteThread(threadAdhocID, SCE_KERNEL_ERROR_THREAD_TERMINATED, "AdhocThread deleted");
threadAdhocID = 0;
}
/*#ifdef _MSC_VER
WSACleanup(); // Might be better to call WSAStartup/WSACleanup from sceNetInit/sceNetTerm isn't? since it's the first/last network function being used, even better to put it in __NetInit/__NetShutdown as it's only called once
#endif*/
Expand Down Expand Up @@ -1426,12 +1434,6 @@ int sceNetAdhocTerm() {

// Library is initialized
if (netAdhocInited) {
// Delete fake PSP Thread
if (threadAdhocID != 0) {
__KernelStopThread(threadAdhocID, SCE_KERNEL_ERROR_THREAD_TERMINATED, "AdhocThread stopped");
__KernelDeleteThread(threadAdhocID, SCE_KERNEL_ERROR_THREAD_TERMINATED, "AdhocThread deleted");
}

// Delete PDP Sockets
deleteAllPDP();

Expand Down Expand Up @@ -2665,7 +2667,7 @@ static int sceNetAdhocMatchingCreate(int mode, int maxnum, int port, int rxbufle
context->hello_int = hello_int; // client might set this to 0
if (keepalive_int < 1) context->keepalive_int = PSP_ADHOCCTL_PING_TIMEOUT; else context->keepalive_int = keepalive_int; // client might set this to 0
context->keepalivecounter = init_count; // used to multiply keepalive_int as timeout
context->timeout = (keepalive_int * init_count);
context->timeout = ((u64_le)keepalive_int * (u64_le)init_count);
if (context->timeout < 5000000) context->timeout = 5000000; // For internet play we need higher timeout than what the game wanted
context->handler = handler;

Expand Down Expand Up @@ -3489,19 +3491,24 @@ void __NetTriggerCallbacks()

for (std::map<int, AdhocctlHandler>::iterator it = adhocctlHandlers.begin(); it != adhocctlHandlers.end(); ++it) {
args[2] = it->second.argument;
__KernelDirectMipsCall(it->second.entryPoint, NULL, args, 3, true);
__KernelSwitchToThread(threadAdhocID, "AdhocctlHandler Mipscall"); // it's not guaranteed to switch successfully tho
//while (__KernelInCallback()) sleep_ms(1);
__KernelDirectMipsCall(it->second.entryPoint, NULL, args, 3, false);
}
}
adhocctlEvents.clear();
adhocctlEvents.clear(); // We should only clear this after making sure all callbacks are placed on the right thread tho

for (auto &param : matchingEvents)
{
u32_le *args = (u32_le *) param;
u32_le *args = (u32_le*)&param;
AfterMatchingMipsCall *after = (AfterMatchingMipsCall *) __KernelCreateAction(actionAfterMatchingMipsCall);
after->SetContextID(args[0], args[1]);
__KernelDirectMipsCall(args[5], after, args, 5, true);
after->SetContextID(args[0], args[1], args[2]);
// Need to make sure currentThread is AdhocMatching's eventThread before calling the callback, and not in the middle of callback
__KernelSwitchToThread(threadAdhocID, "AdhocMatchingEvent Mipscall"); // it's not guaranteed to switch successfully tho
//while (__KernelInCallback()) sleep_ms(1);
__KernelDirectMipsCall(args[5], after, args, 5, false);
}
matchingEvents.clear();
matchingEvents.clear(); // We should only clear this after making sure all callbacks are placed on the right thread tho
}
//magically make this work
hleDelayResult(0, "Prevent Adhoc thread from blocking", 1000);
Expand Down Expand Up @@ -4770,9 +4777,6 @@ void actOnByePacket(SceNetAdhocMatchingContext * context, SceNetEtherAddr * send
*/
int matchingEventThread(int matchingId)
{
u32 bufLen = 0;
u32 bufAddr = 0;

// Multithreading Lock
peerlock.lock();
// Cast Context
Expand All @@ -4785,6 +4789,8 @@ int matchingEventThread(int matchingId)

// Run while needed...
if (context != NULL) {
u32 bufLen = context->rxbuflen; //0;
u32 bufAddr = 0; //userMemory.Alloc(bufLen);
//static u32_le args[5] = { 0, 0, 0, 0, 0 }; // Need to be global/static so it can be accessed from a different thread
u32_le * args = context->handlerArgs;

Expand Down Expand Up @@ -4862,16 +4868,16 @@ int matchingEventThread(int matchingId)
context->eventlock->unlock();
}

// Free memory
//if (Memory::IsValidAddress(bufAddr)) userMemory.Free(bufAddr);

// Delete Pointer Reference (and notify caller about finished cleanup)
//context->eventThread = NULL;
}

// Log Shutdown
INFO_LOG(SCENET, "EventLoop: End of EventLoop[%i] Thread", matchingId);

// Free memory
if (Memory::IsValidAddress(bufAddr)) userMemory.Free(bufAddr);

// Return Zero to shut up Compiler (never reached anyway)
return 0;
}
Expand Down
6 changes: 5 additions & 1 deletion Core/HLE/sceNetAdhoc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@

#pragma once

typedef struct MatchingArgs {
u32_le data[6]; //ContextID, Opcode, bufAddr[ to MAC], OptLen, OptAddr[, EntryPoint]
} PACK;

class PointerWrap;

void Register_sceNetAdhoc();
Expand All @@ -25,7 +29,7 @@ void __NetAdhocInit();
void __NetAdhocShutdown();
void __NetAdhocDoState(PointerWrap &p);
void __UpdateAdhocctlHandlers(u32 flags, u32 error);
void __UpdateMatchingHandler(u64 params);
void __UpdateMatchingHandler(MatchingArgs params);

// I have to call this from netdialog
int sceNetAdhocctlCreate(const char * groupName);
Expand Down