Hi
> On 19 Feb 2025, at 3:19 PM, Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 19.02.25 um 10:37 schrieb Aditya Garg:
>> Hi
>>
>>>> I’ve tried these changes, seem to be breaking the driver:
>>>>
>>>> —>8—
>>>> From 16c920cabf65ec664663ebe1611c0ccf6e81de4a Mon Sep 17 00:00:00 2001
>>>> From: Aditya Garg <gargaditya08@xxxxxxxx>
>>>> Date: Tue, 18 Feb 2025 18:54:10 +0530
>>>> Subject: [PATCH] better error handling
>>>>
>>>> ---
>>>> .../apple-touchbar-advanced-0.1/appletbdrm.c | 68 +++++++++++++------
>>>> 1 file changed, 46 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>>>> index f2d9113..cb13b36 100644
>>>> --- a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>>>> +++ b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>>>> @@ -133,6 +133,17 @@ struct appletbdrm_device {
>>>> struct drm_encoder encoder;
>>>> };
>>>> +struct appletbdrm_plane_state {
>>>> + struct drm_shadow_plane_state base;
>>>> + u8 *request_buffer;
>>>> + u8 *response_buffer;
>>>> +};
>>>> +
>>>> +static inline struct appletbdrm_plane_state *to_appletbdrm_plane_state(struct drm_plane_state *state)
>>>> +{
>>>> + return container_of(state, struct appletbdrm_plane_state, base.base);
>>>> +}
>>>> +
>>>> static int appletbdrm_send_request(struct appletbdrm_device *adev,
>>>> struct appletbdrm_msg_request_header *request, size_t size)
>>>> {
>>>> @@ -311,24 +322,6 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>>>> if (!frames_size)
>>>> return 0;
>>>> - request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
>>>> -
>>>> - request = kzalloc(request_size, GFP_KERNEL);
>>>> - if (!request)
>>>> - return -ENOMEM;
>>>> -
>>>> - response = kzalloc(sizeof(*response), GFP_KERNEL);
>>>> - if (!response) {
>>>> - ret = -ENOMEM;
>>>> - goto free_request;
>>>> - }
>>>> -
>>>> - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>>>> - if (ret) {
>>>> - drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret);
>>>> - goto free_response;
>>>> - }
>>>> -
>>>> request->header.unk_00 = cpu_to_le16(2);
>>>> request->header.unk_02 = cpu_to_le16(0x12);
>>>> request->header.unk_04 = cpu_to_le32(9);
>>>> @@ -389,10 +382,6 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>>>> end_fb_cpu_access:
>>>> drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>>> -free_response:
>>>> - kfree(response);
>>>> -free_request:
>>>> - kfree(request);
>>>> return ret;
>>>> }
>>>> @@ -415,6 +404,15 @@ static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>>> struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>>>> struct drm_crtc *new_crtc = new_plane_state->crtc;
>>>> struct drm_crtc_state *new_crtc_state = NULL;
>>>> + struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(new_plane_state);
>>>> + struct drm_device *drm = plane->dev;
>>>> + struct drm_plane_state *plane_state = plane->state;
>>>> + struct appletbdrm_fb_request_response *response;
>>>> + struct appletbdrm_fb_request_footer *footer;
>>>> + struct drm_framebuffer *fb = plane_state->fb;
>>>> + struct appletbdrm_fb_request *request;
>>>> + size_t frames_size = 0;
>>>> + size_t request_size;
>>>> int ret;
>>>> if (new_crtc)
>>>> @@ -429,6 +427,22 @@ static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>>>> else if (!new_plane_state->visible)
>>>> return 0;
>>>> + request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
>>>> +
>>>> + appletbdrm_state->request_buffer = kzalloc(request_size, GFP_KERNEL);
>>>> + if (!request)
>>>> + return -ENOMEM;
>>>> +
>>>> + appletbdrm_state->response_buffer = kzalloc(sizeof(*response), GFP_KERNEL);
>>>> + if (!response) {
>>>> + ret = -ENOMEM;
>>>> + }
>>>> +
>>>> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>>>> + if (ret) {
>>>> + drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret);
>>>> + }
>>>> +
>>>> return 0;
>>>> }
>>>> @@ -464,6 +478,15 @@ static void appletbdrm_primary_plane_helper_atomic_disable(struct drm_plane *pla
>>>> drm_dev_exit(idx);
>>>> }
>>>> +static void appletbdrm_primary_plane_destroy_state(struct drm_plane *plane,
>>>> + struct drm_plane_state *state)
>>>> +{
>>>> + struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(state);
>>>> +
>>>> + kfree(appletbdrm_state->request_buffer);
>>>> + kfree(appletbdrm_state->response_buffer);
>>>> +}
>>>> +
>>>> static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs = {
>>>> DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>>>> .atomic_check = appletbdrm_primary_plane_helper_atomic_check,
>>>> @@ -474,6 +497,7 @@ static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs
>>>> static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = {
>>>> .update_plane = drm_atomic_helper_update_plane,
>>>> .disable_plane = drm_atomic_helper_disable_plane,
>>>> + .atomic_destroy_state = appletbdrm_primary_plane_destroy_state,
>>>> .destroy = drm_plane_cleanup,
>>>> DRM_GEM_SHADOW_PLANE_FUNCS,
>>> You don't allocate struct appletbdrm_plane_state. Instead of this macro, you also have to set your own helpers for the plane's .reset and .atomic_duplicate_state There's again example code in the ssd130x driver.
>> Any attempt make to allocate request and response outside appletdrm_flush_damage seems to be breaking the driver.
>>
>> If I understand correctly, you want me to allocate them outside appletdrm_flush_damage, in appletbdrm_primary_plane_helper_atomic_check, return -ENOMEM if they fail. After that add kfree(return) and kfree(response) in appletbdrm_primary_plane_destroy_state.
>>
>> The ssd130x driver example isn’t really helping me. Could you please help me out here?
>
> What's the exact error message?
I'm not getting any error message. Modprobing the compiled driver simply gets stuck, or shows killed.
For the patch I sent yesterday, I got this in journalctl:
Feb 18 18:36:33 MacBook kernel: Call Trace:
Feb 18 18:36:33 MacBook kernel: <TASK>
Feb 18 18:36:33 MacBook kernel: ? show_regs+0x6c/0x80
Feb 18 18:36:33 MacBook kernel: ? __die+0x24/0x80
Feb 18 18:36:33 MacBook kernel: ? page_fault_oops+0x175/0x5c0
Feb 18 18:36:33 MacBook kernel: ? do_user_addr_fault+0x4b2/0x870
Feb 18 18:36:33 MacBook kernel: ? exc_page_fault+0x85/0x1c0
Feb 18 18:36:33 MacBook kernel: ? asm_exc_page_fault+0x27/0x30
Feb 18 18:36:33 MacBook kernel: ? drm_gem_fb_begin_cpu_access+0x5/0xc0
Feb 18 18:36:33 MacBook kernel: ? appletbdrm_primary_plane_helper_atomic_check+0x158/0x1b0 [appletbdrm]
Feb 18 18:36:33 MacBook kernel: drm_atomic_helper_check_planes+0xf6/0x250
Feb 18 18:36:33 MacBook kernel: drm_atomic_helper_check+0x51/0xa0
Feb 18 18:36:33 MacBook kernel: drm_atomic_check_only+0x688/0xb00
Feb 18 18:36:33 MacBook kernel: drm_atomic_commit+0x6f/0xf0
Feb 18 18:36:33 MacBook kernel: ? __pfx___drm_printfn_info+0x10/0x10
Feb 18 18:36:33 MacBook kernel: drm_mode_atomic_ioctl+0xc01/0xe60
Feb 18 18:36:33 MacBook kernel: ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
Feb 18 18:36:33 MacBook kernel: drm_ioctl_kernel+0xb6/0x120
Feb 18 18:36:33 MacBook kernel: drm_ioctl+0x2f3/0x5b0
Feb 18 18:36:33 MacBook kernel: ? __pfx_drm_mode_atomic_ioctl+0x10/0x10
Feb 18 18:36:33 MacBook kernel: __x64_sys_ioctl+0xa4/0xe0
Feb 18 18:36:33 MacBook kernel: x64_sys_call+0x131e/0x2650
Feb 18 18:36:33 MacBook kernel: do_syscall_64+0x7e/0x170
Feb 18 18:36:33 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
Feb 18 18:36:33 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
Feb 18 18:36:33 MacBook kernel: ? do_syscall_64+0x8a/0x170
Feb 18 18:36:33 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
Feb 18 18:36:33 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
Feb 18 18:36:33 MacBook kernel: ? do_syscall_64+0x8a/0x170
Feb 18 18:36:33 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
Feb 18 18:36:33 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
Feb 18 18:36:33 MacBook kernel: ? do_syscall_64+0x8a/0x170
Feb 18 18:36:33 MacBook kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e
Feb 18 18:36:33 MacBook kernel: RIP: 0033:0x5e684e58801b
Feb 18 18:36:33 MacBook kernel: Code: ff ff ff 4c 89 f7 e8 44 d0 f4 ff ff 15 1e a6 48 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 89 ff 89 f6 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 0f 9d c1 48 85 c0 0f 98 c2 20 ca 89 c1 c1 e1 10
Feb 18 18:36:33 MacBook kernel: RSP: 002b:00007fff91689798 EFLAGS: 00000206 ORIG_RAX: 0000000000000010
Feb 18 18:36:33 MacBook kernel: RAX: ffffffffffffffda RBX: 00007fff91689850 RCX: 00005e684e58801b
Feb 18 18:36:33 MacBook kernel: RDX: 00007fff916897a0 RSI: 00000000c03864bc RDI: 0000000000000004
Feb 18 18:36:33 MacBook kernel: RBP: 00005e68545f9570 R08: 00005e68545f9590 R09: 00005e68545f9750
Feb 18 18:36:33 MacBook kernel: R10: 00005e68545eb560 R11: 0000000000000206 R12: 00005e68545f9750
Feb 18 18:36:33 MacBook kernel: R13: 00005e68545f9590 R14: 00005e68545f97a0 R15: 0000000000000001
Feb 18 18:36:33 MacBook kernel: </TASK>
Btw, the *working* source code with changes approved by you is available over here:
https://github.com/AdityaGarg8/apple-touchbar-drv/blob/dfr/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>
> 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)
>
