* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-18 10:09 ` Thomas Zimmermann
@ 2025-02-18 13:49 ` Aditya Garg
2025-02-19 7:52 ` Thomas Zimmermann
2025-02-18 20:12 ` Aditya Garg
2025-02-20 10:11 ` Aditya Garg
2 siblings, 1 reply; 19+ messages in thread
From: Aditya Garg @ 2025-02-18 13:49 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
>> drivers/gpu/drm/tiny/appletbdrm.c | 702 ++++++++++++++++++++++++++++++
>
> You also have to add the driver to MAINTAINERS.
Sorry, forgot about that
>
>> 3 files changed, 716 insertions(+)
>> create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
>>
>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>> index 94cbdb133..76d1a2f9f 100644
>> --- a/drivers/gpu/drm/tiny/Kconfig
>> +++ b/drivers/gpu/drm/tiny/Kconfig
>> @@ -1,5 +1,18 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> +config DRM_APPLETBDRM
>> + tristate "DRM support for Apple Touch Bars"
>> + depends on DRM && USB && MMU
>> + select DRM_GEM_SHMEM_HELPER
>> + select DRM_KMS_HELPER
>> + select HID_APPLETB_BL
>> + help
>> + Say Y here if you want support for the display of Touch Bars on x86
>> + MacBook Pros.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called appletbdrm.
>> +
>> config DRM_ARCPGU
>> tristate "ARC PGU"
>> depends on DRM && OF
>> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
>> index 60816d2eb..0a3a7837a 100644
>> --- a/drivers/gpu/drm/tiny/Makefile
>> +++ b/drivers/gpu/drm/tiny/Makefile
>> @@ -1,5 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0-only
>> +obj-$(CONFIG_DRM_APPLETBDRM) += appletbdrm.o
>> obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o
>> obj-$(CONFIG_DRM_BOCHS) += bochs.o
>> obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus-qemu.o
>> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
>> new file mode 100644
>> index 000000000..f056f6280
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
>> @@ -0,0 +1,702 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Apple Touch Bar DRM Driver
>> + *
>> + * Copyright (c) 2023 Kerem Karabay <kekrby@gmail.com>
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#include <linux/module.h>
>> +#include <linux/unaligned.h>
>> +#include <linux/usb.h>
>> +
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_damage_helper.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_encoder.h>
>> +#include <drm/drm_format_helper.h>
>> +#include <drm/drm_fourcc.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/drm_plane.h>
>> +#include <drm/drm_probe_helper.h>
>> +
>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3]))
>> +#define __APPLETBDRM_MSG_TOK4(tok4) __APPLETBDRM_MSG_STR4(#tok4)
>> +
>> +#define APPLETBDRM_PIXEL_FORMAT __APPLETBDRM_MSG_TOK4(RGBA) /* The actual format is BGR888 */
>> +#define APPLETBDRM_BITS_PER_PIXEL 24
>> +
>> +#define APPLETBDRM_MSG_CLEAR_DISPLAY __APPLETBDRM_MSG_TOK4(CLRD)
>> +#define APPLETBDRM_MSG_GET_INFORMATION __APPLETBDRM_MSG_TOK4(GINF)
>> +#define APPLETBDRM_MSG_UPDATE_COMPLETE __APPLETBDRM_MSG_TOK4(UDCL)
>> +#define APPLETBDRM_MSG_SIGNAL_READINESS __APPLETBDRM_MSG_TOK4(REDY)
>> +
>> +#define APPLETBDRM_BULK_MSG_TIMEOUT 1000
>> +
>> +#define drm_to_adev(_drm) container_of(_drm, struct appletbdrm_device, drm)
>> +#define adev_to_udev(adev) interface_to_usbdev(to_usb_interface(adev->dev))
>> +
>> +struct appletbdrm_msg_request_header {
>> + __le16 unk_00;
>> + __le16 unk_02;
>> + __le32 unk_04;
>> + __le32 unk_08;
>> + __le32 size;
>> +} __packed;
>> +
>> +struct appletbdrm_msg_response_header {
>> + u8 unk_00[16];
>> + __le32 msg;
>> +} __packed;
>> +
>> +struct appletbdrm_msg_simple_request {
>> + struct appletbdrm_msg_request_header header;
>> + __le32 msg;
>> + u8 unk_14[8];
>> + __le32 size;
>> +} __packed;
>> +
>> +struct appletbdrm_msg_information {
>> + struct appletbdrm_msg_response_header header;
>> + u8 unk_14[12];
>> + __le32 width;
>> + __le32 height;
>> + u8 bits_per_pixel;
>> + __le32 bytes_per_row;
>> + __le32 orientation;
>> + __le32 bitmap_info;
>> + __le32 pixel_format;
>> + __le32 width_inches; /* floating point */
>> + __le32 height_inches; /* floating point */
>> +} __packed;
>> +
>> +struct appletbdrm_frame {
>> + __le16 begin_x;
>> + __le16 begin_y;
>> + __le16 width;
>> + __le16 height;
>> + __le32 buf_size;
>> + u8 buf[];
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request_footer {
>> + u8 unk_00[12];
>> + __le32 unk_0c;
>> + u8 unk_10[12];
>> + __le32 unk_1c;
>> + __le64 timestamp;
>> + u8 unk_28[12];
>> + __le32 unk_34;
>> + u8 unk_38[20];
>> + __le32 unk_4c;
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request {
>> + struct appletbdrm_msg_request_header header;
>> + __le16 unk_10;
>> + u8 msg_id;
>> + u8 unk_13[29];
>> + /*
>> + * Contents of `data`:
>> + * - struct appletbdrm_frame frames[];
>> + * - struct appletbdrm_fb_request_footer footer;
>> + * - padding to make the total size a multiple of 16
>> + */
>> + u8 data[];
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request_response {
>> + struct appletbdrm_msg_response_header header;
>> + u8 unk_14[12];
>> + __le64 timestamp;
>> +} __packed;
>> +
>> +struct appletbdrm_device {
>> + struct device *dev;
>> +
>> + unsigned int in_ep;
>> + unsigned int out_ep;
>> +
>> + unsigned int width;
>> + unsigned int height;
>> +
>> + struct drm_device drm;
>> + struct drm_display_mode mode;
>> + struct drm_connector connector;
>> + struct drm_plane primary_plane;
>> + struct drm_crtc crtc;
>> + struct drm_encoder encoder;
>> +};
>> +
>> +static int appletbdrm_send_request(struct appletbdrm_device *adev,
>> + struct appletbdrm_msg_request_header *request, size_t size)
>> +{
>> + struct usb_device *udev = adev_to_udev(adev);
>> + struct drm_device *drm = &adev->drm;
>> + int ret, actual_size;
>> +
>> + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, adev->out_ep),
>> + request, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
>> + if (ret) {
>> + drm_err(drm, "Failed to send message (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + if (actual_size != size) {
>> + drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
>> + actual_size, size);
>> + return -EIO;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_read_response(struct appletbdrm_device *adev,
>> + struct appletbdrm_msg_response_header *response,
>> + size_t size, u32 expected_response)
>> +{
>> + struct usb_device *udev = adev_to_udev(adev);
>> + struct drm_device *drm = &adev->drm;
>> + int ret, actual_size;
>> + bool readiness_signal_received = false;
>> +
>> +retry:
>> + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, adev->in_ep),
>> + response, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
>> + if (ret) {
>> + drm_err(drm, "Failed to read response (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + /*
>> + * The device responds to the first request sent in a particular
>> + * timeframe after the USB device configuration is set with a readiness
>> + * signal, in which case the response should be read again
>> + */
>> + if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
>> + if (!readiness_signal_received) {
>> + readiness_signal_received = true;
>> + goto retry;
>> + }
>> +
>> + drm_err(drm, "Encountered unexpected readiness signal\n");
>> + return -EIO;
>> + }
>> +
>> + if (actual_size != size) {
>> + drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
>> + actual_size, size);
>> + return -EIO;
>> + }
>> +
>> + if (response->msg != expected_response) {
>> + drm_err(drm, "Unexpected response from device (expected %p4ch found %p4ch)\n",
>> + &expected_response, &response->msg);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
>> +{
>> + struct appletbdrm_msg_simple_request *request;
>> + int ret;
>> +
>> + request = kzalloc(sizeof(*request), GFP_KERNEL);
>> + if (!request)
>> + return -ENOMEM;
>> +
>> + request->header.unk_00 = cpu_to_le16(2);
>> + request->header.unk_02 = cpu_to_le16(0x1512);
>> + request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
>> + request->msg = msg;
>> + request->size = request->header.size;
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
>> +
>> + kfree(request);
>
> This is temporary data for the send operation and save to free here?
>
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_clear_display(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_CLEAR_DISPLAY);
>> +}
>> +
>> +static int appletbdrm_signal_readiness(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_SIGNAL_READINESS);
>> +}
>> +
>> +static int appletbdrm_get_information(struct appletbdrm_device *adev)
>> +{
>> + struct appletbdrm_msg_information *info;
>> + struct drm_device *drm = &adev->drm;
>> + u8 bits_per_pixel;
>> + u32 pixel_format;
>> + int ret;
>> +
>> + info = kzalloc(sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + return ret;
>> +
>> + ret = appletbdrm_read_response(adev, &info->header, sizeof(*info),
>> + APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + goto free_info;
>> +
>> + bits_per_pixel = info->bits_per_pixel;
>> + pixel_format = get_unaligned(&info->pixel_format);
>> +
>> + adev->width = get_unaligned_le32(&info->width);
>> + adev->height = get_unaligned_le32(&info->height);
>> +
>> + if (bits_per_pixel != APPLETBDRM_BITS_PER_PIXEL) {
>> + drm_err(drm, "Encountered unexpected bits per pixel value (%d)\n", bits_per_pixel);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> + if (pixel_format != APPLETBDRM_PIXEL_FORMAT) {
>> + drm_err(drm, "Encountered unknown pixel format (%p4ch)\n", &pixel_format);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> +free_info:
>> + kfree(info);
>> +
>> + return ret;
>> +}
>> +
>> +static u32 rect_size(struct drm_rect *rect)
>> +{
>> + return drm_rect_width(rect) * drm_rect_height(rect) * (APPLETBDRM_BITS_PER_PIXEL / 8);
>> +}
>> +
>> +static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>> + struct drm_plane_state *old_state,
>> + struct drm_plane_state *state)
>> +{
>> + struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>> + struct appletbdrm_fb_request_response *response;
>> + struct appletbdrm_fb_request_footer *footer;
>> + struct drm_atomic_helper_damage_iter iter;
>> + struct drm_framebuffer *fb = state->fb;
>> + struct appletbdrm_fb_request *request;
>> + struct drm_device *drm = &adev->drm;
>> + struct appletbdrm_frame *frame;
>> + u64 timestamp = ktime_get_ns();
>> + struct drm_rect damage;
>> + size_t frames_size = 0;
>> + size_t request_size;
>> + int ret;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + frames_size += struct_size(frame, buf, rect_size(&damage));
>> + }
>> +
>> + 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;
>> + }
>
> 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@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,
};
--
2.43.0
—>8--
>
>
>> +
>> + 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);
>> + request->header.size = cpu_to_le32(request_size - sizeof(request->header));
>> + request->unk_10 = cpu_to_le16(1);
>> + request->msg_id = timestamp & 0xff;
>> +
>> + frame = (struct appletbdrm_frame *)request->data;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + struct iosys_map dst = IOSYS_MAP_INIT_VADDR(frame->buf);
>> + u32 buf_size = rect_size(&damage);
>> +
>> + /*
>> + * The coordinates need to be translated to the coordinate
>> + * system the device expects, see the comment in
>> + * appletbdrm_setup_mode_config
>> + */
>> + frame->begin_x = cpu_to_le16(damage.y1);
>> + frame->begin_y = cpu_to_le16(adev->height - damage.x2);
>> + frame->width = cpu_to_le16(drm_rect_height(&damage));
>> + frame->height = cpu_to_le16(drm_rect_width(&damage));
>> + frame->buf_size = cpu_to_le32(buf_size);
>> +
>> + ret = drm_fb_blit(&dst, NULL, DRM_FORMAT_BGR888,
>> + &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
>
> Can we void the use of drm_fb_blit()? Since you know all formats in advance, just do
>
> switch (format)
> case XRGB8888: drm_fb_xrgb888_to_bgr888() break default:
> drm_fb_memcpy() break }We use blit in simpledrm and ofdrm, where we don't know the formats and output buffers in advance. But it's really not so great in other drivers, I think.
>> + if (ret) {
>> + drm_err(drm, "Failed to copy damage clip (%d)\n", ret);
>> + goto end_fb_cpu_access;
>> + }
>> +
>> + frame = (void *)frame + struct_size(frame, buf, buf_size);
>> + }
>> +
>> + footer = (struct appletbdrm_fb_request_footer *)&request->data[frames_size];
>> +
>> + footer->unk_0c = cpu_to_le32(0xfffe);
>> + footer->unk_1c = cpu_to_le32(0x80001);
>> + footer->unk_34 = cpu_to_le32(0x80002);
>> + footer->unk_4c = cpu_to_le32(0xffff);
>> + footer->timestamp = cpu_to_le64(timestamp);
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, request_size);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + ret = appletbdrm_read_response(adev, &response->header, sizeof(*response),
>> + APPLETBDRM_MSG_UPDATE_COMPLETE);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + if (response->timestamp != footer->timestamp) {
>> + drm_err(drm, "Response timestamp (%llu) doesn't match request timestamp (%llu)\n",
>> + le64_to_cpu(response->timestamp), timestamp);
>> + goto end_fb_cpu_access;
>> + }
>> +
>> +end_fb_cpu_access:
>> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> +free_response:
>> + kfree(response);
>> +free_request:
>> + kfree(request);
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(connector->dev);
>> +
>> + return drm_connector_helper_get_modes_fixed(connector, &adev->mode);
>> +}
>> +
>> +static const u32 appletbdrm_primary_plane_formats[] = {
>> + DRM_FORMAT_BGR888,
>> + DRM_FORMAT_XRGB8888, /* emulated */
>> +};
>> +
>> +static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> + struct drm_atomic_state *state)
>> +{
>> + 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;
>> + int ret;
>> +
>> + if (new_crtc)
>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>> +
>> + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> + DRM_PLANE_NO_SCALING,
>> + DRM_PLANE_NO_SCALING,
>> + false, false);
>> + if (ret)
>> + return ret;
>> + else if (!new_plane_state->visible)
>> + return 0;
>> +
>> + return 0;
>> +}
>> +
>> +static void appletbdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>> + struct drm_atomic_state *old_state)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(plane->dev);
>> + struct drm_device *drm = plane->dev;
>> + struct drm_plane_state *plane_state = plane->state;
>> + struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
>> + int idx;
>
> plane_state->fb can be NULL if the plane's atomic_disable is not set. At a minimum you should check this first and return if so.
>
> A better idea is to implement atomic_disable. You can try to move the code from appletbdrm_crtc_helper_atomic_disable() to the plane's atomic disable and remove the CRTC helper. That would put all drawing code into the plane. The CRTC is more about programming a display mode.
Code will be moved to plane’s atomic disable in next version
>> +
>> + if (!drm_dev_enter(drm, &idx))
>> + return;
>> +
>> + appletbdrm_flush_damage(adev, old_plane_state, plane_state);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +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,
>> + .atomic_update = appletbdrm_primary_plane_helper_atomic_update,
>> +};
>> +
>> +static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = {
>> + .update_plane = drm_atomic_helper_update_plane,
>> + .disable_plane = drm_atomic_helper_disable_plane,
>> + .destroy = drm_plane_cleanup,
>> + DRM_GEM_SHADOW_PLANE_FUNCS,
>> +};
>> +
>> +static enum drm_mode_status appletbdrm_crtc_helper_mode_valid(struct drm_crtc *crtc,
>> + const struct drm_display_mode *mode)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> +
>> + return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
>> +}
>> +
>> +static void appletbdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> + struct drm_atomic_state *crtc_state)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> + int idx;
>> +
>> + if (!drm_dev_enter(&adev->drm, &idx))
>> + return;
>> +
>> + appletbdrm_clear_display(adev);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +static const struct drm_mode_config_funcs appletbdrm_mode_config_funcs = {
>> + .fb_create = drm_gem_fb_create_with_dirty,
>> + .atomic_check = drm_atomic_helper_check,
>> + .atomic_commit = drm_atomic_helper_commit,
>> +};
>> +
>> +static const struct drm_connector_funcs appletbdrm_connector_funcs = {
>> + .reset = drm_atomic_helper_connector_reset,
>> + .destroy = drm_connector_cleanup,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +};
>> +
>> +static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs = {
>> + .get_modes = appletbdrm_connector_helper_get_modes,
>> +};
>> +
>> +static const struct drm_crtc_helper_funcs appletbdrm_crtc_helper_funcs = {
>> + .mode_valid = appletbdrm_crtc_helper_mode_valid,
>> + .atomic_disable = appletbdrm_crtc_helper_atomic_disable,
>> +};
>> +
>> +static const struct drm_crtc_funcs appletbdrm_crtc_funcs = {
>> + .reset = drm_atomic_helper_crtc_reset,
>> + .destroy = drm_crtc_cleanup,
>> + .set_config = drm_atomic_helper_set_config,
>> + .page_flip = drm_atomic_helper_page_flip,
>> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>> +};
>> +
>> +static const struct drm_encoder_funcs appletbdrm_encoder_funcs = {
>> + .destroy = drm_encoder_cleanup,
>> +};
>> +
>> +DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>> +
>> +static const struct drm_driver appletbdrm_drm_driver = {
>> + DRM_GEM_SHMEM_DRIVER_OPS,
>
> For USB devices, we need special wiring to make PRIME work. The PRIME device must support DMA, but a USB device doesn't. So we pass the USB controller device instead. See [2] for what udl does and how it obtains dmadev.
>
> [2] https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/udl/udl_drv.c#L76
Basically you want me to simply add .gem_prime_import = udl_driver_gem_prime_import, here right?
>
>> + .name = "appletbdrm",
>> + .desc = "Apple Touch Bar DRM Driver",
>> + .date = "20230910",
>
> The date field has been removed recently.
Will be removed
>
>> + .major = 1,
>> + .minor = 0,
>> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>> + .fops = &appletbdrm_drm_fops,
>> +};
>> +
>> +static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>> +{
>> + struct drm_connector *connector = &adev->connector;
>> + struct drm_plane *primary_plane;
>> + struct drm_crtc *crtc;
>> + struct drm_encoder *encoder;
>> + struct drm_device *drm = &adev->drm;
>> + struct device *dev = adev->dev;
>> + int ret;
>> +
>> + ret = drmm_mode_config_init(drm);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
>> +
>> + primary_plane = &adev->primary_plane;
>> + ret = drm_universal_plane_init(drm, primary_plane, 0,
>> + &appletbdrm_primary_plane_funcs,
>> + appletbdrm_primary_plane_formats,
>> + ARRAY_SIZE(appletbdrm_primary_plane_formats),
>> + NULL,
>> + DRM_PLANE_TYPE_PRIMARY, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize universal plane object\n");
>> + drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
>> + drm_plane_enable_fb_damage_clips(primary_plane);
>> +
>> + crtc = &adev->crtc;
>> + ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
>> + &appletbdrm_crtc_funcs, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize CRTC object\n");
>> + drm_crtc_helper_add(crtc, &appletbdrm_crtc_helper_funcs);
>> +
>> + encoder = &adev->encoder;
>> + ret = drm_encoder_init(drm, encoder, &appletbdrm_encoder_funcs,
>> + DRM_MODE_ENCODER_DAC, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize encoder\n");
>> + encoder->possible_crtcs = drm_crtc_mask(crtc);
>> +
>> + /*
>> + * The coordinate system used by the device is different from the
>> + * coordinate system of the framebuffer in that the x and y axes are
>> + * swapped, and that the y axis is inverted; so what the device reports
>> + * as the height is actually the width of the framebuffer and vice
>> + * versa
>> + */
>> + drm->mode_config.min_width = 0;
>> + drm->mode_config.min_height = 0;
>
>> + drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
>> + drm->mode_config.max_height = max(adev->width, DRM_SHADOW_PLANE_MAX_HEIGHT);
>
> This might not work correctly. To use those SHADOW_PLANE_MAX constants, you have to fixup the damage coordinates to account for changes to the plane coordinates. Simpledrm does that by intersecting damage with dst_clip. [3] You also have to put the result at the right place within the display.
>
> [3] https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/tiny/simpledrm.c#L646
>
>> + drm->mode_config.preferred_depth = APPLETBDRM_BITS_PER_PIXEL;
>> + drm->mode_config.funcs = &appletbdrm_mode_config_funcs;
>> +
>> + adev->mode = (struct drm_display_mode) {
>> + DRM_MODE_INIT(60, adev->height, adev->width,
>> + DRM_MODE_RES_MM(adev->height, 218),
>> + DRM_MODE_RES_MM(adev->width, 218))
>> + };
>> +
>> + ret = drm_connector_init(drm, connector,
>> + &appletbdrm_connector_funcs, DRM_MODE_CONNECTOR_USB);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize connector\n");
>> +
>> + drm_connector_helper_add(connector, &appletbdrm_connector_helper_funcs);
>> +
>> + ret = drm_connector_set_panel_orientation(connector,
>> + DRM_MODE_PANEL_ORIENTATION_RIGHT_UP);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set panel orientation\n");
>> +
>> + connector->display_info.non_desktop = true;
>> + ret = drm_object_property_set_value(&connector->base,
>> + drm->mode_config.non_desktop_property, true);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>> +
>> + ret = drm_connector_attach_encoder(connector, encoder);
>> +
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>> +
>> + drm_mode_config_reset(drm);
>> +
>
>
>> + ret = drm_dev_register(drm, 0);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>
> This call does not belong to the mode-setting pipeline and belongs into appletbdrm_probe().
>
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_probe(struct usb_interface *intf,
>> + const struct usb_device_id *id)
>> +{
>> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>> + struct device *dev = &intf->dev;
>> + struct appletbdrm_device *adev;
>> + int ret;
>> +
>> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
>> +
>> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
>> + if (IS_ERR(adev))
>> + return PTR_ERR(adev);
>> +
>> + adev->dev = dev;
>> + adev->in_ep = bulk_in->bEndpointAddress;
>> + adev->out_ep = bulk_out->bEndpointAddress;
>> +
>> + usb_set_intfdata(intf, adev);
>
> Rather set the DRM device here and fetch it in the callbacks below. At some point, we might be able to share some of those helpers among drivers.
>
>> +
>> + ret = appletbdrm_get_information(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to get display information\n");
>> +
>> + ret = appletbdrm_signal_readiness(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to signal readiness\n");
>> +
>
>> + ret = appletbdrm_clear_display(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to clear display\n");
>
> Clearing the display is not something usually done in probe. But I guess there's no better place. I'd do this as the final call in probe; after registering the device. That way, it acts a bit like an in-kernel DRM client.
>
> Best regards
> Thomas
>
>> +
>> + return appletbdrm_setup_mode_config(adev);
>> +}
>> +
>> +static void appletbdrm_disconnect(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> + struct drm_device *drm = &adev->drm;
>> +
>> + drm_dev_unplug(drm);
>> + drm_atomic_helper_shutdown(drm);
>> +}
>> +
>> +static void appletbdrm_shutdown(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> +
>> + /*
>> + * The framebuffer needs to be cleared on shutdown since its content
>> + * persists across boots
>> + */
>> + drm_atomic_helper_shutdown(&adev->drm);
>> +}
>> +
>> +static const struct usb_device_id appletbdrm_usb_id_table[] = {
>> + { USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
>> +
>> +static struct usb_driver appletbdrm_usb_driver = {
>> + .name = "appletbdrm",
>> + .probe = appletbdrm_probe,
>> + .disconnect = appletbdrm_disconnect,
>> + .shutdown = appletbdrm_shutdown,
>> + .id_table = appletbdrm_usb_id_table,
>> +};
>> +module_usb_driver(appletbdrm_usb_driver);
>> +
>> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
>> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
>> +MODULE_LICENSE("GPL");
>
> --
> --
> 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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-18 13:49 ` Aditya Garg
@ 2025-02-19 7:52 ` Thomas Zimmermann
2025-02-19 9:37 ` Aditya Garg
0 siblings, 1 reply; 19+ messages in thread
From: Thomas Zimmermann @ 2025-02-19 7:52 UTC (permalink / raw)
To: Aditya Garg
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
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@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.
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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-19 7:52 ` Thomas Zimmermann
@ 2025-02-19 9:37 ` Aditya Garg
2025-02-19 9:49 ` Thomas Zimmermann
0 siblings, 1 reply; 19+ messages in thread
From: Aditya Garg @ 2025-02-19 9:37 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
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?
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-19 9:37 ` Aditya Garg
@ 2025-02-19 9:49 ` Thomas Zimmermann
2025-02-19 10:19 ` Aditya Garg
0 siblings, 1 reply; 19+ messages in thread
From: Thomas Zimmermann @ 2025-02-19 9:49 UTC (permalink / raw)
To: Aditya Garg
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
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@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?
What's the exact error message?
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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-19 9:49 ` Thomas Zimmermann
@ 2025-02-19 10:19 ` Aditya Garg
2025-02-20 10:20 ` Thomas Zimmermann
0 siblings, 1 reply; 19+ messages in thread
From: Aditya Garg @ 2025-02-19 10:19 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
> On 19 Feb 2025, at 3:19 PM, Thomas Zimmermann <tzimmermann@suse.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@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?
>
> 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)
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-19 10:19 ` Aditya Garg
@ 2025-02-20 10:20 ` Thomas Zimmermann
2025-02-20 11:00 ` Aditya Garg
0 siblings, 1 reply; 19+ messages in thread
From: Thomas Zimmermann @ 2025-02-20 10:20 UTC (permalink / raw)
To: Aditya Garg
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
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@suse.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@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?
>> 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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-20 10:20 ` Thomas Zimmermann
@ 2025-02-20 11:00 ` Aditya Garg
2025-02-20 16:44 ` Aditya Garg
0 siblings, 1 reply; 19+ messages in thread
From: Aditya Garg @ 2025-02-20 11:00 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
> On 20 Feb 2025, at 3:50 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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.)
The thing is I am still new to DRM code and APIs, it's my first driver (and probably the last). I actually thought I could learn more here, but I'll try to find solutions myself now.
I'll send a v2 once all the issues are addressed
Thanks
Aditya
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-20 11:00 ` Aditya Garg
@ 2025-02-20 16:44 ` Aditya Garg
0 siblings, 0 replies; 19+ messages in thread
From: Aditya Garg @ 2025-02-20 16:44 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
>
> The thing is I am still new to DRM code and APIs, it's my first driver (and probably the last). I actually thought I could learn more here, but I'll try to find solutions myself now.
>
> I'll send a v2 once all the issues are addressed
>
V2 with all the problems pointed out solved (hopefully) sent here:
https://lore.kernel.org/dri-devel/716BCB0A-785B-463A-86C2-94BD66D5D22E@live.com/T/#t
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-18 10:09 ` Thomas Zimmermann
2025-02-18 13:49 ` Aditya Garg
@ 2025-02-18 20:12 ` Aditya Garg
2025-02-19 7:57 ` Thomas Zimmermann
2025-02-20 10:11 ` Aditya Garg
2 siblings, 1 reply; 19+ messages in thread
From: Aditya Garg @ 2025-02-18 20:12 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
In continuation to my previous mail.
>> +
>> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
>> +{
>> + struct appletbdrm_msg_simple_request *request;
>> + int ret;
>> +
>> + request = kzalloc(sizeof(*request), GFP_KERNEL);
>> + if (!request)
>> + return -ENOMEM;
>> +
>> + request->header.unk_00 = cpu_to_le16(2);
>> + request->header.unk_02 = cpu_to_le16(0x1512);
>> + request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
>> + request->msg = msg;
>> + request->size = request->header.size;
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
>> +
>> + kfree(request);
>
> This is temporary data for the send operation and save to free here?
Probably yes. If I understand correctly, it’s needed to make the touchbar go into the display mode, from the hid keyboard mode.
We here are doing the same as the Windows driver [1] for this does.
[1] https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/include/Dfr.h#L3
>
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_clear_display(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_CLEAR_DISPLAY);
>> +}
>> +
>> +static int appletbdrm_signal_readiness(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_SIGNAL_READINESS);
>> +}
>> +
>> +static int appletbdrm_get_information(struct appletbdrm_device *adev)
>> +{
>> + struct appletbdrm_msg_information *info;
>> + struct drm_device *drm = &adev->drm;
>> + u8 bits_per_pixel;
>> + u32 pixel_format;
>> + int ret;
>> +
>> + info = kzalloc(sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + return ret;
>> +
>> + ret = appletbdrm_read_response(adev, &info->header, sizeof(*info),
>> + APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + goto free_info;
>> +
>> + bits_per_pixel = info->bits_per_pixel;
>> + pixel_format = get_unaligned(&info->pixel_format);
>> +
>> + adev->width = get_unaligned_le32(&info->width);
>> + adev->height = get_unaligned_le32(&info->height);
>> +
>> + if (bits_per_pixel != APPLETBDRM_BITS_PER_PIXEL) {
>> + drm_err(drm, "Encountered unexpected bits per pixel value (%d)\n", bits_per_pixel);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> + if (pixel_format != APPLETBDRM_PIXEL_FORMAT) {
>> + drm_err(drm, "Encountered unknown pixel format (%p4ch)\n", &pixel_format);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> +free_info:
>> + kfree(info);
>> +
>> + return ret;
>> +}
>> +
>> +static u32 rect_size(struct drm_rect *rect)
>> +{
>> + return drm_rect_width(rect) * drm_rect_height(rect) * (APPLETBDRM_BITS_PER_PIXEL / 8);
>> +}
>> +
>> +static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>> + struct drm_plane_state *old_state,
>> + struct drm_plane_state *state)
>> +{
>> + struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>> + struct appletbdrm_fb_request_response *response;
>> + struct appletbdrm_fb_request_footer *footer;
>> + struct drm_atomic_helper_damage_iter iter;
>> + struct drm_framebuffer *fb = state->fb;
>> + struct appletbdrm_fb_request *request;
>> + struct drm_device *drm = &adev->drm;
>> + struct appletbdrm_frame *frame;
>> + u64 timestamp = ktime_get_ns();
>> + struct drm_rect damage;
>> + size_t frames_size = 0;
>> + size_t request_size;
>> + int ret;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + frames_size += struct_size(frame, buf, rect_size(&damage));
>> + }
>> +
>> + 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;
>> + }
>
> 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
>
>
>> +
>> + 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);
>> + request->header.size = cpu_to_le32(request_size - sizeof(request->header));
>> + request->unk_10 = cpu_to_le16(1);
>> + request->msg_id = timestamp & 0xff;
>> +
>> + frame = (struct appletbdrm_frame *)request->data;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + struct iosys_map dst = IOSYS_MAP_INIT_VADDR(frame->buf);
>> + u32 buf_size = rect_size(&damage);
>> +
>> + /*
>> + * The coordinates need to be translated to the coordinate
>> + * system the device expects, see the comment in
>> + * appletbdrm_setup_mode_config
>> + */
>> + frame->begin_x = cpu_to_le16(damage.y1);
>> + frame->begin_y = cpu_to_le16(adev->height - damage.x2);
>> + frame->width = cpu_to_le16(drm_rect_height(&damage));
>> + frame->height = cpu_to_le16(drm_rect_width(&damage));
>> + frame->buf_size = cpu_to_le32(buf_size);
>> +
>> + ret = drm_fb_blit(&dst, NULL, DRM_FORMAT_BGR888,
>> + &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
>
> Can we void the use of drm_fb_blit()? Since you know all formats in advance, just do
>
> switch (format)
> case XRGB8888: drm_fb_xrgb888_to_bgr888() break default:
> drm_fb_memcpy() break }We use blit in simpledrm and ofdrm, where we don't know the formats and output buffers in advance. But it's really not so great in other drivers, I think.
I think you mean this:
#include <drm/drm_framebuffer.h>
switch (fb->format->format) {
case DRM_FORMAT_XRGB8888:
drm_fb_xrgb8888_to_bgr888(&dst, NULL, &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
break;
default:
drm_fb_memcpy(&dst, NULL, &shadow_plane_state->data[0], fb, &damage);
break;
}
>> + if (ret) {
>> + drm_err(drm, "Failed to copy damage clip (%d)\n", ret);
>> + goto end_fb_cpu_access;
>> + }
>> +
>> + frame = (void *)frame + struct_size(frame, buf, buf_size);
>> + }
>> +
>> + footer = (struct appletbdrm_fb_request_footer *)&request->data[frames_size];
>> +
>> + footer->unk_0c = cpu_to_le32(0xfffe);
>> + footer->unk_1c = cpu_to_le32(0x80001);
>> + footer->unk_34 = cpu_to_le32(0x80002);
>> + footer->unk_4c = cpu_to_le32(0xffff);
>> + footer->timestamp = cpu_to_le64(timestamp);
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, request_size);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + ret = appletbdrm_read_response(adev, &response->header, sizeof(*response),
>> + APPLETBDRM_MSG_UPDATE_COMPLETE);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + if (response->timestamp != footer->timestamp) {
>> + drm_err(drm, "Response timestamp (%llu) doesn't match request timestamp (%llu)\n",
>> + le64_to_cpu(response->timestamp), timestamp);
>> + goto end_fb_cpu_access;
>> + }
>> +
>> +end_fb_cpu_access:
>> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> +free_response:
>> + kfree(response);
>> +free_request:
>> + kfree(request);
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(connector->dev);
>> +
>> + return drm_connector_helper_get_modes_fixed(connector, &adev->mode);
>> +}
>> +
>> +static const u32 appletbdrm_primary_plane_formats[] = {
>> + DRM_FORMAT_BGR888,
>> + DRM_FORMAT_XRGB8888, /* emulated */
>> +};
>> +
>> +static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> + struct drm_atomic_state *state)
>> +{
>> + 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;
>> + int ret;
>> +
>> + if (new_crtc)
>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>> +
>> + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> + DRM_PLANE_NO_SCALING,
>> + DRM_PLANE_NO_SCALING,
>> + false, false);
>> + if (ret)
>> + return ret;
>> + else if (!new_plane_state->visible)
>> + return 0;
>> +
>> + return 0;
>> +}
>> +
>> +static void appletbdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>> + struct drm_atomic_state *old_state)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(plane->dev);
>> + struct drm_device *drm = plane->dev;
>> + struct drm_plane_state *plane_state = plane->state;
>> + struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
>> + int idx;
>
> plane_state->fb can be NULL if the plane's atomic_disable is not set. At a minimum you should check this first and return if so.
>
> A better idea is to implement atomic_disable. You can try to move the code from appletbdrm_crtc_helper_atomic_disable() to the plane's atomic disable and remove the CRTC helper. That would put all drawing code into the plane. The CRTC is more about programming a display mode.
>> +
>> + if (!drm_dev_enter(drm, &idx))
>> + return;
>> +
>> + appletbdrm_flush_damage(adev, old_plane_state, plane_state);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +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,
>> + .atomic_update = appletbdrm_primary_plane_helper_atomic_update,
>> +};
>> +
>> +static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = {
>> + .update_plane = drm_atomic_helper_update_plane,
>> + .disable_plane = drm_atomic_helper_disable_plane,
>> + .destroy = drm_plane_cleanup,
>> + DRM_GEM_SHADOW_PLANE_FUNCS,
>> +};
>> +
>> +static enum drm_mode_status appletbdrm_crtc_helper_mode_valid(struct drm_crtc *crtc,
>> + const struct drm_display_mode *mode)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> +
>> + return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
>> +}
>> +
>> +static void appletbdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> + struct drm_atomic_state *crtc_state)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> + int idx;
>> +
>> + if (!drm_dev_enter(&adev->drm, &idx))
>> + return;
>> +
>> + appletbdrm_clear_display(adev);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +static const struct drm_mode_config_funcs appletbdrm_mode_config_funcs = {
>> + .fb_create = drm_gem_fb_create_with_dirty,
>> + .atomic_check = drm_atomic_helper_check,
>> + .atomic_commit = drm_atomic_helper_commit,
>> +};
>> +
>> +static const struct drm_connector_funcs appletbdrm_connector_funcs = {
>> + .reset = drm_atomic_helper_connector_reset,
>> + .destroy = drm_connector_cleanup,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +};
>> +
>> +static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs = {
>> + .get_modes = appletbdrm_connector_helper_get_modes,
>> +};
>> +
>> +static const struct drm_crtc_helper_funcs appletbdrm_crtc_helper_funcs = {
>> + .mode_valid = appletbdrm_crtc_helper_mode_valid,
>> + .atomic_disable = appletbdrm_crtc_helper_atomic_disable,
>> +};
>> +
>> +static const struct drm_crtc_funcs appletbdrm_crtc_funcs = {
>> + .reset = drm_atomic_helper_crtc_reset,
>> + .destroy = drm_crtc_cleanup,
>> + .set_config = drm_atomic_helper_set_config,
>> + .page_flip = drm_atomic_helper_page_flip,
>> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>> +};
>> +
>> +static const struct drm_encoder_funcs appletbdrm_encoder_funcs = {
>> + .destroy = drm_encoder_cleanup,
>> +};
>> +
>> +DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>> +
>> +static const struct drm_driver appletbdrm_drm_driver = {
>> + DRM_GEM_SHMEM_DRIVER_OPS,
>
> For USB devices, we need special wiring to make PRIME work. The PRIME device must support DMA, but a USB device doesn't. So we pass the USB controller device instead. See [2] for what udl does and how it obtains dmadev.
>
> [2] https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/udl/udl_drv.c#L76
Disregard my previous reply for this. I believe you meant by this?:
—>8—
From b6fda730995b7f28374c1ff38778a6f3e6da65da Mon Sep 17 00:00:00 2001
From: Aditya Garg <gargaditya08@live.com>
Date: Tue, 18 Feb 2025 22:47:44 +0530
Subject: [PATCH] prime
---
drivers/gpu/drm/tiny/appletbdrm.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
index f2d911325..b835063c2 100644
--- a/drivers/gpu/drm/tiny/appletbdrm.c
+++ b/drivers/gpu/drm/tiny/appletbdrm.c
@@ -118,6 +118,7 @@ struct appletbdrm_fb_request_response {
struct appletbdrm_device {
struct device *dev;
+ struct device *dmadev;
unsigned int in_ep;
unsigned int out_ep;
@@ -521,10 +522,22 @@ static const struct drm_encoder_funcs appletbdrm_encoder_funcs = {
.destroy = drm_encoder_cleanup,
};
+static struct drm_gem_object *appletbdrm_driver_gem_prime_import(struct drm_device *dev,
+ struct dma_buf *dma_buf)
+{
+ struct appletbdrm_device *adev = drm_to_adev(dev);
+
+ if (!adev->dmadev)
+ return ERR_PTR(-ENODEV);
+
+ return drm_gem_prime_import_dev(dev, dma_buf, adev->dmadev);
+}
+
DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
static const struct drm_driver appletbdrm_drm_driver = {
DRM_GEM_SHMEM_DRIVER_OPS,
+ .gem_prime_import = appletbdrm_driver_gem_prime_import,
.name = "appletbdrm",
.desc = "Apple Touch Bar DRM Driver",
.major = 1,
--
2.43.0
—>8--
>
>> + .name = "appletbdrm",
>> + .desc = "Apple Touch Bar DRM Driver",
>> + .date = "20230910",
>
> The date field has been removed recently.
>
>> + .major = 1,
>> + .minor = 0,
>> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>> + .fops = &appletbdrm_drm_fops,
>> +};
>> +
>> +static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>> +{
>> + struct drm_connector *connector = &adev->connector;
>> + struct drm_plane *primary_plane;
>> + struct drm_crtc *crtc;
>> + struct drm_encoder *encoder;
>> + struct drm_device *drm = &adev->drm;
>> + struct device *dev = adev->dev;
>> + int ret;
>> +
>> + ret = drmm_mode_config_init(drm);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
>> +
>> + primary_plane = &adev->primary_plane;
>> + ret = drm_universal_plane_init(drm, primary_plane, 0,
>> + &appletbdrm_primary_plane_funcs,
>> + appletbdrm_primary_plane_formats,
>> + ARRAY_SIZE(appletbdrm_primary_plane_formats),
>> + NULL,
>> + DRM_PLANE_TYPE_PRIMARY, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize universal plane object\n");
>> + drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
>> + drm_plane_enable_fb_damage_clips(primary_plane);
>> +
>> + crtc = &adev->crtc;
>> + ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
>> + &appletbdrm_crtc_funcs, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize CRTC object\n");
>> + drm_crtc_helper_add(crtc, &appletbdrm_crtc_helper_funcs);
>> +
>> + encoder = &adev->encoder;
>> + ret = drm_encoder_init(drm, encoder, &appletbdrm_encoder_funcs,
>> + DRM_MODE_ENCODER_DAC, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize encoder\n");
>> + encoder->possible_crtcs = drm_crtc_mask(crtc);
>> +
>> + /*
>> + * The coordinate system used by the device is different from the
>> + * coordinate system of the framebuffer in that the x and y axes are
>> + * swapped, and that the y axis is inverted; so what the device reports
>> + * as the height is actually the width of the framebuffer and vice
>> + * versa
>> + */
>> + drm->mode_config.min_width = 0;
>> + drm->mode_config.min_height = 0;
>
>> + drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
>> + drm->mode_config.max_height = max(adev->width, DRM_SHADOW_PLANE_MAX_HEIGHT);
>
> This might not work correctly. To use those SHADOW_PLANE_MAX constants, you have to fixup the damage coordinates to account for changes to the plane coordinates. Simpledrm does that by intersecting damage with dst_clip. [3] You also have to put the result at the right place within the display.
>
> [3] https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/tiny/simpledrm.c#L646
This one is confusing for me. Could you clarify a bit more here?
>
>> + drm->mode_config.preferred_depth = APPLETBDRM_BITS_PER_PIXEL;
>> + drm->mode_config.funcs = &appletbdrm_mode_config_funcs;
>> +
>> + adev->mode = (struct drm_display_mode) {
>> + DRM_MODE_INIT(60, adev->height, adev->width,
>> + DRM_MODE_RES_MM(adev->height, 218),
>> + DRM_MODE_RES_MM(adev->width, 218))
>> + };
>> +
>> + ret = drm_connector_init(drm, connector,
>> + &appletbdrm_connector_funcs, DRM_MODE_CONNECTOR_USB);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize connector\n");
>> +
>> + drm_connector_helper_add(connector, &appletbdrm_connector_helper_funcs);
>> +
>> + ret = drm_connector_set_panel_orientation(connector,
>> + DRM_MODE_PANEL_ORIENTATION_RIGHT_UP);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set panel orientation\n");
>> +
>> + connector->display_info.non_desktop = true;
>> + ret = drm_object_property_set_value(&connector->base,
>> + drm->mode_config.non_desktop_property, true);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>> +
>> + ret = drm_connector_attach_encoder(connector, encoder);
>> +
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>> +
>> + drm_mode_config_reset(drm);
>> +
>
>
>> + ret = drm_dev_register(drm, 0);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>
> This call does not belong to the mode-setting pipeline and belongs into appletbdrm_probe().
Moving this to appletbdrm_probe() again leads to a freezing Touch Bar. Maybe I am placing it at the wrong place?
>
>
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_probe(struct usb_interface *intf,
>> + const struct usb_device_id *id)
>> +{
>> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>> + struct device *dev = &intf->dev;
>> + struct appletbdrm_device *adev;
>> + int ret;
>> +
>> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
>> +
>> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
>> + if (IS_ERR(adev))
>> + return PTR_ERR(adev);
>> +
>> + adev->dev = dev;
>> + adev->in_ep = bulk_in->bEndpointAddress;
>> + adev->out_ep = bulk_out->bEndpointAddress;
>> +
>> + usb_set_intfdata(intf, adev);
>
> Rather set the DRM device here and fetch it in the callbacks below. At some point, we might be able to share some of those helpers among drivers.
>
>> +
>> + ret = appletbdrm_get_information(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to get display information\n");
>> +
>> + ret = appletbdrm_signal_readiness(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to signal readiness\n");
>> +
>
>> + ret = appletbdrm_clear_display(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to clear display\n");
>
> Clearing the display is not something usually done in probe. But I guess there's no better place. I'd do this as the final call in probe; after registering the device. That way, it acts a bit like an in-kernel DRM client.
>
> Best regards
> Thomas
>
>> +
>> + return appletbdrm_setup_mode_config(adev);
>> +}
>> +
>> +static void appletbdrm_disconnect(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> + struct drm_device *drm = &adev->drm;
>> +
>> + drm_dev_unplug(drm);
>> + drm_atomic_helper_shutdown(drm);
>> +}
>> +
>> +static void appletbdrm_shutdown(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> +
>> + /*
>> + * The framebuffer needs to be cleared on shutdown since its content
>> + * persists across boots
>> + */
>> + drm_atomic_helper_shutdown(&adev->drm);
>> +}
>> +
>> +static const struct usb_device_id appletbdrm_usb_id_table[] = {
>> + { USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
>> +
>> +static struct usb_driver appletbdrm_usb_driver = {
>> + .name = "appletbdrm",
>> + .probe = appletbdrm_probe,
>> + .disconnect = appletbdrm_disconnect,
>> + .shutdown = appletbdrm_shutdown,
>> + .id_table = appletbdrm_usb_id_table,
>> +};
>> +module_usb_driver(appletbdrm_usb_driver);
>> +
>> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
>> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
>> +MODULE_LICENSE("GPL");
>
> --
> --
> 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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-18 20:12 ` Aditya Garg
@ 2025-02-19 7:57 ` Thomas Zimmermann
2025-02-19 8:02 ` Thomas Zimmermann
2025-02-20 17:23 ` Aditya Garg
0 siblings, 2 replies; 19+ messages in thread
From: Thomas Zimmermann @ 2025-02-19 7:57 UTC (permalink / raw)
To: Aditya Garg
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
Am 18.02.25 um 21:12 schrieb Aditya Garg:
> Hi
>
> In continuation to my previous mail.
>
>>> +
>>> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
>>> +{
>>> + struct appletbdrm_msg_simple_request *request;
>>> + int ret;
>>> +
>>> + request = kzalloc(sizeof(*request), GFP_KERNEL);
>>> + if (!request)
>>> + return -ENOMEM;
>>> +
>>> + request->header.unk_00 = cpu_to_le16(2);
>>> + request->header.unk_02 = cpu_to_le16(0x1512);
>>> + request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
>>> + request->msg = msg;
>>> + request->size = request->header.size;
>>> +
>>> + ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
>>> +
>>> + kfree(request);
>> This is temporary data for the send operation and save to free here?
> Probably yes. If I understand correctly, it’s needed to make the touchbar go into the display mode, from the hid keyboard mode.
>
> We here are doing the same as the Windows driver [1] for this does.
>
> [1] https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/include/Dfr.h#L3
Yeah. My concern was that request is being freed while the USB send
operation is still using it. But in the USB code, it doesn't look like that.
[...]
>> Can we void the use of drm_fb_blit()? Since you know all formats in advance, just do
>>
>> switch (format)
>> case XRGB8888: drm_fb_xrgb888_to_bgr888() break default:
>> drm_fb_memcpy() break }We use blit in simpledrm and ofdrm, where we don't know the formats and output buffers in advance. But it's really not so great in other drivers, I think.
> I think you mean this:
>
> #include <drm/drm_framebuffer.h>
>
> switch (fb->format->format) {
> case DRM_FORMAT_XRGB8888:
> drm_fb_xrgb8888_to_bgr888(&dst, NULL, &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
> break;
> default:
> drm_fb_memcpy(&dst, NULL, &shadow_plane_state->data[0], fb, &damage);
> break;
> }
Yes.
[...]
>> For USB devices, we need special wiring to make PRIME work. The PRIME device must support DMA, but a USB device doesn't. So we pass the USB controller device instead. See [2] for what udl does and how it obtains dmadev.
>>
>> [2] https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/udl/udl_drv.c#L76
> Disregard my previous reply for this. I believe you meant by this?:
>
> —>8—
> From b6fda730995b7f28374c1ff38778a6f3e6da65da Mon Sep 17 00:00:00 2001
> From: Aditya Garg <gargaditya08@live.com>
> Date: Tue, 18 Feb 2025 22:47:44 +0530
> Subject: [PATCH] prime
>
> ---
> drivers/gpu/drm/tiny/appletbdrm.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
> index f2d911325..b835063c2 100644
> --- a/drivers/gpu/drm/tiny/appletbdrm.c
> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
> @@ -118,6 +118,7 @@ struct appletbdrm_fb_request_response {
>
> struct appletbdrm_device {
> struct device *dev;
> + struct device *dmadev;
>
> unsigned int in_ep;
> unsigned int out_ep;
> @@ -521,10 +522,22 @@ static const struct drm_encoder_funcs appletbdrm_encoder_funcs = {
> .destroy = drm_encoder_cleanup,
> };
>
> +static struct drm_gem_object *appletbdrm_driver_gem_prime_import(struct drm_device *dev,
> + struct dma_buf *dma_buf)
> +{
> + struct appletbdrm_device *adev = drm_to_adev(dev);
> +
> + if (!adev->dmadev)
> + return ERR_PTR(-ENODEV);
> +
> + return drm_gem_prime_import_dev(dev, dma_buf, adev->dmadev);
> +}
> +
> DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>
> static const struct drm_driver appletbdrm_drm_driver = {
> DRM_GEM_SHMEM_DRIVER_OPS,
> + .gem_prime_import = appletbdrm_driver_gem_prime_import,
Exactly. The TODO item for this problem is at [1], but there's quite a
bit of change involved to fix it. Setting a dedicated DMA device is the
next best thing.
[1]
https://elixir.bootlin.com/linux/v6.13.3/source/Documentation/gpu/todo.rst#L615
Best regards
Thomas
> .name = "appletbdrm",
> .desc = "Apple Touch Bar DRM Driver",
> .major = 1,
--
--
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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-19 7:57 ` Thomas Zimmermann
@ 2025-02-19 8:02 ` Thomas Zimmermann
2025-02-20 17:23 ` Aditya Garg
1 sibling, 0 replies; 19+ messages in thread
From: Thomas Zimmermann @ 2025-02-19 8:02 UTC (permalink / raw)
To: Aditya Garg
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
Am 19.02.25 um 08:57 schrieb Thomas Zimmermann:
> Hi
>
> Am 18.02.25 um 21:12 schrieb Aditya Garg:
>> Hi
>>
>> In continuation to my previous mail.
>>
>>>> +
>>>> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32
>>>> msg)
>>>> +{
>>>> + struct appletbdrm_msg_simple_request *request;
>>>> + int ret;
>>>> +
>>>> + request = kzalloc(sizeof(*request), GFP_KERNEL);
>>>> + if (!request)
>>>> + return -ENOMEM;
>>>> +
>>>> + request->header.unk_00 = cpu_to_le16(2);
>>>> + request->header.unk_02 = cpu_to_le16(0x1512);
>>>> + request->header.size = cpu_to_le32(sizeof(*request) -
>>>> sizeof(request->header));
>>>> + request->msg = msg;
>>>> + request->size = request->header.size;
>>>> +
>>>> + ret = appletbdrm_send_request(adev, &request->header,
>>>> sizeof(*request));
>>>> +
>>>> + kfree(request);
>>> This is temporary data for the send operation and save to free here?
>> Probably yes. If I understand correctly, it’s needed to make the
>> touchbar go into the display mode, from the hid keyboard mode.
>>
>> We here are doing the same as the Windows driver [1] for this does.
>>
>> [1]
>> https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/include/Dfr.h#L3
>
> Yeah. My concern was that request is being freed while the USB send
> operation is still using it. But in the USB code, it doesn't look like
> that.
>
> [...]
>>> Can we void the use of drm_fb_blit()? Since you know all formats in
>>> advance, just do
>>>
>>> switch (format)
>>> case XRGB8888: drm_fb_xrgb888_to_bgr888() break default:
>>> drm_fb_memcpy() break }We use blit in simpledrm and ofdrm, where
>>> we don't know the formats and output buffers in advance. But it's
>>> really not so great in other drivers, I think.
>> I think you mean this:
>>
>> #include <drm/drm_framebuffer.h>
>>
>> switch (fb->format->format) {
>> case DRM_FORMAT_XRGB8888:
>> drm_fb_xrgb8888_to_bgr888(&dst, NULL,
>> &shadow_plane_state->data[0], fb, &damage,
>> &shadow_plane_state->fmtcnv_state);
>> break;
>> default:
>> drm_fb_memcpy(&dst, NULL, &shadow_plane_state->data[0],
>> fb, &damage);
>> break;
>> }
>
> Yes.
>
> [...]
>>> For USB devices, we need special wiring to make PRIME work. The
>>> PRIME device must support DMA, but a USB device doesn't. So we pass
>>> the USB controller device instead. See [2] for what udl does and how
>>> it obtains dmadev.
>>>
>>> [2]
>>> https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/udl/udl_drv.c#L76
>> Disregard my previous reply for this. I believe you meant by this?:
>>
>> —>8—
>> From b6fda730995b7f28374c1ff38778a6f3e6da65da Mon Sep 17 00:00:00 2001
>> From: Aditya Garg <gargaditya08@live.com>
>> Date: Tue, 18 Feb 2025 22:47:44 +0530
>> Subject: [PATCH] prime
>>
>> ---
>> drivers/gpu/drm/tiny/appletbdrm.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c
>> b/drivers/gpu/drm/tiny/appletbdrm.c
>> index f2d911325..b835063c2 100644
>> --- a/drivers/gpu/drm/tiny/appletbdrm.c
>> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
>> @@ -118,6 +118,7 @@ struct appletbdrm_fb_request_response {
>>
>> struct appletbdrm_device {
>> struct device *dev;
>> + struct device *dmadev;
>>
>> unsigned int in_ep;
>> unsigned int out_ep;
>> @@ -521,10 +522,22 @@ static const struct drm_encoder_funcs
>> appletbdrm_encoder_funcs = {
>> .destroy = drm_encoder_cleanup,
>> };
>>
>> +static struct drm_gem_object
>> *appletbdrm_driver_gem_prime_import(struct drm_device *dev,
>> + struct dma_buf *dma_buf)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(dev);
>> +
>> + if (!adev->dmadev)
>> + return ERR_PTR(-ENODEV);
>> +
>> + return drm_gem_prime_import_dev(dev, dma_buf, adev->dmadev);
>> +}
>> +
>> DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>>
>> static const struct drm_driver appletbdrm_drm_driver = {
>> DRM_GEM_SHMEM_DRIVER_OPS,
>> + .gem_prime_import = appletbdrm_driver_gem_prime_import,
>
> Exactly. The TODO item for this problem is at [1], but there's quite a
> bit of change involved to fix it. Setting a dedicated DMA device is
> the next best thing.
>
> [1]
> https://elixir.bootlin.com/linux/v6.13.3/source/Documentation/gpu/todo.rst#L615
To add to this: you also have to set dmadev. See [2] for the respective
code in udl. And please git-grep udl for dmadev and look for the places
were it handles device ref-counting.
[2]
https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/udl/udl_main.c#L314
Best regards
Thomas
>
> Best regards
> Thomas
>
>> .name = "appletbdrm",
>> .desc = "Apple Touch Bar DRM Driver",
>> .major = 1,
>
--
--
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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-19 7:57 ` Thomas Zimmermann
2025-02-19 8:02 ` Thomas Zimmermann
@ 2025-02-20 17:23 ` Aditya Garg
1 sibling, 0 replies; 19+ messages in thread
From: Aditya Garg @ 2025-02-20 17:23 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
> On 19 Feb 2025, at 1:27 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 18.02.25 um 21:12 schrieb Aditya Garg:
>> Hi
>>
>> In continuation to my previous mail.
>>
>>>> +
>>>> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
>>>> +{
>>>> + struct appletbdrm_msg_simple_request *request;
>>>> + int ret;
>>>> +
>>>> + request = kzalloc(sizeof(*request), GFP_KERNEL);
>>>> + if (!request)
>>>> + return -ENOMEM;
>>>> +
>>>> + request->header.unk_00 = cpu_to_le16(2);
>>>> + request->header.unk_02 = cpu_to_le16(0x1512);
>>>> + request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
>>>> + request->msg = msg;
>>>> + request->size = request->header.size;
>>>> +
>>>> + ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
>>>> +
>>>> + kfree(request);
>>> This is temporary data for the send operation and save to free here?
>> Probably yes. If I understand correctly, it’s needed to make the touchbar go into the display mode, from the hid keyboard mode.
>>
>> We here are doing the same as the Windows driver [1] for this does.
>>
>> [1] https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/include/Dfr.h#L3
>
> Yeah. My concern was that request is being freed while the USB send operation is still using it. But in the USB code, it doesn't look like that.
>
For this, we are using usb_bulk_msg [1] which is a synchronous function, so it only returns once the data is completely sent.
So IMO its safe to kfree(request); here.
[1] https://manpages.debian.org/jessie-backports/linux-manual-4.8/usb_bulk_msg.9
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-18 10:09 ` Thomas Zimmermann
2025-02-18 13:49 ` Aditya Garg
2025-02-18 20:12 ` Aditya Garg
@ 2025-02-20 10:11 ` Aditya Garg
2025-02-20 10:15 ` Thomas Zimmermann
2 siblings, 1 reply; 19+ messages in thread
From: Aditya Garg @ 2025-02-20 10:11 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
>
>
>> + ret = drm_dev_register(drm, 0);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>
> This call does not belong to the mode-setting pipeline and belongs into appletbdrm_probe().
>
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_probe(struct usb_interface *intf,
>> + const struct usb_device_id *id)
>> +{
>> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>> + struct device *dev = &intf->dev;
>> + struct appletbdrm_device *adev;
>> + int ret;
>> +
>> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
>> +
>> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
>> + if (IS_ERR(adev))
>> + return PTR_ERR(adev);
>> +
>> + adev->dev = dev;
>> + adev->in_ep = bulk_in->bEndpointAddress;
>> + adev->out_ep = bulk_out->bEndpointAddress;
>> +
>> + usb_set_intfdata(intf, adev);
>
> Rather set the DRM device here and fetch it in the callbacks below. At some point, we might be able to share some of those helpers among drivers.
>
FWIW
Moving register drm device here results in these errors in journalctl:
Feb 20 15:02:46 MacBook kernel: appletbdrm: loading out-of-tree module taints kernel.
Feb 20 15:02:46 MacBook kernel: appletbdrm: module verification failed: signature and/or required key missing - tainting kernel
Feb 20 15:02:46 MacBook kernel: BUG: kernel NULL pointer dereference, address: 0000000000000030
Feb 20 15:02:46 MacBook kernel: #PF: supervisor read access in kernel mode
Feb 20 15:02:46 MacBook kernel: #PF: error_code(0x0000) - not-present page
Feb 20 15:02:46 MacBook kernel: PGD 0 P4D 0
Feb 20 15:02:46 MacBook kernel: Oops: Oops: 0000 [#1] PREEMPT SMP PTI
Feb 20 15:02:46 MacBook kernel: CPU: 10 UID: 0 PID: 3530 Comm: modprobe Tainted: G C OE 6.13.3-1-t2-noble #1
Feb 20 15:02:46 MacBook kernel: Tainted: [C]=CRAP, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
Feb 20 15:02:46 MacBook kernel: Hardware name: Apple Inc. MacBookPro16,1/Mac-E1008331FDC96864, BIOS 2069.80.3.0.0 (iBridge: 22.16.13051.0.0,0) 01/07/2025
Feb 20 15:02:46 MacBook kernel: RIP: 0010:drm_dev_register+0x1c/0x290
Feb 20 15:02:46 MacBook kernel: Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 49 89 f5 41 54 53 48 89 fb 48 83 ec 08 <4c> 8b 77 30 49 83 3e 00 0f 84 09 02 00 00 48 83 7b 20 00 0f 84 0e
Feb 20 15:02:46 MacBook kernel: RSP: 0018:ffffbf4344cbb670 EFLAGS: 00010282
Feb 20 15:02:46 MacBook kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
Feb 20 15:02:46 MacBook kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
Feb 20 15:02:46 MacBook kernel: RBP: ffffbf4344cbb6a0 R08: 0000000000000000 R09: 0000000000000000
Feb 20 15:02:46 MacBook kernel: R10: 0000000000000000 R11: 0000000000000000 R12: ffff992a8f114020
Feb 20 15:02:46 MacBook kernel: R13: 0000000000000000 R14: ffff992a8f115ad8 R15: ffff992a8f114000
Feb 20 15:02:46 MacBook kernel: FS: 000073572877c080(0000) GS:ffff992deed00000(0000) knlGS:0000000000000000
Feb 20 15:02:46 MacBook kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Feb 20 15:02:46 MacBook kernel: CR2: 0000000000000030 CR3: 000000011dd12003 CR4: 00000000003726f0
Feb 20 15:02:46 MacBook kernel: Call Trace:
Feb 20 15:02:46 MacBook kernel: <TASK>
Feb 20 15:02:46 MacBook kernel: ? show_regs+0x6c/0x80
Feb 20 15:02:46 MacBook kernel: ? __die+0x24/0x80
Feb 20 15:02:46 MacBook kernel: ? page_fault_oops+0x175/0x5d0
Feb 20 15:02:46 MacBook kernel: ? do_user_addr_fault+0x4b2/0x870
Feb 20 15:02:46 MacBook kernel: ? exc_page_fault+0x85/0x1c0
Feb 20 15:02:46 MacBook kernel: ? asm_exc_page_fault+0x27/0x30
Feb 20 15:02:46 MacBook kernel: ? drm_dev_register+0x1c/0x290
Feb 20 15:02:46 MacBook kernel: appletbdrm_probe+0x4eb/0x5f0 [appletbdrm]
Feb 20 15:02:46 MacBook kernel: usb_probe_interface+0x168/0x3d0
Feb 20 15:02:46 MacBook kernel: really_probe+0xee/0x3c0
Feb 20 15:02:46 MacBook kernel: __driver_probe_device+0x8c/0x180
Feb 20 15:02:46 MacBook kernel: driver_probe_device+0x24/0xd0
Feb 20 15:02:46 MacBook kernel: __driver_attach+0x10b/0x210
Feb 20 15:02:46 MacBook kernel: ? __pfx___driver_attach+0x10/0x10
Feb 20 15:02:46 MacBook kernel: bus_for_each_dev+0x8a/0xf0
Feb 20 15:02:46 MacBook kernel: driver_attach+0x1e/0x30
Feb 20 15:02:46 MacBook kernel: bus_add_driver+0x14e/0x290
Feb 20 15:02:46 MacBook kernel: driver_register+0x5e/0x130
Feb 20 15:02:46 MacBook kernel: usb_register_driver+0x87/0x170
Feb 20 15:02:46 MacBook kernel: ? __pfx_appletbdrm_usb_driver_init+0x10/0x10 [appletbdrm]
Feb 20 15:02:46 MacBook kernel: appletbdrm_usb_driver_init+0x23/0xff0 [appletbdrm]
Feb 20 15:02:46 MacBook kernel: do_one_initcall+0x5b/0x340
Feb 20 15:02:46 MacBook kernel: do_init_module+0x97/0x2a0
Feb 20 15:02:46 MacBook kernel: load_module+0x2293/0x25c0
Feb 20 15:02:46 MacBook kernel: init_module_from_file+0x97/0xe0
Feb 20 15:02:46 MacBook kernel: ? init_module_from_file+0x97/0xe0
Feb 20 15:02:46 MacBook kernel: idempotent_init_module+0x110/0x300
Feb 20 15:02:46 MacBook kernel: __x64_sys_finit_module+0x77/0x100
Feb 20 15:02:46 MacBook kernel: x64_sys_call+0x1f37/0x2650
Feb 20 15:02:46 MacBook kernel: do_syscall_64+0x7e/0x170
Feb 20 15:02:46 MacBook kernel: ? ksys_read+0x72/0xf0
Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
Feb 20 15:02:46 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
Feb 20 15:02:46 MacBook kernel: ? __do_sys_newfstatat+0x44/0x90
Feb 20 15:02:46 MacBook kernel: ? ext4_llseek+0xc0/0x120
Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
Feb 20 15:02:46 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
Feb 20 15:02:46 MacBook kernel: ? count_memcg_events.constprop.0+0x2a/0x50
Feb 20 15:02:46 MacBook kernel: ? handle_mm_fault+0xaf/0x2e0
Feb 20 15:02:46 MacBook kernel: ? do_user_addr_fault+0x5d5/0x870
Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
Feb 20 15:02:46 MacBook kernel: ? irqentry_exit_to_user_mode+0x2d/0x1d0
Feb 20 15:02:46 MacBook kernel: ? irqentry_exit+0x43/0x50
Feb 20 15:02:46 MacBook kernel: ? exc_page_fault+0x96/0x1c0
Feb 20 15:02:46 MacBook kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e
Feb 20 15:02:46 MacBook kernel: RIP: 0033:0x735727f2725d
Feb 20 15:02:46 MacBook kernel: Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
Feb 20 15:02:46 MacBook kernel: RSP: 002b:00007fffd9f88d18 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
Feb 20 15:02:46 MacBook kernel: RAX: ffffffffffffffda RBX: 000062610c6eb8e0 RCX: 0000735727f2725d
Feb 20 15:02:46 MacBook kernel: RDX: 0000000000000000 RSI: 00006260e7b3be52 RDI: 0000000000000003
Feb 20 15:02:46 MacBook kernel: RBP: 00007fffd9f88dd0 R08: 0000000000000040 R09: 00007fffd9f88e50
Feb 20 15:02:46 MacBook kernel: R10: 0000735728003b20 R11: 0000000000000246 R12: 00006260e7b3be52
Feb 20 15:02:46 MacBook kernel: R13: 0000000000040000 R14: 000062610c6e4920 R15: 0000000000000000
Feb 20 15:02:46 MacBook kernel: </TASK>
The following change was done:
@@ -13,6 +13,7 @@
#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
+#include <drm/drm_client_setup.h>
#include <drm/drm_crtc.h>
#include <drm/drm_damage_helper.h>
#include <drm/drm_drv.h>
@@ -596,7 +597,6 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
* as the height is actually the width of the framebuffer and vice
* versa
*/
-
drm->mode_config.min_width = 0;
drm->mode_config.min_height = 0;
drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
@@ -635,10 +635,6 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
drm_mode_config_reset(drm);
- ret = drm_dev_register(drm, 0);
- if (ret)
- return dev_err_probe(dev, ret, "Failed to register DRM device\n");
-
return 0;
}
@@ -648,6 +644,7 @@ static int appletbdrm_probe(struct usb_interface *intf,
struct usb_endpoint_descriptor *bulk_in, *bulk_out;
struct device *dev = &intf->dev;
struct appletbdrm_device *adev;
+ struct drm_device *drm;
int ret;
ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
@@ -676,7 +673,17 @@ static int appletbdrm_probe(struct usb_interface *intf,
if (ret)
return dev_err_probe(dev, ret, "Failed to clear display\n");
- return appletbdrm_setup_mode_config(adev);
+ ret = appletbdrm_setup_mode_config(adev);
+ if (ret)
+ return ret;
+
+ ret = drm_dev_register(drm, 0);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to register DRM device\n");
+
+ drm_client_setup(drm, NULL);
+
+ return 0;
}
static void appletbdrm_disconnect(struct usb_interface *intf)
>> +
>> + ret = appletbdrm_get_information(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to get display information\n");
>> +
>> + ret = appletbdrm_signal_readiness(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to signal readiness\n");
>> +
>
>> + ret = appletbdrm_clear_display(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to clear display\n");
>
> Clearing the display is not something usually done in probe. But I guess there's no better place. I'd do this as the final call in probe; after registering the device. That way, it acts a bit like an in-kernel DRM client.
>
> Best regards
> Thomas
>
>> +
>> + return appletbdrm_setup_mode_config(adev);
>> +}
>> +
>> +static void appletbdrm_disconnect(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> + struct drm_device *drm = &adev->drm;
>> +
>> + drm_dev_unplug(drm);
>> + drm_atomic_helper_shutdown(drm);
>> +}
>> +
>> +static void appletbdrm_shutdown(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> +
>> + /*
>> + * The framebuffer needs to be cleared on shutdown since its content
>> + * persists across boots
>> + */
>> + drm_atomic_helper_shutdown(&adev->drm);
>> +}
>> +
>> +static const struct usb_device_id appletbdrm_usb_id_table[] = {
>> + { USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
>> +
>> +static struct usb_driver appletbdrm_usb_driver = {
>> + .name = "appletbdrm",
>> + .probe = appletbdrm_probe,
>> + .disconnect = appletbdrm_disconnect,
>> + .shutdown = appletbdrm_shutdown,
>> + .id_table = appletbdrm_usb_id_table,
>> +};
>> +module_usb_driver(appletbdrm_usb_driver);
>> +
>> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
>> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
>> +MODULE_LICENSE("GPL");
Regards
Aditya
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-20 10:11 ` Aditya Garg
@ 2025-02-20 10:15 ` Thomas Zimmermann
2025-02-20 10:48 ` Aditya Garg
0 siblings, 1 reply; 19+ messages in thread
From: Thomas Zimmermann @ 2025-02-20 10:15 UTC (permalink / raw)
To: Aditya Garg
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
Hi
Am 20.02.25 um 11:11 schrieb Aditya Garg:
> Hi
>
>
>>
>>> + ret = drm_dev_register(drm, 0);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>> This call does not belong to the mode-setting pipeline and belongs into appletbdrm_probe().
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int appletbdrm_probe(struct usb_interface *intf,
>>> + const struct usb_device_id *id)
>>> +{
>>> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>>> + struct device *dev = &intf->dev;
>>> + struct appletbdrm_device *adev;
>>> + int ret;
>>> +
>>> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
>>> +
>>> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
>>> + if (IS_ERR(adev))
>>> + return PTR_ERR(adev);
>>> +
>>> + adev->dev = dev;
>>> + adev->in_ep = bulk_in->bEndpointAddress;
>>> + adev->out_ep = bulk_out->bEndpointAddress;
>>> +
>>> + usb_set_intfdata(intf, adev);
>> Rather set the DRM device here and fetch it in the callbacks below. At some point, we might be able to share some of those helpers among drivers.
>>
> FWIW
>
> Moving register drm device here results in these errors in journalctl:
>
> Feb 20 15:02:46 MacBook kernel: appletbdrm: loading out-of-tree module taints kernel.
> Feb 20 15:02:46 MacBook kernel: appletbdrm: module verification failed: signature and/or required key missing - tainting kernel
> Feb 20 15:02:46 MacBook kernel: BUG: kernel NULL pointer dereference, address: 0000000000000030
> Feb 20 15:02:46 MacBook kernel: #PF: supervisor read access in kernel mode
> Feb 20 15:02:46 MacBook kernel: #PF: error_code(0x0000) - not-present page
> Feb 20 15:02:46 MacBook kernel: PGD 0 P4D 0
> Feb 20 15:02:46 MacBook kernel: Oops: Oops: 0000 [#1] PREEMPT SMP PTI
> Feb 20 15:02:46 MacBook kernel: CPU: 10 UID: 0 PID: 3530 Comm: modprobe Tainted: G C OE 6.13.3-1-t2-noble #1
> Feb 20 15:02:46 MacBook kernel: Tainted: [C]=CRAP, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
> Feb 20 15:02:46 MacBook kernel: Hardware name: Apple Inc. MacBookPro16,1/Mac-E1008331FDC96864, BIOS 2069.80.3.0.0 (iBridge: 22.16.13051.0.0,0) 01/07/2025
> Feb 20 15:02:46 MacBook kernel: RIP: 0010:drm_dev_register+0x1c/0x290
> Feb 20 15:02:46 MacBook kernel: Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 49 89 f5 41 54 53 48 89 fb 48 83 ec 08 <4c> 8b 77 30 49 83 3e 00 0f 84 09 02 00 00 48 83 7b 20 00 0f 84 0e
> Feb 20 15:02:46 MacBook kernel: RSP: 0018:ffffbf4344cbb670 EFLAGS: 00010282
> Feb 20 15:02:46 MacBook kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> Feb 20 15:02:46 MacBook kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
> Feb 20 15:02:46 MacBook kernel: RBP: ffffbf4344cbb6a0 R08: 0000000000000000 R09: 0000000000000000
> Feb 20 15:02:46 MacBook kernel: R10: 0000000000000000 R11: 0000000000000000 R12: ffff992a8f114020
> Feb 20 15:02:46 MacBook kernel: R13: 0000000000000000 R14: ffff992a8f115ad8 R15: ffff992a8f114000
> Feb 20 15:02:46 MacBook kernel: FS: 000073572877c080(0000) GS:ffff992deed00000(0000) knlGS:0000000000000000
> Feb 20 15:02:46 MacBook kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Feb 20 15:02:46 MacBook kernel: CR2: 0000000000000030 CR3: 000000011dd12003 CR4: 00000000003726f0
> Feb 20 15:02:46 MacBook kernel: Call Trace:
> Feb 20 15:02:46 MacBook kernel: <TASK>
> Feb 20 15:02:46 MacBook kernel: ? show_regs+0x6c/0x80
> Feb 20 15:02:46 MacBook kernel: ? __die+0x24/0x80
> Feb 20 15:02:46 MacBook kernel: ? page_fault_oops+0x175/0x5d0
> Feb 20 15:02:46 MacBook kernel: ? do_user_addr_fault+0x4b2/0x870
> Feb 20 15:02:46 MacBook kernel: ? exc_page_fault+0x85/0x1c0
> Feb 20 15:02:46 MacBook kernel: ? asm_exc_page_fault+0x27/0x30
> Feb 20 15:02:46 MacBook kernel: ? drm_dev_register+0x1c/0x290
> Feb 20 15:02:46 MacBook kernel: appletbdrm_probe+0x4eb/0x5f0 [appletbdrm]
> Feb 20 15:02:46 MacBook kernel: usb_probe_interface+0x168/0x3d0
> Feb 20 15:02:46 MacBook kernel: really_probe+0xee/0x3c0
> Feb 20 15:02:46 MacBook kernel: __driver_probe_device+0x8c/0x180
> Feb 20 15:02:46 MacBook kernel: driver_probe_device+0x24/0xd0
> Feb 20 15:02:46 MacBook kernel: __driver_attach+0x10b/0x210
> Feb 20 15:02:46 MacBook kernel: ? __pfx___driver_attach+0x10/0x10
> Feb 20 15:02:46 MacBook kernel: bus_for_each_dev+0x8a/0xf0
> Feb 20 15:02:46 MacBook kernel: driver_attach+0x1e/0x30
> Feb 20 15:02:46 MacBook kernel: bus_add_driver+0x14e/0x290
> Feb 20 15:02:46 MacBook kernel: driver_register+0x5e/0x130
> Feb 20 15:02:46 MacBook kernel: usb_register_driver+0x87/0x170
> Feb 20 15:02:46 MacBook kernel: ? __pfx_appletbdrm_usb_driver_init+0x10/0x10 [appletbdrm]
> Feb 20 15:02:46 MacBook kernel: appletbdrm_usb_driver_init+0x23/0xff0 [appletbdrm]
> Feb 20 15:02:46 MacBook kernel: do_one_initcall+0x5b/0x340
> Feb 20 15:02:46 MacBook kernel: do_init_module+0x97/0x2a0
> Feb 20 15:02:46 MacBook kernel: load_module+0x2293/0x25c0
> Feb 20 15:02:46 MacBook kernel: init_module_from_file+0x97/0xe0
> Feb 20 15:02:46 MacBook kernel: ? init_module_from_file+0x97/0xe0
> Feb 20 15:02:46 MacBook kernel: idempotent_init_module+0x110/0x300
> Feb 20 15:02:46 MacBook kernel: __x64_sys_finit_module+0x77/0x100
> Feb 20 15:02:46 MacBook kernel: x64_sys_call+0x1f37/0x2650
> Feb 20 15:02:46 MacBook kernel: do_syscall_64+0x7e/0x170
> Feb 20 15:02:46 MacBook kernel: ? ksys_read+0x72/0xf0
> Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
> Feb 20 15:02:46 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
> Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
> Feb 20 15:02:46 MacBook kernel: ? __do_sys_newfstatat+0x44/0x90
> Feb 20 15:02:46 MacBook kernel: ? ext4_llseek+0xc0/0x120
> Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
> Feb 20 15:02:46 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
> Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
> Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
> Feb 20 15:02:46 MacBook kernel: ? count_memcg_events.constprop.0+0x2a/0x50
> Feb 20 15:02:46 MacBook kernel: ? handle_mm_fault+0xaf/0x2e0
> Feb 20 15:02:46 MacBook kernel: ? do_user_addr_fault+0x5d5/0x870
> Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
> Feb 20 15:02:46 MacBook kernel: ? irqentry_exit_to_user_mode+0x2d/0x1d0
> Feb 20 15:02:46 MacBook kernel: ? irqentry_exit+0x43/0x50
> Feb 20 15:02:46 MacBook kernel: ? exc_page_fault+0x96/0x1c0
> Feb 20 15:02:46 MacBook kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e
> Feb 20 15:02:46 MacBook kernel: RIP: 0033:0x735727f2725d
> Feb 20 15:02:46 MacBook kernel: Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
> Feb 20 15:02:46 MacBook kernel: RSP: 002b:00007fffd9f88d18 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> Feb 20 15:02:46 MacBook kernel: RAX: ffffffffffffffda RBX: 000062610c6eb8e0 RCX: 0000735727f2725d
> Feb 20 15:02:46 MacBook kernel: RDX: 0000000000000000 RSI: 00006260e7b3be52 RDI: 0000000000000003
> Feb 20 15:02:46 MacBook kernel: RBP: 00007fffd9f88dd0 R08: 0000000000000040 R09: 00007fffd9f88e50
> Feb 20 15:02:46 MacBook kernel: R10: 0000735728003b20 R11: 0000000000000246 R12: 00006260e7b3be52
> Feb 20 15:02:46 MacBook kernel: R13: 0000000000040000 R14: 000062610c6e4920 R15: 0000000000000000
> Feb 20 15:02:46 MacBook kernel: </TASK>
>
> The following change was done:
>
> @@ -13,6 +13,7 @@
>
> #include <drm/drm_atomic.h>
> #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_client_setup.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_damage_helper.h>
> #include <drm/drm_drv.h>
> @@ -596,7 +597,6 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
> * as the height is actually the width of the framebuffer and vice
> * versa
> */
> -
> drm->mode_config.min_width = 0;
> drm->mode_config.min_height = 0;
> drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
> @@ -635,10 +635,6 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>
> drm_mode_config_reset(drm);
>
> - ret = drm_dev_register(drm, 0);
> - if (ret)
> - return dev_err_probe(dev, ret, "Failed to register DRM device\n");
> -
> return 0;
> }
>
> @@ -648,6 +644,7 @@ static int appletbdrm_probe(struct usb_interface *intf,
> struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> struct device *dev = &intf->dev;
> struct appletbdrm_device *adev;
> + struct drm_device *drm;
Because you apparently never initialize this variable.
> int ret;
>
> ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
> @@ -676,7 +673,17 @@ static int appletbdrm_probe(struct usb_interface *intf,
> if (ret)
> return dev_err_probe(dev, ret, "Failed to clear display\n");
>
> - return appletbdrm_setup_mode_config(adev);
> + ret = appletbdrm_setup_mode_config(adev);
> + if (ret)
> + return ret;
> +
> + ret = drm_dev_register(drm, 0);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
> +
> + drm_client_setup(drm, NULL);
You won't need a DRM client on the touch bar. Just clearing the display
should be enough.
Best regards
Thomas
> +
> + return 0;
> }
>
> static void appletbdrm_disconnect(struct usb_interface *intf)
>
>>> +
>>> + ret = appletbdrm_get_information(adev);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to get display information\n");
>>> +
>>> + ret = appletbdrm_signal_readiness(adev);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to signal readiness\n");
>>> +
>>> + ret = appletbdrm_clear_display(adev);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to clear display\n");
>> Clearing the display is not something usually done in probe. But I guess there's no better place. I'd do this as the final call in probe; after registering the device. That way, it acts a bit like an in-kernel DRM client.
>>
>> Best regards
>> Thomas
>>
>>> +
>>> + return appletbdrm_setup_mode_config(adev);
>>> +}
>>> +
>>> +static void appletbdrm_disconnect(struct usb_interface *intf)
>>> +{
>>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>>> + struct drm_device *drm = &adev->drm;
>>> +
>>> + drm_dev_unplug(drm);
>>> + drm_atomic_helper_shutdown(drm);
>>> +}
>>> +
>>> +static void appletbdrm_shutdown(struct usb_interface *intf)
>>> +{
>>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>>> +
>>> + /*
>>> + * The framebuffer needs to be cleared on shutdown since its content
>>> + * persists across boots
>>> + */
>>> + drm_atomic_helper_shutdown(&adev->drm);
>>> +}
>>> +
>>> +static const struct usb_device_id appletbdrm_usb_id_table[] = {
>>> + { USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
>>> + {}
>>> +};
>>> +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
>>> +
>>> +static struct usb_driver appletbdrm_usb_driver = {
>>> + .name = "appletbdrm",
>>> + .probe = appletbdrm_probe,
>>> + .disconnect = appletbdrm_disconnect,
>>> + .shutdown = appletbdrm_shutdown,
>>> + .id_table = appletbdrm_usb_id_table,
>>> +};
>>> +module_usb_driver(appletbdrm_usb_driver);
>>> +
>>> +MODULE_AUTHOR("Kerem Karabay <kekrby@gmail.com>");
>>> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
>>> +MODULE_LICENSE("GPL");
> Regards
> Aditya
>
--
--
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)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs
2025-02-20 10:15 ` Thomas Zimmermann
@ 2025-02-20 10:48 ` Aditya Garg
0 siblings, 0 replies; 19+ messages in thread
From: Aditya Garg @ 2025-02-20 10:48 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: maarten.lankhorst, mripard, airlied, simona, Kerem Karabay,
Atharva Tiwari, Linux Kernel Mailing List, dri-devel
> On 20 Feb 2025, at 3:46 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
>> Am 20.02.25 um 11:11 schrieb Aditya Garg:
>> Hi
>>
>>
>>>
>>>> + ret = drm_dev_register(drm, 0);
>>>> + if (ret)
>>>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>>> This call does not belong to the mode-setting pipeline and belongs into appletbdrm_probe().
>>>
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int appletbdrm_probe(struct usb_interface *intf,
>>>> + const struct usb_device_id *id)
>>>> +{
>>>> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>>>> + struct device *dev = &intf->dev;
>>>> + struct appletbdrm_device *adev;
>>>> + int ret;
>>>> +
>>>> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>>>> + if (ret)
>>>> + return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
>>>> +
>>>> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
>>>> + if (IS_ERR(adev))
>>>> + return PTR_ERR(adev);
>>>> +
>>>> + adev->dev = dev;
>>>> + adev->in_ep = bulk_in->bEndpointAddress;
>>>> + adev->out_ep = bulk_out->bEndpointAddress;
>>>> +
>>>> + usb_set_intfdata(intf, adev);
>>> Rather set the DRM device here and fetch it in the callbacks below. At some point, we might be able to share some of those helpers among drivers.
>>>
>> FWIW
>>
>> Moving register drm device here results in these errors in journalctl:
>>
>> Feb 20 15:02:46 MacBook kernel: appletbdrm: loading out-of-tree module taints kernel.
>> Feb 20 15:02:46 MacBook kernel: appletbdrm: module verification failed: signature and/or required key missing - tainting kernel
>> Feb 20 15:02:46 MacBook kernel: BUG: kernel NULL pointer dereference, address: 0000000000000030
>> Feb 20 15:02:46 MacBook kernel: #PF: supervisor read access in kernel mode
>> Feb 20 15:02:46 MacBook kernel: #PF: error_code(0x0000) - not-present page
>> Feb 20 15:02:46 MacBook kernel: PGD 0 P4D 0
>> Feb 20 15:02:46 MacBook kernel: Oops: Oops: 0000 [#1] PREEMPT SMP PTI
>> Feb 20 15:02:46 MacBook kernel: CPU: 10 UID: 0 PID: 3530 Comm: modprobe Tainted: G C OE 6.13.3-1-t2-noble #1
>> Feb 20 15:02:46 MacBook kernel: Tainted: [C]=CRAP, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
>> Feb 20 15:02:46 MacBook kernel: Hardware name: Apple Inc. MacBookPro16,1/Mac-E1008331FDC96864, BIOS 2069.80.3.0.0 (iBridge: 22.16.13051.0.0,0) 01/07/2025
>> Feb 20 15:02:46 MacBook kernel: RIP: 0010:drm_dev_register+0x1c/0x290
>> Feb 20 15:02:46 MacBook kernel: Code: 90 90 90 90 90 90 90 90 90 90 90 90 90 90 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 49 89 f5 41 54 53 48 89 fb 48 83 ec 08 <4c> 8b 77 30 49 83 3e 00 0f 84 09 02 00 00 48 83 7b 20 00 0f 84 0e
>> Feb 20 15:02:46 MacBook kernel: RSP: 0018:ffffbf4344cbb670 EFLAGS: 00010282
>> Feb 20 15:02:46 MacBook kernel: RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
>> Feb 20 15:02:46 MacBook kernel: RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000000
>> Feb 20 15:02:46 MacBook kernel: RBP: ffffbf4344cbb6a0 R08: 0000000000000000 R09: 0000000000000000
>> Feb 20 15:02:46 MacBook kernel: R10: 0000000000000000 R11: 0000000000000000 R12: ffff992a8f114020
>> Feb 20 15:02:46 MacBook kernel: R13: 0000000000000000 R14: ffff992a8f115ad8 R15: ffff992a8f114000
>> Feb 20 15:02:46 MacBook kernel: FS: 000073572877c080(0000) GS:ffff992deed00000(0000) knlGS:0000000000000000
>> Feb 20 15:02:46 MacBook kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> Feb 20 15:02:46 MacBook kernel: CR2: 0000000000000030 CR3: 000000011dd12003 CR4: 00000000003726f0
>> Feb 20 15:02:46 MacBook kernel: Call Trace:
>> Feb 20 15:02:46 MacBook kernel: <TASK>
>> Feb 20 15:02:46 MacBook kernel: ? show_regs+0x6c/0x80
>> Feb 20 15:02:46 MacBook kernel: ? __die+0x24/0x80
>> Feb 20 15:02:46 MacBook kernel: ? page_fault_oops+0x175/0x5d0
>> Feb 20 15:02:46 MacBook kernel: ? do_user_addr_fault+0x4b2/0x870
>> Feb 20 15:02:46 MacBook kernel: ? exc_page_fault+0x85/0x1c0
>> Feb 20 15:02:46 MacBook kernel: ? asm_exc_page_fault+0x27/0x30
>> Feb 20 15:02:46 MacBook kernel: ? drm_dev_register+0x1c/0x290
>> Feb 20 15:02:46 MacBook kernel: appletbdrm_probe+0x4eb/0x5f0 [appletbdrm]
>> Feb 20 15:02:46 MacBook kernel: usb_probe_interface+0x168/0x3d0
>> Feb 20 15:02:46 MacBook kernel: really_probe+0xee/0x3c0
>> Feb 20 15:02:46 MacBook kernel: __driver_probe_device+0x8c/0x180
>> Feb 20 15:02:46 MacBook kernel: driver_probe_device+0x24/0xd0
>> Feb 20 15:02:46 MacBook kernel: __driver_attach+0x10b/0x210
>> Feb 20 15:02:46 MacBook kernel: ? __pfx___driver_attach+0x10/0x10
>> Feb 20 15:02:46 MacBook kernel: bus_for_each_dev+0x8a/0xf0
>> Feb 20 15:02:46 MacBook kernel: driver_attach+0x1e/0x30
>> Feb 20 15:02:46 MacBook kernel: bus_add_driver+0x14e/0x290
>> Feb 20 15:02:46 MacBook kernel: driver_register+0x5e/0x130
>> Feb 20 15:02:46 MacBook kernel: usb_register_driver+0x87/0x170
>> Feb 20 15:02:46 MacBook kernel: ? __pfx_appletbdrm_usb_driver_init+0x10/0x10 [appletbdrm]
>> Feb 20 15:02:46 MacBook kernel: appletbdrm_usb_driver_init+0x23/0xff0 [appletbdrm]
>> Feb 20 15:02:46 MacBook kernel: do_one_initcall+0x5b/0x340
>> Feb 20 15:02:46 MacBook kernel: do_init_module+0x97/0x2a0
>> Feb 20 15:02:46 MacBook kernel: load_module+0x2293/0x25c0
>> Feb 20 15:02:46 MacBook kernel: init_module_from_file+0x97/0xe0
>> Feb 20 15:02:46 MacBook kernel: ? init_module_from_file+0x97/0xe0
>> Feb 20 15:02:46 MacBook kernel: idempotent_init_module+0x110/0x300
>> Feb 20 15:02:46 MacBook kernel: __x64_sys_finit_module+0x77/0x100
>> Feb 20 15:02:46 MacBook kernel: x64_sys_call+0x1f37/0x2650
>> Feb 20 15:02:46 MacBook kernel: do_syscall_64+0x7e/0x170
>> Feb 20 15:02:46 MacBook kernel: ? ksys_read+0x72/0xf0
>> Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
>> Feb 20 15:02:46 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
>> Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
>> Feb 20 15:02:46 MacBook kernel: ? __do_sys_newfstatat+0x44/0x90
>> Feb 20 15:02:46 MacBook kernel: ? ext4_llseek+0xc0/0x120
>> Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
>> Feb 20 15:02:46 MacBook kernel: ? syscall_exit_to_user_mode+0x38/0x1d0
>> Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
>> Feb 20 15:02:46 MacBook kernel: ? do_syscall_64+0x8a/0x170
>> Feb 20 15:02:46 MacBook kernel: ? count_memcg_events.constprop.0+0x2a/0x50
>> Feb 20 15:02:46 MacBook kernel: ? handle_mm_fault+0xaf/0x2e0
>> Feb 20 15:02:46 MacBook kernel: ? do_user_addr_fault+0x5d5/0x870
>> Feb 20 15:02:46 MacBook kernel: ? arch_exit_to_user_mode_prepare.isra.0+0x22/0xd0
>> Feb 20 15:02:46 MacBook kernel: ? irqentry_exit_to_user_mode+0x2d/0x1d0
>> Feb 20 15:02:46 MacBook kernel: ? irqentry_exit+0x43/0x50
>> Feb 20 15:02:46 MacBook kernel: ? exc_page_fault+0x96/0x1c0
>> Feb 20 15:02:46 MacBook kernel: entry_SYSCALL_64_after_hwframe+0x76/0x7e
>> Feb 20 15:02:46 MacBook kernel: RIP: 0033:0x735727f2725d
>> Feb 20 15:02:46 MacBook kernel: Code: ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 8b bb 0d 00 f7 d8 64 89 01 48
>> Feb 20 15:02:46 MacBook kernel: RSP: 002b:00007fffd9f88d18 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
>> Feb 20 15:02:46 MacBook kernel: RAX: ffffffffffffffda RBX: 000062610c6eb8e0 RCX: 0000735727f2725d
>> Feb 20 15:02:46 MacBook kernel: RDX: 0000000000000000 RSI: 00006260e7b3be52 RDI: 0000000000000003
>> Feb 20 15:02:46 MacBook kernel: RBP: 00007fffd9f88dd0 R08: 0000000000000040 R09: 00007fffd9f88e50
>> Feb 20 15:02:46 MacBook kernel: R10: 0000735728003b20 R11: 0000000000000246 R12: 00006260e7b3be52
>> Feb 20 15:02:46 MacBook kernel: R13: 0000000000040000 R14: 000062610c6e4920 R15: 0000000000000000
>> Feb 20 15:02:46 MacBook kernel: </TASK>
>>
>> The following change was done:
>>
>> @@ -13,6 +13,7 @@
>> #include <drm/drm_atomic.h>
>> #include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_client_setup.h>
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_damage_helper.h>
>> #include <drm/drm_drv.h>
>> @@ -596,7 +597,6 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>> * as the height is actually the width of the framebuffer and vice
>> * versa
>> */
>> -
>> drm->mode_config.min_width = 0;
>> drm->mode_config.min_height = 0;
>> drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
>> @@ -635,10 +635,6 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>> drm_mode_config_reset(drm);
>> - ret = drm_dev_register(drm, 0);
>> - if (ret)
>> - return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>> -
>> return 0;
>> }
>> @@ -648,6 +644,7 @@ static int appletbdrm_probe(struct usb_interface *intf,
>> struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>> struct device *dev = &intf->dev;
>> struct appletbdrm_device *adev;
>> + struct drm_device *drm;
>
> Because you apparently never initialize this variable.
Mb, works now. Thanks.
>
>> int ret;
>> ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>> @@ -676,7 +673,17 @@ static int appletbdrm_probe(struct usb_interface *intf,
>> if (ret)
>> return dev_err_probe(dev, ret, "Failed to clear display\n");
>> - return appletbdrm_setup_mode_config(adev);
>> + ret = appletbdrm_setup_mode_config(adev);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_dev_register(drm, 0);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
>> +
>> + drm_client_setup(drm, NULL);
>
> You won't need a DRM client on the touch bar. Just clearing the display should be enough.
Ah alright then
^ permalink raw reply [flat|nested] 19+ messages in thread