On Fri, Feb 21, 2025 at 11:37:57AM +0000, Aditya Garg wrote:
> From: Kerem Karabay <kek...@gmail.com>
>
> 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 Ben (Bingxing) Wang on GitHub [1] for reverse engineering
> most of the protocol.
>
> [1]: https://github.com/imbushuo/DFRDisplayKm
Use Link tag for this.
> +config DRM_APPLETBDRM
> + tristate "DRM support for Apple Touch Bars"
> + depends on DRM && USB && MMU
I dunno if tiny is not only about SPI panels, would be nice to hear somebody
from DRM to confirm that USB ones are okay to have.
> + select DRM_GEM_SHMEM_HELPER
> + select DRM_KMS_HELPER
> + select HID_APPLETB_BL
> + select HID_MULTITOUCH
> + 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.
...
> +/*
> + * Apple Touch Bar DRM Driver
> + *
> + * Copyright (c) 2023 Kerem Karabay <kek...@gmail.com>
No changes in 2024/2025?
> + */
...
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Why? Don't you have a struct device available?
...
> +#include <linux/module.h>
> +#include <linux/unaligned.h>
> +#include <linux/usb.h>
This is way too little list of what you are actually using, please consider
IWYU principle in place.
...
> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) |
> (str4[1] << 16) | (str4[2] << 8) | str4[3]))
Reinventing a wheel from get_unaligned_be32() AFAICS.
...
> +#define drm_to_adev(_drm) container_of(_drm, struct
> appletbdrm_device, drm)
+ container_of.h
...
> +struct appletbdrm_msg_request_header {
> + __le16 unk_00;
+ types.h
> + __le16 unk_02;
> + __le32 unk_04;
> + __le32 unk_08;
> + __le32 size;
> +} __packed;
Why __packed? Please explain and justify for all the data types that are marked
with this attribute.
...
> +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;
return 0;
Or you are expecting something else here?
> +}
...
> +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;
For three different cases the same error code, can it be adjusted more to the
situation?
> + }
> +
> + 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);
+ slab.h
> + if (!request)
> + return -ENOMEM;
+ err.h
> +
> + 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;
> +}
...
> + return drm_rect_width(rect) * drm_rect_height(rect) *
> (APPLETBDRM_BITS_PER_PIXEL / 8);
BITS_TO_BYTES() ? Will need bits.h IIRC.
...
> +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_plane_state *old_plane_state =
> drm_atomic_get_old_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_atomic_helper_damage_iter iter;
> + struct drm_rect damage;
> + size_t frames_size = 0;
> + size_t request_size;
> + 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)
Why 'else'? It's redundant.
> + return 0;
> +
> + drm_atomic_helper_damage_iter_init(&iter, old_plane_state,
> new_plane_state);
> + drm_atomic_for_each_plane_damage(&iter, &damage) {
> + frames_size += struct_size((struct appletbdrm_frame *)0, buf,
> rect_size(&damage));
> + }
> +
> + if (!frames_size)
> + return 0;
> +
> + request_size = ALIGN(sizeof(struct appletbdrm_fb_request) +
> + frames_size +
> + sizeof(struct appletbdrm_fb_request_footer), 16);
Missing header for ALIGN().
But have you checked overflow.h for the possibility of using some helper macros
from there? This is what should be usually done for k*alloc() in the kernel.
> + appletbdrm_state->request = kzalloc(request_size, GFP_KERNEL);
> +
> + if (!appletbdrm_state->request)
> + return -ENOMEM;
> +
> + appletbdrm_state->response =
> kzalloc(sizeof(*appletbdrm_state->response), GFP_KERNEL);
> +
> + if (!appletbdrm_state->response)
> + return -ENOMEM;
> +
> + appletbdrm_state->request_size = request_size;
> + appletbdrm_state->frames_size = frames_size;
> +
> + return 0;
> +}
...
> +static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
> + struct drm_plane_state *old_state,
> + struct drm_plane_state *state)
> +{
> + struct appletbdrm_plane_state *appletbdrm_state =
> to_appletbdrm_plane_state(state);
> + struct drm_shadow_plane_state *shadow_plane_state =
> to_drm_shadow_plane_state(state);
> + struct appletbdrm_fb_request_response *response =
> appletbdrm_state->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 = appletbdrm_state->request;
> + struct drm_device *drm = &adev->drm;
> + struct appletbdrm_frame *frame;
> + u64 timestamp = ktime_get_ns();
> + struct drm_rect damage;
> + size_t frames_size = appletbdrm_state->frames_size;
> + size_t request_size = appletbdrm_state->request_size;
> + int ret;
> +
> + if (!frames_size)
> + return 0;
> +
> + 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 end_fb_cpu_access;
Strange. Is it for real that this API requires to be called in both cases for
success and for an error?
> + }
> +
> + 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;
Why ' & 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 drm_rect dst_clip = state->dst;
> + struct iosys_map dst = IOSYS_MAP_INIT_VADDR(frame->buf);
> + u32 buf_size = rect_size(&damage);
> +
> + if (!drm_rect_intersect(&dst_clip, &damage))
> + continue;
> +
> + /*
> + * 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);
> +
> + 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;
> + }
> +
> + frame = (void *)frame + struct_size(frame, buf, buf_size);
+ overflow.h
> + }
> +
> + 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);
> +
> + return ret;
> +}
...
> +static void appletbdrm_primary_plane_reset(struct drm_plane *plane)
> +{
> + struct appletbdrm_plane_state *appletbdrm_state;
> +
> + WARN_ON(plane->state);
+ bug.h
> + appletbdrm_state = kzalloc(sizeof(*appletbdrm_state), GFP_KERNEL);
> + if (!appletbdrm_state)
> + return;
> +
> + __drm_gem_reset_shadow_plane(plane, &appletbdrm_state->base);
> +}
...
> +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");
+ dev_printk.h
> + 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),
+ array_size.h
> + 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) {
Why do you need a compound literal here? Perhaps you want to have that to be
done directly in DRM_MODE_INIT()?
> + 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);
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
