[PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume
Hans de Goede
hdegoede at redhat.com
Fri Sep 9 08:45:14 UTC 2022
Hi,
On 9/9/22 09:34, Patrik Jakobsson wrote:
> On Thu, Sep 8, 2022 at 3:39 PM Hans de Goede
> <hdegoede at 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 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.
>
> 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 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