[RFC 0/2] drm/xe: Support PCIe FLR

[RFC 0/2] drm/xe: Support PCIe FLR

[Aravind Iddamsetty]

PCI subsystem provides callbacks to inform the driver about a request to
do function level reset by user, initiated by writing to sysfs entry
/sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
without the need to do unbind and rebind as the driver needs to
reinitialize the device afresh post FLR.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>

we get the below splat but the device has recovered, have executed an
IGT post FLR.

[ 3107.473420] xe 0000:aa:00.0: preparing for PCIe FLR reset
[ 3107.473440] xe 0000:aa:00.0: [drm] removing device access to
userspace
[ 3107.477232] ------------[ cut here ]------------
[ 3107.477241] xe 0000:aa:00.0: [drm] Assertion `guc_read_stopped(guc)
== 1` failed!
               platform: 8 subplatform: 1
               graphics: Xe_HPC 12.60 step C0
               media: none 0.00 step **
[ 3107.477286] WARNING: CPU: 57 PID: 11844 at
drivers/gpu/drm/xe/xe_guc_submit.c:1490 xe_guc_submit_stop+0x436/0x450
[xe]
[ 3107.477434] Modules linked in: xe(O) drm_gpuvm video drm_ttm_helper
ttm gpu_sched drm_suballoc_helper drm_kunit_helpers drm_exec
drm_display_helper kunit drm_buddy cfbfillrect cfbimgblt fb_io_fops
cfbcopyarea nfnetlink br_netfilter overlay x86_pkg_temp_thermal coretemp
kvm_intel pmt_telemetry pmt_class kvm irqbypass snd_hda_codec_realtek
snd_hda_codec_generic snd_hda_intel snd_intel_dspcfg snd_hda_codec
snd_hwdep snd_hda_core snd_pcm intel_vsec acpi_power_meter fuse
ip_tables x_tables ast i2c_algo_bit drm_shmem_helper crct10dif_pclmul
crc32_pclmul ghash_clmulni_intel i2c_i801 drm_kms_helper i2c_smbus
syscopyarea sysfillrect sysimgblt fb_sys_fops fb fbdev i2c_ismt igc drm
ptp drm_panel_orientation_quirks pps_core wmi ax88179_178a usbnet mii
[last unloaded: xe(O)]
[ 3107.477652] CPU: 57 PID: 11844 Comm: bash Tainted: G     U  W  O
6.8.0-rc3-xe #5
[ 3107.477664] Hardware name: Intel Corporation ArcherCity/ArcherCity,
BIOS EGSDCRB1.86B.0087.D13.2208261706 08/26/2022
[ 3107.477668] RIP: 0010:xe_guc_submit_stop+0x436/0x450 [xe]
[ 3107.477769] Code: 41 52 8b 4c 24 3c 51 48 c7 c1 3b b5 8e a0 53 4c 8b
5c 24 50 41 53 44 8b 4c 24 60 44 8b 44 24 64 48 8b 54 24 68 e8 5a ca 9e
e0 <0f> 0b 48 83 c4 40 e9 f8 fb ff ff e8 1a 29 77 e1 66 2e 0f 1f 84 00
[ 3107.477775] RSP: 0018:ffa000002ae27cc8 EFLAGS: 00010286
[ 3107.477783] RAX: 0000000000000000 RBX: 000000000000000c RCX:
0000000000000027
[ 3107.477787] RDX: 0000000000000027 RSI: 00000000fff7ffff RDI:
ff11003fffaaca48
[ 3107.477792] RBP: 0000000000000000 R08: 0000000000000000 R09:
ffa000002ae27b60
[ 3107.477796] R10: 0000000000000001 R11: 0000000000000001 R12:
ff1100208e579930
[ 3107.477799] R13: 0000000000000000 R14: ff11002094358000 R15:
ffffffffa08ec49b
[ 3107.477803] FS:  00007f5a7dfeb740(0000) GS:ff11003fffa80000(0000)
knlGS:0000000000000000
[ 3107.477808] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3107.477812] CR2: 000055a4fa2df5c8 CR3: 000000018b5ee001 CR4:
0000000000f71ef0
[ 3107.477817] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 3107.477821] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7:
0000000000000400
[ 3107.477825] PKRU: 55555554
[ 3107.477828] Call Trace:
[ 3107.477833]  <TASK>
[ 3107.477837]  ? __warn+0x84/0x170
[ 3107.477852]  ? xe_guc_submit_stop+0x436/0x450 [xe]
[ 3107.477950]  ? report_bug+0x1bd/0x1d0
[ 3107.477965]  ? handle_bug+0x3c/0x70
[ 3107.477975]  ? exc_invalid_op+0x18/0x70
[ 3107.477984]  ? asm_exc_invalid_op+0x1a/0x20
[ 3107.478004]  ? xe_guc_submit_stop+0x436/0x450 [xe]
[ 3107.478099]  ? xe_guc_submit_stop+0x436/0x450 [xe]
[ 3107.478196]  xe_idle_gt+0x3d/0x60 [xe]
[ 3107.478267]  xe_pci_reset_prepare+0xc2/0x270 [xe]
[ 3107.478354]  pci_dev_save_and_disable+0x29/0x60
[ 3107.478362]  pci_reset_function+0x3b/0x80
[ 3107.478367]  reset_store+0x5b/0xa0
[ 3107.478374]  kernfs_fop_write_iter+0x12e/0x1d0
[ 3107.478380]  vfs_write+0x321/0x550
[ 3107.478390]  ksys_write+0x69/0xf0
[ 3107.478394]  do_syscall_64+0x6f/0x140
[ 3107.478400]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
[ 3107.478404] RIP: 0033:0x7f5a7dd14a37
[ 3107.478408] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f
1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f
05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
[ 3107.478412] RSP: 002b:00007fffbff218e8 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 3107.478417] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
00007f5a7dd14a37
[ 3107.478419] RDX: 0000000000000002 RSI: 000055a4fa300f90 RDI:
0000000000000001
[ 3107.478421] RBP: 000055a4fa300f90 R08: 00007f5a7ddd1460 R09:
000000007fffffff
[ 3107.478424] R10: 0000000000000000 R11: 0000000000000246 R12:
0000000000000002
[ 3107.478426] R13: 00007f5a7de1a780 R14: 00007f5a7de16600 R15:
00007f5a7de15a00
[ 3107.478434]  </TASK>
[ 3107.478436] irq event stamp: 34064143
[ 3107.478438] hardirqs last  enabled at (34064149):
[<ffffffff81181ee5>] console_unlock+0x115/0x130
[ 3107.478444] hardirqs last disabled at (34064154):
[<ffffffff81181eca>] console_unlock+0xfa/0x130
[ 3107.478446] softirqs last  enabled at (34062134):
[<ffffffff81e7902b>] __do_softirq+0x2ab/0x360
[ 3107.478452] softirqs last disabled at (34062125):
[<ffffffff810e271f>] irq_exit_rcu+0x7f/0xa0
[ 3107.478458] ---[ end trace 0000000000000000 ]---
[ 3107.694394] xe 0000:aa:00.0: PCI device went through FLR, reenabling
the device
[ 3107.699786] xe 0000:aa:00.0: [drm] *ERROR* GuC RC enable failed:
-ECANCELED
[ 3107.699863] ------------[ cut here ]------------
[ 3107.699866] WARNING: CPU: 57 PID: 11844 at
drivers/gpu/drm/xe/xe_guc_pc.c:931 xe_guc_pc_fini+0x71/0x80 [xe]
[ 3107.699949] Modules linked in: xe(O) drm_gpuvm video drm_ttm_helper
ttm gpu_sched drm_suballoc_helper drm_kunit_helpers drm_exec
drm_display_helper kunit drm_buddy cfbfillrect cfbimgblt fb_io_fops
cfbcopyarea nfnetlink br_netfilter overlay x86_pkg_temp_thermal coretemp
kvm_intel pmt_telemetry pmt_class kvm irqbypass snd_hda_codec_realtek
snd_hda_codec_generic snd_hda_intel snd_intel_dspcfg snd_hda_codec
snd_hwdep snd_hda_core snd_pcm intel_vsec acpi_power_meter fuse
ip_tables x_tables ast i2c_algo_bit drm_shmem_helper crct10dif_pclmul
crc32_pclmul ghash_clmulni_intel i2c_i801 drm_kms_helper i2c_smbus
syscopyarea sysfillrect sysimgblt fb_sys_fops fb fbdev i2c_ismt igc drm
ptp drm_panel_orientation_quirks pps_core wmi ax88179_178a usbnet mii
[last unloaded: xe(O)]
[ 3107.700034] CPU: 57 PID: 11844 Comm: bash Tainted: G     U  W  O
6.8.0-rc3-xe #5
[ 3107.700037] Hardware name: Intel Corporation ArcherCity/ArcherCity,
BIOS EGSDCRB1.86B.0087.D13.2208261706 08/26/2022
[ 3107.700040] RIP: 0010:xe_guc_pc_fini+0x71/0x80 [xe]
[ 3107.700091] Code: fe ff ff 85 c0 75 1f 4c 89 e7 5d be ff ff 00 00 41
5c e9 c2 62 fe ff 5d 48 8d be e8 db ff ff 41 5c e9 13 eb fe ff 0f 0b eb
dd <0f> 0b eb cd 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90 90 90 90 90
[ 3107.700094] RSP: 0018:ffa000002ae27d08 EFLAGS: 00010282
[ 3107.700098] RAX: 00000000ffffff83 RBX: ff11002094358000 RCX:
0000000000000000
[ 3107.700100] RDX: ff11003fffab8dd0 RSI: ff11003fffaaca40 RDI:
ff11003fffaaca40
[ 3107.700102] RBP: ff1100208e57a440 R08: 0000000000000000 R09:
ffa000002ae27aa0
[ 3107.700104] R10: 0000000000000001 R11: 0000000000000001 R12:
ff1100208e578058
[ 3107.700106] R13: ff11002086dd6c40 R14: dead000000000122 R15:
dead000000000100
[ 3107.700108] FS:  00007f5a7dfeb740(0000) GS:ff11003fffa80000(0000)
knlGS:0000000000000000
[ 3107.700110] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3107.700112] CR2: 000055a4fa2df5c8 CR3: 000000018b5ee001 CR4:
0000000000f71ef0
[ 3107.700115] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 3107.700117] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7:
0000000000000400
[ 3107.700119] PKRU: 55555554
[ 3107.700121] Call Trace:
[ 3107.700123]  <TASK>
[ 3107.700125]  ? __warn+0x84/0x170
[ 3107.700133]  ? xe_guc_pc_fini+0x71/0x80 [xe]
[ 3107.700180]  ? report_bug+0x1bd/0x1d0
[ 3107.700188]  ? handle_bug+0x3c/0x70
[ 3107.700192]  ? exc_invalid_op+0x18/0x70
[ 3107.700196]  ? asm_exc_invalid_op+0x1a/0x20
[ 3107.700205]  ? xe_guc_pc_fini+0x71/0x80 [xe]
[ 3107.700256]  ? xe_guc_pc_fini+0x3e/0x80 [xe]
[ 3107.700302]  drm_managed_release+0xb5/0x160 [drm]
[ 3107.700364]  drm_dev_release+0x33/0x40 [drm]
[ 3107.700392]  ? __pfx_devm_action_release+0x10/0x10
[ 3107.700399]  devres_release+0x28/0x60
[ 3107.700403]  devm_release_action+0x3c/0x70
[ 3107.700407]  ? __pfx_devm_drm_dev_init_release+0x10/0x10 [drm]
[ 3107.700436]  xe_pci_reset_done+0x86/0xc0 [xe]
[ 3107.700529]  pci_reset_function+0x4e/0x80
[ 3107.700538]  reset_store+0x5b/0xa0
[ 3107.700546]  kernfs_fop_write_iter+0x12e/0x1d0
[ 3107.700554]  vfs_write+0x321/0x550
[ 3107.700566]  ksys_write+0x69/0xf0
[ 3107.700571]  do_syscall_64+0x6f/0x140
[ 3107.700579]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
[ 3107.700585] RIP: 0033:0x7f5a7dd14a37
[ 3107.700589] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f
1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f
05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
[ 3107.700593] RSP: 002b:00007fffbff218e8 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 3107.700600] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
00007f5a7dd14a37
[ 3107.700604] RDX: 0000000000000002 RSI: 000055a4fa300f90 RDI:
0000000000000001
[ 3107.700608] RBP: 000055a4fa300f90 R08: 00007f5a7ddd1460 R09:
000000007fffffff
[ 3107.700612] R10: 0000000000000000 R11: 0000000000000246 R12:
0000000000000002
[ 3107.700615] R13: 00007f5a7de1a780 R14: 00007f5a7de16600 R15:
00007f5a7de15a00
[ 3107.700624]  </TASK>
[ 3107.700627] irq event stamp: 34067435
[ 3107.700631] hardirqs last  enabled at (34067441):
[<ffffffff81181ee5>] console_unlock+0x115/0x130
[ 3107.700638] hardirqs last disabled at (34067446):
[<ffffffff81181eca>] console_unlock+0xfa/0x130
[ 3107.700642] softirqs last  enabled at (34066658):
[<ffffffff81e7902b>] __do_softirq+0x2ab/0x360
[ 3107.700649] softirqs last disabled at (34066519):
[<ffffffff810e271f>] irq_exit_rcu+0x7f/0xa0
[ 3107.700656] ---[ end trace 0000000000000000 ]---
[ 3107.700661] xe 0000:aa:00.0: [drm] *ERROR* GuC PC shutdown -ECANCELED
[ 3107.700707] ------------[ cut here ]------------
[ 3107.700710] WARNING: CPU: 57 PID: 11844 at
drivers/gpu/drm/xe/xe_guc_pc.c:932 xe_guc_pc_fini+0x6d/0x80 [xe]
[ 3107.700761] Modules linked in: xe(O) drm_gpuvm video drm_ttm_helper
ttm gpu_sched drm_suballoc_helper drm_kunit_helpers drm_exec
drm_display_helper kunit drm_buddy cfbfillrect cfbimgblt fb_io_fops
cfbcopyarea nfnetlink br_netfilter overlay x86_pkg_temp_thermal coretemp
kvm_intel pmt_telemetry pmt_class kvm irqbypass snd_hda_codec_realtek
snd_hda_codec_generic snd_hda_intel snd_intel_dspcfg snd_hda_codec
snd_hwdep snd_hda_core snd_pcm intel_vsec acpi_power_meter fuse
ip_tables x_tables ast i2c_algo_bit drm_shmem_helper crct10dif_pclmul
crc32_pclmul ghash_clmulni_intel i2c_i801 drm_kms_helper i2c_smbus
syscopyarea sysfillrect sysimgblt fb_sys_fops fb fbdev i2c_ismt igc drm
ptp drm_panel_orientation_quirks pps_core wmi ax88179_178a usbnet mii
[last unloaded: xe(O)]
[ 3107.700860] CPU: 57 PID: 11844 Comm: bash Tainted: G     U  W  O
6.8.0-rc3-xe #5
[ 3107.700863] Hardware name: Intel Corporation ArcherCity/ArcherCity,
BIOS EGSDCRB1.86B.0087.D13.2208261706 08/26/2022
[ 3107.700865] RIP: 0010:xe_guc_pc_fini+0x6d/0x80 [xe]
[ 3107.700914] Code: 89 ef e8 36 fe ff ff 85 c0 75 1f 4c 89 e7 5d be ff
ff 00 00 41 5c e9 c2 62 fe ff 5d 48 8d be e8 db ff ff 41 5c e9 13 eb fe
ff <0f> 0b eb dd 0f 0b eb cd 66 66 2e 0f 1f 84 00 00 00 00 00 90 90 90
[ 3107.700917] RSP: 0018:ffa000002ae27d08 EFLAGS: 00010282
[ 3107.700920] RAX: 00000000ffffff83 RBX: ff11002094358000 RCX:
0000000000000000
[ 3107.700923] RDX: 0000000000000027 RSI: 00000000fff7ffff RDI:
ff11003fffaaca48
[ 3107.700925] RBP: ff1100208e57a440 R08: 0000000000000000 R09:
ffa000002ae27ab0
[ 3107.700927] R10: 0000000000000001 R11: 0000000000000001 R12:
ff1100208e578058
[ 3107.700929] R13: ff11002086dd6c40 R14: dead000000000122 R15:
dead000000000100
[ 3107.700931] FS:  00007f5a7dfeb740(0000) GS:ff11003fffa80000(0000)
knlGS:0000000000000000
[ 3107.700933] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 3107.700936] CR2: 000055a4fa2df5c8 CR3: 000000018b5ee001 CR4:
0000000000f71ef0
[ 3107.700939] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
0000000000000000
[ 3107.700941] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7:
0000000000000400
[ 3107.700944] PKRU: 55555554
[ 3107.700946] Call Trace:
[ 3107.700949]  <TASK>
[ 3107.700951]  ? __warn+0x84/0x170
[ 3107.700956]  ? xe_guc_pc_fini+0x6d/0x80 [xe]
[ 3107.701005]  ? report_bug+0x1bd/0x1d0
[ 3107.701011]  ? handle_bug+0x3c/0x70
[ 3107.701015]  ? exc_invalid_op+0x18/0x70
[ 3107.701019]  ? asm_exc_invalid_op+0x1a/0x20
[ 3107.701026]  ? xe_guc_pc_fini+0x6d/0x80 [xe]
[ 3107.701076]  ? xe_guc_pc_fini+0x4a/0x80 [xe]
[ 3107.701125]  drm_managed_release+0xb5/0x160 [drm]
[ 3107.701160]  drm_dev_release+0x33/0x40 [drm]
[ 3107.701189]  ? __pfx_devm_action_release+0x10/0x10
[ 3107.701193]  devres_release+0x28/0x60
[ 3107.701198]  devm_release_action+0x3c/0x70
[ 3107.701202]  ? __pfx_devm_drm_dev_init_release+0x10/0x10 [drm]
[ 3107.701229]  xe_pci_reset_done+0x86/0xc0 [xe]
[ 3107.701286]  pci_reset_function+0x4e/0x80
[ 3107.701289]  reset_store+0x5b/0xa0
[ 3107.701295]  kernfs_fop_write_iter+0x12e/0x1d0
[ 3107.701300]  vfs_write+0x321/0x550
[ 3107.701308]  ksys_write+0x69/0xf0
[ 3107.701313]  do_syscall_64+0x6f/0x140
[ 3107.701318]  entry_SYSCALL_64_after_hwframe+0x6e/0x76
[ 3107.701321] RIP: 0033:0x7f5a7dd14a37
[ 3107.701325] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f
1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f
05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
[ 3107.701327] RSP: 002b:00007fffbff218e8 EFLAGS: 00000246 ORIG_RAX:
0000000000000001
[ 3107.701331] RAX: ffffffffffffffda RBX: 0000000000000002 RCX:
00007f5a7dd14a37
[ 3107.701333] RDX: 0000000000000002 RSI: 000055a4fa300f90 RDI:
0000000000000001
[ 3107.701335] RBP: 000055a4fa300f90 R08: 00007f5a7ddd1460 R09:
000000007fffffff
[ 3107.701337] R10: 0000000000000000 R11: 0000000000000246 R12:
0000000000000002
[ 3107.701340] R13: 00007f5a7de1a780 R14: 00007f5a7de16600 R15:
00007f5a7de15a00
[ 3107.701348]  </TASK>
[ 3107.701351] irq event stamp: 34068201
[ 3107.701352] hardirqs last  enabled at (34068207):
[<ffffffff81181ee5>] console_unlock+0x115/0x130
[ 3107.701355] hardirqs last disabled at (34068212):
[<ffffffff81181eca>] console_unlock+0xfa/0x130
[ 3107.701358] softirqs last  enabled at (34066658):
[<ffffffff81e7902b>] __do_softirq+0x2ab/0x360
[ 3107.701362] softirqs last disabled at (34066519):
[<ffffffff810e271f>] irq_exit_rcu+0x7f/0xa0
[ 3107.701364] ---[ end trace 0000000000000000 ]---
[ 3114.508489] xe 0000:aa:00.0: [drm] tile_count: 2, reduced_tile_count
1
[ 3114.511963] xe 0000:aa:00.0: [drm] Using GuC firmware from
xe/pvc_guc_70.9.1.bin version 70.9.1
[ 3114.542830] xe 0000:aa:00.0: [drm] VISIBLE VRAM: 0x00002c7000000000,
0x0000001000000000
[ 3114.542855] xe 0000:aa:00.0: [drm] VRAM[0, 0]: Actual physical size
0x0000000c00000000, usable size exclude stolen 0x0000000bff000000, CPU
accessible size 0x0000000bff000000
[ 3114.542858] xe 0000:aa:00.0: [drm] VRAM[0, 0]: DPA range:
[0x0000000000000000-c00000000], io range:
[0x00002c7000000000-2c7bff000000]
[ 3114.542862] xe 0000:aa:00.0: [drm] Total VRAM: 0x00002c7000000000,
0x0000000c00000000
[ 3114.542865] xe 0000:aa:00.0: [drm] Available VRAM:
0x00002c7000000000, 0x0000000bff000000
[ 3114.773724] xe 0000:aa:00.0: [drm] bcs6 fused off
[ 3114.773728] xe 0000:aa:00.0: [drm] bcs7 fused off
[ 3114.806777] xe 0000:aa:00.0: [drm] GT0: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 3114.808267] [drm] Initialized xe 1.1.0 20201103 for 0000:aa:00.0 on
minor 1
[ 3159.484345] Console: switching to colour dummy device 80x25
[ 3159.484422] [IGT] xe_exec_basic: executing
[ 3159.502594] [IGT] xe_exec_basic: starting subtest once-basic
[ 3159.526649] [IGT] xe_exec_basic: finished subtest once-basic, SUCCESS
[ 3159.526929] [IGT] xe_exec_basic: exiting, ret=0

