[PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume
Hans de Goede
hdegoede at redhat.com
Thu Sep 8 13:39:13 UTC 2022
Hi,
On 9/8/22 15:26, Patrik Jakobsson wrote:
> On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede at 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 at 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
>>
>
More information about the dri-devel
mailing list