[PATCH v2 0/3] drm/gma500: Fix 2 locking related WARNs + IRQ handling

[PATCH v2 0/3] drm/gma500: Fix 2 locking related WARNs + IRQ handling

[Hans de Goede]

Hi All,

I have been testing the gma500 code on a Packard Bell Dot SC (Intel Atom
N2600, cedarview) laptop because I'm working on aligning the gma500
backlight code with the changes done to other drivers in the recent
backlight refactoring.

During the testing I noticed dmesg filling with a WARN_ON constantly
triggering as well as gnome-shell hanging after a suspend-resume cycle.

This series fixes 2 locking WARNs as well as vblank IRQs no longer
getting processed after a suspend-resume.

Changes in v2:
- Drop "drm/gma500: Fix crtc_vblank reference leak when userspace queues multiple events"
- Add "drm/gma500: Fix (vblank) IRQs not working after suspend/resume"

Regards,

Hans


Hans de Goede (3):
  drm/gma500: Fix BUG: sleeping function called from invalid context
    errors
  drm/gma500: Fix WARN_ON(lock->magic != lock) error
  drm/gma500: Fix (vblank) IRQs not working after suspend/resume

 drivers/gpu/drm/gma500/cdv_device.c      |  4 +---
 drivers/gpu/drm/gma500/gem.c             |  4 ++--
 drivers/gpu/drm/gma500/gma_display.c     | 11 +++++++----
 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 +-
 9 files changed, 27 insertions(+), 29 deletions(-)

-- 
2.37.2

[PATCH v2 1/3] drm/gma500: Fix BUG: sleeping function called from invalid context errors

[Hans de Goede]

gma_crtc_page_flip() was holding the event_lock spinlock while calling
crtc_funcs->mode_set_base() which takes ww_mutex.

The only reason to hold event_lock is to clear gma_crtc->page_flip_event
on mode_set_base() errors.

Instead unlock it after setting gma_crtc->page_flip_event and on
errors re-take the lock and clear gma_crtc->page_flip_event it
it is still set.

This fixes the following WARN/stacktrace:

[  512.122953] BUG: sleeping function called from invalid context at kernel/locking/mutex.c:870
[  512.123004] in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 1253, name: gnome-shell
[  512.123031] preempt_count: 1, expected: 0
[  512.123048] RCU nest depth: 0, expected: 0
[  512.123066] INFO: lockdep is turned off.
[  512.123080] irq event stamp: 0
[  512.123094] hardirqs last  enabled at (0): [<0000000000000000>] 0x0
[  512.123134] hardirqs last disabled at (0): [<ffffffff8d0ec28c>] copy_process+0x9fc/0x1de0
[  512.123176] softirqs last  enabled at (0): [<ffffffff8d0ec28c>] copy_process+0x9fc/0x1de0
[  512.123207] softirqs last disabled at (0): [<0000000000000000>] 0x0
[  512.123233] Preemption disabled at:
[  512.123241] [<0000000000000000>] 0x0
[  512.123275] CPU: 3 PID: 1253 Comm: gnome-shell Tainted: G        W         5.19.0+ #1
[  512.123304] Hardware name: Packard Bell dot s/SJE01_CT, BIOS V1.10 07/23/2013
[  512.123323] Call Trace:
[  512.123346]  <TASK>
[  512.123370]  dump_stack_lvl+0x5b/0x77
[  512.123412]  __might_resched.cold+0xff/0x13a
[  512.123458]  ww_mutex_lock+0x1e/0xa0
[  512.123495]  psb_gem_pin+0x2c/0x150 [gma500_gfx]
[  512.123601]  gma_pipe_set_base+0x76/0x240 [gma500_gfx]
[  512.123708]  gma_crtc_page_flip+0x95/0x130 [gma500_gfx]
[  512.123808]  drm_mode_page_flip_ioctl+0x57d/0x5d0
[  512.123897]  ? drm_mode_cursor2_ioctl+0x10/0x10
[  512.123936]  drm_ioctl_kernel+0xa1/0x150
[  512.123984]  drm_ioctl+0x21f/0x420
[  512.124025]  ? drm_mode_cursor2_ioctl+0x10/0x10
[  512.124070]  ? rcu_read_lock_bh_held+0xb/0x60
[  512.124104]  ? lock_release+0x1ef/0x2d0
[  512.124161]  __x64_sys_ioctl+0x8d/0xd0
[  512.124203]  do_syscall_64+0x58/0x80
[  512.124239]  ? do_syscall_64+0x67/0x80
[  512.124267]  ? trace_hardirqs_on_prepare+0x55/0xe0
[  512.124300]  ? do_syscall_64+0x67/0x80
[  512.124340]  ? rcu_read_lock_sched_held+0x10/0x80
[  512.124377]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[  512.124411] RIP: 0033:0x7fcc4a70740f
[  512.124442] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 18 48 8b 44 24 18 64 48 2b 04 25 28 00 00
[  512.124470] RSP: 002b:00007ffda73f5390 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[  512.124503] RAX: ffffffffffffffda RBX: 000055cc9e474500 RCX: 00007fcc4a70740f
[  512.124524] RDX: 00007ffda73f5420 RSI: 00000000c01864b0 RDI: 0000000000000009
[  512.124544] RBP: 00007ffda73f5420 R08: 000055cc9c0b0cb0 R09: 0000000000000034
[  512.124564] R10: 0000000000000000 R11: 0000000000000246 R12: 00000000c01864b0
[  512.124584] R13: 0000000000000009 R14: 000055cc9df484d0 R15: 000055cc9af5d0c0
[  512.124647]  </TASK>

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/gma500/gma_display.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index bd40c040a2c9..2f52eceda3a1 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -532,15 +532,18 @@ int gma_crtc_page_flip(struct drm_crtc *crtc,
 		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
 
 		gma_crtc->page_flip_event = event;
+		spin_unlock_irqrestore(&dev->event_lock, flags);
 
 		/* Call this locked if we want an event at vblank interrupt. */
 		ret = crtc_funcs->mode_set_base(crtc, crtc->x, crtc->y, old_fb);
 		if (ret) {
-			gma_crtc->page_flip_event = NULL;
-			drm_crtc_vblank_put(crtc);
+			spin_lock_irqsave(&dev->event_lock, flags);
+			if (gma_crtc->page_flip_event) {
+				gma_crtc->page_flip_event = NULL;
+				drm_crtc_vblank_put(crtc);
+			}
+			spin_unlock_irqrestore(&dev->event_lock, flags);
 		}
-
-		spin_unlock_irqrestore(&dev->event_lock, flags);
 	} else {
 		ret = crtc_funcs->mode_set_base(crtc, crtc->x, crtc->y, old_fb);
 	}
-- 
2.37.2

[PATCH v2 2/3] drm/gma500: Fix WARN_ON(lock->magic != lock) error

[Hans de Goede]

psb_gem_unpin() calls dma_resv_lock() but the underlying ww_mutex
gets destroyed by drm_gem_object_release() move the
drm_gem_object_release() call in psb_gem_free_object() to after
the unpin to fix the below warning:

[   79.693962] ------------[ cut here ]------------
[   79.693992] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
[   79.694015] WARNING: CPU: 0 PID: 240 at kernel/locking/mutex.c:582 __ww_mutex_lock.constprop.0+0x569/0xfb0
[   79.694052] Modules linked in: rfcomm snd_seq_dummy snd_hrtimer qrtr bnep ath9k ath9k_common ath9k_hw snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi snd_hda_intel ath3k snd_intel_dspcfg mac80211 snd_intel_sdw_acpi btusb snd_hda_codec btrtl btbcm btintel btmtk bluetooth at24 snd_hda_core snd_hwdep uvcvideo snd_seq libarc4 videobuf2_vmalloc ath videobuf2_memops videobuf2_v4l2 videobuf2_common snd_seq_device videodev acer_wmi intel_powerclamp coretemp mc snd_pcm joydev sparse_keymap ecdh_generic pcspkr wmi_bmof cfg80211 i2c_i801 i2c_smbus snd_timer snd r8169 rfkill lpc_ich soundcore acpi_cpufreq zram rtsx_pci_sdmmc mmc_core serio_raw rtsx_pci gma500_gfx(E) video wmi ip6_tables ip_tables i2c_dev fuse
[   79.694436] CPU: 0 PID: 240 Comm: plymouthd Tainted: G        W   E      6.0.0-rc3+ #490
[   79.694457] Hardware name: Packard Bell dot s/SJE01_CT, BIOS V1.10 07/23/2013
[   79.694469] RIP: 0010:__ww_mutex_lock.constprop.0+0x569/0xfb0
[   79.694496] Code: ff 85 c0 0f 84 15 fb ff ff 8b 05 ca 3c 11 01 85 c0 0f 85 07 fb ff ff 48 c7 c6 30 cb 84 aa 48 c7 c7 a3 e1 82 aa e8 ac 29 f8 ff <0f> 0b e9 ed fa ff ff e8 5b 83 8a ff 85 c0 74 10 44 8b 0d 98 3c 11
[   79.694513] RSP: 0018:ffffad1dc048bbe0 EFLAGS: 00010282
[   79.694623] RAX: 0000000000000028 RBX: 0000000000000000 RCX: 0000000000000000
[   79.694636] RDX: 0000000000000001 RSI: ffffffffaa8b0ffc RDI: 00000000ffffffff
[   79.694650] RBP: ffffad1dc048bc80 R08: 0000000000000000 R09: ffffad1dc048ba90
[   79.694662] R10: 0000000000000003 R11: ffffffffaad62fe8 R12: ffff9ff302103138
[   79.694675] R13: ffff9ff306ec8000 R14: ffff9ff307779078 R15: ffff9ff3014c0270
[   79.694690] FS:  00007ff1cccf1740(0000) GS:ffff9ff3bc200000(0000) knlGS:0000000000000000
[   79.694705] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   79.694719] CR2: 0000559ecbcb4420 CR3: 0000000013210000 CR4: 00000000000006f0
[   79.694734] Call Trace:
[   79.694749]  <TASK>
[   79.694761]  ? __schedule+0x47f/0x1670
[   79.694796]  ? psb_gem_unpin+0x27/0x1a0 [gma500_gfx]
[   79.694830]  ? lock_is_held_type+0xe3/0x140
[   79.694864]  ? ww_mutex_lock+0x38/0xa0
[   79.694885]  ? __cond_resched+0x1c/0x30
[   79.694902]  ww_mutex_lock+0x38/0xa0
[   79.694925]  psb_gem_unpin+0x27/0x1a0 [gma500_gfx]
[   79.694964]  psb_gem_unpin+0x199/0x1a0 [gma500_gfx]
[   79.694996]  drm_gem_object_release_handle+0x50/0x60
[   79.695020]  ? drm_gem_object_handle_put_unlocked+0xf0/0xf0
[   79.695042]  idr_for_each+0x4b/0xb0
[   79.695066]  ? _raw_spin_unlock_irqrestore+0x30/0x60
[   79.695095]  drm_gem_release+0x1c/0x30
[   79.695118]  drm_file_free.part.0+0x1ea/0x260
[   79.695150]  drm_release+0x6a/0x120
[   79.695175]  __fput+0x9f/0x260
[   79.695203]  task_work_run+0x59/0xa0
[   79.695227]  do_exit+0x387/0xbe0
[   79.695250]  ? seqcount_lockdep_reader_access.constprop.0+0x82/0x90
[   79.695275]  ? lockdep_hardirqs_on+0x7d/0x100
[   79.695304]  do_group_exit+0x33/0xb0
[   79.695331]  __x64_sys_exit_group+0x14/0x20
[   79.695353]  do_syscall_64+0x58/0x80
[   79.695376]  ? up_read+0x17/0x20
[   79.695401]  ? lock_is_held_type+0xe3/0x140
[   79.695429]  ? asm_exc_page_fault+0x22/0x30
[   79.695450]  ? lockdep_hardirqs_on+0x7d/0x100
[   79.695473]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
[   79.695493] RIP: 0033:0x7ff1ccefe3f1
[   79.695516] Code: Unable to access opcode bytes at RIP 0x7ff1ccefe3c7.
[   79.695607] RSP: 002b:00007ffed4413378 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
[   79.695629] RAX: ffffffffffffffda RBX: 00007ff1cd0159e0 RCX: 00007ff1ccefe3f1
[   79.695644] RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000
[   79.695656] RBP: 0000000000000000 R08: ffffffffffffff80 R09: 00007ff1cd020b20
[   79.695671] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff1cd0159e0
[   79.695684] R13: 0000000000000000 R14: 00007ff1cd01aee8 R15: 00007ff1cd01af00
[   79.695733]  </TASK>
[   79.695746] irq event stamp: 725979
[   79.695757] hardirqs last  enabled at (725979): [<ffffffffa9132d54>] finish_task_switch.isra.0+0xe4/0x3f0
[   79.695780] hardirqs last disabled at (725978): [<ffffffffa9eb4113>] __schedule+0xdd3/0x1670
[   79.695803] softirqs last  enabled at (725974): [<ffffffffa90fbc9d>] __irq_exit_rcu+0xed/0x160
[   79.695825] softirqs last disabled at (725969): [<ffffffffa90fbc9d>] __irq_exit_rcu+0xed/0x160
[   79.695845] ---[ end trace 0000000000000000 ]---

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/gma500/gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
index dffe37490206..4b7627a72637 100644
--- a/drivers/gpu/drm/gma500/gem.c
+++ b/drivers/gpu/drm/gma500/gem.c
@@ -112,12 +112,12 @@ static void psb_gem_free_object(struct drm_gem_object *obj)
 {
 	struct psb_gem_object *pobj = to_psb_gem_object(obj);
 
-	drm_gem_object_release(obj);
-
 	/* Undo the mmap pin if we are destroying the object */
 	if (pobj->mmapping)
 		psb_gem_unpin(pobj);
 
+	drm_gem_object_release(obj);
+
 	WARN_ON(pobj->in_gart && !pobj->stolen);
 
 	release_resource(&pobj->resource);
-- 
2.37.2

[PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume

[Hans de Goede]

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(-)

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

Re: [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume

[Patrik Jakobsson]

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
>

Re: [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume

[Hans de Goede]

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
>>
> 

Re: [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume

[Patrik Jakobsson]

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
> >>
> >
>

Re: [PATCH v2 2/3] drm/gma500: Fix WARN_ON(lock->magic != lock) error

[Patrik Jakobsson]

On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> psb_gem_unpin() calls dma_resv_lock() but the underlying ww_mutex
> gets destroyed by drm_gem_object_release() move the
> drm_gem_object_release() call in psb_gem_free_object() to after
> the unpin to fix the below warning:

Looks good. I'll apply this to drm-misc-fixes. Let me know if it
should go somewhere else.

Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>

>
> [   79.693962] ------------[ cut here ]------------
> [   79.693992] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> [   79.694015] WARNING: CPU: 0 PID: 240 at kernel/locking/mutex.c:582 __ww_mutex_lock.constprop.0+0x569/0xfb0
> [   79.694052] Modules linked in: rfcomm snd_seq_dummy snd_hrtimer qrtr bnep ath9k ath9k_common ath9k_hw snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi snd_hda_intel ath3k snd_intel_dspcfg mac80211 snd_intel_sdw_acpi btusb snd_hda_codec btrtl btbcm btintel btmtk bluetooth at24 snd_hda_core snd_hwdep uvcvideo snd_seq libarc4 videobuf2_vmalloc ath videobuf2_memops videobuf2_v4l2 videobuf2_common snd_seq_device videodev acer_wmi intel_powerclamp coretemp mc snd_pcm joydev sparse_keymap ecdh_generic pcspkr wmi_bmof cfg80211 i2c_i801 i2c_smbus snd_timer snd r8169 rfkill lpc_ich soundcore acpi_cpufreq zram rtsx_pci_sdmmc mmc_core serio_raw rtsx_pci gma500_gfx(E) video wmi ip6_tables ip_tables i2c_dev fuse
> [   79.694436] CPU: 0 PID: 240 Comm: plymouthd Tainted: G        W   E      6.0.0-rc3+ #490
> [   79.694457] Hardware name: Packard Bell dot s/SJE01_CT, BIOS V1.10 07/23/2013
> [   79.694469] RIP: 0010:__ww_mutex_lock.constprop.0+0x569/0xfb0
> [   79.694496] Code: ff 85 c0 0f 84 15 fb ff ff 8b 05 ca 3c 11 01 85 c0 0f 85 07 fb ff ff 48 c7 c6 30 cb 84 aa 48 c7 c7 a3 e1 82 aa e8 ac 29 f8 ff <0f> 0b e9 ed fa ff ff e8 5b 83 8a ff 85 c0 74 10 44 8b 0d 98 3c 11
> [   79.694513] RSP: 0018:ffffad1dc048bbe0 EFLAGS: 00010282
> [   79.694623] RAX: 0000000000000028 RBX: 0000000000000000 RCX: 0000000000000000
> [   79.694636] RDX: 0000000000000001 RSI: ffffffffaa8b0ffc RDI: 00000000ffffffff
> [   79.694650] RBP: ffffad1dc048bc80 R08: 0000000000000000 R09: ffffad1dc048ba90
> [   79.694662] R10: 0000000000000003 R11: ffffffffaad62fe8 R12: ffff9ff302103138
> [   79.694675] R13: ffff9ff306ec8000 R14: ffff9ff307779078 R15: ffff9ff3014c0270
> [   79.694690] FS:  00007ff1cccf1740(0000) GS:ffff9ff3bc200000(0000) knlGS:0000000000000000
> [   79.694705] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   79.694719] CR2: 0000559ecbcb4420 CR3: 0000000013210000 CR4: 00000000000006f0
> [   79.694734] Call Trace:
> [   79.694749]  <TASK>
> [   79.694761]  ? __schedule+0x47f/0x1670
> [   79.694796]  ? psb_gem_unpin+0x27/0x1a0 [gma500_gfx]
> [   79.694830]  ? lock_is_held_type+0xe3/0x140
> [   79.694864]  ? ww_mutex_lock+0x38/0xa0
> [   79.694885]  ? __cond_resched+0x1c/0x30
> [   79.694902]  ww_mutex_lock+0x38/0xa0
> [   79.694925]  psb_gem_unpin+0x27/0x1a0 [gma500_gfx]
> [   79.694964]  psb_gem_unpin+0x199/0x1a0 [gma500_gfx]
> [   79.694996]  drm_gem_object_release_handle+0x50/0x60
> [   79.695020]  ? drm_gem_object_handle_put_unlocked+0xf0/0xf0
> [   79.695042]  idr_for_each+0x4b/0xb0
> [   79.695066]  ? _raw_spin_unlock_irqrestore+0x30/0x60
> [   79.695095]  drm_gem_release+0x1c/0x30
> [   79.695118]  drm_file_free.part.0+0x1ea/0x260
> [   79.695150]  drm_release+0x6a/0x120
> [   79.695175]  __fput+0x9f/0x260
> [   79.695203]  task_work_run+0x59/0xa0
> [   79.695227]  do_exit+0x387/0xbe0
> [   79.695250]  ? seqcount_lockdep_reader_access.constprop.0+0x82/0x90
> [   79.695275]  ? lockdep_hardirqs_on+0x7d/0x100
> [   79.695304]  do_group_exit+0x33/0xb0
> [   79.695331]  __x64_sys_exit_group+0x14/0x20
> [   79.695353]  do_syscall_64+0x58/0x80
> [   79.695376]  ? up_read+0x17/0x20
> [   79.695401]  ? lock_is_held_type+0xe3/0x140
> [   79.695429]  ? asm_exc_page_fault+0x22/0x30
> [   79.695450]  ? lockdep_hardirqs_on+0x7d/0x100
> [   79.695473]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> [   79.695493] RIP: 0033:0x7ff1ccefe3f1
> [   79.695516] Code: Unable to access opcode bytes at RIP 0x7ff1ccefe3c7.
> [   79.695607] RSP: 002b:00007ffed4413378 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
> [   79.695629] RAX: ffffffffffffffda RBX: 00007ff1cd0159e0 RCX: 00007ff1ccefe3f1
> [   79.695644] RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000
> [   79.695656] RBP: 0000000000000000 R08: ffffffffffffff80 R09: 00007ff1cd020b20
> [   79.695671] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff1cd0159e0
> [   79.695684] R13: 0000000000000000 R14: 00007ff1cd01aee8 R15: 00007ff1cd01af00
> [   79.695733]  </TASK>
> [   79.695746] irq event stamp: 725979
> [   79.695757] hardirqs last  enabled at (725979): [<ffffffffa9132d54>] finish_task_switch.isra.0+0xe4/0x3f0
> [   79.695780] hardirqs last disabled at (725978): [<ffffffffa9eb4113>] __schedule+0xdd3/0x1670
> [   79.695803] softirqs last  enabled at (725974): [<ffffffffa90fbc9d>] __irq_exit_rcu+0xed/0x160
> [   79.695825] softirqs last disabled at (725969): [<ffffffffa90fbc9d>] __irq_exit_rcu+0xed/0x160
> [   79.695845] ---[ end trace 0000000000000000 ]---
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/gma500/gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index dffe37490206..4b7627a72637 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -112,12 +112,12 @@ static void psb_gem_free_object(struct drm_gem_object *obj)
>  {
>         struct psb_gem_object *pobj = to_psb_gem_object(obj);
>
> -       drm_gem_object_release(obj);
> -
>         /* Undo the mmap pin if we are destroying the object */
>         if (pobj->mmapping)
>                 psb_gem_unpin(pobj);
>
> +       drm_gem_object_release(obj);
> +
>         WARN_ON(pobj->in_gart && !pobj->stolen);
>
>         release_resource(&pobj->resource);
> --
> 2.37.2
>

Re: [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume

[Hans de Goede]

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
>>>>
>>>
>>
> 

Re: [PATCH v2 2/3] drm/gma500: Fix WARN_ON(lock->magic != lock) error

[Hans de Goede]

Hi,

On 9/9/22 10:20, Patrik Jakobsson wrote:
> On Tue, Sep 6, 2022 at 10:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> psb_gem_unpin() calls dma_resv_lock() but the underlying ww_mutex
>> gets destroyed by drm_gem_object_release() move the
>> drm_gem_object_release() call in psb_gem_free_object() to after
>> the unpin to fix the below warning:
> 
> Looks good. I'll apply this to drm-misc-fixes. Let me know if it
> should go somewhere else.

drm-misc-fixes sounds good to me, thank you.

Regards,

Hans


> 
> Signed-off-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> 
>>
>> [   79.693962] ------------[ cut here ]------------
>> [   79.693992] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
>> [   79.694015] WARNING: CPU: 0 PID: 240 at kernel/locking/mutex.c:582 __ww_mutex_lock.constprop.0+0x569/0xfb0
>> [   79.694052] Modules linked in: rfcomm snd_seq_dummy snd_hrtimer qrtr bnep ath9k ath9k_common ath9k_hw snd_hda_codec_realtek snd_hda_codec_generic ledtrig_audio snd_hda_codec_hdmi snd_hda_intel ath3k snd_intel_dspcfg mac80211 snd_intel_sdw_acpi btusb snd_hda_codec btrtl btbcm btintel btmtk bluetooth at24 snd_hda_core snd_hwdep uvcvideo snd_seq libarc4 videobuf2_vmalloc ath videobuf2_memops videobuf2_v4l2 videobuf2_common snd_seq_device videodev acer_wmi intel_powerclamp coretemp mc snd_pcm joydev sparse_keymap ecdh_generic pcspkr wmi_bmof cfg80211 i2c_i801 i2c_smbus snd_timer snd r8169 rfkill lpc_ich soundcore acpi_cpufreq zram rtsx_pci_sdmmc mmc_core serio_raw rtsx_pci gma500_gfx(E) video wmi ip6_tables ip_tables i2c_dev fuse
>> [   79.694436] CPU: 0 PID: 240 Comm: plymouthd Tainted: G        W   E      6.0.0-rc3+ #490
>> [   79.694457] Hardware name: Packard Bell dot s/SJE01_CT, BIOS V1.10 07/23/2013
>> [   79.694469] RIP: 0010:__ww_mutex_lock.constprop.0+0x569/0xfb0
>> [   79.694496] Code: ff 85 c0 0f 84 15 fb ff ff 8b 05 ca 3c 11 01 85 c0 0f 85 07 fb ff ff 48 c7 c6 30 cb 84 aa 48 c7 c7 a3 e1 82 aa e8 ac 29 f8 ff <0f> 0b e9 ed fa ff ff e8 5b 83 8a ff 85 c0 74 10 44 8b 0d 98 3c 11
>> [   79.694513] RSP: 0018:ffffad1dc048bbe0 EFLAGS: 00010282
>> [   79.694623] RAX: 0000000000000028 RBX: 0000000000000000 RCX: 0000000000000000
>> [   79.694636] RDX: 0000000000000001 RSI: ffffffffaa8b0ffc RDI: 00000000ffffffff
>> [   79.694650] RBP: ffffad1dc048bc80 R08: 0000000000000000 R09: ffffad1dc048ba90
>> [   79.694662] R10: 0000000000000003 R11: ffffffffaad62fe8 R12: ffff9ff302103138
>> [   79.694675] R13: ffff9ff306ec8000 R14: ffff9ff307779078 R15: ffff9ff3014c0270
>> [   79.694690] FS:  00007ff1cccf1740(0000) GS:ffff9ff3bc200000(0000) knlGS:0000000000000000
>> [   79.694705] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [   79.694719] CR2: 0000559ecbcb4420 CR3: 0000000013210000 CR4: 00000000000006f0
>> [   79.694734] Call Trace:
>> [   79.694749]  <TASK>
>> [   79.694761]  ? __schedule+0x47f/0x1670
>> [   79.694796]  ? psb_gem_unpin+0x27/0x1a0 [gma500_gfx]
>> [   79.694830]  ? lock_is_held_type+0xe3/0x140
>> [   79.694864]  ? ww_mutex_lock+0x38/0xa0
>> [   79.694885]  ? __cond_resched+0x1c/0x30
>> [   79.694902]  ww_mutex_lock+0x38/0xa0
>> [   79.694925]  psb_gem_unpin+0x27/0x1a0 [gma500_gfx]
>> [   79.694964]  psb_gem_unpin+0x199/0x1a0 [gma500_gfx]
>> [   79.694996]  drm_gem_object_release_handle+0x50/0x60
>> [   79.695020]  ? drm_gem_object_handle_put_unlocked+0xf0/0xf0
>> [   79.695042]  idr_for_each+0x4b/0xb0
>> [   79.695066]  ? _raw_spin_unlock_irqrestore+0x30/0x60
>> [   79.695095]  drm_gem_release+0x1c/0x30
>> [   79.695118]  drm_file_free.part.0+0x1ea/0x260
>> [   79.695150]  drm_release+0x6a/0x120
>> [   79.695175]  __fput+0x9f/0x260
>> [   79.695203]  task_work_run+0x59/0xa0
>> [   79.695227]  do_exit+0x387/0xbe0
>> [   79.695250]  ? seqcount_lockdep_reader_access.constprop.0+0x82/0x90
>> [   79.695275]  ? lockdep_hardirqs_on+0x7d/0x100
>> [   79.695304]  do_group_exit+0x33/0xb0
>> [   79.695331]  __x64_sys_exit_group+0x14/0x20
>> [   79.695353]  do_syscall_64+0x58/0x80
>> [   79.695376]  ? up_read+0x17/0x20
>> [   79.695401]  ? lock_is_held_type+0xe3/0x140
>> [   79.695429]  ? asm_exc_page_fault+0x22/0x30
>> [   79.695450]  ? lockdep_hardirqs_on+0x7d/0x100
>> [   79.695473]  entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> [   79.695493] RIP: 0033:0x7ff1ccefe3f1
>> [   79.695516] Code: Unable to access opcode bytes at RIP 0x7ff1ccefe3c7.
>> [   79.695607] RSP: 002b:00007ffed4413378 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
>> [   79.695629] RAX: ffffffffffffffda RBX: 00007ff1cd0159e0 RCX: 00007ff1ccefe3f1
>> [   79.695644] RDX: 000000000000003c RSI: 00000000000000e7 RDI: 0000000000000000
>> [   79.695656] RBP: 0000000000000000 R08: ffffffffffffff80 R09: 00007ff1cd020b20
>> [   79.695671] R10: 0000000000000000 R11: 0000000000000246 R12: 00007ff1cd0159e0
>> [   79.695684] R13: 0000000000000000 R14: 00007ff1cd01aee8 R15: 00007ff1cd01af00
>> [   79.695733]  </TASK>
>> [   79.695746] irq event stamp: 725979
>> [   79.695757] hardirqs last  enabled at (725979): [<ffffffffa9132d54>] finish_task_switch.isra.0+0xe4/0x3f0
>> [   79.695780] hardirqs last disabled at (725978): [<ffffffffa9eb4113>] __schedule+0xdd3/0x1670
>> [   79.695803] softirqs last  enabled at (725974): [<ffffffffa90fbc9d>] __irq_exit_rcu+0xed/0x160
>> [   79.695825] softirqs last disabled at (725969): [<ffffffffa90fbc9d>] __irq_exit_rcu+0xed/0x160
>> [   79.695845] ---[ end trace 0000000000000000 ]---
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/gma500/gem.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
>> index dffe37490206..4b7627a72637 100644
>> --- a/drivers/gpu/drm/gma500/gem.c
>> +++ b/drivers/gpu/drm/gma500/gem.c
>> @@ -112,12 +112,12 @@ static void psb_gem_free_object(struct drm_gem_object *obj)
>>  {
>>         struct psb_gem_object *pobj = to_psb_gem_object(obj);
>>
>> -       drm_gem_object_release(obj);
>> -
>>         /* Undo the mmap pin if we are destroying the object */
>>         if (pobj->mmapping)
>>                 psb_gem_unpin(pobj);
>>
>> +       drm_gem_object_release(obj);
>> +
>>         WARN_ON(pobj->in_gart && !pobj->stolen);
>>
>>         release_resource(&pobj->resource);
>> --
>> 2.37.2
>>
> 

Re: [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume

[Hans de Goede]

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

Re: [PATCH v2 3/3] drm/gma500: Fix (vblank) IRQs not working after suspend/resume

[Patrik Jakobsson]

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
>