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

From: Thomas Zimmermann
Date: Wed Feb 19 2025 - 02:52:54 EST

Hi

Am 18.02.25 um 14:49 schrieb Aditya Garg:
[...]
This code runs in the middle of the atomic update. It's then too late to do error handling. If these allocs fail, the display hardware will remain in undefined state.

It is much preferable to allocate this memory in the atomic_check. To do so, you need a plane-state structure. Allocate the memory for request and response in the plane's atomic-check helper. If that fails, you can return an error there. Store the pointers and the request size in your plane-state structure and fetch them here. The memory should later be freed in the plane's destroy callback. For an example, see how the ssd130x driver treats a buffer for a similar use case at [1].

[1] https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/solomon/ssd130x.c#L1136
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.

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)