Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

12 participants
3020 discussions

Re: Warnings in next-20250703 caused by commit 582111e630f5

by Thomas Zimmermann

Hi Am 03.07.25 um 19:23 schrieb Bert Karwatzki: > Am Donnerstag, dem 03.07.2025 um 18:09 +0200 schrieb Thomas Zimmermann: >> Hi, >> >> before I give up on the issue, could you please test the attached patch? >> >> Best regards >> Thomas >> >> >> -- >> 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) > I applied the patch on top of next-20250703 > > $ git log --oneline > 18ee3ed3cb60 (HEAD -> drm_gem_object_handle_put) drm/amdgpu: Provide custom framebuffer destroy function > 8d6c58332c7a (tag: next-20250703, origin/master, origin/HEAD, master) Add linux-next specific files for 20250703 > > and it solves the issue for me (i.e. no warnings). Great, thanks for testing. If nothing else, that's the minimal workaround. Here's another patch, which should solve the problem for all drivers. Could you please revert the old fix and apply the new one and test again? Best regards Thomas > > Bert Karwatzki -- -- 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)

2 months, 1 week

Re: Warnings in next-20250703 caused by commit 582111e630f5

by Thomas Zimmermann

Hi, before I give up on the issue, could you please test the attached patch? Best regards Thomas Am 03.07.25 um 13:59 schrieb Bert Karwatzki: > When booting next-20250703 on my Msi Alpha 15 Laptop running debian sid (last > updated 20250703) I get a several warnings of the following kind: > > [ 8.702999] [ T1628] ------------[ cut here ]------------ > [ 8.703001] [ T1628] WARNING: drivers/gpu/drm/drm_gem.c:287 at drm_gem_object_handle_put_unlocked+0xaa/0xe0, CPU#14: Xorg/1628 > [ 8.703007] [ T1628] Modules linked in: snd_seq_dummy snd_hrtimer snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device rfcomm bnep nls_ascii nls_cp437 vfat fat snd_ctl_led snd_hda_codec_realtek snd_hda_codec_generic snd_hda_scodec_component snd_hda_codec_hdmi snd_hda_intel btusb snd_intel_dspcfg btrtl btintel snd_hda_codec uvcvideo snd_soc_dmic snd_acp3x_pdm_dma btbcm snd_acp3x_rn btmtk snd_hwdep videobuf2_vmalloc snd_soc_core snd_hda_core videobuf2_memops snd_pcm_oss uvc videobuf2_v4l2 bluetooth snd_mixer_oss videodev snd_pcm snd_rn_pci_acp3x videobuf2_common snd_acp_config snd_timer msi_wmi ecdh_generic snd_soc_acpi ecc mc sparse_keymap snd wmi_bmof edac_mce_amd k10temp soundcore snd_pci_acp3x ccp ac battery button joydev hid_sensor_accel_3d hid_sensor_prox hid_sensor_als hid_sensor_magn_3d hid_sensor_gyro_3d hid_sensor_trigger industrialio_triggered_buffer kfifo_buf industrialio hid_sensor_iio_common amd_pmc evdev mt7921e mt7921_common mt792x_lib mt76_connac_lib mt76 mac80211 libarc4 cfg80211 rfkill msr fuse > [ 8.703056] [ T1628] nvme_fabrics efi_pstore configfs efivarfs autofs4 ext4 mbcache jbd2 usbhid amdgpu drm_client_lib i2c_algo_bit drm_ttm_helper ttm drm_panel_backlight_quirks drm_exec drm_suballoc_helper amdxcp drm_buddy xhci_pci gpu_sched xhci_hcd drm_display_helper hid_sensor_hub hid_multitouch mfd_core hid_generic drm_kms_helper psmouse i2c_hid_acpi nvme usbcore amd_sfh i2c_hid hid cec serio_raw nvme_core r8169 crc16 i2c_piix4 usb_common i2c_smbus i2c_designware_platform i2c_designware_core > [ 8.703082] [ T1628] CPU: 14 UID: 1000 PID: 1628 Comm: Xorg Not tainted 6.16.0-rc4-next-20250703-master #127 PREEMPT_{RT,(full)} > [ 8.703085] [ T1628] Hardware name: Micro-Star International Co., Ltd. Alpha 15 B5EEK/MS-158L, BIOS E158LAMS.10F 11/11/2024 > [ 8.703086] [ T1628] RIP: 0010:drm_gem_object_handle_put_unlocked+0xaa/0xe0 > [ 8.703088] [ T1628] Code: c7 f6 8a ff 48 89 ef e8 94 d4 2e 00 eb d8 48 8b 43 08 48 8d b8 d8 06 00 00 e8 52 78 2b 00 c7 83 08 01 00 00 00 00 00 00 eb 98 <0f> 0b 5b 5d e9 98 f6 8a ff 48 8b 83 68 01 00 00 48 8b 00 48 85 c0 > [ 8.703089] [ T1628] RSP: 0018:ffffb8e8c7fbfb00 EFLAGS: 00010246 > [ 8.703091] [ T1628] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000 > [ 8.703092] [ T1628] RDX: 0000000000000000 RSI: ffff94cdc062b478 RDI: ffff94ce71390448 > [ 8.703093] [ T1628] RBP: ffff94ce14780010 R08: ffff94cdc062b618 R09: ffff94ce14780278 > [ 8.703094] [ T1628] R10: 0000000000000001 R11: ffff94cdc062b478 R12: ffff94ce14780010 > [ 8.703095] [ T1628] R13: 0000000000000007 R14: 0000000000000004 R15: ffff94ce14780010 > [ 8.703096] [ T1628] FS: 00007fc164276b00(0000) GS:ffff94dcb49cf000(0000) knlGS:0000000000000000 > [ 8.703097] [ T1628] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 8.703098] [ T1628] CR2: 00005647ccd53008 CR3: 000000012533f000 CR4: 0000000000750ef0 > [ 8.703099] [ T1628] PKRU: 55555554 > [ 8.703100] [ T1628] Call Trace: > [ 8.703101] [ T1628] <TASK> > [ 8.703104] [ T1628] drm_gem_fb_destroy+0x27/0x50 [drm_kms_helper] > [ 8.703113] [ T1628] __drm_atomic_helper_plane_destroy_state+0x1a/0xa0 [drm_kms_helper] > [ 8.703119] [ T1628] drm_atomic_helper_plane_destroy_state+0x10/0x20 [drm_kms_helper] > [ 8.703124] [ T1628] drm_atomic_state_default_clear+0x1c0/0x2e0 > [ 8.703127] [ T1628] __drm_atomic_state_free+0x6c/0xb0 > [ 8.703129] [ T1628] drm_atomic_helper_disable_plane+0x92/0xe0 [drm_kms_helper] > [ 8.703135] [ T1628] drm_mode_cursor_universal+0xf2/0x2a0 > [ 8.703140] [ T1628] drm_mode_cursor_common.part.0+0x9c/0x1e0 > [ 8.703144] [ T1628] ? drm_mode_setplane+0x320/0x320 > [ 8.703146] [ T1628] drm_mode_cursor_ioctl+0x8a/0xa0 > [ 8.703148] [ T1628] drm_ioctl_kernel+0xa1/0xf0 > [ 8.703151] [ T1628] drm_ioctl+0x26a/0x510 > [ 8.703153] [ T1628] ? drm_mode_setplane+0x320/0x320 > [ 8.703155] [ T1628] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 8.703157] [ T1628] ? rt_spin_unlock+0x12/0x40 > [ 8.703159] [ T1628] ? do_setitimer+0x185/0x1d0 > [ 8.703161] [ T1628] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 8.703164] [ T1628] amdgpu_drm_ioctl+0x46/0x90 [amdgpu] > [ 8.703283] [ T1628] __x64_sys_ioctl+0x91/0xe0 > [ 8.703286] [ T1628] do_syscall_64+0x65/0xfc0 > [ 8.703289] [ T1628] entry_SYSCALL_64_after_hwframe+0x55/0x5d > [ 8.703291] [ T1628] RIP: 0033:0x7fc1645f78db > [ 8.703292] [ T1628] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00 > [ 8.703294] [ T1628] RSP: 002b:00007ffd75bce430 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > [ 8.703295] [ T1628] RAX: ffffffffffffffda RBX: 000056224e896ea0 RCX: 00007fc1645f78db > [ 8.703296] [ T1628] RDX: 00007ffd75bce4c0 RSI: 00000000c01c64a3 RDI: 000000000000000f > [ 8.703297] [ T1628] RBP: 00007ffd75bce4c0 R08: 0000000000000100 R09: 0000562210547ab0 > [ 8.703298] [ T1628] R10: 000000000000004c R11: 0000000000000246 R12: 00000000c01c64a3 > [ 8.703298] [ T1628] R13: 000000000000000f R14: 0000000000000000 R15: 000056224e5c1cd0 > [ 8.703302] [ T1628] </TASK> > [ 8.703303] [ T1628] ---[ end trace 0000000000000000 ]--- > > As the warnings do not occur in next-20250702, I looked at the commits given by > $ git log --oneline next-20250702..next-20250703 drivers/gpu/drm > to search for a culprit. So I reverted the most likely candidate, > commit 582111e630f5 ("drm/gem: Acquire references on GEM handles for framebuffers"), > in next-20250703 and the warnings disappeared. > This is the hardware I used: > $ lspci > 00:00.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne Root Complex > 00:00.2 IOMMU: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne IOMMU > 00:01.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy Host Bridge > 00:01.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe GPP Bridge > 00:02.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy Host Bridge > 00:02.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:02.2 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:02.3 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:02.4 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:08.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy Host Bridge > 00:08.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir Internal PCIe GPP Bridge to Bus > 00:14.0 SMBus: Advanced Micro Devices, Inc. [AMD] FCH SMBus Controller (rev 51) > 00:14.3 ISA bridge: Advanced Micro Devices, Inc. [AMD] FCH LPC Bridge (rev 51) > 00:18.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 0 > 00:18.1 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 1 > 00:18.2 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 2 > 00:18.3 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 3 > 00:18.4 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 4 > 00:18.5 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 5 > 00:18.6 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 6 > 00:18.7 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 7 > 01:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI] Navi 10 XL Upstream Port of PCI Express Switch (rev c3) > 02:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI] Navi 10 XL Downstream Port of PCI Express Switch > 03:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Navi 23 [Radeon RX 6600/6600 XT/6600M] (rev c3) > 03:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Navi 21/23 HDMI/DP Audio Controller > 04:00.0 Network controller: MEDIATEK Corp. MT7921K (RZ608) Wi-Fi 6E 80MHz > 05:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8211/8411 PCI Express Gigabit Ethernet Controller (rev 15) > 06:00.0 Non-Volatile memory controller: Kingston Technology Company, Inc. KC3000/FURY Renegade NVMe SSD [E18] (rev 01) > 07:00.0 Non-Volatile memory controller: Micron/Crucial Technology P1 NVMe PCIe SSD[Frampton] (rev 03) > 08:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Cezanne [Radeon Vega Series / Radeon Vega Mobile Series] (rev c5) > 08:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Renoir Radeon High Definition Audio Controller > 08:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Family 17h (Models 10h-1fh) Platform Security Processor > 08:00.3 USB controller: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne USB 3.1 > 08:00.4 USB controller: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne USB 3.1 > 08:00.5 Multimedia controller: Advanced Micro Devices, Inc. [AMD] Audio Coprocessor (rev 01) > 08:00.6 Audio device: Advanced Micro Devices, Inc. [AMD] Family 17h/19h/1ah HD Audio Controller > 08:00.7 Signal processing controller: Advanced Micro Devices, Inc. [AMD] Sensor Fusion Hub > > > Bert Karwatzki -- -- 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)

