Message-ID: <95bcb1d4-c8dd-4a30-a496-e7e54717f124@suse.de>
Date: Thu, 20 Feb 2025 11:20:11 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Aditya Garg <gargaditya08@...e.com>
Cc: "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
 "mripard@...nel.org" <mripard@...nel.org>,
 "airlied@...il.com" <airlied@...il.com>, "simona@...ll.ch"
 <simona@...ll.ch>, Kerem Karabay <kekrby@...il.com>,
 Atharva Tiwari <evepolonium@...il.com>,
 Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs

Hi,

to be honest: you are just throwing patches and errors at me and want me 
to fix your driver for you. If you want to maintain a kernel driver, you 
need to be able to debug at least such basic problems. I suggest you 
start to debug these issues by yourself and try to find their causes. 
(printk() will be helpful.)

Best regards
Thomas

Am 19.02.25 um 11:19 schrieb Aditya Garg:
> Hi
>
>> On 19 Feb 2025, at 3:19 PM, Thomas Zimmermann <tzimmermann@...e.de> 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@...e.com>
>>>>> 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)
>>

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

Powered by blists - more mailing lists