Hi, On 9/9/22 09:34, Patrik Jakobsson wrote: > On Thu, Sep 8, 2022 at 3:39 PM Hans de Goede > <hdegoede@xxxxxxxxxx> wrote: >> >> Hi, >> >> On 9/8/22 15:26, Patrik Jakobsson wrote: >>> On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: >>>> >>>> Fix gnome-shell (and other page-flip users) hanging after suspend/resume >>>> because of the gma500's IRQs not working. >>>> >>>> This fixes 2 problems with the IRQ handling: >>>> >>>> 1. gma_power_off() calls gma_irq_uninstall() which does a free_irq(), but >>>> gma_power_on() called gma_irq_preinstall() + gma_irq_postinstall() which >>>> do not call request_irq. Replace the pre- + post-install calls with >>>> gma_irq_install() which does prep + request + post. >>> >>> Hmm, I don't think we're supposed to do free_irq() during suspend in >>> the first place. request_irq() and free_irq() are normally only called >>> in the pci device probe/remove hooks. Same is true for msi >>> enable/disable. >> >> Right. I first tried to switch to disable_irq() / enable_irq() which are >> expected to be used during suspend/resume. That did resolve the issue >> of there no longer being an IRQ handler registered after suspend/resume, >> but the IRQ still would no longer fire after a suspend/resume. > > The irq enable/disable is handled by writing PSB_INT_ENABLE_R in > gma_irq_install/uninstall(). Also using disable_irq() and enable_irq() > shouldn't be required. But the docs could be wrong and this might fix > the interrupts I'm seeing on PSB after resume. That just disables the sender of the IRQ, where as disable_irq() disables the IRQ at the receiving end. And the sending end goes in full powerdown during suspend, after which the IRQ line might float or be pulled in a specific direction by the powered-down display-engine block. So yes this might be the cause of the "irq 16: nobody cared" message. Note that on the Packard Bell Dot SC when I disable MSI the IRQ line is shared with an UHCI controller, so that might be part of the problem here too. >> So then I tried the pci_disable_msi() + pci_enable_msi() and that >> did the trick. And since we should not call pci_disable_msi() with an >> IRQ handler registered I decided to keep the free_irq + request_irq >> over suspend/resume. > > Ok, then I understand your motivation for the free/request dance. > However, I would argue that if this problem is specific to your > Packard Bell Dot SC it is better handled with a quirk. I only have the one machine to test on. But the Packard Bell Dot SC is actually a rebrand of the somewhat popular Acer Aspire One D270 up to the point where they both use the same BIOS updater. So if we go with a quirk this should at least also apply to the Acer AOD270, but I expect more models to be impacted by this. Typically the BIOS-es on these devices are a copy & paste job from some reference implementation. And this workaround should work fine on machines which don't stricly need it too. So my preference would be to just do this everywhere. >> Another option is to never call pci_enable_msi() and use APIC style >> IRQs instead. At least on the Packard Bell Dot SC (cedarview) that >> works. > > Yes, the quirk could be to not use MSI on the Packard Bell Dot SC. But > let me check this on other Cedarview systems first. Not using MSI means sharing the IRQ, which although it should work fine is better to avoid. I wonder if you have ever tried enabling MSI on a poulsebo machine ? >>> I can take this patch as is since it improves on the current situation >>> but feel free to dig deeper if you like. >> >> I'm not sure what else I can do to dig deeper though. TBH I'm happy >> I managed to come up with something which works at all. > > Digging deeper would be to figure out why pci_restore_msi_state() is > not doing its job. Yes that would be an option. I suspect that the issue actually a setting on the PCI host bridge side which gets poked by the BIOS during S3 suspend and pci_restore_msi_state() presumable only touched the devices state. While pci_enable_msi() (presumably) also re-does the host side of the MSI setup. Anyways I'm afraid I don't have time to investigate this further. > The fact that gma500 is touching those MSI > registers in PCI config space manually is worrying. Did you test if > MSI works after resume if you remove the save/restore of > PSB_PCIx_MSI_ADDR_LOC and PSB_PCIx_MSI_DATA_LOC? Yes I did try removing those save/restores and that did not help. Note that this patch does get rid of them. >>> On Poulsbo I can see >>> interrupts not getting handled during suspend/resume even with this >>> patch applied. >> >> "during" ? I guess you mean _after_ a suspend/resume ? > > Yes. I get: irq 16: nobody cared (try booting with the "irqpoll" option). > But perhaps the system is just too slow to respond. > >> >> I have been refactoring the backlight (detection) code on >> x86/acpi devices. See: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=for-next >> >> As you can see there are also some drm driver changes involved for >> all the (non virtual) drm/kms drivers used on x86/acpi laptops. >> >> I am working on also making matching changes (*) to the gma500 code, >> which is why I scrounged up the Packard Bell Dot SC I'm testing this on. > > I'll have a look. > >> >> So all the fixes in this series are somewhat of a distraction of what >> I'm actually trying to acomplish. > > Fair enough, let's not focus too much on the details here. I can take > this patch as is so you can continue work on the backlight code. > Sounds good? Yes if you can take this patch as is that would be great. Note I have commit/push rights to all the drm-* repositories myself too. So if you prefer you can just give your Acked-by or Reviewed-by and then I can push things out myself. >> I have also scrounged up a Sony Vaio VPCX11S1E which has an Atom >> Z540 with PSB graphics. I have yet to test on that one though... > > If you do, bring lots of patience. Those systems are very slow. You > can literally sip on coffee while waiting for the cursor to move :) Hehe, I'll survive :) Regards, Hans >> *) For wip code see: >> >> https://github.com/jwrdegoede/linux-sunxi/commits/main >> >> and specifically: >> >> https://github.com/jwrdegoede/linux-sunxi/commit/97a1dbbd320b0bdbacf935e52f786e8185005298 >> >> which unifies the backlight handling between all 3 different >> SoC types supported by the gma500 code resulting in a nice cleanup. >> >> >> >> >> >>>> 2. After fixing 1. IRQs still do not work on a Packard Bell Dot SC (Intel >>>> Atom N2600, cedarview) netbook. >>>> >>>> Cederview uses MSI interrupts and it seems that the BIOS re-configures >>>> things back to normal APIC based interrupts during S3 suspend. There is >>>> some MSI PCI-config registers save/restore code which tries to deal with >>>> this, but on the Packard Bell Dot SC this is not sufficient to restore >>>> MSI IRQ functionality after a suspend/resume. >>>> >>>> Replace the PCI-config registers save/restore with pci_disable_msi() on >>>> suspend + pci_enable_msi() on resume. Fixing e.g. gnome-shell hanging. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> >>>> --- >>>> drivers/gpu/drm/gma500/cdv_device.c | 4 +--- >>>> drivers/gpu/drm/gma500/oaktrail_device.c | 5 +---- >>>> drivers/gpu/drm/gma500/power.c | 8 +------- >>>> drivers/gpu/drm/gma500/psb_drv.c | 2 +- >>>> drivers/gpu/drm/gma500/psb_drv.h | 5 +---- >>>> drivers/gpu/drm/gma500/psb_irq.c | 15 ++++++++++++--- >>>> drivers/gpu/drm/gma500/psb_irq.h | 2 +- >>>> 7 files changed, 18 insertions(+), 23 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/gma500/cdv_device.c b/drivers/gpu/drm/gma500/cdv_device.c >>>> index dd32b484dd82..ce96234f3df2 100644 >>>> --- a/drivers/gpu/drm/gma500/cdv_device.c >>>> +++ b/drivers/gpu/drm/gma500/cdv_device.c >>>> @@ -581,11 +581,9 @@ static const struct psb_offset cdv_regmap[2] = { >>>> static int cdv_chip_setup(struct drm_device *dev) >>>> { >>>> struct drm_psb_private *dev_priv = to_drm_psb_private(dev); >>>> - struct pci_dev *pdev = to_pci_dev(dev->dev); >>>> INIT_WORK(&dev_priv->hotplug_work, cdv_hotplug_work_func); >>>> >>>> - if (pci_enable_msi(pdev)) >>>> - dev_warn(dev->dev, "Enabling MSI failed!\n"); >>>> + dev_priv->use_msi = true; >>>> dev_priv->regmap = cdv_regmap; >>>> gma_get_core_freq(dev); >>>> psb_intel_opregion_init(dev); >>>> diff --git a/drivers/gpu/drm/gma500/oaktrail_device.c b/drivers/gpu/drm/gma500/oaktrail_device.c >>>> index 5923a9c89312..f90e628cb482 100644 >>>> --- a/drivers/gpu/drm/gma500/oaktrail_device.c >>>> +++ b/drivers/gpu/drm/gma500/oaktrail_device.c >>>> @@ -501,12 +501,9 @@ static const struct psb_offset oaktrail_regmap[2] = { >>>> static int oaktrail_chip_setup(struct drm_device *dev) >>>> { >>>> struct drm_psb_private *dev_priv = to_drm_psb_private(dev); >>>> - struct pci_dev *pdev = to_pci_dev(dev->dev); >>>> int ret; >>>> >>>> - if (pci_enable_msi(pdev)) >>>> - dev_warn(dev->dev, "Enabling MSI failed!\n"); >>>> - >>>> + dev_priv->use_msi = true; >>>> dev_priv->regmap = oaktrail_regmap; >>>> >>>> ret = mid_chip_setup(dev); >>>> diff --git a/drivers/gpu/drm/gma500/power.c b/drivers/gpu/drm/gma500/power.c >>>> index b91de6d36e41..66873085d450 100644 >>>> --- a/drivers/gpu/drm/gma500/power.c >>>> +++ b/drivers/gpu/drm/gma500/power.c >>>> @@ -139,8 +139,6 @@ static void gma_suspend_pci(struct pci_dev *pdev) >>>> dev_priv->regs.saveBSM = bsm; >>>> pci_read_config_dword(pdev, 0xFC, &vbt); >>>> dev_priv->regs.saveVBT = vbt; >>>> - pci_read_config_dword(pdev, PSB_PCIx_MSI_ADDR_LOC, &dev_priv->msi_addr); >>>> - pci_read_config_dword(pdev, PSB_PCIx_MSI_DATA_LOC, &dev_priv->msi_data); >>>> >>>> pci_disable_device(pdev); >>>> pci_set_power_state(pdev, PCI_D3hot); >>>> @@ -168,9 +166,6 @@ static bool gma_resume_pci(struct pci_dev *pdev) >>>> pci_restore_state(pdev); >>>> pci_write_config_dword(pdev, 0x5c, dev_priv->regs.saveBSM); >>>> pci_write_config_dword(pdev, 0xFC, dev_priv->regs.saveVBT); >>>> - /* restoring MSI address and data in PCIx space */ >>>> - pci_write_config_dword(pdev, PSB_PCIx_MSI_ADDR_LOC, dev_priv->msi_addr); >>>> - pci_write_config_dword(pdev, PSB_PCIx_MSI_DATA_LOC, dev_priv->msi_data); >>>> ret = pci_enable_device(pdev); >>>> >>>> if (ret != 0) >>>> @@ -223,8 +218,7 @@ int gma_power_resume(struct device *_dev) >>>> mutex_lock(&power_mutex); >>>> gma_resume_pci(pdev); >>>> gma_resume_display(pdev); >>>> - gma_irq_preinstall(dev); >>>> - gma_irq_postinstall(dev); >>>> + gma_irq_install(dev); >>>> mutex_unlock(&power_mutex); >>>> return 0; >>>> } >>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c >>>> index 1d8744f3e702..54e756b48606 100644 >>>> --- a/drivers/gpu/drm/gma500/psb_drv.c >>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c >>>> @@ -383,7 +383,7 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags) >>>> PSB_WVDC32(0xFFFFFFFF, PSB_INT_MASK_R); >>>> spin_unlock_irqrestore(&dev_priv->irqmask_lock, irqflags); >>>> >>>> - gma_irq_install(dev, pdev->irq); >>>> + gma_irq_install(dev); >>>> >>>> dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */ >>>> >>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.h b/drivers/gpu/drm/gma500/psb_drv.h >>>> index 0ea3d23575f3..731cc356c07a 100644 >>>> --- a/drivers/gpu/drm/gma500/psb_drv.h >>>> +++ b/drivers/gpu/drm/gma500/psb_drv.h >>>> @@ -490,6 +490,7 @@ struct drm_psb_private { >>>> int rpm_enabled; >>>> >>>> /* MID specific */ >>>> + bool use_msi; >>>> bool has_gct; >>>> struct oaktrail_gct_data gct_data; >>>> >>>> @@ -499,10 +500,6 @@ struct drm_psb_private { >>>> /* Register state */ >>>> struct psb_save_area regs; >>>> >>>> - /* MSI reg save */ >>>> - uint32_t msi_addr; >>>> - uint32_t msi_data; >>>> - >>>> /* Hotplug handling */ >>>> struct work_struct hotplug_work; >>>> >>>> diff --git a/drivers/gpu/drm/gma500/psb_irq.c b/drivers/gpu/drm/gma500/psb_irq.c >>>> index e6e6d61bbeab..038f18ed0a95 100644 >>>> --- a/drivers/gpu/drm/gma500/psb_irq.c >>>> +++ b/drivers/gpu/drm/gma500/psb_irq.c >>>> @@ -316,17 +316,24 @@ void gma_irq_postinstall(struct drm_device *dev) >>>> spin_unlock_irqrestore(&dev_priv->irqmask_lock, irqflags); >>>> } >>>> >>>> -int gma_irq_install(struct drm_device *dev, unsigned int irq) >>>> +int gma_irq_install(struct drm_device *dev) >>>> { >>>> + struct drm_psb_private *dev_priv = to_drm_psb_private(dev); >>>> + struct pci_dev *pdev = to_pci_dev(dev->dev); >>>> int ret; >>>> >>>> - if (irq == IRQ_NOTCONNECTED) >>>> + if (dev_priv->use_msi && pci_enable_msi(pdev)) { >>>> + dev_warn(dev->dev, "Enabling MSI failed!\n"); >>>> + dev_priv->use_msi = false; >>>> + } >>>> + >>>> + if (pdev->irq == IRQ_NOTCONNECTED) >>>> return -ENOTCONN; >>>> >>>> gma_irq_preinstall(dev); >>>> >>>> /* PCI devices require shared interrupts. */ >>>> - ret = request_irq(irq, gma_irq_handler, IRQF_SHARED, dev->driver->name, dev); >>>> + ret = request_irq(pdev->irq, gma_irq_handler, IRQF_SHARED, dev->driver->name, dev); >>>> if (ret) >>>> return ret; >>>> >>>> @@ -369,6 +376,8 @@ void gma_irq_uninstall(struct drm_device *dev) >>>> spin_unlock_irqrestore(&dev_priv->irqmask_lock, irqflags); >>>> >>>> free_irq(pdev->irq, dev); >>>> + if (dev_priv->use_msi) >>>> + pci_disable_msi(pdev); >>>> } >>>> >>>> int gma_crtc_enable_vblank(struct drm_crtc *crtc) >>>> diff --git a/drivers/gpu/drm/gma500/psb_irq.h b/drivers/gpu/drm/gma500/psb_irq.h >>>> index b51e395194ff..7648f69824a5 100644 >>>> --- a/drivers/gpu/drm/gma500/psb_irq.h >>>> +++ b/drivers/gpu/drm/gma500/psb_irq.h >>>> @@ -17,7 +17,7 @@ struct drm_device; >>>> >>>> void gma_irq_preinstall(struct drm_device *dev); >>>> void gma_irq_postinstall(struct drm_device *dev); >>>> -int gma_irq_install(struct drm_device *dev, unsigned int irq); >>>> +int gma_irq_install(struct drm_device *dev); >>>> void gma_irq_uninstall(struct drm_device *dev); >>>> >>>> int gma_crtc_enable_vblank(struct drm_crtc *crtc); >>>> -- >>>> 2.37.2 >>>> >>> >> >