2 months, 1 week

Re: [RFC 00/12] io_uring dmabuf read/write support

by Christoph Hellwig

[Note: it would be really useful to Cc all relevant maintainers] On Fri, Jun 27, 2025 at 04:10:27PM +0100, Pavel Begunkov wrote: > This series implements it for read/write io_uring requests. The uAPI > looks similar to normal registered buffers, the user will need to > register a dmabuf in io_uring first and then use it as any other > registered buffer. On registration the user also specifies a file > to map the dmabuf for. Just commenting from the in-kernel POV here, where the interface feels wrong. You can't just expose 'the DMA device' up file operations, because there can be and often is more than one. Similarly stuffing a dma_addr_t into an iovec is rather dangerous. The model that should work much better is to have file operations to attach to / detach from a dma_buf, and then have an iter that specifies a dmabuf and offsets into. That way the code behind the file operations can forward the attachment to all the needed devices (including more/less while it remains attached to the file) and can pick the right dma address for each device. I also remember some discussion that new dma-buf importers should use the dynamic imported model for long-term imports, but as I'm everything but an expert in that area I'll let the dma-buf folks speak.

2 months, 1 week

Re: Warnings in next-20250703 caused by commit 582111e630f5

by Thomas Zimmermann

Hi Am 03.07.25 um 13:59 schrieb Bert Karwatzki: > When booting next-20250703 on my Msi Alpha 15 Laptop running debian sid (last > updated 20250703) I get a several warnings of the following kind: > > [ 8.702999] [ T1628] ------------[ cut here ]------------ > [ 8.703001] [ T1628] WARNING: drivers/gpu/drm/drm_gem.c:287 at drm_gem_object_handle_put_unlocked+0xaa/0xe0, CPU#14: Xorg/1628 Well, that didn't take long to blow up. Thanks for reporting the bug. I have an idea how to fix this, but it would likely just trigger the next issue. Christian, can we revert this patch, and also the other patches that switch from import_attach->dmabuf to ->dma_buf that cased the problem? Best regards Thomas > [ 8.703007] [ T1628] Modules linked in: snd_seq_dummy snd_hrtimer snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device rfcomm bnep nls_ascii nls_cp437 vfat fat snd_ctl_led snd_hda_codec_realtek snd_hda_codec_generic snd_hda_scodec_component snd_hda_codec_hdmi snd_hda_intel btusb snd_intel_dspcfg btrtl btintel snd_hda_codec uvcvideo snd_soc_dmic snd_acp3x_pdm_dma btbcm snd_acp3x_rn btmtk snd_hwdep videobuf2_vmalloc snd_soc_core snd_hda_core videobuf2_memops snd_pcm_oss uvc videobuf2_v4l2 bluetooth snd_mixer_oss videodev snd_pcm snd_rn_pci_acp3x videobuf2_common snd_acp_config snd_timer msi_wmi ecdh_generic snd_soc_acpi ecc mc sparse_keymap snd wmi_bmof edac_mce_amd k10temp soundcore snd_pci_acp3x ccp ac battery button joydev hid_sensor_accel_3d hid_sensor_prox hid_sensor_als hid_sensor_magn_3d hid_sensor_gyro_3d hid_sensor_trigger industrialio_triggered_buffer kfifo_buf industrialio hid_sensor_iio_common amd_pmc evdev mt7921e mt7921_common mt792x_lib mt76_connac_lib mt76 mac80211 libarc4 cfg80211 rfkill msr fuse > [ 8.703056] [ T1628] nvme_fabrics efi_pstore configfs efivarfs autofs4 ext4 mbcache jbd2 usbhid amdgpu drm_client_lib i2c_algo_bit drm_ttm_helper ttm drm_panel_backlight_quirks drm_exec drm_suballoc_helper amdxcp drm_buddy xhci_pci gpu_sched xhci_hcd drm_display_helper hid_sensor_hub hid_multitouch mfd_core hid_generic drm_kms_helper psmouse i2c_hid_acpi nvme usbcore amd_sfh i2c_hid hid cec serio_raw nvme_core r8169 crc16 i2c_piix4 usb_common i2c_smbus i2c_designware_platform i2c_designware_core > [ 8.703082] [ T1628] CPU: 14 UID: 1000 PID: 1628 Comm: Xorg Not tainted 6.16.0-rc4-next-20250703-master #127 PREEMPT_{RT,(full)} > [ 8.703085] [ T1628] Hardware name: Micro-Star International Co., Ltd. Alpha 15 B5EEK/MS-158L, BIOS E158LAMS.10F 11/11/2024 > [ 8.703086] [ T1628] RIP: 0010:drm_gem_object_handle_put_unlocked+0xaa/0xe0 > [ 8.703088] [ T1628] Code: c7 f6 8a ff 48 89 ef e8 94 d4 2e 00 eb d8 48 8b 43 08 48 8d b8 d8 06 00 00 e8 52 78 2b 00 c7 83 08 01 00 00 00 00 00 00 eb 98 <0f> 0b 5b 5d e9 98 f6 8a ff 48 8b 83 68 01 00 00 48 8b 00 48 85 c0 > [ 8.703089] [ T1628] RSP: 0018:ffffb8e8c7fbfb00 EFLAGS: 00010246 > [ 8.703091] [ T1628] RAX: 0000000000000000 RBX: 0000000000000001 RCX: 0000000000000000 > [ 8.703092] [ T1628] RDX: 0000000000000000 RSI: ffff94cdc062b478 RDI: ffff94ce71390448 > [ 8.703093] [ T1628] RBP: ffff94ce14780010 R08: ffff94cdc062b618 R09: ffff94ce14780278 > [ 8.703094] [ T1628] R10: 0000000000000001 R11: ffff94cdc062b478 R12: ffff94ce14780010 > [ 8.703095] [ T1628] R13: 0000000000000007 R14: 0000000000000004 R15: ffff94ce14780010 > [ 8.703096] [ T1628] FS: 00007fc164276b00(0000) GS:ffff94dcb49cf000(0000) knlGS:0000000000000000 > [ 8.703097] [ T1628] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 8.703098] [ T1628] CR2: 00005647ccd53008 CR3: 000000012533f000 CR4: 0000000000750ef0 > [ 8.703099] [ T1628] PKRU: 55555554 > [ 8.703100] [ T1628] Call Trace: > [ 8.703101] [ T1628] <TASK> > [ 8.703104] [ T1628] drm_gem_fb_destroy+0x27/0x50 [drm_kms_helper] > [ 8.703113] [ T1628] __drm_atomic_helper_plane_destroy_state+0x1a/0xa0 [drm_kms_helper] > [ 8.703119] [ T1628] drm_atomic_helper_plane_destroy_state+0x10/0x20 [drm_kms_helper] > [ 8.703124] [ T1628] drm_atomic_state_default_clear+0x1c0/0x2e0 > [ 8.703127] [ T1628] __drm_atomic_state_free+0x6c/0xb0 > [ 8.703129] [ T1628] drm_atomic_helper_disable_plane+0x92/0xe0 [drm_kms_helper] > [ 8.703135] [ T1628] drm_mode_cursor_universal+0xf2/0x2a0 > [ 8.703140] [ T1628] drm_mode_cursor_common.part.0+0x9c/0x1e0 > [ 8.703144] [ T1628] ? drm_mode_setplane+0x320/0x320 > [ 8.703146] [ T1628] drm_mode_cursor_ioctl+0x8a/0xa0 > [ 8.703148] [ T1628] drm_ioctl_kernel+0xa1/0xf0 > [ 8.703151] [ T1628] drm_ioctl+0x26a/0x510 > [ 8.703153] [ T1628] ? drm_mode_setplane+0x320/0x320 > [ 8.703155] [ T1628] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 8.703157] [ T1628] ? rt_spin_unlock+0x12/0x40 > [ 8.703159] [ T1628] ? do_setitimer+0x185/0x1d0 > [ 8.703161] [ T1628] ? srso_alias_return_thunk+0x5/0xfbef5 > [ 8.703164] [ T1628] amdgpu_drm_ioctl+0x46/0x90 [amdgpu] > [ 8.703283] [ T1628] __x64_sys_ioctl+0x91/0xe0 > [ 8.703286] [ T1628] do_syscall_64+0x65/0xfc0 > [ 8.703289] [ T1628] entry_SYSCALL_64_after_hwframe+0x55/0x5d > [ 8.703291] [ T1628] RIP: 0033:0x7fc1645f78db > [ 8.703292] [ T1628] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00 > [ 8.703294] [ T1628] RSP: 002b:00007ffd75bce430 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > [ 8.703295] [ T1628] RAX: ffffffffffffffda RBX: 000056224e896ea0 RCX: 00007fc1645f78db > [ 8.703296] [ T1628] RDX: 00007ffd75bce4c0 RSI: 00000000c01c64a3 RDI: 000000000000000f > [ 8.703297] [ T1628] RBP: 00007ffd75bce4c0 R08: 0000000000000100 R09: 0000562210547ab0 > [ 8.703298] [ T1628] R10: 000000000000004c R11: 0000000000000246 R12: 00000000c01c64a3 > [ 8.703298] [ T1628] R13: 000000000000000f R14: 0000000000000000 R15: 000056224e5c1cd0 > [ 8.703302] [ T1628] </TASK> > [ 8.703303] [ T1628] ---[ end trace 0000000000000000 ]--- > > As the warnings do not occur in next-20250702, I looked at the commits given by > $ git log --oneline next-20250702..next-20250703 drivers/gpu/drm > to search for a culprit. So I reverted the most likely candidate, > commit 582111e630f5 ("drm/gem: Acquire references on GEM handles for framebuffers"), > in next-20250703 and the warnings disappeared. > This is the hardware I used: > $ lspci > 00:00.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne Root Complex > 00:00.2 IOMMU: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne IOMMU > 00:01.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy Host Bridge > 00:01.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe GPP Bridge > 00:02.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy Host Bridge > 00:02.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:02.2 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:02.3 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:02.4 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne PCIe GPP Bridge > 00:08.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy Host Bridge > 00:08.1 PCI bridge: Advanced Micro Devices, Inc. [AMD] Renoir Internal PCIe GPP Bridge to Bus > 00:14.0 SMBus: Advanced Micro Devices, Inc. [AMD] FCH SMBus Controller (rev 51) > 00:14.3 ISA bridge: Advanced Micro Devices, Inc. [AMD] FCH LPC Bridge (rev 51) > 00:18.0 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 0 > 00:18.1 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 1 > 00:18.2 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 2 > 00:18.3 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 3 > 00:18.4 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 4 > 00:18.5 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 5 > 00:18.6 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 6 > 00:18.7 Host bridge: Advanced Micro Devices, Inc. [AMD] Cezanne Data Fabric; Function 7 > 01:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI] Navi 10 XL Upstream Port of PCI Express Switch (rev c3) > 02:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI] Navi 10 XL Downstream Port of PCI Express Switch > 03:00.0 Display controller: Advanced Micro Devices, Inc. [AMD/ATI] Navi 23 [Radeon RX 6600/6600 XT/6600M] (rev c3) > 03:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Navi 21/23 HDMI/DP Audio Controller > 04:00.0 Network controller: MEDIATEK Corp. MT7921K (RZ608) Wi-Fi 6E 80MHz > 05:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd. RTL8111/8168/8211/8411 PCI Express Gigabit Ethernet Controller (rev 15) > 06:00.0 Non-Volatile memory controller: Kingston Technology Company, Inc. KC3000/FURY Renegade NVMe SSD [E18] (rev 01) > 07:00.0 Non-Volatile memory controller: Micron/Crucial Technology P1 NVMe PCIe SSD[Frampton] (rev 03) > 08:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Cezanne [Radeon Vega Series / Radeon Vega Mobile Series] (rev c5) > 08:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Renoir Radeon High Definition Audio Controller > 08:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Family 17h (Models 10h-1fh) Platform Security Processor > 08:00.3 USB controller: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne USB 3.1 > 08:00.4 USB controller: Advanced Micro Devices, Inc. [AMD] Renoir/Cezanne USB 3.1 > 08:00.5 Multimedia controller: Advanced Micro Devices, Inc. [AMD] Audio Coprocessor (rev 01) > 08:00.6 Audio device: Advanced Micro Devices, Inc. [AMD] Family 17h/19h/1ah HD Audio Controller > 08:00.7 Signal processing controller: Advanced Micro Devices, Inc. [AMD] Sensor Fusion Hub > > > Bert Karwatzki -- -- 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)

