Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs

[Aditya Garg <gargaditya08@live.com>]

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@live.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?