Aravind Iddamsetty (2):
  drm: add devm release action
  drm/xe/FLR: Support PCIe FLR

 drivers/gpu/drm/drm_drv.c            |  6 ++
 drivers/gpu/drm/xe/Makefile          |  1 +
 drivers/gpu/drm/xe/xe_device_types.h |  3 +
 drivers/gpu/drm/xe/xe_gt.c           | 31 ++++++---
 drivers/gpu/drm/xe/xe_gt.h           |  1 +
 drivers/gpu/drm/xe/xe_pci.c          | 53 ++++++++++++++--
 drivers/gpu/drm/xe/xe_pci.h          |  6 +-
 drivers/gpu/drm/xe/xe_pci_err.c      | 94 ++++++++++++++++++++++++++++
 include/drm/drm_drv.h                |  4 ++
 9 files changed, 184 insertions(+), 15 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c

-- 
2.25.1

[RFC 1/2] drm: add devm release action

[Aravind Iddamsetty]

In scenarios where drm_dev_put is directly called by driver we want to
release devm_drm_dev_init_release action associated with struct
drm_device.

Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>

Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
---
 drivers/gpu/drm/drm_drv.c | 6 ++++++
 include/drm/drm_drv.h     | 4 ++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 243cacb3575c..b23c5081812b 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
 					devm_drm_dev_init_release, dev);
 }
 
+void __devm_drm_dev_release_action(struct drm_device *dev)
+{
+	devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
+}
+EXPORT_SYMBOL(__devm_drm_dev_release_action);
+
 void *__devm_drm_dev_alloc(struct device *parent,
 			   const struct drm_driver *driver,
 			   size_t size, size_t offset)
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 8878260d7529..a1d12cb72088 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -444,6 +444,8 @@ struct drm_driver {
 	const struct file_operations *fops;
 };
 
+void __devm_drm_dev_release_action(struct drm_device *dev);
+
 void *__devm_drm_dev_alloc(struct device *parent,
 			   const struct drm_driver *driver,
 			   size_t size, size_t offset);
