From: Hans de Goede <hdegoede@redhat.com>
To: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume
Date: Thu, 8 Sep 2022 15:39:13 +0200 [thread overview]
Message-ID: <69fc33c6-b6b0-ba98-d2c6-0fb35df63933@redhat.com> (raw)
In-Reply-To: <CAMeQTsYwrtAwb2_Lj2RyrWCov88Nq=-_tScD5dXC548Fog3X0w@mail.gmail.com>
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
>>
>
next prev parent reply other threads:[~2022-09-08 13:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-06 20:38 [PATCH v2 0/3] drm/gma500: Fix 2 locking related WARNs + IRQ handling Hans de Goede
2022-09-06 20:38 ` [PATCH v2 1/3] drm/gma500: Fix BUG: sleeping function called from invalid context errors Hans de Goede
2022-09-06 20:38 ` [PATCH v2 2/3] drm/gma500: Fix WARN_ON(lock->magic != lock) error Hans de Goede
2022-09-09 8:20 ` Patrik Jakobsson
2022-09-09 9:03 ` Hans de Goede
2022-09-06 20:38 ` [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume Hans de Goede
2022-09-08 13:26 ` Patrik Jakobsson
2022-09-08 13:39 ` Hans de Goede [this message]
2022-09-09 7:34 ` Patrik Jakobsson
2022-09-09 8:45 ` Hans de Goede
2022-09-10 19:50 ` Hans de Goede
2022-09-12 13:15 ` Patrik Jakobsson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=69fc33c6-b6b0-ba98-d2c6-0fb35df63933@redhat.com \
--to=hdegoede@redhat.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=patrik.r.jakobsson@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).