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

Commit Message

Aditya Garg Feb. 17, 2025, 6:52 p.m. UTC

From: Kerem Karabay <[e-mail]>

The Touch Bars found on x86 Macs support two USB configurations: one
where the device presents itself as a HID keyboard and can display
predefined sets of keys, and one where the operating system has full
control over what is displayed.

This commit adds support for the display functionality of the second
configuration. Functionality for the first configuration has been
merged in the HID tree.

Note that this driver has only been tested on T2 Macs, and only includes
the USB device ID for these devices. Testing on T1 Macs would be
appreciated.

Credit goes to @imbushuo on GitHub for reverse engineering most of the
protocol.

Signed-off-by: Kerem Karabay <[e-mail]>
Co-developed-by: Atharva Tiwari <[e-mail]>
Signed-off-by: Atharva Tiwari <[e-mail]>
Co-developed-by: Aditya Garg <[e-mail]>
Signed-off-by: Aditya Garg <[e-mail]>
---
 drivers/gpu/drm/tiny/Kconfig      |  13 +
 drivers/gpu/drm/tiny/Makefile     |   1 +
 drivers/gpu/drm/tiny/appletbdrm.c | 702 ++++++++++++++++++++++++++++++
 3 files changed, 716 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c

Comments

Thomas Zimmermann Feb. 18, 2025, 10:09 a.m. UTC | #1

Hi

Am 17.02.25 um 19:52 schrieb Aditya Garg:
> From: Kerem Karabay <[e-mail]>
>
> The Touch Bars found on x86 Macs support two USB configurations: one
> where the device presents itself as a HID keyboard and can display
> predefined sets of keys, and one where the operating system has full
> control over what is displayed.
>
> This commit adds support for the display functionality of the second
> configuration. Functionality for the first configuration has been
> merged in the HID tree.
>
> Note that this driver has only been tested on T2 Macs, and only includes
> the USB device ID for these devices. Testing on T1 Macs would be
> appreciated.
>
> Credit goes to @imbushuo on GitHub for reverse engineering most of the
> protocol.
>
> Signed-off-by: Kerem Karabay <[e-mail]>
> Co-developed-by: Atharva Tiwari <[e-mail]>
> Signed-off-by: Atharva Tiwari <[e-mail]>
> Co-developed-by: Aditya Garg <[e-mail]>
> Signed-off-by: Aditya Garg <[e-mail]>
> ---
>   drivers/gpu/drm/tiny/Kconfig      |  13 +
>   drivers/gpu/drm/tiny/Makefile     |   1 +
>   drivers/gpu/drm/tiny/appletbdrm.c | 702 ++++++++++++++++++++++++++++++

You also have to add the driver to MAINTAINERS.

>   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 <[e-mail]>
> + */
> +
> +#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


> +
> +	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.
> +
> +	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

> +	.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

> +	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 <[e-mail]>");
> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
> +MODULE_LICENSE("GPL");

Aditya Garg Feb. 18, 2025, 1:49 p.m. UTC | #2

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 <[e-mail]>
>> + */
>> +
>> +#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 <[e-mail]>
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,
 };

Aditya Garg Feb. 18, 2025, 8:12 p.m. UTC | #3

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 <[e-mail]>
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,

Thomas Zimmermann Feb. 19, 2025, 7:52 a.m. UTC | #4

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 <[e-mail]>
> 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 Feb. 19, 2025, 7:57 a.m. UTC | #5

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 <[e-mail]>
> 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 Feb. 19, 2025, 8:02 a.m. UTC | #6

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 <[e-mail]>
>> 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,
>

Aditya Garg Feb. 19, 2025, 9:37 a.m. UTC | #7

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 <[e-mail]>
>> 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?

Thomas Zimmermann Feb. 19, 2025, 9:49 a.m. UTC | #8

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 <[e-mail]>
>>> 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

Aditya Garg Feb. 19, 2025, 10:19 a.m. UTC | #9

Hi

> On 19 Feb 2025, at 3:19 PM, Thomas Zimmermann <[e-mail]> 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 <[e-mail]>
>>>> 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)
>

Aditya Garg Feb. 20, 2025, 10:11 a.m. UTC | #10

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 <[e-mail]>");
>> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
>> +MODULE_LICENSE("GPL");

Regards
Aditya

Thomas Zimmermann Feb. 20, 2025, 10:15 a.m. UTC | #11

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 <[e-mail]>");
>>> +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
>>> +MODULE_LICENSE("GPL");
> Regards
> Aditya
>

Thomas Zimmermann Feb. 20, 2025, 10:20 a.m. UTC | #12

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 <[e-mail]> 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 <[e-mail]>
>>>>> 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)
>>

Aditya Garg Feb. 20, 2025, 10:48 a.m. UTC | #13

> On 20 Feb 2025, at 3:46 PM, Thomas Zimmermann <[e-mail]> 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

Aditya Garg Feb. 20, 2025, 11 a.m. UTC | #14

> On 20 Feb 2025, at 3:50 PM, Thomas Zimmermann <[e-mail]> 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

Aditya Garg Feb. 20, 2025, 4:44 p.m. UTC | #15

> 
> 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/[e-mail]/T/#t

Aditya Garg Feb. 20, 2025, 5:23 p.m. UTC | #16

Hi

> On 19 Feb 2025, at 1:27 PM, Thomas Zimmermann <[e-mail]> 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

Patch

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 <[e-mail]>
+ */
+
+#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);
+
+	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;
+	}
+
+	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);
+		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;
+
+	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,
+	.name			= "appletbdrm",
+	.desc			= "Apple Touch Bar DRM Driver",
+	.date			= "20230910",
+	.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);
+	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");
+
+	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);
+
+	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");
+
+	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 <[e-mail]>");
+MODULE_DESCRIPTION("Apple Touch Bar DRM Driver");
+MODULE_LICENSE("GPL");