| Message ID | 20220906203852.527663-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/gma500: Fix 2 locking related WARNs + IRQ handling | expand |
On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede@redhat.com> 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. I can take this patch as is since it improves on the current situation but feel free to dig deeper if you like. On Poulsbo I can see interrupts not getting handled during suspend/resume even with this patch applied. -Patrik > > 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@redhat.com> > --- > 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 >
Hi, On 9/8/22 15:26, Patrik Jakobsson wrote: > On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede@redhat.com> 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. 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. 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. > 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. > 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 ? 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. So all the fixes in this series are somewhat of a distraction of what I'm actually trying to acomplish. 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... 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@redhat.com> >> --- >> 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 >> >
pci_restore_msi_stateOn Thu, Sep 8, 2022 at 3:39 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 9/8/22 15:26, Patrik Jakobsson wrote: > > On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede@redhat.com> 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. > > 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. > > 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. > > > 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. 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? > > > 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? > > 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 :) > > 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@redhat.com> > >> --- > >> 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 > >> > > >
Hi, On 9/9/22 09:34, Patrik Jakobsson wrote: > On Thu, Sep 8, 2022 at 3:39 PM Hans de Goede > <hdegoede@redhat.com> wrote: >> >> Hi, >> >> On 9/8/22 15:26, Patrik Jakobsson wrote: >>> On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede@redhat.com> 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@redhat.com> >>>> --- >>>> 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 >>>> >>> >> >
Hi Patrik, On 9/9/22 10:45, Hans de Goede wrote: > Hi, > > On 9/9/22 09:34, Patrik Jakobsson wrote: >> On Thu, Sep 8, 2022 at 3:39 PM Hans de Goede >> <hdegoede@redhat.com> wrote: >>> >>> Hi, >>> >>> On 9/8/22 15:26, Patrik Jakobsson wrote: <snip> >>>> 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. So I've just tested on a Sony Vaio VPCX11S1E (Atom Z540 with PSB graphics) and with my entire patch-set (did not test without) suspend/resume works fine there without any "irq xx: nobody cared" messages. Note that on the Vaio VPCX11S1E the gma500 is using irq 18 rather then 16 and that irq is not shared with anything. So I wonder if this is related to the irq being shared? Regards, Hans
On Sat, Sep 10, 2022 at 9:50 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi Patrik, > > On 9/9/22 10:45, Hans de Goede wrote: > > Hi, > > > > On 9/9/22 09:34, Patrik Jakobsson wrote: > >> On Thu, Sep 8, 2022 at 3:39 PM Hans de Goede > >> <hdegoede@redhat.com> wrote: > >>> > >>> Hi, > >>> > >>> On 9/8/22 15:26, Patrik Jakobsson wrote: > > <snip> > > >>>> 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. > > So I've just tested on a Sony Vaio VPCX11S1E (Atom Z540 with PSB graphics) > and with my entire patch-set (did not test without) suspend/resume works > fine there without any "irq xx: nobody cared" messages. Your patch fixes the problem on my FitPC (Atom Z530) as well. Great! I have the Acer AOD270 so I can look further into the MSI problem. But as you say, the current solution might be what we want. -Patrik > > Note that on the Vaio VPCX11S1E the gma500 is using irq 18 rather then > 16 and that irq is not shared with anything. So I wonder if this is > related to the irq being shared? > > Regards, > > Hans >
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);
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. 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@redhat.com> --- 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(-)