Hi Thomas
>> From: Kerem Karabay <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Aditya Garg <[email protected]>
>> ---
>> 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 <[email protected]>
>> +L: [email protected]
>> +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 <[email protected]>
>> + */
>> +
>> +#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