2 months, 1 week

Re: [PATCH v10 5/9] tee: new ioctl to a register tee_shm from a dmabuf file descriptor

by Jens Wiklander

On Thu, Jul 3, 2025 at 9:22 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Wed, Jun 18, 2025 at 08:47:51AM +0200, Jens Wiklander wrote: > > On Tue, Jun 17, 2025 at 12:48 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Tue, Jun 10, 2025 at 03:13:49PM +0200, Jens Wiklander wrote: > > > > From: Etienne Carriere <etienne.carriere(a)foss.st.com> > > > > > > > > Add a userspace API to create a tee_shm object that refers to a dmabuf > > > > reference. > > > > > > > > Userspace registers the dmabuf file descriptor as in a tee_shm object. > > > > The registration is completed with a tee_shm returned file descriptor. > > > > > > > > Userspace is free to close the dmabuf file descriptor after it has been > > > > registered since all the resources are now held via the new tee_shm > > > > object. > > > > > > > > Closing the tee_shm file descriptor will eventually release all > > > > resources used by the tee_shm object when all references are released. > > > > > > > > The new IOCTL, TEE_IOC_SHM_REGISTER_FD, supports dmabuf references to > > > > physically contiguous memory buffers. Dmabuf references acquired from > > > > the TEE DMA-heap can be used as protected memory for Secure Video Path > > > > and such use cases. It depends on the TEE and the TEE driver if dmabuf > > > > references acquired by other means can be used. > > > > > > > > A new tee_shm flag is added to identify tee_shm objects built from a > > > > registered dmabuf, TEE_SHM_DMA_BUF. > > > > > > > > Signed-off-by: Etienne Carriere <etienne.carriere(a)foss.st.com> > > > > Signed-off-by: Olivier Masse <olivier.masse(a)nxp.com> > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > > > --- > > > > drivers/tee/tee_core.c | 63 +++++++++++++++++++++- > > > > drivers/tee/tee_private.h | 10 ++++ > > > > drivers/tee/tee_shm.c | 106 ++++++++++++++++++++++++++++++++++++-- > > > > include/linux/tee_core.h | 1 + > > > > include/linux/tee_drv.h | 10 ++++ > > > > include/uapi/linux/tee.h | 31 +++++++++++ > > > > 6 files changed, 217 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c > > > > index 5259b8223c27..0e9d9e5872a4 100644 > > > > --- a/drivers/tee/tee_core.c > > > > +++ b/drivers/tee/tee_core.c > > > > @@ -353,11 +353,49 @@ tee_ioctl_shm_register(struct tee_context *ctx, > > > > return ret; > > > > } > > > > > > > > +static int > > > > +tee_ioctl_shm_register_fd(struct tee_context *ctx, > > > > + struct tee_ioctl_shm_register_fd_data __user *udata) > > > > +{ > > > > + struct tee_ioctl_shm_register_fd_data data; > > > > + struct tee_shm *shm; > > > > + long ret; > > > > + > > > > + if (copy_from_user(&data, udata, sizeof(data))) > > > > + return -EFAULT; > > > > + > > > > + /* Currently no input flags are supported */ > > > > + if (data.flags) > > > > + return -EINVAL; > > > > + > > > > + shm = tee_shm_register_fd(ctx, data.fd); > > > > + if (IS_ERR(shm)) > > > > + return -EINVAL; > > > > + > > > > + data.id = shm->id; > > > > + data.flags = shm->flags; > > > > + data.size = shm->size; > > > > + > > > > + if (copy_to_user(udata, &data, sizeof(data))) > > > > + ret = -EFAULT; > > > > + else > > > > + ret = tee_shm_get_fd(shm); > > > > + > > > > + /* > > > > + * When user space closes the file descriptor the shared memory > > > > + * should be freed or if tee_shm_get_fd() failed then it will > > > > + * be freed immediately. > > > > + */ > > > > + tee_shm_put(shm); > > > > + return ret; > > > > +} > > > > + > > > > static int param_from_user_memref(struct tee_context *ctx, > > > > struct tee_param_memref *memref, > > > > struct tee_ioctl_param *ip) > > > > { > > > > struct tee_shm *shm; > > > > + size_t offs = 0; > > > > > > > > /* > > > > * If a NULL pointer is passed to a TA in the TEE, > > > > @@ -388,6 +426,26 @@ static int param_from_user_memref(struct tee_context *ctx, > > > > tee_shm_put(shm); > > > > return -EINVAL; > > > > } > > > > + > > > > + if (shm->flags & TEE_SHM_DMA_BUF) { > > > > + struct tee_shm_dmabuf_ref *ref; > > > > + > > > > + ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > > > + if (ref->parent_shm) { > > > > + /* > > > > + * The shm already has one reference to > > > > + * ref->parent_shm so we are clear of 0. > > > > + * We're getting another reference since > > > > + * this shm will be used in the parameter > > > > + * list instead of the shm we got with > > > > + * tee_shm_get_from_id() above. > > > > + */ > > > > + refcount_inc(&ref->parent_shm->refcount); > > > > + tee_shm_put(shm); > > > > + shm = ref->parent_shm; > > > > + offs = ref->offset; > > > > + } > > > > + } > > > > } else if (ctx->cap_memref_null) { > > > > /* Pass NULL pointer to OP-TEE */ > > > > shm = NULL; > > > > @@ -395,7 +453,7 @@ static int param_from_user_memref(struct tee_context *ctx, > > > > return -EINVAL; > > > > } > > > > > > > > - memref->shm_offs = ip->a; > > > > + memref->shm_offs = ip->a + offs; > > > > memref->size = ip->b; > > > > memref->shm = shm; > > > > > > > > @@ -841,6 +899,8 @@ static long tee_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > > return tee_ioctl_shm_alloc(ctx, uarg); > > > > case TEE_IOC_SHM_REGISTER: > > > > return tee_ioctl_shm_register(ctx, uarg); > > > > + case TEE_IOC_SHM_REGISTER_FD: > > > > + return tee_ioctl_shm_register_fd(ctx, uarg); > > > > case TEE_IOC_OPEN_SESSION: > > > > return tee_ioctl_open_session(ctx, uarg); > > > > case TEE_IOC_INVOKE: > > > > @@ -1300,3 +1360,4 @@ MODULE_AUTHOR("Linaro"); > > > > MODULE_DESCRIPTION("TEE Driver"); > > > > MODULE_VERSION("1.0"); > > > > MODULE_LICENSE("GPL v2"); > > > > +MODULE_IMPORT_NS("DMA_BUF"); > > > > diff --git a/drivers/tee/tee_private.h b/drivers/tee/tee_private.h > > > > index 6c6ff5d5eed2..308467705da6 100644 > > > > --- a/drivers/tee/tee_private.h > > > > +++ b/drivers/tee/tee_private.h > > > > @@ -13,6 +13,16 @@ > > > > #include <linux/mutex.h> > > > > #include <linux/types.h> > > > > > > > > +/* extra references appended to shm object for registered shared memory */ > > > > +struct tee_shm_dmabuf_ref { > > > > + struct tee_shm shm; > > > > + size_t offset; > > > > + struct dma_buf *dmabuf; > > > > + struct dma_buf_attachment *attach; > > > > + struct sg_table *sgt; > > > > + struct tee_shm *parent_shm; > > > > +}; > > > > + > > > > int tee_shm_get_fd(struct tee_shm *shm); > > > > > > > > bool tee_device_get(struct tee_device *teedev); > > > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > > > index daf6e5cfd59a..e63095e84644 100644 > > > > --- a/drivers/tee/tee_shm.c > > > > +++ b/drivers/tee/tee_shm.c > > > > @@ -4,6 +4,7 @@ > > > > */ > > > > #include <linux/anon_inodes.h> > > > > #include <linux/device.h> > > > > +#include <linux/dma-buf.h> > > > > #include <linux/idr.h> > > > > #include <linux/io.h> > > > > #include <linux/mm.h> > > > > @@ -45,7 +46,21 @@ static void release_registered_pages(struct tee_shm *shm) > > > > > > > > static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > > > { > > > > - if (shm->flags & TEE_SHM_POOL) { > > > > + void *p = shm; > > > > + > > > > + if (shm->flags & TEE_SHM_DMA_BUF) { > > > > + struct tee_shm_dmabuf_ref *ref; > > > > + > > > > + ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > > > + p = ref; > > > > + if (ref->attach) { > > > > + dma_buf_unmap_attachment(ref->attach, ref->sgt, > > > > + DMA_BIDIRECTIONAL); > > > > + > > > > + dma_buf_detach(ref->dmabuf, ref->attach); > > > > + } > > > > + dma_buf_put(ref->dmabuf); > > > > + } else if (shm->flags & TEE_SHM_POOL) { > > > > teedev->pool->ops->free(teedev->pool, shm); > > > > } else if (shm->flags & TEE_SHM_DYNAMIC) { > > > > int rc = teedev->desc->ops->shm_unregister(shm->ctx, shm); > > > > @@ -59,7 +74,7 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > > > > > > > teedev_ctx_put(shm->ctx); > > > > > > > > - kfree(shm); > > > > + kfree(p); > > > > > > > > tee_device_put(teedev); > > > > } > > > > @@ -169,7 +184,7 @@ struct tee_shm *tee_shm_alloc_user_buf(struct tee_context *ctx, size_t size) > > > > * tee_client_invoke_func(). The memory allocated is later freed with a > > > > * call to tee_shm_free(). > > > > * > > > > - * @returns a pointer to 'struct tee_shm' > > > > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure > > > > */ > > > > struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size) > > > > { > > > > @@ -179,6 +194,91 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size) > > > > } > > > > EXPORT_SYMBOL_GPL(tee_shm_alloc_kernel_buf); > > > > > > > > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd) > > > > +{ > > > > + struct tee_shm_dmabuf_ref *ref; > > > > + int rc; > > > > + > > > > + if (!tee_device_get(ctx->teedev)) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + teedev_ctx_get(ctx); > > > > + > > > > + ref = kzalloc(sizeof(*ref), GFP_KERNEL); > > > > + if (!ref) { > > > > + rc = -ENOMEM; > > > > + goto err_put_tee; > > > > + } > > > > + > > > > + refcount_set(&ref->shm.refcount, 1); > > > > + ref->shm.ctx = ctx; > > > > + ref->shm.id = -1; > > > > + ref->shm.flags = TEE_SHM_DMA_BUF; > > > > + > > > > + ref->dmabuf = dma_buf_get(fd); > > > > + if (IS_ERR(ref->dmabuf)) { > > > > + rc = PTR_ERR(ref->dmabuf); > > > > + goto err_kfree_ref; > > > > + } > > > > + > > > > + rc = tee_heap_update_from_dma_buf(ctx->teedev, ref->dmabuf, > > > > + &ref->offset, &ref->shm, > > > > + &ref->parent_shm); > > > > + if (!rc) > > > > + goto out; > > > > > > One odd thing I find here, why do we bail out on success case here? > > > Don't we need the DMA buffer attach and map APIs to be invoked on > > > success case here? > > > > No, because if tee_heap_update_from_dma_buf() succeeds, we know > > everything we need about the buffer. Note that we're returning a valid > > pointer below to indicate success. > > AFAICS, protmem_pool_op_dyn_update_shm() and > protmem_pool_op_static_update_shm() both return 0 on success case... > > > > > > > > + if (rc != -EINVAL) > > > > + goto err_put_dmabuf; > > ...and with this check the below code path is only executed when rc == > -EINVAL, is that expected behaviour? Shouldn't we error out when -EINVAL > is returned? > > > > > + > > > > + ref->attach = dma_buf_attach(ref->dmabuf, &ctx->teedev->dev); > > > > + if (IS_ERR(ref->attach)) { > > > > + rc = PTR_ERR(ref->attach); > > > > + goto err_put_dmabuf; > > > > + } > > > > + > > > > + ref->sgt = dma_buf_map_attachment(ref->attach, DMA_BIDIRECTIONAL); > > > > + if (IS_ERR(ref->sgt)) { > > > > + rc = PTR_ERR(ref->sgt); > > > > + goto err_detach; > > > > + } > > Given above, have we really tested this code leg? I haven't tested it in this version of the patchset, but the code path has been used in the earliest version of the patchset. The idea was to support DMA-bufs allocated by other means, but it's not needed in this patchset, so let's remove it. Cheers, Jens > > -Sumit > > > > > + > > > > + if (sg_nents(ref->sgt->sgl) != 1) { > > > > + rc = -EINVAL; > > > > + goto err_unmap_attachement; > > > > + } > > > > + > > > > + ref->shm.paddr = page_to_phys(sg_page(ref->sgt->sgl)); > > > > + ref->shm.size = ref->sgt->sgl->length; > > > > + > > > > +out: > > > > + mutex_lock(&ref->shm.ctx->teedev->mutex); > > > > + ref->shm.id = idr_alloc(&ref->shm.ctx->teedev->idr, &ref->shm, > > > > + 1, 0, GFP_KERNEL); > > > > + mutex_unlock(&ref->shm.ctx->teedev->mutex); > > > > + if (ref->shm.id < 0) { > > > > + rc = ref->shm.id; > > > > + if (ref->attach) > > > > + goto err_unmap_attachement; > > > > + goto err_put_dmabuf; > > > > + } > > > > + > > > > + return &ref->shm; > > > > + > > > > +err_unmap_attachement: > > > > + dma_buf_unmap_attachment(ref->attach, ref->sgt, DMA_BIDIRECTIONAL); > > > > +err_detach: > > > > + dma_buf_detach(ref->dmabuf, ref->attach); > > > > +err_put_dmabuf: > > > > + dma_buf_put(ref->dmabuf); > > > > +err_kfree_ref: > > > > + kfree(ref); > > > > +err_put_tee: > > > > + teedev_ctx_put(ctx); > > > > + tee_device_put(ctx->teedev); > > > > + > > > > + return ERR_PTR(rc); > > > > +} > > > > +EXPORT_SYMBOL_GPL(tee_shm_register_fd); > > > > + > > > > /** > > > > * tee_shm_alloc_priv_buf() - Allocate shared memory for a privately shared > > > > * kernel buffer > > > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > > > index 22e03d897dc3..f17710196c4c 100644 > > > > --- a/include/linux/tee_core.h > > > > +++ b/include/linux/tee_core.h > > > > @@ -28,6 +28,7 @@ > > > > #define TEE_SHM_USER_MAPPED BIT(1) /* Memory mapped in user space */ > > > > #define TEE_SHM_POOL BIT(2) /* Memory allocated from pool */ > > > > #define TEE_SHM_PRIV BIT(3) /* Memory private to TEE driver */ > > > > +#define TEE_SHM_DMA_BUF BIT(4) /* Memory with dma-buf handle */ > > > > > > > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > > > #define TEE_MAX_DEV_NAME_LEN 32 > > > > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h > > > > index a54c203000ed..824f1251de60 100644 > > > > --- a/include/linux/tee_drv.h > > > > +++ b/include/linux/tee_drv.h > > > > @@ -116,6 +116,16 @@ struct tee_shm *tee_shm_alloc_kernel_buf(struct tee_context *ctx, size_t size); > > > > struct tee_shm *tee_shm_register_kernel_buf(struct tee_context *ctx, > > > > void *addr, size_t length); > > > > > > > > +/** > > > > + * tee_shm_register_fd() - Register shared memory from file descriptor > > > > + * > > > > + * @ctx: Context that allocates the shared memory > > > > + * @fd: Shared memory file descriptor reference > > > > + * > > > > + * @returns a pointer to 'struct tee_shm' on success, and ERR_PTR on failure > > > > + */ > > > > +struct tee_shm *tee_shm_register_fd(struct tee_context *ctx, int fd); > > > > + > > > > /** > > > > * tee_shm_free() - Free shared memory > > > > * @shm: Handle to shared memory to free > > > > diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h > > > > index d0430bee8292..d843cf980d98 100644 > > > > --- a/include/uapi/linux/tee.h > > > > +++ b/include/uapi/linux/tee.h > > > > @@ -378,6 +378,37 @@ struct tee_ioctl_shm_register_data { > > > > __s32 id; > > > > }; > > > > > > > > +/** > > > > + * struct tee_ioctl_shm_register_fd_data - Shared memory registering argument > > > > + * @fd: [in] File descriptor identifying dmabuf reference > > > > + * @size: [out] Size of referenced memory > > > > + * @flags: [in] Flags to/from allocation. > > > > + * @id: [out] Identifier of the shared memory > > > > + * > > > > + * The flags field should currently be zero as input. Updated by the call > > > > + * with actual flags as defined by TEE_IOCTL_SHM_* above. > > > > + * This structure is used as argument for TEE_IOC_SHM_REGISTER_FD below. > > > > + */ > > > > +struct tee_ioctl_shm_register_fd_data { > > > > + __s64 fd; > > > > + __u64 size; > > > > + __u32 flags; > > > > + __s32 id; > > > > +}; > > > > + > > > > +/** > > > > + * TEE_IOC_SHM_REGISTER_FD - register a shared memory from a file descriptor > > > > + * > > > > + * Returns a file descriptor on success or < 0 on failure > > > > + * > > > > + * The returned file descriptor refers to the shared memory object in the > > > > + * kernel. The supplied file deccriptor can be closed if it's not needed > > > > + * for other purposes. The shared memory is freed when the descriptor is > > > > + * closed. > > > > + */ > > > > +#define TEE_IOC_SHM_REGISTER_FD _IOWR(TEE_IOC_MAGIC, TEE_IOC_BASE + 8, \ > > > > + struct tee_ioctl_shm_register_fd_data) > > > > + > > > > /** > > > > * TEE_IOC_SHM_REGISTER - Register shared memory argument > > > > * > > > > -- > > > > 2.43.0 > > > >

2 months, 1 week

Re: [PATCH v10 6/9] tee: add tee_shm_alloc_dma_mem()

by Jens Wiklander

On Thu, Jul 3, 2025 at 8:28 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Wed, Jun 18, 2025 at 09:03:00AM +0200, Jens Wiklander wrote: > > On Tue, Jun 17, 2025 at 1:32 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Tue, Jun 10, 2025 at 03:13:50PM +0200, Jens Wiklander wrote: > > > > Add tee_shm_alloc_dma_mem() to allocate DMA memory. The memory is > > > > represented by a tee_shm object using the new flag TEE_SHM_DMA_MEM to > > > > identify it as DMA memory. The allocated memory will later be lent to > > > > the TEE to be used as protected memory. > > > > > > > > Signed-off-by: Jens Wiklander <jens.wiklander(a)linaro.org> > > > > --- > > > > drivers/tee/tee_shm.c | 85 +++++++++++++++++++++++++++++++++++++++- > > > > include/linux/tee_core.h | 5 +++ > > > > 2 files changed, 88 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c > > > > index e63095e84644..60b0f3932cee 100644 > > > > --- a/drivers/tee/tee_shm.c > > > > +++ b/drivers/tee/tee_shm.c > > > > @@ -5,6 +5,8 @@ > > > > #include <linux/anon_inodes.h> > > > > #include <linux/device.h> > > > > #include <linux/dma-buf.h> > > > > +#include <linux/dma-mapping.h> > > > > +#include <linux/highmem.h> > > > > #include <linux/idr.h> > > > > #include <linux/io.h> > > > > #include <linux/mm.h> > > > > @@ -13,9 +15,14 @@ > > > > #include <linux/tee_core.h> > > > > #include <linux/uaccess.h> > > > > #include <linux/uio.h> > > > > -#include <linux/highmem.h> > > > > #include "tee_private.h" > > > > > > > > +struct tee_shm_dma_mem { > > > > + struct tee_shm shm; > > > > + dma_addr_t dma_addr; > > > > + struct page *page; > > > > +}; > > > > + > > > > static void shm_put_kernel_pages(struct page **pages, size_t page_count) > > > > { > > > > size_t n; > > > > @@ -48,7 +55,16 @@ static void tee_shm_release(struct tee_device *teedev, struct tee_shm *shm) > > > > { > > > > void *p = shm; > > > > > > > > - if (shm->flags & TEE_SHM_DMA_BUF) { > > > > + if (shm->flags & TEE_SHM_DMA_MEM) { > > > > +#if IS_ENABLED(CONFIG_TEE_DMABUF_HEAPS) > > > > > > nit: this config check can be merged into the above if check. > > > > No, because dma_free_pages() is only defined if > > CONFIG_TEE_DMABUF_HEAPS is enabled. > > It looks like you misunderstood my above comment, I rather meant: > > if (IS_ENABLED(CONFIG_TEE_DMABUF_HEAPS) && > (shm->flags & TEE_SHM_DMA_MEM)) That depends on the compiler optimizing away the call to dma_free_pages() if CONFIG_TEE_DMABUF_HEAPS isn't defined. This is normally the case, but if you compile for debugging, you may get unresolved symbols. Cheers, Jens > > -Sumit > > > > > > > > > > + struct tee_shm_dma_mem *dma_mem; > > > > + > > > > + dma_mem = container_of(shm, struct tee_shm_dma_mem, shm); > > > > + p = dma_mem; > > > > + dma_free_pages(&teedev->dev, shm->size, dma_mem->page, > > > > + dma_mem->dma_addr, DMA_BIDIRECTIONAL); > > > > +#endif > > > > + } else if (shm->flags & TEE_SHM_DMA_BUF) { > > > > > > Do we need a similar config check for this flag too? > > > > No, because DMA_SHARED_BUFFER is selected, so the dma_buf functions are defined. > > > > Cheers, > > Jens > > > > > > > > > > With these addressed, feel free to add: > > > > > > Reviewed-by: Sumit Garg <sumit.garg(a)oss.qualcomm.com> > > > > > > -Sumit > > > > > > > struct tee_shm_dmabuf_ref *ref; > > > > > > > > ref = container_of(shm, struct tee_shm_dmabuf_ref, shm); > > > > @@ -303,6 +319,71 @@ struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size) > > > > } > > > > EXPORT_SYMBOL_GPL(tee_shm_alloc_priv_buf); > > > > > > > > +#if IS_ENABLED(CONFIG_TEE_DMABUF_HEAPS) > > > > +/** > > > > + * tee_shm_alloc_dma_mem() - Allocate DMA memory as shared memory object > > > > + * @ctx: Context that allocates the shared memory > > > > + * @page_count: Number of pages > > > > + * > > > > + * The allocated memory is expected to be lent (made inaccessible to the > > > > + * kernel) to the TEE while it's used and returned (accessible to the > > > > + * kernel again) before it's freed. > > > > + * > > > > + * This function should normally only be used internally in the TEE > > > > + * drivers. > > > > + * > > > > + * @returns a pointer to 'struct tee_shm' > > > > + */ > > > > +struct tee_shm *tee_shm_alloc_dma_mem(struct tee_context *ctx, > > > > + size_t page_count) > > > > +{ > > > > + struct tee_device *teedev = ctx->teedev; > > > > + struct tee_shm_dma_mem *dma_mem; > > > > + dma_addr_t dma_addr; > > > > + struct page *page; > > > > + > > > > + if (!tee_device_get(teedev)) > > > > + return ERR_PTR(-EINVAL); > > > > + > > > > + page = dma_alloc_pages(&teedev->dev, page_count * PAGE_SIZE, > > > > + &dma_addr, DMA_BIDIRECTIONAL, GFP_KERNEL); > > > > + if (!page) > > > > + goto err_put_teedev; > > > > + > > > > + dma_mem = kzalloc(sizeof(*dma_mem), GFP_KERNEL); > > > > + if (!dma_mem) > > > > + goto err_free_pages; > > > > + > > > > + refcount_set(&dma_mem->shm.refcount, 1); > > > > + dma_mem->shm.ctx = ctx; > > > > + dma_mem->shm.paddr = page_to_phys(page); > > > > + dma_mem->dma_addr = dma_addr; > > > > + dma_mem->page = page; > > > > + dma_mem->shm.size = page_count * PAGE_SIZE; > > > > + dma_mem->shm.flags = TEE_SHM_DMA_MEM; > > > > + > > > > + teedev_ctx_get(ctx); > > > > + > > > > + return &dma_mem->shm; > > > > + > > > > +err_free_pages: > > > > + dma_free_pages(&teedev->dev, page_count * PAGE_SIZE, page, dma_addr, > > > > + DMA_BIDIRECTIONAL); > > > > +err_put_teedev: > > > > + tee_device_put(teedev); > > > > + > > > > + return ERR_PTR(-ENOMEM); > > > > +} > > > > +EXPORT_SYMBOL_GPL(tee_shm_alloc_dma_mem); > > > > +#else > > > > +struct tee_shm *tee_shm_alloc_dma_mem(struct tee_context *ctx, > > > > + size_t page_count) > > > > +{ > > > > + return ERR_PTR(-EINVAL); > > > > +} > > > > +EXPORT_SYMBOL_GPL(tee_shm_alloc_dma_mem); > > > > +#endif > > > > + > > > > int tee_dyn_shm_alloc_helper(struct tee_shm *shm, size_t size, size_t align, > > > > int (*shm_register)(struct tee_context *ctx, > > > > struct tee_shm *shm, > > > > diff --git a/include/linux/tee_core.h b/include/linux/tee_core.h > > > > index f17710196c4c..e46a53e753af 100644 > > > > --- a/include/linux/tee_core.h > > > > +++ b/include/linux/tee_core.h > > > > @@ -29,6 +29,8 @@ > > > > #define TEE_SHM_POOL BIT(2) /* Memory allocated from pool */ > > > > #define TEE_SHM_PRIV BIT(3) /* Memory private to TEE driver */ > > > > #define TEE_SHM_DMA_BUF BIT(4) /* Memory with dma-buf handle */ > > > > +#define TEE_SHM_DMA_MEM BIT(5) /* Memory allocated with */ > > > > + /* dma_alloc_pages() */ > > > > > > > > #define TEE_DEVICE_FLAG_REGISTERED 0x1 > > > > #define TEE_MAX_DEV_NAME_LEN 32 > > > > @@ -310,6 +312,9 @@ void *tee_get_drvdata(struct tee_device *teedev); > > > > */ > > > > struct tee_shm *tee_shm_alloc_priv_buf(struct tee_context *ctx, size_t size); > > > > > > > > +struct tee_shm *tee_shm_alloc_dma_mem(struct tee_context *ctx, > > > > + size_t page_count); > > > > + > > > > int tee_dyn_shm_alloc_helper(struct tee_shm *shm, size_t size, size_t align, > > > > int (*shm_register)(struct tee_context *ctx, > > > > struct tee_shm *shm, > > > > -- > > > > 2.43.0 > > > >

2 months, 1 week

Re: [PATCH v7 03/10] accel/rocket: Add IOCTL for BO creation

by Heiko Stübner

Am Freitag, 6. Juni 2025, 08:28:23 Mitteleuropäische Sommerzeit schrieb Tomeu Vizoso: > This uses the SHMEM DRM helpers and we map right away to the CPU and NPU > sides, as all buffers are expected to be accessed from both. > > v2: > - Sync the IOMMUs for the other cores when mapping and unmapping. > > v3: > - Make use of GPL-2.0-only for the copyright notice (Jeff Hugo) > > v6: > - Use mutexes guard (Markus Elfring) > > v7: > - Assign its own IOMMU domain to each client, for isolation (Daniel > Stone and Robin Murphy) > > Reviewed-by: Jeffrey Hugo <quic_jhugo(a)quicinc.com> > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > --- > diff --git a/drivers/accel/rocket/rocket_gem.c b/drivers/accel/rocket/rocket_gem.c > new file mode 100644 > index 0000000000000000000000000000000000000000..61b7f970a6885aa13784daa1222611a02aa10dee > --- /dev/null > +++ b/drivers/accel/rocket/rocket_gem.c > @@ -0,0 +1,115 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright 2024-2025 Tomeu Vizoso <tomeu(a)tomeuvizoso.net> */ > + > +#include <drm/drm_device.h> > +#include <drm/drm_utils.h> > +#include <drm/rocket_accel.h> > +#include <linux/dma-mapping.h> > +#include <linux/iommu.h> > + > +#include "rocket_device.h" > +#include "rocket_drv.h" > +#include "rocket_gem.h" > + > +static void rocket_gem_bo_free(struct drm_gem_object *obj) > +{ > + struct rocket_device *rdev = to_rocket_device(obj->dev); > + struct rocket_gem_object *bo = to_rocket_bo(obj); > + size_t unmapped; > + > + drm_WARN_ON(obj->dev, bo->base.pages_use_count > 1); This should probably be drm_WARN_ON(obj->dev, refcount_read(&bo->base.pages_use_count) > 1); as pages_use_count is of type refcount_t since commit 051b6646d36d ("drm/shmem-helper: Use refcount_t for pages_use_count") Heiko

2 months, 1 week

Re: [PATCH v7 04/10] accel/rocket: Add job submission IOCTL

by Rob Herring

On Fri, Jun 6, 2025 at 1:29 AM Tomeu Vizoso <tomeu(a)tomeuvizoso.net> wrote: > > Using the DRM GPU scheduler infrastructure, with a scheduler for each > core. > > Userspace can decide for a series of tasks to be executed sequentially > in the same core, so SRAM locality can be taken advantage of. > > The job submission code was initially based on Panfrost. > > v2: > - Remove hardcoded number of cores > - Misc. style fixes (Jeffrey Hugo) > - Repack IOCTL struct (Jeffrey Hugo) > > v3: > - Adapt to a split of the register block in the DT bindings (Nicolas > Frattaroli) > - Make use of GPL-2.0-only for the copyright notice (Jeff Hugo) > - Use drm_* logging functions (Thomas Zimmermann) > - Rename reg i/o macros (Thomas Zimmermann) > - Add padding to ioctls and check for zero (Jeff Hugo) > - Improve error handling (Nicolas Frattaroli) > > v6: > - Use mutexes guard (Markus Elfring) > - Use u64_to_user_ptr (Jeff Hugo) > - Drop rocket_fence (Rob Herring) > > v7: > - Assign its own IOMMU domain to each client, for isolation (Daniel > Stone and Robin Murphy) > > Signed-off-by: Tomeu Vizoso <tomeu(a)tomeuvizoso.net> > --- [...] > --- a/include/uapi/drm/rocket_accel.h > +++ b/include/uapi/drm/rocket_accel.h > @@ -12,8 +12,10 @@ extern "C" { > #endif > > #define DRM_ROCKET_CREATE_BO 0x00 > +#define DRM_ROCKET_SUBMIT 0x01 > > #define DRM_IOCTL_ROCKET_CREATE_BO DRM_IOWR(DRM_COMMAND_BASE + DRM_ROCKET_CREATE_BO, struct drm_rocket_create_bo) > +#define DRM_IOCTL_ROCKET_SUBMIT DRM_IOW(DRM_COMMAND_BASE + DRM_ROCKET_SUBMIT, struct drm_rocket_submit) > > /** > * struct drm_rocket_create_bo - ioctl argument for creating Rocket BOs. > @@ -37,6 +39,68 @@ struct drm_rocket_create_bo { > __u64 offset; > }; > > +/** > + * struct drm_rocket_task - A task to be run on the NPU > + * > + * A task is the smallest unit of work that can be run on the NPU. > + */ > +struct drm_rocket_task { > + /** Input: DMA address to NPU mapping of register command buffer */ > + __u64 regcmd; > + > + /** Input: Number of commands in the register command buffer */ > + __u32 regcmd_count; > + > + /** Reserved, must be zero. */ > + __u32 reserved; > +}; > + > +/** > + * struct drm_rocket_job - A job to be run on the NPU > + * > + * The kernel will schedule the execution of this job taking into account its > + * dependencies with other jobs. All tasks in the same job will be executed > + * sequentially on the same core, to benefit from memory residency in SRAM. > + */ > +struct drm_rocket_job { > + /** Input: Pointer to an array of struct drm_rocket_task. */ > + __u64 tasks; > + > + /** Input: Pointer to a u32 array of the BOs that are read by the job. */ > + __u64 in_bo_handles; > + > + /** Input: Pointer to a u32 array of the BOs that are written to by the job. */ > + __u64 out_bo_handles; > + > + /** Input: Number of tasks passed in. */ > + __u32 task_count; > + > + /** Input: Number of input BO handles passed in (size is that times 4). */ > + __u32 in_bo_handle_count; > + > + /** Input: Number of output BO handles passed in (size is that times 4). */ > + __u32 out_bo_handle_count; > + > + /** Reserved, must be zero. */ > + __u32 reserved; > +}; > + > +/** > + * struct drm_rocket_submit - ioctl argument for submitting commands to the NPU. > + * > + * The kernel will schedule the execution of these jobs in dependency order. > + */ > +struct drm_rocket_submit { > + /** Input: Pointer to an array of struct drm_rocket_job. */ > + __u64 jobs; > + > + /** Input: Number of jobs passed in. */ > + __u32 job_count; Isn't there a problem if you need to expand drm_rocket_job beyond using the 1 reserved field? You can't add to the struct because then you don't know the size here. So you have to modify drm_rocket_submit to modify drm_rocket_job. Maybe better if you plan for that now rather than later by making the size explicit. Though etnaviv at least has similar issues. Rob > + > + /** Reserved, must be zero. */ > + __u32 reserved; > +};

2 months, 1 week

[PATCH v2] drm/gem: Acquire references on GEM handles for framebuffers

by Thomas Zimmermann

A GEM handle can be released while the GEM buffer object is attached to a DRM framebuffer. This leads to the release of the dma-buf backing the buffer object, if any. [1] Trying to use the framebuffer in further mode-setting operations leads to a segmentation fault. Most easily happens with driver that use shadow planes for vmap-ing the dma-buf during a page flip. An example is shown below. [ 156.791968] ------------[ cut here ]------------ [ 156.796830] WARNING: CPU: 2 PID: 2255 at drivers/dma-buf/dma-buf.c:1527 dma_buf_vmap+0x224/0x430 [...] [ 156.942028] RIP: 0010:dma_buf_vmap+0x224/0x430 [ 157.043420] Call Trace: [ 157.045898] <TASK> [ 157.048030] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.052436] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.056836] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.061253] ? drm_gem_shmem_vmap+0x74/0x710 [ 157.065567] ? dma_buf_vmap+0x224/0x430 [ 157.069446] ? __warn.cold+0x58/0xe4 [ 157.073061] ? dma_buf_vmap+0x224/0x430 [ 157.077111] ? report_bug+0x1dd/0x390 [ 157.080842] ? handle_bug+0x5e/0xa0 [ 157.084389] ? exc_invalid_op+0x14/0x50 [ 157.088291] ? asm_exc_invalid_op+0x16/0x20 [ 157.092548] ? dma_buf_vmap+0x224/0x430 [ 157.096663] ? dma_resv_get_singleton+0x6d/0x230 [ 157.101341] ? __pfx_dma_buf_vmap+0x10/0x10 [ 157.105588] ? __pfx_dma_resv_get_singleton+0x10/0x10 [ 157.110697] drm_gem_shmem_vmap+0x74/0x710 [ 157.114866] drm_gem_vmap+0xa9/0x1b0 [ 157.118763] drm_gem_vmap_unlocked+0x46/0xa0 [ 157.123086] drm_gem_fb_vmap+0xab/0x300 [ 157.126979] drm_atomic_helper_prepare_planes.part.0+0x487/0xb10 [ 157.133032] ? lockdep_init_map_type+0x19d/0x880 [ 157.137701] drm_atomic_helper_commit+0x13d/0x2e0 [ 157.142671] ? drm_atomic_nonblocking_commit+0xa0/0x180 [ 157.147988] drm_mode_atomic_ioctl+0x766/0xe40 [...] [ 157.346424] ---[ end trace 0000000000000000 ]--- Acquiring GEM handles for the framebuffer's GEM buffer objects prevents this from happening. The framebuffer's cleanup later puts the handle references. Commit 1a148af06000 ("drm/gem-shmem: Use dma_buf from GEM object instance") triggers the segmentation fault easily by using the dma-buf field more widely. The underlying issue with reference counting has been present before. v2: - acquire the handle instead of the BO (Christian) - fix comment style (Christian) - drop the Fixes tag (Christian) - rename err_ gotos - add missing Link tag Suggested-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Link: https://elixir.bootlin.com/linux/v6.15/source/drivers/gpu/drm/drm_gem.c#L241 # [1] Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> --- drivers/gpu/drm/drm_gem.c | 44 ++++++++++++++++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 +++---- drivers/gpu/drm/drm_internal.h | 2 + 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 19d50d254fe6..bc505d938b3e 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -213,6 +213,35 @@ void drm_gem_private_object_fini(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_private_object_fini); +static void drm_gem_object_handle_get(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + drm_WARN_ON(dev, !mutex_is_locked(&dev->object_name_lock)); + + if (obj->handle_count++ == 0) + drm_gem_object_get(obj); +} + +/** + * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles + * @obj: GEM object + * + * Acquires a reference on the GEM buffer object's handle. Required + * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked() + * to release the reference. + */ +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + guard(mutex)(&dev->object_name_lock); + + drm_WARN_ON(dev, !obj->handle_count); /* first ref taken in create-tail helper */ + drm_gem_object_handle_get(obj); +} +EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked); + /** * drm_gem_object_handle_free - release resources bound to userspace handles * @obj: GEM object to clean up. @@ -243,8 +272,14 @@ static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj) } } -static void -drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) +/** + * drm_gem_object_handle_put_unlocked - releases reference on user-space handles + * @obj: GEM object + * + * Releases a reference on the GEM buffer object's handle. Possibly releases + * the GEM buffer object and associated dma-buf objects. + */ +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; bool final = false; @@ -269,6 +304,7 @@ drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) if (final) drm_gem_object_put(obj); } +EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked); /* * Called at device or object close to release the file's @@ -390,8 +426,8 @@ drm_gem_handle_create_tail(struct drm_file *file_priv, int ret; WARN_ON(!mutex_is_locked(&dev->object_name_lock)); - if (obj->handle_count++ == 0) - drm_gem_object_get(obj); + + drm_gem_object_handle_get(obj); /* * Get the user-visible handle using idr. Preload and perform diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 618ce725cd75..c60d0044d036 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -100,7 +100,7 @@ void drm_gem_fb_destroy(struct drm_framebuffer *fb) unsigned int i; for (i = 0; i < fb->format->num_planes; i++) - drm_gem_object_put(fb->obj[i]); + drm_gem_object_handle_put_unlocked(fb->obj[i]); drm_framebuffer_cleanup(fb); kfree(fb); @@ -183,8 +183,10 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, if (!objs[i]) { drm_dbg_kms(dev, "Failed to lookup GEM object\n"); ret = -ENOENT; - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; } + drm_gem_object_handle_get_unlocked(objs[i]); + drm_gem_object_put(objs[i]); min_size = (height - 1) * mode_cmd->pitches[i] + drm_format_info_min_pitch(info, i, width) @@ -194,22 +196,22 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, drm_dbg_kms(dev, "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n", objs[i]->size, min_size, i); - drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); ret = -EINVAL; - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; } } ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs); if (ret) - goto err_gem_object_put; + goto err_gem_object_handle_put_unlocked; return 0; -err_gem_object_put: +err_gem_object_handle_put_unlocked: while (i > 0) { --i; - drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); } return ret; } diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 442eb31351dd..f7b414a813ae 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -161,6 +161,8 @@ void drm_sysfs_lease_event(struct drm_device *dev); /* drm_gem.c */ int drm_gem_init(struct drm_device *dev); +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj); +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj); int drm_gem_handle_create_tail(struct drm_file *file_priv, struct drm_gem_object *obj, u32 *handlep); -- 2.50.0