@@ -477,6 +479,8 @@ void *__devm_drm_dev_alloc(struct device *parent,
 #define devm_drm_dev_alloc(parent, driver, type, member) \
 	((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \
 				       offsetof(type, member)))
+#define devm_drm_release_action(drm_dev) \
+	__devm_drm_dev_release_action(drm_dev)
 
 struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
 				 struct device *parent);
-- 
2.25.1

Re: [RFC 1/2] drm: add devm release action

[Rodrigo Vivi]

On Wed, Mar 20, 2024 at 04:14:25PM +0530, Aravind Iddamsetty wrote:
> In scenarios where drm_dev_put is directly called by driver we want to
> release devm_drm_dev_init_release action associated with struct
> drm_device.
> 
> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
> 
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 6 ++++++
>  include/drm/drm_drv.h     | 4 ++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 243cacb3575c..b23c5081812b 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
>  					devm_drm_dev_init_release, dev);
>  }
>  
> +void __devm_drm_dev_release_action(struct drm_device *dev)
> +{
> +	devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
> +}
> +EXPORT_SYMBOL(__devm_drm_dev_release_action);
> +
>  void *__devm_drm_dev_alloc(struct device *parent,
>  			   const struct drm_driver *driver,
>  			   size_t size, size_t offset)
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 8878260d7529..a1d12cb72088 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -444,6 +444,8 @@ struct drm_driver {
>  	const struct file_operations *fops;
>  };
>  
> +void __devm_drm_dev_release_action(struct drm_device *dev);
> +
>  void *__devm_drm_dev_alloc(struct device *parent,
>  			   const struct drm_driver *driver,
>  			   size_t size, size_t offset);
> @@ -477,6 +479,8 @@ void *__devm_drm_dev_alloc(struct device *parent,
>  #define devm_drm_dev_alloc(parent, driver, type, member) \
>  	((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \
>  				       offsetof(type, member)))
> +#define devm_drm_release_action(drm_dev) \
> +	__devm_drm_dev_release_action(drm_dev)

why not simply renaming the original function?

>  
>  struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
>  				 struct device *parent);
> -- 
> 2.25.1
> 

Re: [RFC 1/2] drm: add devm release action

[Aravind Iddamsetty]

On 21/03/24 02:23, Rodrigo Vivi wrote:
> On Wed, Mar 20, 2024 at 04:14:25PM +0530, Aravind Iddamsetty wrote:
>> In scenarios where drm_dev_put is directly called by driver we want to
>> release devm_drm_dev_init_release action associated with struct
>> drm_device.
>>
>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_drv.c | 6 ++++++
>>  include/drm/drm_drv.h     | 4 ++++
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 243cacb3575c..b23c5081812b 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
>>  					devm_drm_dev_init_release, dev);
>>  }
>>  
>> +void __devm_drm_dev_release_action(struct drm_device *dev)
>> +{
>> +	devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
>> +}
>> +EXPORT_SYMBOL(__devm_drm_dev_release_action);
>> +
>>  void *__devm_drm_dev_alloc(struct device *parent,
>>  			   const struct drm_driver *driver,
>>  			   size_t size, size_t offset)
>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>> index 8878260d7529..a1d12cb72088 100644
>> --- a/include/drm/drm_drv.h
>> +++ b/include/drm/drm_drv.h
>> @@ -444,6 +444,8 @@ struct drm_driver {
>>  	const struct file_operations *fops;
>>  };
>>  
>> +void __devm_drm_dev_release_action(struct drm_device *dev);
>> +
>>  void *__devm_drm_dev_alloc(struct device *parent,
>>  			   const struct drm_driver *driver,
>>  			   size_t size, size_t offset);
>> @@ -477,6 +479,8 @@ void *__devm_drm_dev_alloc(struct device *parent,
>>  #define devm_drm_dev_alloc(parent, driver, type, member) \
>>  	((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \
>>  				       offsetof(type, member)))
>> +#define devm_drm_release_action(drm_dev) \
>> +	__devm_drm_dev_release_action(drm_dev)
> why not simply renaming the original function?

didn't get you which function are you referring to?

Thanks,
Aravind.
>
>>  
>>  struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
>>  				 struct device *parent);
>> -- 
>> 2.25.1
>>

Re: [RFC 1/2] drm: add devm release action

[Jani Nikula]

On Thu, 21 Mar 2024, Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com> wrote:
> On 21/03/24 02:23, Rodrigo Vivi wrote:
>> On Wed, Mar 20, 2024 at 04:14:25PM +0530, Aravind Iddamsetty wrote:
>>> In scenarios where drm_dev_put is directly called by driver we want to
>>> release devm_drm_dev_init_release action associated with struct
>>> drm_device.
>>>
>>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>>>
>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/drm_drv.c | 6 ++++++
>>>  include/drm/drm_drv.h     | 4 ++++
>>>  2 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 243cacb3575c..b23c5081812b 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
>>>  					devm_drm_dev_init_release, dev);
>>>  }
>>>  
>>> +void __devm_drm_dev_release_action(struct drm_device *dev)
>>> +{
>>> +	devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
>>> +}
>>> +EXPORT_SYMBOL(__devm_drm_dev_release_action);
>>> +
>>>  void *__devm_drm_dev_alloc(struct device *parent,
>>>  			   const struct drm_driver *driver,
>>>  			   size_t size, size_t offset)
>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>> index 8878260d7529..a1d12cb72088 100644
>>> --- a/include/drm/drm_drv.h
>>> +++ b/include/drm/drm_drv.h
>>> @@ -444,6 +444,8 @@ struct drm_driver {
>>>  	const struct file_operations *fops;
>>>  };
>>>  
>>> +void __devm_drm_dev_release_action(struct drm_device *dev);
>>> +
>>>  void *__devm_drm_dev_alloc(struct device *parent,
>>>  			   const struct drm_driver *driver,
>>>  			   size_t size, size_t offset);
>>> @@ -477,6 +479,8 @@ void *__devm_drm_dev_alloc(struct device *parent,
>>>  #define devm_drm_dev_alloc(parent, driver, type, member) \
>>>  	((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \
>>>  				       offsetof(type, member)))
>>> +#define devm_drm_release_action(drm_dev) \
>>> +	__devm_drm_dev_release_action(drm_dev)
>> why not simply renaming the original function?
>
> didn't get you which function are you referring to?

Why add a function with double-underscores, and then wrap it in a macro
without the underscores?

devm_drm_dev_alloc() is that way only because it needs sizeof and
offsetof on the arguments, not to set a general example.

BR,
Jani.

>
> Thanks,
> Aravind.
>>
>>>  
>>>  struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
>>>  				 struct device *parent);
>>> -- 
>>> 2.25.1
>>>

-- 
Jani Nikula, Intel

Re: [RFC 1/2] drm: add devm release action

[Aravind Iddamsetty]

[-- Attachment #1: Type: text/plain, Size: 2904 bytes --]


On 21/03/24 12:17, Jani Nikula wrote:
> On Thu, 21 Mar 2024, Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com> wrote:
>> On 21/03/24 02:23, Rodrigo Vivi wrote:
>>> On Wed, Mar 20, 2024 at 04:14:25PM +0530, Aravind Iddamsetty wrote:
>>>> In scenarios where drm_dev_put is directly called by driver we want to
>>>> release devm_drm_dev_init_release action associated with struct
>>>> drm_device.
>>>>
>>>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>>>>
>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_drv.c | 6 ++++++
>>>>  include/drm/drm_drv.h     | 4 ++++
>>>>  2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 243cacb3575c..b23c5081812b 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
>>>>  					devm_drm_dev_init_release, dev);
>>>>  }
>>>>  
>>>> +void __devm_drm_dev_release_action(struct drm_device *dev)
>>>> +{
>>>> +	devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
>>>> +}
>>>> +EXPORT_SYMBOL(__devm_drm_dev_release_action);
>>>> +
>>>>  void *__devm_drm_dev_alloc(struct device *parent,
>>>>  			   const struct drm_driver *driver,
>>>>  			   size_t size, size_t offset)
>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>> index 8878260d7529..a1d12cb72088 100644
>>>> --- a/include/drm/drm_drv.h
>>>> +++ b/include/drm/drm_drv.h
>>>> @@ -444,6 +444,8 @@ struct drm_driver {
>>>>  	const struct file_operations *fops;
>>>>  };
>>>>  
>>>> +void __devm_drm_dev_release_action(struct drm_device *dev);
>>>> +
>>>>  void *__devm_drm_dev_alloc(struct device *parent,
>>>>  			   const struct drm_driver *driver,
>>>>  			   size_t size, size_t offset);
>>>> @@ -477,6 +479,8 @@ void *__devm_drm_dev_alloc(struct device *parent,
>>>>  #define devm_drm_dev_alloc(parent, driver, type, member) \
>>>>  	((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \
>>>>  				       offsetof(type, member)))
>>>> +#define devm_drm_release_action(drm_dev) \
>>>> +	__devm_drm_dev_release_action(drm_dev)
>>> why not simply renaming the original function?
>> didn't get you which function are you referring to?
> Why add a function with double-underscores, and then wrap it in a macro
> without the underscores?
>
> devm_drm_dev_alloc() is that way only because it needs sizeof and
> offsetof on the arguments, not to set a general example.

Oh ok got it Rodrigo was referring to _devm_drm_dev_release_action.

right, I took the reference of devm_drm_dev_alloc, thanks for the explanation
will change it.
Regards,
Aravind.
>
> BR,
> Jani.
>
>> Thanks,
>> Aravind.
>>>>  
>>>>  struct drm_device *drm_dev_alloc(const struct drm_driver *driver,
>>>>  				 struct device *parent);
>>>> -- 
>>>> 2.25.1
>>>>

[-- Attachment #2: Type: text/html, Size: 4645 bytes --]

[RFC 2/2] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

PCI subsystem provides callbacks to inform the driver about a request to
do function level reset by user, initiated by writing to sysfs entry
/sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
without the need to do unbind and rebind as the driver needs to
reinitialize the device afresh post FLR.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
---
 drivers/gpu/drm/xe/Makefile          |  1 +
 drivers/gpu/drm/xe/xe_device_types.h |  3 +
 drivers/gpu/drm/xe/xe_gt.c           | 31 ++++++---
 drivers/gpu/drm/xe/xe_gt.h           |  1 +
 drivers/gpu/drm/xe/xe_pci.c          | 53 ++++++++++++++--
 drivers/gpu/drm/xe/xe_pci.h          |  6 +-
 drivers/gpu/drm/xe/xe_pci_err.c      | 94 ++++++++++++++++++++++++++++
 7 files changed, 174 insertions(+), 15 deletions(-)
 create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 3c3e67885559..1447712fec65 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -114,6 +114,7 @@ xe-y += xe_bb.o \
 	xe_module.o \
 	xe_pat.o \
 	xe_pci.o \
+	xe_pci_err.o \
 	xe_pcode.o \
 	xe_pm.o \
 	xe_preempt_fence.o \
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 9785eef2e5a4..e9b8c7cbb428 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -455,6 +455,9 @@ struct xe_device {
 	/** @needs_flr_on_fini: requests function-reset on fini */
 	bool needs_flr_on_fini;
 
+	/** @pci_state: PCI state of device */
+	struct pci_saved_state *pci_state;
+
 	/* private: */
 
 #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
index 85408e7a932b..437874a9a5a0 100644
--- a/drivers/gpu/drm/xe/xe_gt.c
+++ b/drivers/gpu/drm/xe/xe_gt.c
@@ -629,6 +629,26 @@ static int do_gt_restart(struct xe_gt *gt)
 	return 0;
 }
 
+/* Idle the GT */
+int xe_idle_gt(struct xe_gt *gt)
+{
+	int err;
+
+	xe_gt_sanitize(gt);
+
+	xe_uc_gucrc_disable(&gt->uc);
+	xe_uc_stop_prepare(&gt->uc);
+	xe_gt_pagefault_reset(gt);
+
+	err = xe_uc_stop(&gt->uc);
+	if (err)
+		return err;
+
+	xe_gt_tlb_invalidation_reset(gt);
+
+	return err;
+}
+
 static int gt_reset(struct xe_gt *gt)
 {
 	int err;
@@ -645,21 +665,12 @@ static int gt_reset(struct xe_gt *gt)
 	}
 
 	xe_pm_runtime_get(gt_to_xe(gt));
-	xe_gt_sanitize(gt);
 
 	err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
 	if (err)
 		goto err_msg;
 
-	xe_uc_gucrc_disable(&gt->uc);
-	xe_uc_stop_prepare(&gt->uc);
-	xe_gt_pagefault_reset(gt);
-
-	err = xe_uc_stop(&gt->uc);
-	if (err)
-		goto err_out;
-
-	xe_gt_tlb_invalidation_reset(gt);
+	xe_idle_gt(gt);
 
 	err = do_gt_reset(gt);
 	if (err)
diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
index ed6ea8057e35..77df919199cc 100644
--- a/drivers/gpu/drm/xe/xe_gt.h
+++ b/drivers/gpu/drm/xe/xe_gt.h
@@ -43,6 +43,7 @@ int xe_gt_resume(struct xe_gt *gt);
 void xe_gt_reset_async(struct xe_gt *gt);
 void xe_gt_sanitize(struct xe_gt *gt);
 void xe_gt_remove(struct xe_gt *gt);
+int xe_idle_gt(struct xe_gt *gt);
 
 /**
  * xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index c401d4890386..fcd2a7f66f7b 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -383,6 +383,41 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
 
 #undef INTEL_VGA_DEVICE
 
+static bool xe_save_pci_state(struct pci_dev *pdev)
+{
+	struct xe_device *xe = pci_get_drvdata(pdev);
+
+	if (pci_save_state(pdev))
+		return false;
+
+	kfree(xe->pci_state);
+
+	xe->pci_state = pci_store_saved_state(pdev);
+
+	if (!xe->pci_state) {
+		drm_err(&xe->drm, "Failed to store PCI saved state\n");
+		return false;
+	}
+
+	return true;
+}
+
+void xe_load_pci_state(struct pci_dev *pdev)
+{
+	struct xe_device *xe = pci_get_drvdata(pdev);
+	int ret;
+
+	if (!xe->pci_state)
+		return;
+
+	ret = pci_load_saved_state(pdev, xe->pci_state);
+	if (!ret) {
+		pci_restore_state(pdev);
+	} else {
+		drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
+	}
+}
+
 /* is device_id present in comma separated list of ids */
 static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
 {
@@ -688,10 +723,12 @@ static void xe_pci_remove(struct pci_dev *pdev)
 
 	xe_device_remove(xe);
 	xe_pm_runtime_fini(xe);
+
+	kfree(xe->pci_state);
 	pci_set_drvdata(pdev, NULL);
 }
 
-static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	const struct xe_device_desc *desc = (const void *)ent->driver_data;
 	const struct xe_subplatform_desc *subplatform_desc;
@@ -786,6 +823,9 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	drm_dbg(&xe->drm, "d3cold: capable=%s\n",
 		str_yes_no(xe->d3cold.capable));
 
+	if (xe_save_pci_state(pdev))
+		pci_restore_state(pdev);
+
 	return 0;
 }
 
@@ -833,7 +873,7 @@ static int xe_pci_suspend(struct device *dev)
 	 */
 	d3cold_toggle(pdev, D3COLD_ENABLE);
 
-	pci_save_state(pdev);
+	xe_save_pci_state(pdev);
 	pci_disable_device(pdev);
 
 	return 0;
@@ -857,6 +897,8 @@ static int xe_pci_resume(struct device *dev)
 
 	pci_set_master(pdev);
 
+	xe_load_pci_state(pdev);
+
 	err = xe_pm_resume(pdev_to_xe_device(pdev));
 	if (err)
 		return err;
@@ -874,7 +916,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
 	if (err)
 		return err;
 
-	pci_save_state(pdev);
+	xe_save_pci_state(pdev);
 
 	if (xe->d3cold.allowed) {
 		d3cold_toggle(pdev, D3COLD_ENABLE);
@@ -899,7 +941,7 @@ static int xe_pci_runtime_resume(struct device *dev)
 	if (err)
 		return err;
 
-	pci_restore_state(pdev);
+	xe_load_pci_state(pdev);
 
 	if (xe->d3cold.allowed) {
 		err = pci_enable_device(pdev);
@@ -928,6 +970,8 @@ static const struct dev_pm_ops xe_pm_ops = {
 };
 #endif
 
+extern const struct pci_error_handlers xe_pci_err_handlers;
+
 static struct pci_driver xe_pci_driver = {
 	.name = DRIVER_NAME,
 	.id_table = pciidlist,
@@ -937,6 +981,7 @@ static struct pci_driver xe_pci_driver = {
 #ifdef CONFIG_PM_SLEEP
 	.driver.pm = &xe_pm_ops,
 #endif
+	.err_handler = &xe_pci_err_handlers,
 };
 
 int xe_register_pci_driver(void)
diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
index 611c1209b14c..9faf5380a09e 100644
--- a/drivers/gpu/drm/xe/xe_pci.h
+++ b/drivers/gpu/drm/xe/xe_pci.h
@@ -6,7 +6,11 @@
 #ifndef _XE_PCI_H_
 #define _XE_PCI_H_
 
+struct pci_dev;
+struct pci_device_id;
+
 int xe_register_pci_driver(void);
 void xe_unregister_pci_driver(void);
-
+void xe_load_pci_state(struct pci_dev *pdev);
+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
 #endif
diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
new file mode 100644
index 000000000000..ecc467a9c3af
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pci_err.c
@@ -0,0 +1,94 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include <linux/pci.h>
+#include <drm/drm_drv.h>
+
+#include "xe_device.h"
+#include "xe_gt.h"
+#include "xe_gt_printk.h"
+#include "xe_pci.h"
+#include "xe_pm.h"
+
+/**
+ * xe_pci_reset_prepare - Called when user issued a function level reset
+ * via /sys/bus/pci/devices/.../reset.
+ * @pdev: PCI device struct
+ */
+static void xe_pci_reset_prepare(struct pci_dev *pdev)
+{
+	struct xe_device *xe = pci_get_drvdata(pdev);
+	struct xe_gt *gt;
+	int id, err;
+
+	pci_warn(pdev, "preparing for PCIe FLR reset\n");
+
+	drm_warn(&xe->drm, "removing device access to userspace\n");
+	drm_dev_unplug(&xe->drm);
+
+	xe_pm_runtime_get(xe);
+	/* idle the GTs */
+	for_each_gt(gt, xe, id) {
+		err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+		if (err)
+			goto reset;
+		err = xe_idle_gt(gt);
+		if (err) {
+			xe_gt_err(gt, "failed to idle gt (%pe)\n", ERR_PTR(err));
+			goto reset;
+		}
+
+		err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+		XE_WARN_ON(err);
+	}
+	xe_pm_runtime_put(xe);
+
+reset:
+	pci_disable_device(pdev);
+}
+
+/**
+ * xe_pci_reset_done - Called when function level reset is done.
+ * @pdev: PCI device struct
+ */
+static void xe_pci_reset_done(struct pci_dev *pdev)
+{
+	const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
+	struct xe_device *xe = pci_get_drvdata(pdev);
+
+	dev_info(&pdev->dev,
+		 "PCI device went through FLR, reenabling the device\n");
+
+	if (pci_enable_device(pdev)) {
+		dev_err(&pdev->dev,
+			"Cannot re-enable PCI device after reset\n");
+		return;
+	}
+	pci_set_master(pdev);
+	xe_load_pci_state(pdev);
+
+	/*
+	 * We want to completely clean the driver and even destroy
+	 * the xe private data and reinitialize afresh similar to
+	 * probe
+	 */
+	pdev->driver->remove(pdev);
+	if (pci_dev_msi_enabled(pdev))
+		pci_free_irq_vectors(pdev);
+
+	devm_drm_release_action(&xe->drm);
+	pci_disable_device(pdev);
+
+	/*
+	 * if this fails the driver might be in a stale state, only option is
+	 * to unbind and rebind
+	 */
+	xe_pci_probe(pdev, ent);
+}
+
+const struct pci_error_handlers xe_pci_err_handlers = {
+	.reset_prepare = xe_pci_reset_prepare,
+	.reset_done = xe_pci_reset_done,
+};
-- 
2.25.1

Re: [RFC 2/2] drm/xe/FLR: Support PCIe FLR

[Rodrigo Vivi]

On Wed, Mar 20, 2024 at 04:14:26PM +0530, Aravind Iddamsetty wrote:
> PCI subsystem provides callbacks to inform the driver about a request to
> do function level reset by user, initiated by writing to sysfs entry
> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
> without the need to do unbind and rebind as the driver needs to
> reinitialize the device afresh post FLR.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> ---
>  drivers/gpu/drm/xe/Makefile          |  1 +
>  drivers/gpu/drm/xe/xe_device_types.h |  3 +
>  drivers/gpu/drm/xe/xe_gt.c           | 31 ++++++---
>  drivers/gpu/drm/xe/xe_gt.h           |  1 +
>  drivers/gpu/drm/xe/xe_pci.c          | 53 ++++++++++++++--
>  drivers/gpu/drm/xe/xe_pci.h          |  6 +-
>  drivers/gpu/drm/xe/xe_pci_err.c      | 94 ++++++++++++++++++++++++++++
>  7 files changed, 174 insertions(+), 15 deletions(-)
>  create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 3c3e67885559..1447712fec65 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -114,6 +114,7 @@ xe-y += xe_bb.o \
>  	xe_module.o \
>  	xe_pat.o \
>  	xe_pci.o \
> +	xe_pci_err.o \
>  	xe_pcode.o \
>  	xe_pm.o \
>  	xe_preempt_fence.o \
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 9785eef2e5a4..e9b8c7cbb428 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -455,6 +455,9 @@ struct xe_device {
>  	/** @needs_flr_on_fini: requests function-reset on fini */
>  	bool needs_flr_on_fini;
>  
> +	/** @pci_state: PCI state of device */
> +	struct pci_saved_state *pci_state;
> +
>  	/* private: */
>  
>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 85408e7a932b..437874a9a5a0 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -629,6 +629,26 @@ static int do_gt_restart(struct xe_gt *gt)
>  	return 0;
>  }
>  
> +/* Idle the GT */
> +int xe_idle_gt(struct xe_gt *gt)
> +{
> +	int err;
> +
> +	xe_gt_sanitize(gt);
> +
> +	xe_uc_gucrc_disable(&gt->uc);
> +	xe_uc_stop_prepare(&gt->uc);
> +	xe_gt_pagefault_reset(gt);
> +
> +	err = xe_uc_stop(&gt->uc);
> +	if (err)
> +		return err;
> +
> +	xe_gt_tlb_invalidation_reset(gt);
> +
> +	return err;
> +}
> +
>  static int gt_reset(struct xe_gt *gt)
>  {
>  	int err;
> @@ -645,21 +665,12 @@ static int gt_reset(struct xe_gt *gt)
>  	}
>  
>  	xe_pm_runtime_get(gt_to_xe(gt));
> -	xe_gt_sanitize(gt);
>  
>  	err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>  	if (err)
>  		goto err_msg;
>  
> -	xe_uc_gucrc_disable(&gt->uc);
> -	xe_uc_stop_prepare(&gt->uc);
> -	xe_gt_pagefault_reset(gt);
> -
> -	err = xe_uc_stop(&gt->uc);
> -	if (err)
> -		goto err_out;
> -
> -	xe_gt_tlb_invalidation_reset(gt);
> +	xe_idle_gt(gt);
>  
>  	err = do_gt_reset(gt);
>  	if (err)
> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> index ed6ea8057e35..77df919199cc 100644
> --- a/drivers/gpu/drm/xe/xe_gt.h
> +++ b/drivers/gpu/drm/xe/xe_gt.h
> @@ -43,6 +43,7 @@ int xe_gt_resume(struct xe_gt *gt);
>  void xe_gt_reset_async(struct xe_gt *gt);
>  void xe_gt_sanitize(struct xe_gt *gt);
>  void xe_gt_remove(struct xe_gt *gt);
> +int xe_idle_gt(struct xe_gt *gt);
>  
>  /**
>   * xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index c401d4890386..fcd2a7f66f7b 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -383,6 +383,41 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>  
>  #undef INTEL_VGA_DEVICE
>  
> +static bool xe_save_pci_state(struct pci_dev *pdev)
> +{
> +	struct xe_device *xe = pci_get_drvdata(pdev);
> +
> +	if (pci_save_state(pdev))
> +		return false;
> +
> +	kfree(xe->pci_state);
> +
> +	xe->pci_state = pci_store_saved_state(pdev);
> +
> +	if (!xe->pci_state) {
> +		drm_err(&xe->drm, "Failed to store PCI saved state\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +void xe_load_pci_state(struct pci_dev *pdev)
> +{
> +	struct xe_device *xe = pci_get_drvdata(pdev);
> +	int ret;
> +
> +	if (!xe->pci_state)
> +		return;
> +
> +	ret = pci_load_saved_state(pdev, xe->pci_state);
> +	if (!ret) {
> +		pci_restore_state(pdev);
> +	} else {
> +		drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
> +	}
> +}
> +
>  /* is device_id present in comma separated list of ids */
>  static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
>  {
> @@ -688,10 +723,12 @@ static void xe_pci_remove(struct pci_dev *pdev)
>  
>  	xe_device_remove(xe);
>  	xe_pm_runtime_fini(xe);
> +
> +	kfree(xe->pci_state);
>  	pci_set_drvdata(pdev, NULL);
>  }
>  
> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	const struct xe_device_desc *desc = (const void *)ent->driver_data;
>  	const struct xe_subplatform_desc *subplatform_desc;
> @@ -786,6 +823,9 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	drm_dbg(&xe->drm, "d3cold: capable=%s\n",
>  		str_yes_no(xe->d3cold.capable));
>  
> +	if (xe_save_pci_state(pdev))
> +		pci_restore_state(pdev);
> +
>  	return 0;
>  }
>  
> @@ -833,7 +873,7 @@ static int xe_pci_suspend(struct device *dev)
>  	 */
>  	d3cold_toggle(pdev, D3COLD_ENABLE);
>  
> -	pci_save_state(pdev);
> +	xe_save_pci_state(pdev);
>  	pci_disable_device(pdev);
>  
>  	return 0;
> @@ -857,6 +897,8 @@ static int xe_pci_resume(struct device *dev)
>  
>  	pci_set_master(pdev);
>  
> +	xe_load_pci_state(pdev);
> +
>  	err = xe_pm_resume(pdev_to_xe_device(pdev));
>  	if (err)
>  		return err;
> @@ -874,7 +916,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
>  	if (err)
>  		return err;
>  
> -	pci_save_state(pdev);
> +	xe_save_pci_state(pdev);
>  
>  	if (xe->d3cold.allowed) {
>  		d3cold_toggle(pdev, D3COLD_ENABLE);
> @@ -899,7 +941,7 @@ static int xe_pci_runtime_resume(struct device *dev)
>  	if (err)
>  		return err;
>  
> -	pci_restore_state(pdev);
> +	xe_load_pci_state(pdev);
>  
>  	if (xe->d3cold.allowed) {
>  		err = pci_enable_device(pdev);
> @@ -928,6 +970,8 @@ static const struct dev_pm_ops xe_pm_ops = {
>  };
>  #endif
>  
> +extern const struct pci_error_handlers xe_pci_err_handlers;
> +
>  static struct pci_driver xe_pci_driver = {
>  	.name = DRIVER_NAME,
>  	.id_table = pciidlist,
> @@ -937,6 +981,7 @@ static struct pci_driver xe_pci_driver = {
>  #ifdef CONFIG_PM_SLEEP
>  	.driver.pm = &xe_pm_ops,
>  #endif
> +	.err_handler = &xe_pci_err_handlers,
>  };
>  
>  int xe_register_pci_driver(void)
> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
> index 611c1209b14c..9faf5380a09e 100644
> --- a/drivers/gpu/drm/xe/xe_pci.h
> +++ b/drivers/gpu/drm/xe/xe_pci.h
> @@ -6,7 +6,11 @@
>  #ifndef _XE_PCI_H_
>  #define _XE_PCI_H_
>  
> +struct pci_dev;
> +struct pci_device_id;
> +
>  int xe_register_pci_driver(void);
>  void xe_unregister_pci_driver(void);
> -
> +void xe_load_pci_state(struct pci_dev *pdev);
> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
> new file mode 100644
> index 000000000000..ecc467a9c3af
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
> @@ -0,0 +1,94 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include <linux/pci.h>
> +#include <drm/drm_drv.h>
> +
> +#include "xe_device.h"
> +#include "xe_gt.h"
> +#include "xe_gt_printk.h"
> +#include "xe_pci.h"
> +#include "xe_pm.h"
> +
> +/**
> + * xe_pci_reset_prepare - Called when user issued a function level reset
> + * via /sys/bus/pci/devices/.../reset.
> + * @pdev: PCI device struct
> + */
> +static void xe_pci_reset_prepare(struct pci_dev *pdev)
> +{
> +	struct xe_device *xe = pci_get_drvdata(pdev);
> +	struct xe_gt *gt;
> +	int id, err;
> +
> +	pci_warn(pdev, "preparing for PCIe FLR reset\n");
> +
> +	drm_warn(&xe->drm, "removing device access to userspace\n");
> +	drm_dev_unplug(&xe->drm);
> +
> +	xe_pm_runtime_get(xe);
> +	/* idle the GTs */
> +	for_each_gt(gt, xe, id) {
> +		err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +		if (err)
> +			goto reset;
> +		err = xe_idle_gt(gt);
> +		if (err) {
> +			xe_gt_err(gt, "failed to idle gt (%pe)\n", ERR_PTR(err));
> +			goto reset;
> +		}
> +
> +		err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +		XE_WARN_ON(err);
> +	}
> +	xe_pm_runtime_put(xe);

perhaps we should only return the ref at the end of the done call?

> +
> +reset:
> +	pci_disable_device(pdev);
> +}
> +
> +/**
> + * xe_pci_reset_done - Called when function level reset is done.
> + * @pdev: PCI device struct
> + */
> +static void xe_pci_reset_done(struct pci_dev *pdev)
> +{
> +	const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
> +	struct xe_device *xe = pci_get_drvdata(pdev);
> +
> +	dev_info(&pdev->dev,
> +		 "PCI device went through FLR, reenabling the device\n");
> +
> +	if (pci_enable_device(pdev)) {
> +		dev_err(&pdev->dev,
> +			"Cannot re-enable PCI device after reset\n");
> +		return;
> +	}
> +	pci_set_master(pdev);
> +	xe_load_pci_state(pdev);
> +
> +	/*
> +	 * We want to completely clean the driver and even destroy
> +	 * the xe private data and reinitialize afresh similar to
> +	 * probe
> +	 */
> +	pdev->driver->remove(pdev);
> +	if (pci_dev_msi_enabled(pdev))
> +		pci_free_irq_vectors(pdev);
> +
> +	devm_drm_release_action(&xe->drm);

is there any opportunity to have that done at the prepare fn?
or do we really need that to happen after the pci state restoration?

> +	pci_disable_device(pdev);
> +
> +	/*
> +	 * if this fails the driver might be in a stale state, only option is
> +	 * to unbind and rebind
> +	 */
> +	xe_pci_probe(pdev, ent);
> +}
> +
> +const struct pci_error_handlers xe_pci_err_handlers = {
> +	.reset_prepare = xe_pci_reset_prepare,
> +	.reset_done = xe_pci_reset_done,

I had recently got to this functions while working on the xe_wedged/busted
thing and I was considering something exactly like that.
But you actually got something much simpler then I had thought.

how's the test of that? working reliably?
any IGT?

Thanks for taking care of that.

> +};
> -- 
> 2.25.1
> 

Re: [RFC 2/2] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

On 21/03/24 02:22, Rodrigo Vivi wrote:
> On Wed, Mar 20, 2024 at 04:14:26PM +0530, Aravind Iddamsetty wrote:
>> PCI subsystem provides callbacks to inform the driver about a request to
>> do function level reset by user, initiated by writing to sysfs entry
>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>> without the need to do unbind and rebind as the driver needs to
>> reinitialize the device afresh post FLR.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>>  drivers/gpu/drm/xe/Makefile          |  1 +
>>  drivers/gpu/drm/xe/xe_device_types.h |  3 +
>>  drivers/gpu/drm/xe/xe_gt.c           | 31 ++++++---
>>  drivers/gpu/drm/xe/xe_gt.h           |  1 +
>>  drivers/gpu/drm/xe/xe_pci.c          | 53 ++++++++++++++--
>>  drivers/gpu/drm/xe/xe_pci.h          |  6 +-
>>  drivers/gpu/drm/xe/xe_pci_err.c      | 94 ++++++++++++++++++++++++++++
>>  7 files changed, 174 insertions(+), 15 deletions(-)
>>  create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 3c3e67885559..1447712fec65 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -114,6 +114,7 @@ xe-y += xe_bb.o \
>>  	xe_module.o \
>>  	xe_pat.o \
>>  	xe_pci.o \
>> +	xe_pci_err.o \
>>  	xe_pcode.o \
>>  	xe_pm.o \
>>  	xe_preempt_fence.o \
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 9785eef2e5a4..e9b8c7cbb428 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -455,6 +455,9 @@ struct xe_device {
>>  	/** @needs_flr_on_fini: requests function-reset on fini */
>>  	bool needs_flr_on_fini;
>>  
>> +	/** @pci_state: PCI state of device */
>> +	struct pci_saved_state *pci_state;
>> +
>>  	/* private: */
>>  
>>  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index 85408e7a932b..437874a9a5a0 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -629,6 +629,26 @@ static int do_gt_restart(struct xe_gt *gt)
>>  	return 0;
>>  }
>>  
>> +/* Idle the GT */
>> +int xe_idle_gt(struct xe_gt *gt)
>> +{
>> +	int err;
>> +
>> +	xe_gt_sanitize(gt);
>> +
>> +	xe_uc_gucrc_disable(&gt->uc);
>> +	xe_uc_stop_prepare(&gt->uc);
>> +	xe_gt_pagefault_reset(gt);
>> +
>> +	err = xe_uc_stop(&gt->uc);
>> +	if (err)
>> +		return err;
>> +
>> +	xe_gt_tlb_invalidation_reset(gt);
>> +
>> +	return err;
>> +}
>> +
>>  static int gt_reset(struct xe_gt *gt)
>>  {
>>  	int err;
>> @@ -645,21 +665,12 @@ static int gt_reset(struct xe_gt *gt)
>>  	}
>>  
>>  	xe_pm_runtime_get(gt_to_xe(gt));
>> -	xe_gt_sanitize(gt);
>>  
>>  	err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>  	if (err)
>>  		goto err_msg;
>>  
>> -	xe_uc_gucrc_disable(&gt->uc);
>> -	xe_uc_stop_prepare(&gt->uc);
>> -	xe_gt_pagefault_reset(gt);
>> -
>> -	err = xe_uc_stop(&gt->uc);
>> -	if (err)
>> -		goto err_out;
>> -
>> -	xe_gt_tlb_invalidation_reset(gt);
>> +	xe_idle_gt(gt);
>>  
>>  	err = do_gt_reset(gt);
>>  	if (err)
>> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
>> index ed6ea8057e35..77df919199cc 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.h
>> +++ b/drivers/gpu/drm/xe/xe_gt.h
>> @@ -43,6 +43,7 @@ int xe_gt_resume(struct xe_gt *gt);
>>  void xe_gt_reset_async(struct xe_gt *gt);
>>  void xe_gt_sanitize(struct xe_gt *gt);
>>  void xe_gt_remove(struct xe_gt *gt);
>> +int xe_idle_gt(struct xe_gt *gt);
>>  
>>  /**
>>   * xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index c401d4890386..fcd2a7f66f7b 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -383,6 +383,41 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>>  
>>  #undef INTEL_VGA_DEVICE
>>  
>> +static bool xe_save_pci_state(struct pci_dev *pdev)
>> +{
>> +	struct xe_device *xe = pci_get_drvdata(pdev);
>> +
>> +	if (pci_save_state(pdev))
>> +		return false;
>> +
>> +	kfree(xe->pci_state);
>> +
>> +	xe->pci_state = pci_store_saved_state(pdev);
>> +
>> +	if (!xe->pci_state) {
>> +		drm_err(&xe->drm, "Failed to store PCI saved state\n");
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +void xe_load_pci_state(struct pci_dev *pdev)
>> +{
>> +	struct xe_device *xe = pci_get_drvdata(pdev);
>> +	int ret;
>> +
>> +	if (!xe->pci_state)
>> +		return;
>> +
>> +	ret = pci_load_saved_state(pdev, xe->pci_state);
>> +	if (!ret) {
>> +		pci_restore_state(pdev);
>> +	} else {
>> +		drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
>> +	}
>> +}
>> +
>>  /* is device_id present in comma separated list of ids */
>>  static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
>>  {
>> @@ -688,10 +723,12 @@ static void xe_pci_remove(struct pci_dev *pdev)
>>  
>>  	xe_device_remove(xe);
>>  	xe_pm_runtime_fini(xe);
>> +
>> +	kfree(xe->pci_state);
>>  	pci_set_drvdata(pdev, NULL);
>>  }
>>  
>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>>  	const struct xe_device_desc *desc = (const void *)ent->driver_data;
>>  	const struct xe_subplatform_desc *subplatform_desc;
>> @@ -786,6 +823,9 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  	drm_dbg(&xe->drm, "d3cold: capable=%s\n",
>>  		str_yes_no(xe->d3cold.capable));
>>  
>> +	if (xe_save_pci_state(pdev))
>> +		pci_restore_state(pdev);
>> +
>>  	return 0;
>>  }
>>  
>> @@ -833,7 +873,7 @@ static int xe_pci_suspend(struct device *dev)
>>  	 */
>>  	d3cold_toggle(pdev, D3COLD_ENABLE);
>>  
>> -	pci_save_state(pdev);
>> +	xe_save_pci_state(pdev);
>>  	pci_disable_device(pdev);
>>  
>>  	return 0;
>> @@ -857,6 +897,8 @@ static int xe_pci_resume(struct device *dev)
>>  
>>  	pci_set_master(pdev);
>>  
>> +	xe_load_pci_state(pdev);
>> +
>>  	err = xe_pm_resume(pdev_to_xe_device(pdev));
>>  	if (err)
>>  		return err;
>> @@ -874,7 +916,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
>>  	if (err)
>>  		return err;
>>  
>> -	pci_save_state(pdev);
>> +	xe_save_pci_state(pdev);
>>  
>>  	if (xe->d3cold.allowed) {
>>  		d3cold_toggle(pdev, D3COLD_ENABLE);
>> @@ -899,7 +941,7 @@ static int xe_pci_runtime_resume(struct device *dev)
>>  	if (err)
>>  		return err;
>>  
>> -	pci_restore_state(pdev);
>> +	xe_load_pci_state(pdev);
>>  
>>  	if (xe->d3cold.allowed) {
>>  		err = pci_enable_device(pdev);
>> @@ -928,6 +970,8 @@ static const struct dev_pm_ops xe_pm_ops = {
>>  };
>>  #endif
>>  
>> +extern const struct pci_error_handlers xe_pci_err_handlers;
>> +
>>  static struct pci_driver xe_pci_driver = {
>>  	.name = DRIVER_NAME,
>>  	.id_table = pciidlist,
>> @@ -937,6 +981,7 @@ static struct pci_driver xe_pci_driver = {
>>  #ifdef CONFIG_PM_SLEEP
>>  	.driver.pm = &xe_pm_ops,
>>  #endif
>> +	.err_handler = &xe_pci_err_handlers,
>>  };
>>  
>>  int xe_register_pci_driver(void)
>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>> index 611c1209b14c..9faf5380a09e 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.h
>> +++ b/drivers/gpu/drm/xe/xe_pci.h
>> @@ -6,7 +6,11 @@
>>  #ifndef _XE_PCI_H_
>>  #define _XE_PCI_H_
>>  
>> +struct pci_dev;
>> +struct pci_device_id;
>> +
>>  int xe_register_pci_driver(void);
>>  void xe_unregister_pci_driver(void);
>> -
>> +void xe_load_pci_state(struct pci_dev *pdev);
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>> new file mode 100644
>> index 000000000000..ecc467a9c3af
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
>> @@ -0,0 +1,94 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <drm/drm_drv.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_printk.h"
>> +#include "xe_pci.h"
>> +#include "xe_pm.h"
>> +
>> +/**
>> + * xe_pci_reset_prepare - Called when user issued a function level reset
>> + * via /sys/bus/pci/devices/.../reset.
>> + * @pdev: PCI device struct
>> + */
>> +static void xe_pci_reset_prepare(struct pci_dev *pdev)
>> +{
>> +	struct xe_device *xe = pci_get_drvdata(pdev);
>> +	struct xe_gt *gt;
>> +	int id, err;
>> +
>> +	pci_warn(pdev, "preparing for PCIe FLR reset\n");
>> +
>> +	drm_warn(&xe->drm, "removing device access to userspace\n");
>> +	drm_dev_unplug(&xe->drm);
>> +
>> +	xe_pm_runtime_get(xe);
>> +	/* idle the GTs */
>> +	for_each_gt(gt, xe, id) {
>> +		err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +		if (err)
>> +			goto reset;
>> +		err = xe_idle_gt(gt);
>> +		if (err) {
>> +			xe_gt_err(gt, "failed to idle gt (%pe)\n", ERR_PTR(err));
>> +			goto reset;
>> +		}
>> +
>> +		err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +		XE_WARN_ON(err);
>> +	}
>> +	xe_pm_runtime_put(xe);
> perhaps we should only return the ref at the end of the done call?
it looks to be that the runtime ref is already taken before reset_prepare
is invoked and released only after below done call.

drivers/pci/pci-sysfs.c: 

reset_store {

        pm_runtime_get_sync(dev);
        result = pci_reset_function(pdev);
        pm_runtime_put(dev);

}

>
>> +
>> +reset:
>> +	pci_disable_device(pdev);
>> +}
>> +
>> +/**
>> + * xe_pci_reset_done - Called when function level reset is done.
>> + * @pdev: PCI device struct
>> + */
>> +static void xe_pci_reset_done(struct pci_dev *pdev)
>> +{
>> +	const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>> +	struct xe_device *xe = pci_get_drvdata(pdev);
>> +
>> +	dev_info(&pdev->dev,
>> +		 "PCI device went through FLR, reenabling the device\n");
>> +
>> +	if (pci_enable_device(pdev)) {
>> +		dev_err(&pdev->dev,
>> +			"Cannot re-enable PCI device after reset\n");
>> +		return;
>> +	}
>> +	pci_set_master(pdev);
>> +	xe_load_pci_state(pdev);
>> +
>> +	/*
>> +	 * We want to completely clean the driver and even destroy
>> +	 * the xe private data and reinitialize afresh similar to
>> +	 * probe
>> +	 */
>> +	pdev->driver->remove(pdev);
>> +	if (pci_dev_msi_enabled(pdev))
>> +		pci_free_irq_vectors(pdev);
>> +
>> +	devm_drm_release_action(&xe->drm);
> is there any opportunity to have that done at the prepare fn?
> or do we really need that to happen after the pci state restoration?
ideally we better do it here, as the device might be in a non functional state
in reset prepare and in remove path we touch the device which might result in 
unwanted failures. it is safe to do here as we know the reset is done and we can clear the stale state.
>
>> +	pci_disable_device(pdev);
>> +
>> +	/*
>> +	 * if this fails the driver might be in a stale state, only option is
>> +	 * to unbind and rebind
>> +	 */
>> +	xe_pci_probe(pdev, ent);
>> +}
>> +
>> +const struct pci_error_handlers xe_pci_err_handlers = {
>> +	.reset_prepare = xe_pci_reset_prepare,
>> +	.reset_done = xe_pci_reset_done,
> I had recently got to this functions while working on the xe_wedged/busted
> thing and I was considering something exactly like that.
> But you actually got something much simpler then I had thought.
>
> how's the test of that? working reliably?
> any IGT?
I had tested manually for couple of times it worked, except for some warnings 
the output of which I pasted in cover letter. I'm yet to develop any IGT.
>
> Thanks for taking care of that.

Thank you!

Regards,
Aravind.
>
>> +};
>> -- 
>> 2.25.1
>>

Re: [RFC 2/2] drm/xe/FLR: Support PCIe FLR

[Lucas De Marchi]

On Wed, Mar 20, 2024 at 04:14:26PM +0530, Aravind Iddamsetty wrote:
>PCI subsystem provides callbacks to inform the driver about a request to
>do function level reset by user, initiated by writing to sysfs entry
>/sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>without the need to do unbind and rebind as the driver needs to
>reinitialize the device afresh post FLR.
>
>Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>---
> drivers/gpu/drm/xe/Makefile          |  1 +
> drivers/gpu/drm/xe/xe_device_types.h |  3 +
> drivers/gpu/drm/xe/xe_gt.c           | 31 ++++++---
> drivers/gpu/drm/xe/xe_gt.h           |  1 +
> drivers/gpu/drm/xe/xe_pci.c          | 53 ++++++++++++++--
> drivers/gpu/drm/xe/xe_pci.h          |  6 +-
> drivers/gpu/drm/xe/xe_pci_err.c      | 94 ++++++++++++++++++++++++++++
> 7 files changed, 174 insertions(+), 15 deletions(-)
> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>
>diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>index 3c3e67885559..1447712fec65 100644
>--- a/drivers/gpu/drm/xe/Makefile
>+++ b/drivers/gpu/drm/xe/Makefile
>@@ -114,6 +114,7 @@ xe-y += xe_bb.o \
> 	xe_module.o \
> 	xe_pat.o \
> 	xe_pci.o \
>+	xe_pci_err.o \
> 	xe_pcode.o \
> 	xe_pm.o \
> 	xe_preempt_fence.o \
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index 9785eef2e5a4..e9b8c7cbb428 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -455,6 +455,9 @@ struct xe_device {
> 	/** @needs_flr_on_fini: requests function-reset on fini */
> 	bool needs_flr_on_fini;
>
>+	/** @pci_state: PCI state of device */
>+	struct pci_saved_state *pci_state;
>+
> 	/* private: */
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>index 85408e7a932b..437874a9a5a0 100644
>--- a/drivers/gpu/drm/xe/xe_gt.c
>+++ b/drivers/gpu/drm/xe/xe_gt.c
>@@ -629,6 +629,26 @@ static int do_gt_restart(struct xe_gt *gt)
> 	return 0;
> }
>
>+/* Idle the GT */
>+int xe_idle_gt(struct xe_gt *gt)

any non-static function should use xe_gt_ prefix.

>+{
>+	int err;
>+
>+	xe_gt_sanitize(gt);
>+
>+	xe_uc_gucrc_disable(&gt->uc);
>+	xe_uc_stop_prepare(&gt->uc);
>+	xe_gt_pagefault_reset(gt);
>+
>+	err = xe_uc_stop(&gt->uc);
>+	if (err)
>+		return err;
>+
>+	xe_gt_tlb_invalidation_reset(gt);
>+
>+	return err;
>+}
>+
> static int gt_reset(struct xe_gt *gt)
> {
> 	int err;
>@@ -645,21 +665,12 @@ static int gt_reset(struct xe_gt *gt)
> 	}
>
> 	xe_pm_runtime_get(gt_to_xe(gt));
>-	xe_gt_sanitize(gt);
>
> 	err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> 	if (err)
> 		goto err_msg;
>
>-	xe_uc_gucrc_disable(&gt->uc);
>-	xe_uc_stop_prepare(&gt->uc);
>-	xe_gt_pagefault_reset(gt);
>-
>-	err = xe_uc_stop(&gt->uc);
>-	if (err)
>-		goto err_out;
>-
>-	xe_gt_tlb_invalidation_reset(gt);
>+	xe_idle_gt(gt);

this and the above should be in a commit alone
"drm/xe: Extract xe_gt_idle() helper" with explanation it will be used
in other places outside of gt_reset path.

but I'm a little bit confused here... why do you need to remove
xe_gt_sanitize() inside the function to make gt idle?

Lucas De Marchi

>
> 	err = do_gt_reset(gt);
> 	if (err)
>diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
>index ed6ea8057e35..77df919199cc 100644
>--- a/drivers/gpu/drm/xe/xe_gt.h
>+++ b/drivers/gpu/drm/xe/xe_gt.h
>@@ -43,6 +43,7 @@ int xe_gt_resume(struct xe_gt *gt);
> void xe_gt_reset_async(struct xe_gt *gt);
> void xe_gt_sanitize(struct xe_gt *gt);
> void xe_gt_remove(struct xe_gt *gt);
>+int xe_idle_gt(struct xe_gt *gt);
>
> /**
>  * xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
>diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>index c401d4890386..fcd2a7f66f7b 100644
>--- a/drivers/gpu/drm/xe/xe_pci.c
>+++ b/drivers/gpu/drm/xe/xe_pci.c
>@@ -383,6 +383,41 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>
> #undef INTEL_VGA_DEVICE
>
>+static bool xe_save_pci_state(struct pci_dev *pdev)
>+{
>+	struct xe_device *xe = pci_get_drvdata(pdev);
>+
>+	if (pci_save_state(pdev))
>+		return false;
>+
>+	kfree(xe->pci_state);
>+
>+	xe->pci_state = pci_store_saved_state(pdev);
>+
>+	if (!xe->pci_state) {
>+		drm_err(&xe->drm, "Failed to store PCI saved state\n");
>+		return false;
>+	}
>+
>+	return true;
>+}
>+
>+void xe_load_pci_state(struct pci_dev *pdev)
>+{
>+	struct xe_device *xe = pci_get_drvdata(pdev);
>+	int ret;
>+
>+	if (!xe->pci_state)
>+		return;
>+
>+	ret = pci_load_saved_state(pdev, xe->pci_state);
>+	if (!ret) {
>+		pci_restore_state(pdev);
>+	} else {
>+		drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
>+	}

please check coding style here and invert condition so you keep the
return-early pattern you started in the xe->pci_state check.


ret = pci_load_saved_state(pdev, xe->pci_state);
if (ret) {
	drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
	return;
}

>+}
>+
> /* is device_id present in comma separated list of ids */
> static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
> {
>@@ -688,10 +723,12 @@ static void xe_pci_remove(struct pci_dev *pdev)
>
> 	xe_device_remove(xe);
> 	xe_pm_runtime_fini(xe);
>+
>+	kfree(xe->pci_state);
> 	pci_set_drvdata(pdev, NULL);
> }
>
>-static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> 	const struct xe_device_desc *desc = (const void *)ent->driver_data;
> 	const struct xe_subplatform_desc *subplatform_desc;
>@@ -786,6 +823,9 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> 	drm_dbg(&xe->drm, "d3cold: capable=%s\n",
> 		str_yes_no(xe->d3cold.capable));
>
>+	if (xe_save_pci_state(pdev))
>+		pci_restore_state(pdev);

why do we save and restore in sequence when probing? If we only have to
have a saved stated for possible errors later, why do we restore on
success? /me confused

>+
> 	return 0;
> }
>
>@@ -833,7 +873,7 @@ static int xe_pci_suspend(struct device *dev)
> 	 */
> 	d3cold_toggle(pdev, D3COLD_ENABLE);
>
>-	pci_save_state(pdev);
>+	xe_save_pci_state(pdev);

all these no change in behavior with xe_ functions wrapping the the pci
ones could be delegated to a prep patch so it's easier to separate the
noise from the actual changes.

> 	pci_disable_device(pdev);
>
> 	return 0;
>@@ -857,6 +897,8 @@ static int xe_pci_resume(struct device *dev)
>
> 	pci_set_master(pdev);
>
>+	xe_load_pci_state(pdev);
>+
> 	err = xe_pm_resume(pdev_to_xe_device(pdev));
> 	if (err)
> 		return err;
>@@ -874,7 +916,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
> 	if (err)
> 		return err;
>
>-	pci_save_state(pdev);
>+	xe_save_pci_state(pdev);
>
> 	if (xe->d3cold.allowed) {
> 		d3cold_toggle(pdev, D3COLD_ENABLE);
>@@ -899,7 +941,7 @@ static int xe_pci_runtime_resume(struct device *dev)
> 	if (err)
> 		return err;
>
>-	pci_restore_state(pdev);
>+	xe_load_pci_state(pdev);
>
> 	if (xe->d3cold.allowed) {
> 		err = pci_enable_device(pdev);
>@@ -928,6 +970,8 @@ static const struct dev_pm_ops xe_pm_ops = {
> };
> #endif
>
>+extern const struct pci_error_handlers xe_pci_err_handlers;

not sure this really belongs into a separate compilation unit. Just
leave it in xe_pci.c? Adding the extern here doesn't make it pretty.

if we are adding a separate .c, then the struct definition should
probably be here, and you do it like

	static const struct pci_error_handlers xe_pci_err_handlers = {
		.reset_prepare = xe_pci_err_reset_prepare,
		.reset_done = xe_pci_err_reset_done,
	};


See xe_pm_ops, that was left here rather than in xe_pm.c.

Lucas De Marchi

>+
> static struct pci_driver xe_pci_driver = {
> 	.name = DRIVER_NAME,
> 	.id_table = pciidlist,
>@@ -937,6 +981,7 @@ static struct pci_driver xe_pci_driver = {
> #ifdef CONFIG_PM_SLEEP
> 	.driver.pm = &xe_pm_ops,
> #endif
>+	.err_handler = &xe_pci_err_handlers,
> };
>
> int xe_register_pci_driver(void)
>diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>index 611c1209b14c..9faf5380a09e 100644
>--- a/drivers/gpu/drm/xe/xe_pci.h
>+++ b/drivers/gpu/drm/xe/xe_pci.h
>@@ -6,7 +6,11 @@
> #ifndef _XE_PCI_H_
> #define _XE_PCI_H_
>
>+struct pci_dev;
>+struct pci_device_id;
>+
> int xe_register_pci_driver(void);
> void xe_unregister_pci_driver(void);
>-
>+void xe_load_pci_state(struct pci_dev *pdev);
>+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> #endif
>diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>new file mode 100644
>index 000000000000..ecc467a9c3af
>--- /dev/null
>+++ b/drivers/gpu/drm/xe/xe_pci_err.c
>@@ -0,0 +1,94 @@
>+// SPDX-License-Identifier: MIT
>+/*
>+ * Copyright © 2024 Intel Corporation
>+ */
>+
>+#include <linux/pci.h>
>+#include <drm/drm_drv.h>
>+
>+#include "xe_device.h"
>+#include "xe_gt.h"
>+#include "xe_gt_printk.h"
>+#include "xe_pci.h"
>+#include "xe_pm.h"
>+
>+/**
>+ * xe_pci_reset_prepare - Called when user issued a function level reset
>+ * via /sys/bus/pci/devices/.../reset.
>+ * @pdev: PCI device struct
>+ */
>+static void xe_pci_reset_prepare(struct pci_dev *pdev)
>+{
>+	struct xe_device *xe = pci_get_drvdata(pdev);
>+	struct xe_gt *gt;
>+	int id, err;
>+
>+	pci_warn(pdev, "preparing for PCIe FLR reset\n");
>+
>+	drm_warn(&xe->drm, "removing device access to userspace\n");
>+	drm_dev_unplug(&xe->drm);
>+
>+	xe_pm_runtime_get(xe);
>+	/* idle the GTs */
>+	for_each_gt(gt, xe, id) {
>+		err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>+		if (err)
>+			goto reset;
>+		err = xe_idle_gt(gt);
>+		if (err) {
>+			xe_gt_err(gt, "failed to idle gt (%pe)\n", ERR_PTR(err));
>+			goto reset;
>+		}
>+
>+		err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>+		XE_WARN_ON(err);
>+	}
>+	xe_pm_runtime_put(xe);
>+
>+reset:
>+	pci_disable_device(pdev);
>+}
>+
>+/**
>+ * xe_pci_reset_done - Called when function level reset is done.
>+ * @pdev: PCI device struct
>+ */
>+static void xe_pci_reset_done(struct pci_dev *pdev)
>+{
>+	const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>+	struct xe_device *xe = pci_get_drvdata(pdev);
>+
>+	dev_info(&pdev->dev,
>+		 "PCI device went through FLR, reenabling the device\n");
>+
>+	if (pci_enable_device(pdev)) {
>+		dev_err(&pdev->dev,
>+			"Cannot re-enable PCI device after reset\n");
>+		return;
>+	}
>+	pci_set_master(pdev);
>+	xe_load_pci_state(pdev);
>+
>+	/*
>+	 * We want to completely clean the driver and even destroy
>+	 * the xe private data and reinitialize afresh similar to
>+	 * probe
>+	 */
>+	pdev->driver->remove(pdev);
>+	if (pci_dev_msi_enabled(pdev))
>+		pci_free_irq_vectors(pdev);
>+
>+	devm_drm_release_action(&xe->drm);
>+	pci_disable_device(pdev);
>+
>+	/*
>+	 * if this fails the driver might be in a stale state, only option is
>+	 * to unbind and rebind
>+	 */
>+	xe_pci_probe(pdev, ent);
>+}
>+
>+const struct pci_error_handlers xe_pci_err_handlers = {
>+	.reset_prepare = xe_pci_reset_prepare,
>+	.reset_done = xe_pci_reset_done,
>+};
>-- 
>2.25.1
>

Re: [RFC 2/2] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

On 21/03/24 04:31, Lucas De Marchi wrote:
> On Wed, Mar 20, 2024 at 04:14:26PM +0530, Aravind Iddamsetty wrote:
>> PCI subsystem provides callbacks to inform the driver about a request to
>> do function level reset by user, initiated by writing to sysfs entry
>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>> without the need to do unbind and rebind as the driver needs to
>> reinitialize the device afresh post FLR.
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile          |  1 +
>> drivers/gpu/drm/xe/xe_device_types.h |  3 +
>> drivers/gpu/drm/xe/xe_gt.c           | 31 ++++++---
>> drivers/gpu/drm/xe/xe_gt.h           |  1 +
>> drivers/gpu/drm/xe/xe_pci.c          | 53 ++++++++++++++--
>> drivers/gpu/drm/xe/xe_pci.h          |  6 +-
>> drivers/gpu/drm/xe/xe_pci_err.c      | 94 ++++++++++++++++++++++++++++
>> 7 files changed, 174 insertions(+), 15 deletions(-)
>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 3c3e67885559..1447712fec65 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -114,6 +114,7 @@ xe-y += xe_bb.o \
>>     xe_module.o \
>>     xe_pat.o \
>>     xe_pci.o \
>> +    xe_pci_err.o \
>>     xe_pcode.o \
>>     xe_pm.o \
>>     xe_preempt_fence.o \
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 9785eef2e5a4..e9b8c7cbb428 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -455,6 +455,9 @@ struct xe_device {
>>     /** @needs_flr_on_fini: requests function-reset on fini */
>>     bool needs_flr_on_fini;
>>
>> +    /** @pci_state: PCI state of device */
>> +    struct pci_saved_state *pci_state;
>> +
>>     /* private: */
>>
>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
>> index 85408e7a932b..437874a9a5a0 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.c
>> +++ b/drivers/gpu/drm/xe/xe_gt.c
>> @@ -629,6 +629,26 @@ static int do_gt_restart(struct xe_gt *gt)
>>     return 0;
>> }
>>
>> +/* Idle the GT */
>> +int xe_idle_gt(struct xe_gt *gt)
>
> any non-static function should use xe_gt_ prefix.
Ok will change it.
>
>> +{
>> +    int err;
>> +
>> +    xe_gt_sanitize(gt);
>> +
>> +    xe_uc_gucrc_disable(&gt->uc);
>> +    xe_uc_stop_prepare(&gt->uc);
>> +    xe_gt_pagefault_reset(gt);
>> +
>> +    err = xe_uc_stop(&gt->uc);
>> +    if (err)
>> +        return err;
>> +
>> +    xe_gt_tlb_invalidation_reset(gt);
>> +
>> +    return err;
>> +}
>> +
>> static int gt_reset(struct xe_gt *gt)
>> {
>>     int err;
>> @@ -645,21 +665,12 @@ static int gt_reset(struct xe_gt *gt)
>>     }
>>
>>     xe_pm_runtime_get(gt_to_xe(gt));
>> -    xe_gt_sanitize(gt);
>>
>>     err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>     if (err)
>>         goto err_msg;
>>
>> -    xe_uc_gucrc_disable(&gt->uc);
>> -    xe_uc_stop_prepare(&gt->uc);
>> -    xe_gt_pagefault_reset(gt);
>> -
>> -    err = xe_uc_stop(&gt->uc);
>> -    if (err)
>> -        goto err_out;
>> -
>> -    xe_gt_tlb_invalidation_reset(gt);
>> +    xe_idle_gt(gt);
>
> this and the above should be in a commit alone
> "drm/xe: Extract xe_gt_idle() helper" with explanation it will be used
> in other places outside of gt_reset path.
sure will separate it out.
>
> but I'm a little bit confused here... why do you need to remove
> xe_gt_sanitize() inside the function to make gt idle?
if I understood right this controls how the invalidation is 

sent via guc or mmio. hence moved into this function as we disable guc.

>
> Lucas De Marchi
>
>>
>>     err = do_gt_reset(gt);
>>     if (err)
>> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
>> index ed6ea8057e35..77df919199cc 100644
>> --- a/drivers/gpu/drm/xe/xe_gt.h
>> +++ b/drivers/gpu/drm/xe/xe_gt.h
>> @@ -43,6 +43,7 @@ int xe_gt_resume(struct xe_gt *gt);
>> void xe_gt_reset_async(struct xe_gt *gt);
>> void xe_gt_sanitize(struct xe_gt *gt);
>> void xe_gt_remove(struct xe_gt *gt);
>> +int xe_idle_gt(struct xe_gt *gt);
>>
>> /**
>>  * xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index c401d4890386..fcd2a7f66f7b 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -383,6 +383,41 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>>
>> #undef INTEL_VGA_DEVICE
>>
>> +static bool xe_save_pci_state(struct pci_dev *pdev)
>> +{
>> +    struct xe_device *xe = pci_get_drvdata(pdev);
>> +
>> +    if (pci_save_state(pdev))
>> +        return false;
>> +
>> +    kfree(xe->pci_state);
>> +
>> +    xe->pci_state = pci_store_saved_state(pdev);
>> +
>> +    if (!xe->pci_state) {
>> +        drm_err(&xe->drm, "Failed to store PCI saved state\n");
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +void xe_load_pci_state(struct pci_dev *pdev)
>> +{
>> +    struct xe_device *xe = pci_get_drvdata(pdev);
>> +    int ret;
>> +
>> +    if (!xe->pci_state)
>> +        return;
>> +
>> +    ret = pci_load_saved_state(pdev, xe->pci_state);
>> +    if (!ret) {
>> +        pci_restore_state(pdev);
>> +    } else {
>> +        drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
>> +    }
>
> please check coding style here and invert condition so you keep the
> return-early pattern you started in the xe->pci_state check.
>
>
> ret = pci_load_saved_state(pdev, xe->pci_state);
> if (ret) {
>     drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
>     return;
> }
Ok will correct it.
>
>> +}
>> +
>> /* is device_id present in comma separated list of ids */
>> static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
>> {
>> @@ -688,10 +723,12 @@ static void xe_pci_remove(struct pci_dev *pdev)
>>
>>     xe_device_remove(xe);
>>     xe_pm_runtime_fini(xe);
>> +
>> +    kfree(xe->pci_state);
>>     pci_set_drvdata(pdev, NULL);
>> }
>>
>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> {
>>     const struct xe_device_desc *desc = (const void *)ent->driver_data;
>>     const struct xe_subplatform_desc *subplatform_desc;
>> @@ -786,6 +823,9 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>     drm_dbg(&xe->drm, "d3cold: capable=%s\n",
>>         str_yes_no(xe->d3cold.capable));
>>
>> +    if (xe_save_pci_state(pdev))
>> +        pci_restore_state(pdev);
>
> why do we save and restore in sequence when probing? If we only have to
> have a saved stated for possible errors later, why do we restore on
> success? /me confused

this is needed to avoid breaking the PCI core functionality of suspend/resume

to unset dev->state_saved which is checked in suspend

Documentation/power/pci.rst:

"Moreover, if the driver calls pci_save_state(),
the PCI subsystem will not execute either pci_prepare_to_sleep(), or
pci_set_power_state() for its device, so the driver is then responsible for
handling the device as appropriate."

>
>> +
>>     return 0;
>> }
>>
>> @@ -833,7 +873,7 @@ static int xe_pci_suspend(struct device *dev)
>>      */
>>     d3cold_toggle(pdev, D3COLD_ENABLE);
>>
>> -    pci_save_state(pdev);
>> +    xe_save_pci_state(pdev);
>
> all these no change in behavior with xe_ functions wrapping the the pci
> ones could be delegated to a prep patch so it's easier to separate the
> noise from the actual changes.
Ok.
>
>>     pci_disable_device(pdev);
>>
>>     return 0;
>> @@ -857,6 +897,8 @@ static int xe_pci_resume(struct device *dev)
>>
>>     pci_set_master(pdev);
>>
>> +    xe_load_pci_state(pdev);
>> +
>>     err = xe_pm_resume(pdev_to_xe_device(pdev));
>>     if (err)
>>         return err;
>> @@ -874,7 +916,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
>>     if (err)
>>         return err;
>>
>> -    pci_save_state(pdev);
>> +    xe_save_pci_state(pdev);
>>
>>     if (xe->d3cold.allowed) {
>>         d3cold_toggle(pdev, D3COLD_ENABLE);
>> @@ -899,7 +941,7 @@ static int xe_pci_runtime_resume(struct device *dev)
>>     if (err)
>>         return err;
>>
>> -    pci_restore_state(pdev);
>> +    xe_load_pci_state(pdev);
>>
>>     if (xe->d3cold.allowed) {
>>         err = pci_enable_device(pdev);
>> @@ -928,6 +970,8 @@ static const struct dev_pm_ops xe_pm_ops = {
>> };
>> #endif
>>
>> +extern const struct pci_error_handlers xe_pci_err_handlers;
>
> not sure this really belongs into a separate compilation unit. Just
> leave it in xe_pci.c? Adding the extern here doesn't make it pretty.
>
I wanted to keep it separate so that it will be clean, will do as suggested below.

Thanks,
Aravind.
> if we are adding a separate .c, then the struct definition should
> probably be here, and you do it like
>
>     static const struct pci_error_handlers xe_pci_err_handlers = {
>         .reset_prepare = xe_pci_err_reset_prepare,
>         .reset_done = xe_pci_err_reset_done,
>     };
>
>
> See xe_pm_ops, that was left here rather than in xe_pm.c.
>
> Lucas De Marchi
>
>> +
>> static struct pci_driver xe_pci_driver = {
>>     .name = DRIVER_NAME,
>>     .id_table = pciidlist,
>> @@ -937,6 +981,7 @@ static struct pci_driver xe_pci_driver = {
>> #ifdef CONFIG_PM_SLEEP
>>     .driver.pm = &xe_pm_ops,
>> #endif
>> +    .err_handler = &xe_pci_err_handlers,
>> };
>>
>> int xe_register_pci_driver(void)
>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>> index 611c1209b14c..9faf5380a09e 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.h
>> +++ b/drivers/gpu/drm/xe/xe_pci.h
>> @@ -6,7 +6,11 @@
>> #ifndef _XE_PCI_H_
>> #define _XE_PCI_H_
>>
>> +struct pci_dev;
>> +struct pci_device_id;
>> +
>> int xe_register_pci_driver(void);
>> void xe_unregister_pci_driver(void);
>> -
>> +void xe_load_pci_state(struct pci_dev *pdev);
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>> #endif
>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>> new file mode 100644
>> index 000000000000..ecc467a9c3af
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
>> @@ -0,0 +1,94 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <drm/drm_drv.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_printk.h"
>> +#include "xe_pci.h"
>> +#include "xe_pm.h"
>> +
>> +/**
>> + * xe_pci_reset_prepare - Called when user issued a function level reset
>> + * via /sys/bus/pci/devices/.../reset.
>> + * @pdev: PCI device struct
>> + */
>> +static void xe_pci_reset_prepare(struct pci_dev *pdev)
>> +{
>> +    struct xe_device *xe = pci_get_drvdata(pdev);
>> +    struct xe_gt *gt;
>> +    int id, err;
>> +
>> +    pci_warn(pdev, "preparing for PCIe FLR reset\n");
>> +
>> +    drm_warn(&xe->drm, "removing device access to userspace\n");
>> +    drm_dev_unplug(&xe->drm);
>> +
>> +    xe_pm_runtime_get(xe);
>> +    /* idle the GTs */
>> +    for_each_gt(gt, xe, id) {
>> +        err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +        if (err)
>> +            goto reset;
>> +        err = xe_idle_gt(gt);
>> +        if (err) {
>> +            xe_gt_err(gt, "failed to idle gt (%pe)\n", ERR_PTR(err));
>> +            goto reset;
>> +        }
>> +
>> +        err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> +        XE_WARN_ON(err);
>> +    }
>> +    xe_pm_runtime_put(xe);
>> +
>> +reset:
>> +    pci_disable_device(pdev);
>> +}
>> +
>> +/**
>> + * xe_pci_reset_done - Called when function level reset is done.
>> + * @pdev: PCI device struct
>> + */
>> +static void xe_pci_reset_done(struct pci_dev *pdev)
>> +{
>> +    const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>> +    struct xe_device *xe = pci_get_drvdata(pdev);
>> +
>> +    dev_info(&pdev->dev,
>> +         "PCI device went through FLR, reenabling the device\n");
>> +
>> +    if (pci_enable_device(pdev)) {
>> +        dev_err(&pdev->dev,
>> +            "Cannot re-enable PCI device after reset\n");
>> +        return;
>> +    }
>> +    pci_set_master(pdev);
>> +    xe_load_pci_state(pdev);
>> +
>> +    /*
>> +     * We want to completely clean the driver and even destroy
>> +     * the xe private data and reinitialize afresh similar to
>> +     * probe
>> +     */
>> +    pdev->driver->remove(pdev);
>> +    if (pci_dev_msi_enabled(pdev))
>> +        pci_free_irq_vectors(pdev);
>> +
>> +    devm_drm_release_action(&xe->drm);
>> +    pci_disable_device(pdev);
>> +
>> +    /*
>> +     * if this fails the driver might be in a stale state, only option is
>> +     * to unbind and rebind
>> +     */
>> +    xe_pci_probe(pdev, ent);
>> +}
>> +
>> +const struct pci_error_handlers xe_pci_err_handlers = {
>> +    .reset_prepare = xe_pci_reset_prepare,
>> +    .reset_done = xe_pci_reset_done,
>> +};
>> -- 
>> 2.25.1
>>

Re: [RFC 0/2] drm/xe: Support PCIe FLR

[Lucas De Marchi]

On Wed, Mar 20, 2024 at 04:14:24PM +0530, Aravind Iddamsetty wrote:
>PCI subsystem provides callbacks to inform the driver about a request to
>do function level reset by user, initiated by writing to sysfs entry
>/sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>without the need to do unbind and rebind as the driver needs to
>reinitialize the device afresh post FLR.
>
>Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>
>we get the below splat but the device has recovered, have executed an
>IGT post FLR.

ok, but that also means we we will need a few commits between patch 1
and 2 to get it in an acceptable state.

Lucas De Marchi