[PATCH v2] drm/atomic-helpers: Invoke end_fb_access while owning plane state

[PATCH v2] drm/atomic-helpers: Invoke end_fb_access while owning plane state

[Thomas Zimmermann]

Invoke drm_plane_helper_funcs.end_fb_access before
drm_atomic_helper_commit_hw_done(). The latter function hands over
ownership of the plane state to the following commit, which might
free it. Releasing resources in end_fb_access then operates on undefined
state. This bug has been observed with non-blocking commits when they
are being queued up quickly.

Here is an example stack trace from the bug report. The plane state has
been free'd already, so the pages for drm_gem_fb_vunmap() are gone.

Unable to handle kernel paging request at virtual address 0000000100000049
[...]
 drm_gem_fb_vunmap+0x18/0x74
 drm_gem_end_shadow_fb_access+0x1c/0x2c
 drm_atomic_helper_cleanup_planes+0x58/0xd8
 drm_atomic_helper_commit_tail+0x90/0xa0
 commit_tail+0x15c/0x188
 commit_work+0x14/0x20

For aborted commits, it is still ok to run end_fb_access as part of the
plane's cleanup. Add a test to drm_atomic_helper_cleanup_planes().

v2:
	* fix test in drm_atomic_helper_cleanup_planes()

Reported-by: Alyssa Ross <hi@alyssa.is>
Closes: https://lore.kernel.org/dri-devel/87leazm0ya.fsf@alyssa.is/
Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Fixes: 94d879eaf7fb ("drm/atomic-helper: Add {begin,end}_fb_access to plane helpers")
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Cc: <stable@vger.kernel.org> # v6.2+
---
 drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index c3f677130def0..bedb42ddd1341 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2784,6 +2784,17 @@ void drm_atomic_helper_commit_planes(struct drm_device *dev,
 
 		funcs->atomic_flush(crtc, old_state);
 	}
+
+	/*
+	 * Signal end of framebuffer access here before hw_done. After hw_done,
+	 * a later commit might have already released the plane state.
+	 */
+	for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) {
+		const struct drm_plane_helper_funcs *funcs = plane->helper_private;
+
+		if (funcs->end_fb_access)
+			funcs->end_fb_access(plane, new_plane_state);
+	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_planes);
 
@@ -2924,6 +2935,12 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 	for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs = plane->helper_private;
 
+		/*
+		 * Only clean up here if we're aborting the commit.
+		 */
+		if (old_plane_state == plane->state)
+			continue;
+
 		if (funcs->end_fb_access)
 			funcs->end_fb_access(plane, new_plane_state);
 	}
-- 
2.43.0

Re: [PATCH v2] drm/atomic-helpers: Invoke end_fb_access while owning plane state

[Alyssa Ross]

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

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Invoke drm_plane_helper_funcs.end_fb_access before
> drm_atomic_helper_commit_hw_done(). The latter function hands over
> ownership of the plane state to the following commit, which might
> free it. Releasing resources in end_fb_access then operates on undefined
> state. This bug has been observed with non-blocking commits when they
> are being queued up quickly.
>
> Here is an example stack trace from the bug report. The plane state has
> been free'd already, so the pages for drm_gem_fb_vunmap() are gone.
>
> Unable to handle kernel paging request at virtual address 0000000100000049
> [...]
>  drm_gem_fb_vunmap+0x18/0x74
>  drm_gem_end_shadow_fb_access+0x1c/0x2c
>  drm_atomic_helper_cleanup_planes+0x58/0xd8
>  drm_atomic_helper_commit_tail+0x90/0xa0
>  commit_tail+0x15c/0x188
>  commit_work+0x14/0x20
>
> For aborted commits, it is still ok to run end_fb_access as part of the
> plane's cleanup. Add a test to drm_atomic_helper_cleanup_planes().
>
> v2:
> 	* fix test in drm_atomic_helper_cleanup_planes()
>
> Reported-by: Alyssa Ross <hi@alyssa.is>
> Closes: https://lore.kernel.org/dri-devel/87leazm0ya.fsf@alyssa.is/
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Fixes: 94d879eaf7fb ("drm/atomic-helper: Add {begin,end}_fb_access to plane helpers")
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: <stable@vger.kernel.org> # v6.2+
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)

Got this basically immediately. :(

simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 000000004935bdca state to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000d25f613d state to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:38] for [PLANE:31:plane-0] state 000000004935bdca
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000020d19f10 state to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_nonblocking_commit] committing 00000000cfb3f1f2 nonblocking
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 00000000cfb3f1f2
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 0000000083f22dc6 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000eec339c5 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:37] for [PLANE:31:plane-0] state 0000000083f22dc6
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000022495ce9 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 0000000083f22dc6 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000eec339c5 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:37] for [PLANE:31:plane-0] state 0000000083f22dc6
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000022495ce9 state to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_nonblocking_commit] committing 0000000003dc0c0b nonblocking
simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 000000000a78a23c
simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 000000000a78a23c
Unable to handle kernel paging request at virtual address ffff80009033c000
Mem abort info:
  ESR = 0x0000000096000007
  EC = 0x25: DABT (current EL), IL = 32 bits
  SET = 0, FnV = 0
  EA = 0, S1PTW = 0
  FSC = 0x07: level 3 translation fault
Data abort info:
  ISV = 0, ISS = 0x00000007, ISS2 = 0x00000000
  CM = 0, WnR = 0, TnD = 0, TagAccess = 0
  GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
swapper pgtable: 16k pages, 48-bit VAs, pgdp=0000000dc5c44000
[ffff80009033c000] pgd=1000000dce9a0003, p4d=1000000dce9a0003, pud=1000000dce99c003, pmd=10000008105c8003, pte=0000000000000000
Internal error: Oops: 0000000096000007 [#1] PREEMPT SMP
Modules linked in: rfcomm snd_seq_dummy snd_hrtimer snd_seq snd_seq_device bnep des_generic libdes md4 brcmfmac_wcc joydev brcmfmac hci_bcm4377 brcmutil bluetooth ecdh_generic hid_magicmouse cfg80211 ecc rfkill snd_soc_macaudio macsmc_power macsmc_reboot macsmc_hid xt_conntrack apple_isp videobuf2_dma_sg videobuf2_memops videobuf2_v4l2 nf_conntrack snd_soc_cs42l84 nf_defrag_ipv6 videodev nf_defrag_ipv4 videobuf2_common clk_apple_nco ofpart snd_soc_tas2764 spi_nor snd_soc_apple_mca mc apple_admac pwm_apple apple_soc_cpufreq leds_pwm ip6t_rpfilter hid_apple ipt_rpfilter xt_pkttype xt_LOG nf_log_syslog nft_compat nf_tables nfnetlink loop tun tap macvlan bridge stp llc fuse zstd zram dm_crypt xhci_plat_hcd xhci_hcd nvmem_spmi_mfd rtc_macsmc gpio_macsmc pcie_apple simple_mfd_spmi tps6598x dockchannel_hid regmap_spmi dwc3 phy_apple_atc pci_host_common udc_core typec nvme_apple macsmc_rtkit apple_sart apple_rtkit_helper apple_dockchannel macsmc apple_rtkit mfd_core spmi_apple_controller nvmem_apple_efuses
 pinctrl_apple_gpio spi_apple i2c_apple apple_dart apple_mailbox btrfs xor xor_neon raid6_pq
CPU: 2 PID: 507 Comm: kworker/u16:10 Tainted: G S                 6.5.0-asahi #1-NixOS
Hardware name: Apple MacBook Pro (13-inch, M2, 2022) (DT)
Workqueue: events_unbound commit_work
pstate: 21400009 (nzCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
pc : __memcpy+0x15c/0x230
lr : __drm_fb_xfrm_toio.isra.0+0xcc/0x15c
sp : ffff800082bf3b90
x29: ffff800082bf3b90 x28: ffff8000807773f4 x27: ffff80009033a800
x26: 0000000000000012 x25: 0000000000000a00 x24: 0000000000002800
x23: ffff000035604700 x22: 0000000000000640 x21: ffff000128070000
x20: ffff000128072800 x19: ffff80008402a800 x18: ffffffffffffffff
x17: 746174735f63696d x16: 6f74615f6d72645f x15: ff090f19ff090f19
x14: 0000000000000000 x13: ff0a1320ff0a1320 x12: ff0a1320ff0b1321
x11: ff0a1320ff0b1321 x10: ff0b1321ff0b1321 x9 : ff0b1321ff0a1320
x8 : ff0a1320ff0a1320 x7 : ff0a1320ff0a1320 x6 : ff0a1320ff0a1320
x5 : ffff000128075000 x4 : ffff80009033d000 x3 : ffff000128073fc0
x2 : 0000000000000ff0 x1 : ffff80009033bfc0 x0 : ffff000128072800
Call trace:
 __memcpy+0x15c/0x230
 drm_fb_xfrm.isra.0+0x44/0x60
 drm_fb_blit+0x234/0x2ec
 simpledrm_primary_plane_helper_atomic_update+0x12c/0x164
 drm_atomic_helper_commit_planes+0xe4/0x2d0
 drm_atomic_helper_commit_tail+0x54/0xa0
 commit_tail+0x15c/0x188
 commit_work+0x14/0x20
 process_one_work+0x1e0/0x344
 worker_thread+0x68/0x424
 kthread+0xf4/0x100
 ret_from_fork+0x10/0x20
Code: a9422428 a9032c6a a9432c2a a984346c (a9c4342c) 
---[ end trace 0000000000000000 ]---

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2] drm/atomic-helpers: Invoke end_fb_access while owning plane state

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 10491 bytes --]

Hi

Am 27.11.23 um 17:25 schrieb Alyssa Ross:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Invoke drm_plane_helper_funcs.end_fb_access before
>> drm_atomic_helper_commit_hw_done(). The latter function hands over
>> ownership of the plane state to the following commit, which might
>> free it. Releasing resources in end_fb_access then operates on undefined
>> state. This bug has been observed with non-blocking commits when they
>> are being queued up quickly.
>>
>> Here is an example stack trace from the bug report. The plane state has
>> been free'd already, so the pages for drm_gem_fb_vunmap() are gone.
>>
>> Unable to handle kernel paging request at virtual address 0000000100000049
>> [...]
>>   drm_gem_fb_vunmap+0x18/0x74
>>   drm_gem_end_shadow_fb_access+0x1c/0x2c
>>   drm_atomic_helper_cleanup_planes+0x58/0xd8
>>   drm_atomic_helper_commit_tail+0x90/0xa0
>>   commit_tail+0x15c/0x188
>>   commit_work+0x14/0x20
>>
>> For aborted commits, it is still ok to run end_fb_access as part of the
>> plane's cleanup. Add a test to drm_atomic_helper_cleanup_planes().
>>
>> v2:
>> 	* fix test in drm_atomic_helper_cleanup_planes()
>>
>> Reported-by: Alyssa Ross <hi@alyssa.is>
>> Closes: https://lore.kernel.org/dri-devel/87leazm0ya.fsf@alyssa.is/
>> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
>> Fixes: 94d879eaf7fb ("drm/atomic-helper: Add {begin,end}_fb_access to plane helpers")
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: <stable@vger.kernel.org> # v6.2+
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
> 
> Got this basically immediately. :(

I've never seen such problems on other systems. Is there anything 
different about the Mac systems? How do you trigger these errors?

Best regards
Thomas

> 
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 000000004935bdca state to 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000d25f613d state to 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:38] for [PLANE:31:plane-0] state 000000004935bdca
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000020d19f10 state to 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
> simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_nonblocking_commit] committing 00000000cfb3f1f2 nonblocking
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 00000000cfb3f1f2
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 0000000083f22dc6 state to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000eec339c5 state to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:37] for [PLANE:31:plane-0] state 0000000083f22dc6
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000022495ce9 state to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
> simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_init] Allocated atomic state 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_plane_state] Added [PLANE:31:plane-0] 0000000083f22dc6 state to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_crtc_state] Added [CRTC:33:crtc-0] 00000000eec339c5 state to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_set_fb_for_plane] Set [FB:37] for [PLANE:31:plane-0] state 0000000083f22dc6
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_get_connector_state] Added [CONNECTOR:35:Unknown-1] 0000000022495ce9 state to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_check_only] checking 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] Updating routing for [CONNECTOR:35:Unknown-1]
> simple-framebuffer dd53a4000.framebuffer: [drm:update_connector_routing] [CONNECTOR:35:Unknown-1] keeps [ENCODER:34:None-34], now on [CRTC:33:crtc-0]
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_add_encoder_bridges] Adding all bridges for [encoder:34:None-34] to 0000000003dc0c0b
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_nonblocking_commit] committing 0000000003dc0c0b nonblocking
> simple-framebuffer dd53a4000.framebuffer: [drm:drm_atomic_state_default_clear] Clearing atomic state 000000000a78a23c
> simple-framebuffer dd53a4000.framebuffer: [drm:__drm_atomic_state_free] Freeing atomic state 000000000a78a23c
> Unable to handle kernel paging request at virtual address ffff80009033c000
> Mem abort info:
>    ESR = 0x0000000096000007
>    EC = 0x25: DABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>    FSC = 0x07: level 3 translation fault
> Data abort info:
>    ISV = 0, ISS = 0x00000007, ISS2 = 0x00000000
>    CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>    GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> swapper pgtable: 16k pages, 48-bit VAs, pgdp=0000000dc5c44000
> [ffff80009033c000] pgd=1000000dce9a0003, p4d=1000000dce9a0003, pud=1000000dce99c003, pmd=10000008105c8003, pte=0000000000000000
> Internal error: Oops: 0000000096000007 [#1] PREEMPT SMP
> Modules linked in: rfcomm snd_seq_dummy snd_hrtimer snd_seq snd_seq_device bnep des_generic libdes md4 brcmfmac_wcc joydev brcmfmac hci_bcm4377 brcmutil bluetooth ecdh_generic hid_magicmouse cfg80211 ecc rfkill snd_soc_macaudio macsmc_power macsmc_reboot macsmc_hid xt_conntrack apple_isp videobuf2_dma_sg videobuf2_memops videobuf2_v4l2 nf_conntrack snd_soc_cs42l84 nf_defrag_ipv6 videodev nf_defrag_ipv4 videobuf2_common clk_apple_nco ofpart snd_soc_tas2764 spi_nor snd_soc_apple_mca mc apple_admac pwm_apple apple_soc_cpufreq leds_pwm ip6t_rpfilter hid_apple ipt_rpfilter xt_pkttype xt_LOG nf_log_syslog nft_compat nf_tables nfnetlink loop tun tap macvlan bridge stp llc fuse zstd zram dm_crypt xhci_plat_hcd xhci_hcd nvmem_spmi_mfd rtc_macsmc gpio_macsmc pcie_apple simple_mfd_spmi tps6598x dockchannel_hid regmap_spmi dwc3 phy_apple_atc pci_host_common udc_core typec nvme_apple macsmc_rtkit apple_sart apple_rtkit_helper apple_dockchannel macsmc apple_rtkit mfd_core spmi_apple_controller nvmem_apple_efuses
>   pinctrl_apple_gpio spi_apple i2c_apple apple_dart apple_mailbox btrfs xor xor_neon raid6_pq
> CPU: 2 PID: 507 Comm: kworker/u16:10 Tainted: G S                 6.5.0-asahi #1-NixOS
> Hardware name: Apple MacBook Pro (13-inch, M2, 2022) (DT)
> Workqueue: events_unbound commit_work
> pstate: 21400009 (nzCv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> pc : __memcpy+0x15c/0x230
> lr : __drm_fb_xfrm_toio.isra.0+0xcc/0x15c
> sp : ffff800082bf3b90
> x29: ffff800082bf3b90 x28: ffff8000807773f4 x27: ffff80009033a800
> x26: 0000000000000012 x25: 0000000000000a00 x24: 0000000000002800
> x23: ffff000035604700 x22: 0000000000000640 x21: ffff000128070000
> x20: ffff000128072800 x19: ffff80008402a800 x18: ffffffffffffffff
> x17: 746174735f63696d x16: 6f74615f6d72645f x15: ff090f19ff090f19
> x14: 0000000000000000 x13: ff0a1320ff0a1320 x12: ff0a1320ff0b1321
> x11: ff0a1320ff0b1321 x10: ff0b1321ff0b1321 x9 : ff0b1321ff0a1320
> x8 : ff0a1320ff0a1320 x7 : ff0a1320ff0a1320 x6 : ff0a1320ff0a1320
> x5 : ffff000128075000 x4 : ffff80009033d000 x3 : ffff000128073fc0
> x2 : 0000000000000ff0 x1 : ffff80009033bfc0 x0 : ffff000128072800
> Call trace:
>   __memcpy+0x15c/0x230
>   drm_fb_xfrm.isra.0+0x44/0x60
>   drm_fb_blit+0x234/0x2ec
>   simpledrm_primary_plane_helper_atomic_update+0x12c/0x164
>   drm_atomic_helper_commit_planes+0xe4/0x2d0
>   drm_atomic_helper_commit_tail+0x54/0xa0
>   commit_tail+0x15c/0x188
>   commit_work+0x14/0x20
>   process_one_work+0x1e0/0x344
>   worker_thread+0x68/0x424
>   kthread+0xf4/0x100
>   ret_from_fork+0x10/0x20
> Code: a9422428 a9032c6a a9432c2a a984346c (a9c4342c)
> ---[ end trace 0000000000000000 ]---

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2] drm/atomic-helpers: Invoke end_fb_access while owning plane state

[Alyssa Ross]

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

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 27.11.23 um 17:25 schrieb Alyssa Ross:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>>> Invoke drm_plane_helper_funcs.end_fb_access before
>>> drm_atomic_helper_commit_hw_done(). The latter function hands over
>>> ownership of the plane state to the following commit, which might
>>> free it. Releasing resources in end_fb_access then operates on undefined
>>> state. This bug has been observed with non-blocking commits when they
>>> are being queued up quickly.
>>>
>>> Here is an example stack trace from the bug report. The plane state has
>>> been free'd already, so the pages for drm_gem_fb_vunmap() are gone.
>>>
>>> Unable to handle kernel paging request at virtual address 0000000100000049
>>> [...]
>>>   drm_gem_fb_vunmap+0x18/0x74
>>>   drm_gem_end_shadow_fb_access+0x1c/0x2c
>>>   drm_atomic_helper_cleanup_planes+0x58/0xd8
>>>   drm_atomic_helper_commit_tail+0x90/0xa0
>>>   commit_tail+0x15c/0x188
>>>   commit_work+0x14/0x20
>>>
>>> For aborted commits, it is still ok to run end_fb_access as part of the
>>> plane's cleanup. Add a test to drm_atomic_helper_cleanup_planes().
>>>
>>> v2:
>>> 	* fix test in drm_atomic_helper_cleanup_planes()
>>>
>>> Reported-by: Alyssa Ross <hi@alyssa.is>
>>> Closes: https://lore.kernel.org/dri-devel/87leazm0ya.fsf@alyssa.is/
>>> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
>>> Fixes: 94d879eaf7fb ("drm/atomic-helper: Add {begin,end}_fb_access to plane helpers")
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: <stable@vger.kernel.org> # v6.2+
>>> ---
>>>   drivers/gpu/drm/drm_atomic_helper.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>> 
>> Got this basically immediately. :(
>
> I've never seen such problems on other systems. Is there anything 
> different about the Mac systems? How do you trigger these errors?

My understanding is that all sorts of things are different, but I don't
know too much about the details.  There's of course a chance that there
could be some other change in the Asahi Linux kernel that causes this
problem to surface — as I said, I reviewed the diff with mainline and
didn't see anything that looked relevant, but I could well have missed
something.  I don't think I can test mainline directly, as it doesn't
yet support enough of the hardware — for slightly older Apple Silicon
Mac models, I think enough is upstream that this would be possible, but
I don't have access to any.

I started off encountering these errors every few days.  I noticed them
because they would sometimes result in my system either starting to
freeze for 10 seconds at a time, or until I switched VT.  They seem to
correlate with the system being under high CPU load.  I was also able to
substantially increase the frequency with which they occurred by adding
logging to the kernel — even just drm.debug=0x10 makes a big difference,
and when I also added a few dump_backtrace() calls when I was trying to
understand the code and diagnose the problem, I would relatively
consistently encounter an Oops within a few minutes of load.

BTW: v3 is looking good so far.  I've only been testing it since this
morning, though, so I'll keep trying it out for a bit longer before I
declare the problem to have been solved and send a Tested-by.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]