2 months, 1 week

[PATCH] drm/gem: Acquire references on GEM handles for framebuffers

by Thomas Zimmermann

A GEM handle can be released while the GEM buffer object is attached to a DRM framebuffer. This leads to the release of the dma-buf backing the buffer object, if any. [1] Trying to use the framebuffer in further mode-setting operations leads to a segmentation fault. Most easily happens with driver that use shadow planes for vmap-ing the dma-buf during a page flip. An example is shown below. [ 156.791968] ------------[ cut here ]------------ [ 156.796830] WARNING: CPU: 2 PID: 2255 at drivers/dma-buf/dma-buf.c:1527 dma_buf_vmap+0x224/0x430 [...] [ 156.942028] RIP: 0010:dma_buf_vmap+0x224/0x430 [ 157.043420] Call Trace: [ 157.045898] <TASK> [ 157.048030] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.052436] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.056836] ? show_trace_log_lvl+0x1af/0x2c0 [ 157.061253] ? drm_gem_shmem_vmap+0x74/0x710 [ 157.065567] ? dma_buf_vmap+0x224/0x430 [ 157.069446] ? __warn.cold+0x58/0xe4 [ 157.073061] ? dma_buf_vmap+0x224/0x430 [ 157.077111] ? report_bug+0x1dd/0x390 [ 157.080842] ? handle_bug+0x5e/0xa0 [ 157.084389] ? exc_invalid_op+0x14/0x50 [ 157.088291] ? asm_exc_invalid_op+0x16/0x20 [ 157.092548] ? dma_buf_vmap+0x224/0x430 [ 157.096663] ? dma_resv_get_singleton+0x6d/0x230 [ 157.101341] ? __pfx_dma_buf_vmap+0x10/0x10 [ 157.105588] ? __pfx_dma_resv_get_singleton+0x10/0x10 [ 157.110697] drm_gem_shmem_vmap+0x74/0x710 [ 157.114866] drm_gem_vmap+0xa9/0x1b0 [ 157.118763] drm_gem_vmap_unlocked+0x46/0xa0 [ 157.123086] drm_gem_fb_vmap+0xab/0x300 [ 157.126979] drm_atomic_helper_prepare_planes.part.0+0x487/0xb10 [ 157.133032] ? lockdep_init_map_type+0x19d/0x880 [ 157.137701] drm_atomic_helper_commit+0x13d/0x2e0 [ 157.142671] ? drm_atomic_nonblocking_commit+0xa0/0x180 [ 157.147988] drm_mode_atomic_ioctl+0x766/0xe40 [...] [ 157.346424] ---[ end trace 0000000000000000 ]--- Acquiring GEM handles for the framebuffer's GEM buffer objects prevents this from happening. The framebuffer's cleanup later puts the handle references. The Fixes tag points to commit 1a148af06000 ("drm/gem-shmem: Use dma_buf from GEM object instance"), which triggers the segmentation fault. The issue has been present before. Suggested-by: Christian König <christian.koenig(a)amd.com> Signed-off-by: Thomas Zimmermann <tzimmermann(a)suse.de> Fixes: 1a148af06000 ("drm/gem-shmem: Use dma_buf from GEM object instance") Cc: Thomas Zimmermann <tzimmermann(a)suse.de> Cc: Anusha Srivatsa <asrivats(a)redhat.com> Cc: Christian König <christian.koenig(a)amd.com> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org Cc: <stable(a)vger.kernel.org> --- drivers/gpu/drm/drm_gem.c | 44 ++++++++++++++++++-- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 7 +++- drivers/gpu/drm/drm_internal.h | 2 + 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 19d50d254fe6..8be50b3cc9c2 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -213,6 +213,35 @@ void drm_gem_private_object_fini(struct drm_gem_object *obj) } EXPORT_SYMBOL(drm_gem_private_object_fini); +static void drm_gem_object_handle_get(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + drm_WARN_ON(dev, !mutex_is_locked(&dev->object_name_lock)); + + if (obj->handle_count++ == 0) + drm_gem_object_get(obj); +} + +/** + * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles + * @obj: GEM object + * + * Acquires a reference on the GEM buffer object's handle. Required + * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked() + * to release the reference. + */ +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj) +{ + struct drm_device *dev = obj->dev; + + guard(mutex)(&dev->object_name_lock); + + drm_WARN_ON(dev, !obj->handle_count); // first ref taken in create-tail helper + drm_gem_object_handle_get(obj); +} +EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked); + /** * drm_gem_object_handle_free - release resources bound to userspace handles * @obj: GEM object to clean up. @@ -243,8 +272,14 @@ static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj) } } -static void -drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) +/** + * drm_gem_object_handle_put_unlocked - releases reference on user-space handles + * @obj: GEM object + * + * Releases a reference on the GEM buffer object's handle. Possibly releases + * the GEM buffer object and associated dma-buf objects. + */ +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) { struct drm_device *dev = obj->dev; bool final = false; @@ -269,6 +304,7 @@ drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj) if (final) drm_gem_object_put(obj); } +EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked); /* * Called at device or object close to release the file's @@ -390,8 +426,8 @@ drm_gem_handle_create_tail(struct drm_file *file_priv, int ret; WARN_ON(!mutex_is_locked(&dev->object_name_lock)); - if (obj->handle_count++ == 0) - drm_gem_object_get(obj); + + drm_gem_object_handle_get(obj); /* * Get the user-visible handle using idr. Preload and perform diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c index 618ce725cd75..723f1d652c01 100644 --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c @@ -99,8 +99,10 @@ void drm_gem_fb_destroy(struct drm_framebuffer *fb) { unsigned int i; - for (i = 0; i < fb->format->num_planes; i++) + for (i = 0; i < fb->format->num_planes; i++) { + drm_gem_object_handle_put_unlocked(fb->obj[i]); drm_gem_object_put(fb->obj[i]); + } drm_framebuffer_cleanup(fb); kfree(fb); @@ -185,6 +187,7 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, ret = -ENOENT; goto err_gem_object_put; } + drm_gem_object_handle_get_unlocked(objs[i]); min_size = (height - 1) * mode_cmd->pitches[i] + drm_format_info_min_pitch(info, i, width) @@ -195,6 +198,7 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n", objs[i]->size, min_size, i); drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); ret = -EINVAL; goto err_gem_object_put; } @@ -210,6 +214,7 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev, while (i > 0) { --i; drm_gem_object_put(objs[i]); + drm_gem_object_handle_put_unlocked(objs[i]); } return ret; } diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 442eb31351dd..f7b414a813ae 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -161,6 +161,8 @@ void drm_sysfs_lease_event(struct drm_device *dev); /* drm_gem.c */ int drm_gem_init(struct drm_device *dev); +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj); +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj); int drm_gem_handle_create_tail(struct drm_file *file_priv, struct drm_gem_object *obj, u32 *handlep); -- 2.50.0

2 months, 2 weeks

← Newer
1
...
9
10
11
12
13
14
15
...
302
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig