Hi Thomas >> From: Kerem Karabay <kekrby@xxxxxxxxx> >> >> 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. >> >> 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 <kekrby@xxxxxxxxx> >> Signed-off-by: Aditya Garg <gargaditya08@xxxxxxxx> >> --- >> MAINTAINERS | 6 + >> drivers/gpu/drm/tiny/Kconfig | 12 + >> drivers/gpu/drm/tiny/Makefile | 1 + >> drivers/gpu/drm/tiny/appletbdrm.c | 624 ++++++++++++++++++++++++++++++ >> 4 files changed, 643 insertions(+) >> create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index 8766f3e5e..2665e6c57 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -6889,6 +6889,12 @@ S: Supported >> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git >> F: drivers/gpu/drm/sun4i/sun8i* >> +DRM DRIVER FOR APPLE TOUCH BARS >> +M: Kerem Karabay <kekrby@xxxxxxxxx> >> +L: dri-devel@xxxxxxxxxxxxxxxxxxxxx >> +S: Maintained >> +F: drivers/gpu/drm/tiny/appletbdrm.c >> + >> DRM DRIVER FOR ARM PL111 CLCD >> S: Orphan >> T: git https://gitlab.freedesktop.org/drm/misc/kernel.git >> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig >> index f6889f649..559a97bce 100644 >> --- a/drivers/gpu/drm/tiny/Kconfig >> +++ b/drivers/gpu/drm/tiny/Kconfig >> @@ -1,5 +1,17 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> +config DRM_APPLETBDRM >> + tristate "DRM support for Apple Touch Bars" >> + depends on DRM && USB && MMU >> + select DRM_KMS_HELPER >> + select DRM_GEM_SHMEM_HELPER > > nit: Selects sorted alphabetically. > >> + 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 76dde89a0..9a1b412e7 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.o >> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c >> new file mode 100644 >> index 000000000..b9440ce00 >> --- /dev/null >> +++ b/drivers/gpu/drm/tiny/appletbdrm.c >> @@ -0,0 +1,624 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Apple Touch Bar DRM Driver >> + * >> + * Copyright (c) 2023 Kerem Karabay <kekrby@xxxxxxxxx> >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <asm/unaligned.h> >> + >> +#include <linux/usb.h> >> +#include <linux/module.h> > > Include statements should be sorted alphabetically. > >> + >> +#include <drm/drm_drv.h> >> +#include <drm/drm_fourcc.h> >> +#include <drm/drm_probe_helper.h> >> +#include <drm/drm_atomic_helper.h> >> +#include <drm/drm_damage_helper.h> >> +#include <drm/drm_format_helper.h> >> +#include <drm/drm_gem_shmem_helper.h> >> +#include <drm/drm_gem_atomic_helper.h> >> +#include <drm/drm_simple_kms_helper.h> >> +#include <drm/drm_gem_framebuffer_helper.h> > > Sorting. > >> + >> +#define _APPLETBDRM_FOURCC(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3]) >> +#define APPLETBDRM_FOURCC(s) _APPLETBDRM_FOURCC(#s) >> + >> +#define APPLETBDRM_PIXEL_FORMAT APPLETBDRM_FOURCC(RGBA) /* The actual format is BGR888 */ > > Pleasedon't call it FOURCC (because it isn't). Rather do something like > > #define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force) /* bit shifting here */) > #define __APPLETBDRM_MSG_TOK4(tok4) __APPLETBDRM_MSG_STR4(#tok4) > > (two underscores) > >> +#define APPLETBDRM_BITS_PER_PIXEL 24 >> + >> +#define APPLETBDRM_MSG_CLEAR_DISPLAY APPLETBDRM_FOURCC(CLRD) >> +#define APPLETBDRM_MSG_GET_INFORMATION APPLETBDRM_FOURCC(GINF) >> +#define APPLETBDRM_MSG_UPDATE_COMPLETE APPLETBDRM_FOURCC(UDCL) >> +#define APPLETBDRM_MSG_SIGNAL_READINESS APPLETBDRM_FOURCC(REDY) > > Maybe infix all such constants with _MSG_ or _PROTO_ so that it's clear that these are part of the device protocol. > > I'd also change the style a bit to something like > APPLETBDRM_MSG_REDY __APPLETBDRM_MSG_TOK4(REDY) /* device signals readiness */ > >> + >> +#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_device { >> + struct device *dev; >> + >> + u8 in_ep; >> + u8 out_ep; >> + >> + u32 width; >> + u32 height; > > For software state, it's common to use regular types (unsigned int) without a specific size. > >> + >> + struct drm_device drm; >> + struct drm_display_mode mode; >> + struct drm_connector connector; >> + struct drm_simple_display_pipe pipe; > > Simple-display pipe is deprecated. Please don't add any new drivers using it. You can easily replace it by taking its code and data structures and inline them into the driver. Change the naming to fit the driver and it should be good. I attempted to use atomic helper instead of simple display helpers for the driver, but for some reason, modprobing the driver results in “killed” message. I request you to comment upon what’s wrong here. The changes done are here: —>8— diff --git a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c index 7a74c8a..8b11a34 100644 --- a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c +++ b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c @@ -13,6 +13,8 @@ #include <linux/module.h> #include <drm/drm_drv.h> +#include <drm/drm_atomic.h> +#include <drm/drm_plane_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_probe_helper.h> #include <drm/drm_atomic_helper.h> @@ -51,7 +53,9 @@ struct appletbdrm_device { struct drm_device drm; struct drm_display_mode mode; struct drm_connector connector; - struct drm_simple_display_pipe pipe; + struct drm_plane primary_plane; + struct drm_crtc crtc; + struct drm_encoder encoder; bool readiness_signal_received; }; @@ -401,44 +405,82 @@ static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector return drm_connector_helper_get_modes_fixed(connector, &adev->mode); } -static enum drm_mode_status appletbdrm_pipe_mode_valid(struct drm_simple_display_pipe *pipe, - const struct drm_display_mode *mode) +static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane, + struct drm_atomic_state *state) { - struct drm_crtc *crtc = &pipe->crtc; - struct appletbdrm_device *adev = drm_to_adev(crtc->dev); + 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; - return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode); + 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_pipe_disable(struct drm_simple_display_pipe *pipe) +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(pipe->crtc.dev); + 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(&adev->drm, &idx)) + if (!drm_dev_enter(drm, &idx)) return; - appletbdrm_clear_display(adev); + appletbdrm_flush_damage(adev, old_plane_state, plane_state); drm_dev_exit(idx); } -static void appletbdrm_pipe_update(struct drm_simple_display_pipe *pipe, - struct drm_plane_state *old_state) +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 drm_crtc *crtc = &pipe->crtc; struct appletbdrm_device *adev = drm_to_adev(crtc->dev); int idx; - if (!crtc->state->active || !drm_dev_enter(&adev->drm, &idx)) + if (!drm_dev_enter(&adev->drm, &idx)) return; - appletbdrm_flush_damage(adev, old_state, pipe->plane.state); + appletbdrm_clear_display(adev); drm_dev_exit(idx); } -static const u32 appletbdrm_formats[] = { +static const u32 appletbdrm_primary_plane_formats[] = { DRM_FORMAT_BGR888, DRM_FORMAT_XRGB8888, /* emulated */ }; @@ -461,11 +503,22 @@ static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs .get_modes = appletbdrm_connector_helper_get_modes, }; -static const struct drm_simple_display_pipe_funcs appletbdrm_pipe_funcs = { - DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS, - .update = appletbdrm_pipe_update, - .disable = appletbdrm_pipe_disable, - .mode_valid = appletbdrm_pipe_mode_valid, +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); @@ -484,10 +537,38 @@ static const struct drm_driver appletbdrm_drm_driver = { 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; + 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 ret; + drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs); + + crtc = &adev->crtc; + ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL, + &appletbdrm_crtc_funcs, NULL); + if (ret) + return ret; + 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 ret; + encoder->possible_crtcs = drm_crtc_mask(crtc); + ret = drmm_mode_config_init(drm); if (ret) return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n"); @@ -530,13 +611,13 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev) if (ret) return dev_err_probe(dev, ret, "Failed to set non-desktop property\n"); - ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs, - appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats), - NULL, &adev->connector); + ret = drm_connector_attach_encoder(connector, encoder); + if (ret) return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n"); - drm_plane_enable_fb_damage_clips(&adev->pipe.plane); + drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs); + drm_plane_enable_fb_damage_clips(&adev->primary_plane); drm_mode_config_reset(drm); The commit history having both old and new revisions of the driver is here: https://github.com/AdityaGarg8/apple-touchbar-drv/blob/atomic/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c Thanks Aditya
