From 3f42a0b99543f04673d7b8b38fe64a0aef2fb30a Mon Sep 17 00:00:00 2001 From: Erictoby <54815823+Erictoby@users.noreply.github.com> Date: Mon, 2 Sep 2019 15:49:39 -0400 Subject: [PATCH 1/5] Fixes cursor movement lagging or jumping since 2.1.4+ --- VoodooGPIO/VoodooGPIO.cpp | 20 -------------------- VoodooGPIO/VoodooGPIO.hpp | 2 -- 2 files changed, 22 deletions(-) diff --git a/VoodooGPIO/VoodooGPIO.cpp b/VoodooGPIO/VoodooGPIO.cpp index d29fc75..e919fb0 100644 --- a/VoodooGPIO/VoodooGPIO.cpp +++ b/VoodooGPIO/VoodooGPIO.cpp @@ -85,26 +85,6 @@ void VoodooGPIO::writel(UInt32 b, IOVirtualAddress addr) { *(volatile UInt32 *)(addr) = b; } -IOWorkLoop* VoodooGPIO::getWorkLoop() { - // Do we have a work loop already?, if so return it NOW. - if ((vm_address_t) workLoop >> 1) - return workLoop; - - if (OSCompareAndSwap(0, 1, reinterpret_cast(&workLoop))) { - // Construct the workloop and set the cntrlSync variable - // to whatever the result is and return - workLoop = IOWorkLoop::workLoop(); - } else { - while (reinterpret_cast(workLoop) == reinterpret_cast(1)) { - // Spin around the cntrlSync variable until the - // initialization finishes. - thread_block(0); - } - } - - return workLoop; -} - struct intel_community *VoodooGPIO::intel_get_community(unsigned pin) { struct intel_community *community; for (int i = 0; i < ncommunities; i++) { diff --git a/VoodooGPIO/VoodooGPIO.hpp b/VoodooGPIO/VoodooGPIO.hpp index 265e0ce..3f3518b 100644 --- a/VoodooGPIO/VoodooGPIO.hpp +++ b/VoodooGPIO/VoodooGPIO.hpp @@ -203,8 +203,6 @@ class VoodooGPIO : public IOService { UInt32 readl(IOVirtualAddress addr); void writel(UInt32 b, IOVirtualAddress addr); - IOWorkLoop* getWorkLoop(); - struct intel_community *intel_get_community(unsigned pin); const struct intel_padgroup *intel_community_get_padgroup(const struct intel_community *community, unsigned pin); IOVirtualAddress intel_get_padcfg(unsigned pin, unsigned reg); From cf8387fbf82982643348847f594a471747b6188d Mon Sep 17 00:00:00 2001 From: erictoby <54815823+Erictoby@users.noreply.github.com> Date: Sun, 8 Sep 2019 12:28:14 -0400 Subject: [PATCH 2/5] Fixes deadlock or delays in interrupt loop. runAction is not allowed in interrupt loop. --- VoodooGPIO/VoodooGPIO.cpp | 20 ++++++++++++++++---- VoodooGPIO/VoodooGPIO.hpp | 5 +++-- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/VoodooGPIO/VoodooGPIO.cpp b/VoodooGPIO/VoodooGPIO.cpp index e919fb0..11ed167 100644 --- a/VoodooGPIO/VoodooGPIO.cpp +++ b/VoodooGPIO/VoodooGPIO.cpp @@ -265,7 +265,7 @@ void VoodooGPIO::intel_gpio_irq_enable(UInt32 pin) { gpp = padgrp->reg_num; gpp_offset = padgroup_offset(padgrp, pin); /* Clear interrupt status first to avoid unexpected interrupt */ - writel(BIT(gpp_offset), community->regs + GPI_IS + gpp * 4); + writel((UInt32)BIT(gpp_offset), community->regs + GPI_IS + gpp * 4); value = readl(community->regs + community->ie_offset + gpp * 4); value |= BIT(gpp_offset); @@ -365,7 +365,7 @@ bool VoodooGPIO::intel_pinctrl_add_padgroups(intel_community *community) { unsigned gpp_size = community->gpp_size; gpps[i].reg_num = i; gpps[i].base = community->pin_base + i * gpp_size; - gpps[i].size = min(gpp_size, npins); + gpps[i].size = (UInt32)min(gpp_size, npins); gpps[i].gpio_base = 0; npins -= gpps[i].size; } @@ -544,6 +544,8 @@ bool VoodooGPIO::start(IOService *provider) { if (!IOService::start(provider)) return false; + isInterruptBusy = false; + PMinit(); workLoop = getWorkLoop(); @@ -696,7 +698,7 @@ void VoodooGPIO::stop(IOService *provider) { } if (workLoop) { - workLoop->release(); + // workLoop->release(); workLoop = NULL; } @@ -882,11 +884,21 @@ IOReturn VoodooGPIO::setInterruptTypeForPin(int pin, int type) { } void VoodooGPIO::InterruptOccurred(OSObject *owner, IOInterruptEventSource *src, int intCount) { - command_gate->runAction(OSMemberFunctionCast(IOCommandGate::Action, this, &VoodooGPIO::interruptOccurredGated)); + if (isInterruptBusy) + return; + isInterruptBusy = true; + + IOReturn ret = command_gate->attemptAction(OSMemberFunctionCast(IOCommandGate::Action, this, &VoodooGPIO::interruptOccurredGated)); + if (ret != kIOReturnSuccess) { + isInterruptBusy = false; + IOLog("%s:InterruptOccurred:Failed on attemptAction(). Error code = %X", getName(), ret); + } } + void VoodooGPIO::interruptOccurredGated() { for (int i = 0; i < ncommunities; i++) { struct intel_community *community = &communities[i]; intel_gpio_community_irq_handler(community); } + isInterruptBusy = false; } diff --git a/VoodooGPIO/VoodooGPIO.hpp b/VoodooGPIO/VoodooGPIO.hpp index 3f3518b..389bc40 100644 --- a/VoodooGPIO/VoodooGPIO.hpp +++ b/VoodooGPIO/VoodooGPIO.hpp @@ -197,8 +197,9 @@ class VoodooGPIO : public IOService { bool controllerIsAwake; IOWorkLoop *workLoop; - IOInterruptEventSource *interruptSource; - IOCommandGate* command_gate; + IOInterruptEventSource *interruptSource = NULL; + IOCommandGate* command_gate = NULL; + bool isInterruptBusy; UInt32 readl(IOVirtualAddress addr); void writel(UInt32 b, IOVirtualAddress addr); From eb42516854b9cb5fbe7eb2ad071cbdf1da316425 Mon Sep 17 00:00:00 2001 From: erictoby <54815823+Erictoby@users.noreply.github.com> Date: Sat, 14 Sep 2019 18:29:59 -0400 Subject: [PATCH 3/5] Enable GPIO interrupt after initing all parameters The bug can cause panic or unresponsive touchpad. --- VoodooGPIO/VoodooGPIO.cpp | 44 +++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/VoodooGPIO/VoodooGPIO.cpp b/VoodooGPIO/VoodooGPIO.cpp index 11ed167..8c935f4 100644 --- a/VoodooGPIO/VoodooGPIO.cpp +++ b/VoodooGPIO/VoodooGPIO.cpp @@ -544,7 +544,7 @@ bool VoodooGPIO::start(IOService *provider) { if (!IOService::start(provider)) return false; - isInterruptBusy = false; + isInterruptBusy = true; PMinit(); @@ -556,22 +556,21 @@ bool VoodooGPIO::start(IOService *provider) { } workLoop->retain(); + command_gate = IOCommandGate::commandGate(this); + if (!command_gate || (workLoop->addEventSource(command_gate) != kIOReturnSuccess)) { + IOLog("%s Could not open command gate\n", getName()); + stop(provider); + return false; + } + interruptSource = IOInterruptEventSource::interruptEventSource(this, OSMemberFunctionCast(IOInterruptEventAction, this, &VoodooGPIO::InterruptOccurred), provider); if (!interruptSource) { IOLog("%s::Failed to get GPIO Controller interrupt!\n", getName()); stop(provider); return false; } - workLoop->addEventSource(interruptSource); interruptSource->enable(); - - command_gate = IOCommandGate::commandGate(this); - if (!command_gate || (workLoop->addEventSource(command_gate) != kIOReturnSuccess)) { - IOLog("%s Could not open command gate\n", getName()); - stop(provider); - return false; - } IOLog("%s::VoodooGPIO Init!\n", getName()); @@ -634,6 +633,7 @@ bool VoodooGPIO::start(IOService *provider) { intel_pinctrl_pm_init(); + isInterruptBusy = false; controllerIsAwake = true; registerService(); @@ -666,6 +666,17 @@ bool VoodooGPIO::start(IOService *provider) { void VoodooGPIO::stop(IOService *provider) { IOLog("%s::VoodooGPIO stop!\n", getName()); + if (interruptSource) { + interruptSource->disable(); + workLoop->removeEventSource(interruptSource); + OSSafeReleaseNULL(interruptSource); + } + + if (command_gate) { + workLoop->removeEventSource(command_gate); + OSSafeReleaseNULL(command_gate); + } + intel_pinctrl_pm_release(); for (int i = 0; i < ncommunities; i++) { @@ -686,17 +697,6 @@ void VoodooGPIO::stop(IOService *provider) { IOFree(communities[i].pinInterruptRefcons, sizeof(void *) * communities[i].npins); } - if (interruptSource) { - interruptSource->disable(); - workLoop->removeEventSource(interruptSource); - OSSafeReleaseNULL(interruptSource); - } - - if (command_gate) { - workLoop->removeEventSource(command_gate); - OSSafeReleaseNULL(command_gate); - } - if (workLoop) { // workLoop->release(); workLoop = NULL; @@ -806,7 +806,7 @@ IOReturn VoodooGPIO::registerInterrupt(int pin, OSObject *target, IOInterruptAct if (hw_pin < 0) return kIOReturnNoInterrupt; - IOLog("%s::Registering hardware pin %d for GPIO IRQ pin %u", getName(), hw_pin, pin); + IOLog("%s::Registering hardware pin %d for GPIO IRQ pin %u\n", getName(), hw_pin, pin); unsigned communityidx = hw_pin - community->pin_base; @@ -891,7 +891,7 @@ void VoodooGPIO::InterruptOccurred(OSObject *owner, IOInterruptEventSource *src, IOReturn ret = command_gate->attemptAction(OSMemberFunctionCast(IOCommandGate::Action, this, &VoodooGPIO::interruptOccurredGated)); if (ret != kIOReturnSuccess) { isInterruptBusy = false; - IOLog("%s:InterruptOccurred:Failed on attemptAction(). Error code = %X", getName(), ret); + IOLog("%s:InterruptOccurred:Failed on attemptAction(). Error code = %X\n", getName(), ret); } } From bd3b177ae396fce8c4a9a4df5e0c2b8ac83792f9 Mon Sep 17 00:00:00 2001 From: erictoby <54815823+Erictoby@users.noreply.github.com> Date: Mon, 23 Sep 2019 12:07:44 -0400 Subject: [PATCH 4/5] Minor change - PMInit location. --- VoodooGPIO/VoodooGPIO.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/VoodooGPIO/VoodooGPIO.cpp b/VoodooGPIO/VoodooGPIO.cpp index 8c935f4..74b530f 100644 --- a/VoodooGPIO/VoodooGPIO.cpp +++ b/VoodooGPIO/VoodooGPIO.cpp @@ -546,8 +546,6 @@ bool VoodooGPIO::start(IOService *provider) { isInterruptBusy = true; - PMinit(); - workLoop = getWorkLoop(); if (!workLoop) { IOLog("%s::Failed to get workloop!\n", getName()); @@ -656,6 +654,7 @@ bool VoodooGPIO::start(IOService *provider) { myPowerStates[1].outputPowerCharacter = kIOPMPowerOn; myPowerStates[1].inputPowerRequirement = kIOPMPowerOn; + PMinit(); provider->joinPMtree(this); registerPowerDriver(this, myPowerStates, kMyNumberOfStates); @@ -698,7 +697,7 @@ void VoodooGPIO::stop(IOService *provider) { } if (workLoop) { - // workLoop->release(); + workLoop->release(); workLoop = NULL; } From ec0b2d97acf0bdcfd41c903040bf5a0ff20f3b52 Mon Sep 17 00:00:00 2001 From: erictoby <54815823+Erictoby@users.noreply.github.com> Date: Sat, 28 Sep 2019 17:36:21 -0400 Subject: [PATCH 5/5] Reduce CPU load caused by too many GPIO interrupts Removed unnecessary read activities and added passive delay instead. --- VoodooGPIO/VoodooGPIO.cpp | 44 +++++++++++++++++++++++++++++---------- VoodooGPIO/VoodooGPIO.hpp | 8 ++++--- 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/VoodooGPIO/VoodooGPIO.cpp b/VoodooGPIO/VoodooGPIO.cpp index 74b530f..788202f 100644 --- a/VoodooGPIO/VoodooGPIO.cpp +++ b/VoodooGPIO/VoodooGPIO.cpp @@ -562,12 +562,11 @@ bool VoodooGPIO::start(IOService *provider) { } interruptSource = IOInterruptEventSource::interruptEventSource(this, OSMemberFunctionCast(IOInterruptEventAction, this, &VoodooGPIO::InterruptOccurred), provider); - if (!interruptSource) { + if (!interruptSource || (workLoop->addEventSource(interruptSource) != kIOReturnSuccess)) { IOLog("%s::Failed to get GPIO Controller interrupt!\n", getName()); stop(provider); return false; } - workLoop->addEventSource(interruptSource); interruptSource->enable(); IOLog("%s::VoodooGPIO Init!\n", getName()); @@ -627,7 +626,11 @@ bool VoodooGPIO::start(IOService *provider) { sz = sizeof(void *) * communities[i].npins; communities[i].pinInterruptRefcons = (void **)IOMalloc(sz); memset(communities[i].pinInterruptRefcons, 0, sz); + + communities[i].isActiveCommunity = (bool *)IOMalloc(1);; + *communities[i].isActiveCommunity = false; } + nInactiveCommunities = (UInt32)ncommunities - 1; intel_pinctrl_pm_init(); @@ -694,6 +697,7 @@ void VoodooGPIO::stop(IOService *provider) { IOFree(communities[i].pinInterruptAction, sizeof(IOInterruptAction) * communities[i].npins); IOFree(communities[i].interruptTypes, sizeof(unsigned) * communities[i].npins); IOFree(communities[i].pinInterruptRefcons, sizeof(void *) * communities[i].npins); + IOFree(communities[i].isActiveCommunity, 1); } if (workLoop) { @@ -746,9 +750,13 @@ IOReturn VoodooGPIO::setPowerState(unsigned long powerState, IOService *whatDevi return kIOPMAckImplied; } -void VoodooGPIO::intel_gpio_community_irq_handler(struct intel_community *community) { +void VoodooGPIO::intel_gpio_community_irq_handler(struct intel_community *community, bool *firstdelay) { for (int gpp = 0; gpp < community->ngpps; gpp++) { const struct intel_padgroup *padgrp = &community->gpps[gpp]; + + unsigned padno = padgrp->base - community->pin_base; + if (padno >= community->npins) + break; unsigned long pending, enabled; @@ -759,13 +767,12 @@ void VoodooGPIO::intel_gpio_community_irq_handler(struct intel_community *commun /* Only interrupts that are enabled */ pending &= enabled; - unsigned padno = padgrp->base - community->pin_base; - if (padno >= community->npins) - break; - - for (int i = 0; i < 32; i++) { - bool isPin = (pending >> i) & 0x1; - if (isPin) { + if (pending) { + for (int i = 0; i < 32; i++) { + bool isPin = (pending >> i) & 0x1; + if (!isPin) + continue; + unsigned pin = padno + i; OSObject *owner = community->pinInterruptActionOwners[pin]; @@ -773,6 +780,10 @@ void VoodooGPIO::intel_gpio_community_irq_handler(struct intel_community *commun IOInterruptAction handler = community->pinInterruptAction[pin]; void *refcon = community->pinInterruptRefcons[pin]; handler(owner, refcon, this, pin); + if (*firstdelay) { + *firstdelay = false; + IODelay(25 * nInactiveCommunities); // Reduce CPU load. 25~30us per inactive community was reasonable. + } } if (community->interruptTypes[pin] & IRQ_TYPE_LEVEL_MASK) @@ -815,6 +826,7 @@ IOReturn VoodooGPIO::registerInterrupt(int pin, OSObject *target, IOInterruptAct community->pinInterruptActionOwners[communityidx] = target; community->pinInterruptAction[communityidx] = handler; community->pinInterruptRefcons[communityidx] = refcon; + *community->isActiveCommunity = true; return kIOReturnSuccess; } @@ -879,6 +891,8 @@ IOReturn VoodooGPIO::setInterruptTypeForPin(int pin, int type) { unsigned communityidx = hw_pin - community->pin_base; community->interruptTypes[communityidx] = type; + if (type & IRQ_TYPE_LEVEL_MASK) + *community->isActiveCommunity = true; return kIOReturnSuccess; } @@ -895,9 +909,17 @@ void VoodooGPIO::InterruptOccurred(OSObject *owner, IOInterruptEventSource *src, } void VoodooGPIO::interruptOccurredGated() { + UInt32 inactive = 0; + bool firstdelay = true; + for (int i = 0; i < ncommunities; i++) { struct intel_community *community = &communities[i]; - intel_gpio_community_irq_handler(community); + if (*community->isActiveCommunity) + intel_gpio_community_irq_handler(community, &firstdelay); + else + inactive++; } + + nInactiveCommunities = (inactive < ncommunities)? inactive : ((UInt32)ncommunities - 1); isInterruptBusy = false; } diff --git a/VoodooGPIO/VoodooGPIO.hpp b/VoodooGPIO/VoodooGPIO.hpp index 389bc40..9f58685 100644 --- a/VoodooGPIO/VoodooGPIO.hpp +++ b/VoodooGPIO/VoodooGPIO.hpp @@ -120,6 +120,7 @@ struct intel_community { const struct intel_padgroup *gpps; size_t ngpps; bool gpps_alloc; + bool *isActiveCommunity; /* Reserved for the core driver */ IOMemoryMap *mmap; IOVirtualAddress regs; @@ -197,9 +198,10 @@ class VoodooGPIO : public IOService { bool controllerIsAwake; IOWorkLoop *workLoop; - IOInterruptEventSource *interruptSource = NULL; - IOCommandGate* command_gate = NULL; + IOInterruptEventSource *interruptSource; + IOCommandGate* command_gate; bool isInterruptBusy; + UInt32 nInactiveCommunities; UInt32 readl(IOVirtualAddress addr); void writel(UInt32 b, IOVirtualAddress addr); @@ -228,7 +230,7 @@ class VoodooGPIO : public IOService { void intel_gpio_irq_init(); void intel_pinctrl_resume(); - void intel_gpio_community_irq_handler(struct intel_community *community); + void intel_gpio_community_irq_handler(struct intel_community *community, bool *firstdelay); void InterruptOccurred(OSObject *owner, IOInterruptEventSource *src, int intCount); void interruptOccurredGated();