Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

16 participants
7076 discussions

Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs

by Thomas Zimmermann

Hi Am 21.02.25 um 12:37 schrieb Aditya Garg: > From: Kerem Karabay <kekrby(a)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 > > Signed-off-by: Kerem Karabay <kekrby(a)gmail.com> > Co-developed-by: Atharva Tiwari <evepolonium(a)gmail.com> > Signed-off-by: Atharva Tiwari <evepolonium(a)gmail.com> > Co-developed-by: Aditya Garg <gargaditya08(a)live.com> > Signed-off-by: Aditya Garg <gargaditya08(a)live.com> > Signed-off-by: Aun-Ali Zaidi <admin(a)kodeit.net> > --- > v2 -> > - Add the driver to MAINTAINERS. > - Allocate memory for request and response in plane's atomic-check helper > - Void the use of drm_fb_blit() > - Implement atomic_disable > - Make PRIME work > - Remove the date field from struct drm_driver > - intersect damage with dst_clip > - Register DRM device in appletbdrm_probe > - Clear the display as the final call in probe > - Select hid_multitouch as well in kconfig > > v3 -> > - Change commit message to credit Ben (Bingxing) Wang > MAINTAINERS | 7 + > drivers/gpu/drm/tiny/Kconfig | 14 + > drivers/gpu/drm/tiny/Makefile | 1 + > drivers/gpu/drm/tiny/appletbdrm.c | 806 ++++++++++++++++++++++++++++++ > 4 files changed, 828 insertions(+) > create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index efee40ea5..43fafaab3 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -7148,6 +7148,13 @@ 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: Aun-Ali Zaidi <admin(a)kodeit.net> You cannot put someone else here. Who is that? > +L: dri-devel(a)lists.freedesktop.org > +S: Maintained > +T: git https://gitlab.freedesktop.org/drm/misc/kernel.git > +F: drivers/gpu/drm/tiny/appletbdrm.c > + > DRM DRIVER FOR ARM PL111 CLCD > M: Linus Walleij <linus.walleij(a)linaro.org> > S: Maintained > diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig > index 94cbdb133..25471791c 100644 > --- a/drivers/gpu/drm/tiny/Kconfig > +++ b/drivers/gpu/drm/tiny/Kconfig > @@ -1,5 +1,19 @@ > # 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 > + 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. > + > 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 Putting this driver into tiny is ok. It's the preferred place for single-file DRM drivers like this one. > new file mode 100644 > index 000000000..a17a3ecc3 > --- /dev/null > +++ b/drivers/gpu/drm/tiny/appletbdrm.c > @@ -0,0 +1,806 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Apple Touch Bar DRM Driver > + * > + * Copyright (c) 2023 Kerem Karabay <kekrby(a)gmail.com> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/module.h> > +#include <linux/unaligned.h> > +#include <linux/usb.h> > + > +#include <drm/drm_atomic.h> > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_damage_helper.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_encoder.h> > +#include <drm/drm_format_helper.h> > +#include <drm/drm_fourcc.h> > +#include <drm/drm_framebuffer.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; This field should not be here. The Linux device is already available at drm.dev. > + struct device *dmadev; > + > + 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; > +}; > + > +struct appletbdrm_plane_state { > + struct drm_shadow_plane_state base; > + struct appletbdrm_fb_request *request; > + struct appletbdrm_fb_request_response *response; > + size_t request_size; > + size_t frames_size; > +}; > + > +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) > +{ > + 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_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_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) > + 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); > + > + 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; AFAIU you're preallocating the memory for the drawing commands. Makes sense to me. > + > + 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; > + } > + > + 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 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); > + } > + > + 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); And here you're building the drawing commands from the allocated frame memory. Also makes sense. > + > + 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_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 void appletbdrm_primary_plane_helper_atomic_disable(struct drm_plane *plane, > + struct drm_atomic_state *state) > +{ > + struct drm_device *dev = plane->dev; > + struct appletbdrm_device *adev = drm_to_adev(dev); > + int idx; > + > + if (!drm_dev_enter(dev, &idx)) > + return; > + > + appletbdrm_clear_display(adev); > + > + drm_dev_exit(idx); > +} > + > +static void appletbdrm_primary_plane_reset(struct drm_plane *plane) > +{ > + struct appletbdrm_plane_state *appletbdrm_state; > + > + WARN_ON(plane->state); > + > + appletbdrm_state = kzalloc(sizeof(*appletbdrm_state), GFP_KERNEL); > + if (!appletbdrm_state) > + return; > + > + __drm_gem_reset_shadow_plane(plane, &appletbdrm_state->base); > +} > + > +static struct drm_plane_state *appletbdrm_primary_plane_duplicate_state(struct drm_plane *plane) > +{ > + struct drm_shadow_plane_state *new_shadow_plane_state; > + struct appletbdrm_plane_state *old_appletbdrm_state; > + struct appletbdrm_plane_state *appletbdrm_state; > + > + if (WARN_ON(!plane->state)) > + return NULL; > + > + old_appletbdrm_state = to_appletbdrm_plane_state(plane->state); > + appletbdrm_state = kmemdup(old_appletbdrm_state, sizeof(*appletbdrm_state), GFP_KERNEL); > + if (!appletbdrm_state) > + return NULL; > + > + /* Request and response are not duplicated and are allocated in .atomic_check */ > + appletbdrm_state->request = NULL; > + appletbdrm_state->response = NULL; You also have to clear the frame and request size, I think. Duplicated state will be processed by the atomic_check before being used. So you'll have a chance of recomputing these temporary buffers. Simply allocate with kzalloc instead of kmemdup() and you'll be fine. > + > + new_shadow_plane_state = &appletbdrm_state->base; > + > + __drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state); This call is important to do. > + > + return &new_shadow_plane_state->base; > +} > + > +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); > + kfree(appletbdrm_state->response); > + > + __drm_gem_destroy_shadow_plane_state(&appletbdrm_state->base); > + > + kfree(appletbdrm_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, > + .atomic_disable = appletbdrm_primary_plane_helper_atomic_disable, > +}; > + > +static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = { > + .update_plane = drm_atomic_helper_update_plane, > + .disable_plane = drm_atomic_helper_disable_plane, > + .reset = appletbdrm_primary_plane_reset, > + .atomic_duplicate_state = appletbdrm_primary_plane_duplicate_state, > + .atomic_destroy_state = appletbdrm_primary_plane_destroy_state, > + .destroy = drm_plane_cleanup, > +}; > + > +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 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, > +}; > + > +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, > +}; > + > +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, > + .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"); Use drm_err() here. It does the right thing. We don't have a drm_dev_probe() yet, but you're discouraged from useing the dev_() print helpers. Same below. > + > + 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; No need to clear this to zero. > + 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); > + > + 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; > + struct drm_device *drm; > + 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; > + > + drm = &adev->drm; > + > + 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_setup_mode_config(adev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to setup mode config\n"); > + > + ret = drm_dev_register(drm, 0); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to register DRM device\n"); > + > + ret = appletbdrm_clear_display(adev); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to clear display\n"); > + > + return 0; > +} > + > +static void appletbdrm_disconnect(struct usb_interface *intf) > +{ > + struct appletbdrm_device *adev = usb_get_intfdata(intf); > + struct drm_device *drm = &adev->drm; > + You have to put_device(dmadev) to release the DMA device. Best regards Thomas > + drm_dev_unplug(drm); > + drm_atomic_helper_shutdown(drm); > +} > + > +static void appletbdrm_shutdown(struct usb_interface *intf) > +{ > + struct appletbdrm_device *adev = usb_get_intfdata(intf); > + > + /* > + * The framebuffer needs to be cleared on shutdown since its content > + * persists across boots > + */ > + drm_atomic_helper_shutdown(&adev->drm); > +} > + > +static const struct usb_device_id appletbdrm_usb_id_table[] = { > + { USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) }, > + {} > +}; > +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table); > + > +static struct usb_driver appletbdrm_usb_driver = { > + .name = "appletbdrm", > + .probe = appletbdrm_probe, > + .disconnect = appletbdrm_disconnect, > + .shutdown = appletbdrm_shutdown, > + .id_table = appletbdrm_usb_id_table, > +}; > +module_usb_driver(appletbdrm_usb_driver); > + > +MODULE_AUTHOR("Kerem Karabay <kekrby(a)gmail.com>"); > +MODULE_DESCRIPTION("Apple Touch Bar DRM Driver"); > +MODULE_LICENSE("GPL"); -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)

1 year, 4 months

Re: [PATCH v3 3/3] drm/tiny: add driver for Apple Touch Bars in x86 Macs

by Thomas Zimmermann

Hi Am 22.02.25 um 10:07 schrieb Aditya Garg: >> What padding, please? Why TCP UAPI headers do not have these attributes? >> Think about it, and think about what actually __packed does and how it affects >> (badly) the code generation. Otherwise it looks like a cargo cult. >> >>> I tried removing __packed btw and driver no longer works. >> So, you need to find a justification why. But definitely not due to padding in >> many of them. They can go without __packed as they are naturally aligned. > Alright, I did some debugging, basically printk sizeof(struct). Did it for both packed and unpacked with the following results: > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_request_header is 16 > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_request_header_unpacked is 16 > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_response_header is 20 > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_response_header_unpacked is 20 > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_simple_request is 32 > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_simple_request_unpacked is 32 > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_information is 65 > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_msg_information_unpacked is 68 In the unpacked version, there is a 3-byte gap after the 'bits_per_pixel' to align the next field. Using __packed removes those gaps at the expense of runtime overhead. > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_frame is 12 > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_frame_unpacked is 12 > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_footer is 80 > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_footer_unpacked is 80 > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request is 48 > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_unpacked is 48 > > Feb 22 13:02:03 MacBook kernel: size of struct appletbdrm_fb_request_response is 40 > Feb 22 13:02:04 MacBook kernel: size of struct appletbdrm_fb_request_response_unpacked is 40 > > So, the difference in sizeof in unpacked and packed is only in appletbdrm_msg_information. So, I kept this packed, and removed it from others. The Touch Bar still works. > > So maybe keep just this packed? The fields in the TCP header are aligned by design. Unfortunately, this hardware's protocol is not. And there's no way of fixing this now. Just keep all of them packed if you want. At least it's clear then what happens. And if your hardware requires this, you can't do much anyway. Best regards Thomas >> >> >> ... >> >>>>> + 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? >>> All these are I/O errors, you got any suggestion? >> Your email client mangled the code so badly that it's hard to read. But I would >> suggest to use -EINTR in the first case, and -EBADMSG. But also you may consider >> -EPROTO. > Thanks >>>>> + } >> ... >> >>>>> + if (ret) >>>>> + return ret; >>>>> + else if (!new_plane_state->visible) >>>> Why 'else'? It's redundant. >>> I’ve just followed what other drm drivers are doing here: >>> >>> https://elixir.bootlin.com/linux/v6.13.3/source/drivers/gpu/drm/tiny/bochs.… >>> https://elixir.bootlin.com/linux/v6.13.3/source/drivers/gpu/drm/tiny/cirrus… >>> >>> And plenty more >> A bad example is still a bad example. 'else' is simply redundant in this >> case and add a noisy to the code. >> >>> I won’t mind removing else. You want that? >> Sure. >> >> ... >> >>>>> + 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. >>> I don’t really think we need a macro here. >> Hmm... is frames_size known to be in a guaranteed range to make sure no >> potential overflow happens? > I don’t really find any cause of potential overflow. > > >>>>> + appletbdrm_state->request = kzalloc(request_size, GFP_KERNEL); >>>>> + >>>>> + if (!appletbdrm_state->request) >>>>> + return -ENOMEM; >> ... >> >>>>> + request->msg_id = timestamp & 0xff; >>>> Why ' & 0xff'? >>> https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/DfrDi… >> This is not an answer. >> Why do you need this here? Isn't the type of msg_id enough? > Hmm, I double checked this. msg_id is u8 in the Linux port so would anyways never exceed 0xff. I’ll remove this. > Its different in the Windows driver. >> ... >> >>>>> + 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()? >>> I really don’t find this as an issue. You want me to declare another structure, basically this?: >> Nope, I'm asking if the DRM_MODE_INIT() is done in a way that it only can be >> used for the static data. Seems like the case. Have you tried to convert >> DRM_MODE_INIT() to be always a compound literal? Does it break things? > Seems to be breaking things. >>> struct drm_display_mode mode = { >>> DRM_MODE_INIT(60, adev->height, adev->width, >>> DRM_MODE_RES_MM(adev->height, 218), >>> DRM_MODE_RES_MM(adev->width, 218)) >>> }; >>> adev->mode = mode; >>> >>>>> + DRM_MODE_INIT(60, adev->height, adev->width, >>>>> + DRM_MODE_RES_MM(adev->height, 218), >>>>> + DRM_MODE_RES_MM(adev->width, 218)) >>>>> + }; >> -- >> With Best Regards, >> Andy Shevchenko >> >> -- -- 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)

1 year, 4 months

Re: [RFC PATCH 0/5] drm/panthor: Protected mode support for Mali CSF GPUs

by Maxime Ripard

On Tue, Feb 11, 2025 at 03:32:23PM +0100, Boris Brezillon wrote: > On Tue, 11 Feb 2025 14:46:56 +0100 > Maxime Ripard <mripard(a)kernel.org> wrote: > > > Hi Boris, > > > > On Fri, Feb 07, 2025 at 04:02:53PM +0100, Boris Brezillon wrote: > > > Sorry for joining the party late, a couple of comments to back Akash > > > and Nicolas' concerns. > > > > > > On Wed, 05 Feb 2025 13:14:14 -0500 > > > Nicolas Dufresne <nicolas(a)ndufresne.ca> wrote: > > > > > > > Le mercredi 05 février 2025 à 15:52 +0100, Maxime Ripard a écrit : > > > > > On Mon, Feb 03, 2025 at 04:43:23PM +0000, Florent Tomasin wrote: > > > > > > Hi Maxime, Nicolas > > > > > > > > > > > > On 30/01/2025 17:47, Nicolas Dufresne wrote: > > > > > > > Le jeudi 30 janvier 2025 à 17:38 +0100, Maxime Ripard a écrit : > > > > > > > > Hi Nicolas, > > > > > > > > > > > > > > > > On Thu, Jan 30, 2025 at 10:59:56AM -0500, Nicolas Dufresne wrote: > > > > > > > > > Le jeudi 30 janvier 2025 à 14:46 +0100, Maxime Ripard a écrit : > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > I started to review it, but it's probably best to discuss it here. > > > > > > > > > > > > > > > > > > > > On Thu, Jan 30, 2025 at 01:08:56PM +0000, Florent Tomasin wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > This is a patch series covering the support for protected mode execution in > > > > > > > > > > > Mali Panthor CSF kernel driver. > > > > > > > > > > > > > > > > > > > > > > The Mali CSF GPUs come with the support for protected mode execution at the > > > > > > > > > > > HW level. This feature requires two main changes in the kernel driver: > > > > > > > > > > > > > > > > > > > > > > 1) Configure the GPU with a protected buffer. The system must provide a DMA > > > > > > > > > > > heap from which the driver can allocate a protected buffer. > > > > > > > > > > > It can be a carved-out memory or dynamically allocated protected memory region. > > > > > > > > > > > Some system includes a trusted FW which is in charge of the protected memory. > > > > > > > > > > > Since this problem is integration specific, the Mali Panthor CSF kernel > > > > > > > > > > > driver must import the protected memory from a device specific exporter. > > > > > > > > > > > > > > > > > > > > Why do you need a heap for it in the first place? My understanding of > > > > > > > > > > your series is that you have a carved out memory region somewhere, and > > > > > > > > > > you want to allocate from that carved out memory region your buffers. > > > > > > > > > > > > > > > > > > > > How is that any different from using a reserved-memory region, adding > > > > > > > > > > the reserved-memory property to the GPU device and doing all your > > > > > > > > > > allocation through the usual dma_alloc_* API? > > > > > > > > > > > > > > > > > > How do you then multiplex this region so it can be shared between > > > > > > > > > GPU/Camera/Display/Codec drivers and also userspace ? > > > > > > > > > > > > > > > > You could point all the devices to the same reserved memory region, and > > > > > > > > they would all allocate from there, including for their userspace-facing > > > > > > > > allocations. > > > > > > > > > > > > > > I get that using memory region is somewhat more of an HW description, and > > > > > > > aligned with what a DT is supposed to describe. One of the challenge is that > > > > > > > Mediatek heap proposal endup calling into their TEE, meaning knowing the region > > > > > > > is not that useful. You actually need the TEE APP guid and its IPC protocol. If > > > > > > > we can dell drivers to use a head instead, we can abstract that SoC specific > > > > > > > complexity. I believe each allocated addressed has to be mapped to a zone, and > > > > > > > that can only be done in the secure application. I can imagine similar needs > > > > > > > when the protection is done using some sort of a VM / hypervisor. > > > > > > > > > > > > > > Nicolas > > > > > > > > > > > > > > > > > > > The idea in this design is to abstract the heap management from the > > > > > > Panthor kernel driver (which consumes a DMA buffer from it). > > > > > > > > > > > > In a system, an integrator would have implemented a secure heap driver, > > > > > > and could be based on TEE or a carved-out memory with restricted access, > > > > > > or else. This heap driver would be responsible of implementing the > > > > > > logic to: allocate, free, refcount, etc. > > > > > > > > > > > > The heap would be retrieved by the Panthor kernel driver in order to > > > > > > allocate protected memory to load the FW and allow the GPU to enter/exit > > > > > > protected mode. This memory would not belong to a user space process. > > > > > > The driver allocates it at the time of loading the FW and initialization > > > > > > of the GPU HW. This is a device globally owned protected memory. > > > > > > > > > > The thing is, it's really not clear why you absolutely need to have the > > > > > Panthor driver involved there. It won't be transparent to userspace, > > > > > since you'd need an extra flag at allocation time, and the buffers > > > > > behave differently. If userspace has to be aware of it, what's the > > > > > advantage to your approach compared to just exposing a heap for those > > > > > secure buffers, and letting userspace allocate its buffers from there? > > > > > > > > Unless I'm mistaken, the Panthor driver loads its own firmware. Since loading > > > > the firmware requires placing the data in a protected memory region, and that > > > > this aspect has no exposure to userspace, how can Panthor not be implicated ? > > > > > > Right, the very reason we need protected memory early is because some > > > FW sections need to be allocated from the protected pool, otherwise the > > > TEE will fault as soon at the FW enters the so-called 'protected mode'. > > > > How does that work if you don't have some way to allocate the protected > > memory? You can still submit jobs to the GPU, but you can't submit / > > execute "protected jobs"? > > Exactly. > > > > > > Now, it's not impossible to work around this limitation. For instance, > > > we could load the FW without this protected section by default (what we > > > do right now), and then provide a DRM_PANTHOR_ENABLE_FW_PROT_MODE > > > ioctl that would take a GEM object imported from a dmabuf allocated > > > from the protected dma-heap by userspace. We can then reset the FW and > > > allow it to operate in protected mode after that point. > > > > Urgh, I'd rather avoid that dance if possible :) > > Me too. > > > > > > This approach has two downsides though: > > > > > > 1. We have no way of checking that the memory we're passed is actually > > > suitable for FW execution in a protected context. If we're passed > > > random memory, this will likely hang the platform as soon as we enter > > > protected mode. > > > > It's a current limitation of dma-buf in general, and you'd have the same > > issue right now if someone imports a buffer, or misconfigure the heap > > for a !protected heap. > > > > I'd really like to have some way to store some metadata in dma_buf, if > > only to tell that the buffer is protected. > > The dma_buf has a pointer to its ops, so it should be relatively easy > to add an is_dma_buf_coming_from_this_heap() helper. Of course this > implies linking the consumer driver to the heap it's supposed to take > protected buffers from, which is basically the thing being discussed > here :-). I'm not sure looking at the ops would be enough. Like, you can compare that the buffer you allocated come from the heap you got from the DT, but if that heap doesn't allocate protected buffers, you're screwed and you have no way to tell. It also falls apart if we have a heap driver with multiple instances, which is pretty likely if we ever merge the carveout heap driver. > > > > I suspect you'd also need that if you do things like do protected video > > playback through a codec, get a protected frame, and want to import that > > into the GPU. Depending on how you allocate it, either the codec or the > > GPU or both will want to make sure it's protected. > > If it's all allocated from a central "protected" heap (even if that > goes through the driver calling the dma_heap_alloc_buffer()), it > shouldn't be an issue. Right, assuming we have a way to identify the heap the buffer was allocated from somehow. This kind of assumes that you only ever get one source of protected memory, and you'd never allocate a protected buffer from a different one in the codec driver for example. > > > 2. If the driver already boot the FW and exposed a DRI node, we might > > > have GPU workloads running, and doing a FW reset might incur a slight > > > delay in GPU jobs execution. > > > > > > I think #1 is a more general issue that applies to suspend buffers > > > allocated for GPU contexts too. If we expose ioctls where we take > > > protected memory buffers that can possibly lead to crashes if they are > > > not real protected memory regions, and we have no way to ensure the > > > memory is protected, we probably want to restrict these ioctls/modes to > > > some high-privilege CAP_SYS_. > > > > > > For #2, that's probably something we can live with, since it's a > > > one-shot thing. If it becomes an issue, we can even make sure we enable > > > the FW protected-mode before the GPU starts being used for real. > > > > > > This being said, I think the problem applies outside Panthor, and it > > > might be that the video codec can't reset the FW/HW block to switch to > > > protected mode as easily as Panthor. > > > > > > Note that there's also downsides to the reserved-memory node approach, > > > where some bootloader stage would ask the secure FW to reserve a > > > portion of mem and pass this through the DT. This sort of things tend to > > > be an integration mess, where you need all the pieces of the stack (TEE, > > > u-boot, MTK dma-heap driver, gbm, ...) to be at a certain version to > > > work properly. If we go the ioctl() way, we restrict the scope to the > > > TEE, gbm/mesa and the protected-dma-heap driver, which is still a lot, > > > but we've ripped the bootloader out of the equation at least. > > > > Yeah. I also think there's two discussions in parallel here: > > > > 1) Being able to allocate protected buffers from the driver > > 2) Exposing an interface to allocate those to userspace > > > > I'm not really convinced we need 2, but 1 is obviously needed from what > > you're saying. > > I suspect we need #2 for GBM, still. But that's what dma-heaps are for, > so I don't think that's a problem. Yeah, that was my point too, we have the heaps for that already. Maxime

1 year, 4 months

[PATCH v5 0/7] TEE subsystem for restricted dma-buf allocations

by Jens Wiklander

Hi, [Now with a Gstreamer demo, see below.] This patch set allocates the restricted DMA-bufs via the TEE subsystem. The TEE subsystem handles the DMA-buf allocations since it is the TEE (OP-TEE, AMD-TEE, TS-TEE, or perhaps a future QCOMTEE) which sets up the restrictions for the memory used for the DMA-bufs. I've added a new IOCTL, TEE_IOC_RSTMEM_ALLOC, to allocate the restricted DMA-bufs. This IOCTL reaches the backend TEE driver, allowing it to choose how to allocate the restricted physical memory. TEE_IOC_RSTMEM_ALLOC takes in addition to a size and flags parameters also a use-case parameter. This is used by the backend TEE driver to decide on allocation policy and which devices should be able to access the memory. Three use-cases (Secure Video Playback, Trusted UI, and Secure Video Recording) has been identified so far to serve as examples of what can be expected. More use-cases can be added in userspace ABI, but it's up to the backend TEE drivers to provide the implementation. Each use-case has it's own restricted memory pool since different use-cases requires isolation from different parts of the system. A restricted memory pool can be based on a static carveout instantiated while probing the TEE backend driver, or dynamically allocated from CMA and made restricted as needed by the TEE. This can be tested on a RockPi 4B+ with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m rockpi4.xml \ -b prototype/sdp-v5 repo sync -j8 cd build make toolchains -j$(nproc) make all -j$(nproc) # Copy ../out/rockpi4.img to an SD card and boot the RockPi from that # Connect a monitor to the RockPi # login and at the prompt: gst-launch-1.0 videotestsrc ! \ aesenc key=1f9423681beb9a79215820f6bda73d0f \ iv=e9aa8e834d8d70b7e0d254ff670dd718 serialize-iv=true ! \ aesdec key=1f9423681beb9a79215820f6bda73d0f ! \ kmssink The aesdec module has been hacked to use an OP-TEE TA to decrypt the stream into restricted DMA-bufs which are consumed by the kmssink. The primitive QEMU tests from previous patch set can be tested on RockPi in the same way with: xtest --sdp-basic The primitive test are tested on QEMU with the following steps: repo init -u https://github.com/jenswi-linaro/manifest.git -m qemu_v8.xml \ -b prototype/sdp-v5 repo sync -j8 cd build make toolchains -j$(nproc) make SPMC_AT_EL=1 all -j$(nproc) make SPMC_AT_EL=1 run-only # login and at the prompt: xtest --sdp-basic The SPMC_AT_EL=1 parameter configures the build with FF-A and an SPMC at S-EL1 inside OP-TEE. The parameter can be changed into SPMC_AT_EL=n to test without FF-A using the original SMC ABI instead. Please remember to do %rm -rf ../trusted-firmware-a/build/qemu for TF-A to be rebuilt properly using the new configuration. https://optee.readthedocs.io/en/latest/building/prerequisites.html list dependencies needed to build the above. The tests are pretty basic, mostly checking that a Trusted Application in the secure world can access and manipulate the memory. There are also some negative tests for out of bounds buffers etc. Thanks, Jens Changes since V4: * Adding the patch "tee: add TEE_IOC_RSTMEM_FD_INFO" needed by the GStreamer demo * Removing the dummy CPU access and mmap functions from the dma_buf_ops * Fixing a compile error in "optee: FF-A: dynamic restricted memory allocation" reported by kernel test robot <lkp(a)intel.com> Changes since V3: * Make the use_case and flags field in struct tee_shm u32's instead of u16's * Add more description for TEE_IOC_RSTMEM_ALLOC in the header file * Import namespace DMA_BUF in module tee, reported by lkp(a)intel.com * Added a note in the commit message for "optee: account for direction while converting parameters" why it's needed * Factor out dynamic restricted memory allocation from "optee: support restricted memory allocation" into two new commits "optee: FF-A: dynamic restricted memory allocation" and "optee: smc abi: dynamic restricted memory allocation" * Guard CMA usage with #ifdef CONFIG_CMA, effectively disabling dynamic restricted memory allocate if CMA isn't configured Changes since the V2 RFC: * Based on v6.12 * Replaced the flags for SVP and Trusted UID memory with a u32 field with unique id for each use case * Added dynamic allocation of restricted memory pools * Added OP-TEE ABI both with and without FF-A for dynamic restricted memory * Added support for FF-A with FFA_LEND Changes since the V1 RFC: * Based on v6.11 * Complete rewrite, replacing the restricted heap with TEE_IOC_RSTMEM_ALLOC Changes since Olivier's post [2]: * Based on Yong Wu's post [1] where much of dma-buf handling is done in the generic restricted heap * Simplifications and cleanup * New commit message for "dma-buf: heaps: add Linaro restricted dmabuf heap support" * Replaced the word "secure" with "restricted" where applicable Jens Wiklander (7): tee: add restricted memory allocation tee: add TEE_IOC_RSTMEM_FD_INFO optee: account for direction while converting parameters optee: sync secure world ABI headers optee: support restricted memory allocation optee: FF-A: dynamic restricted memory allocation optee: smc abi: dynamic restricted memory allocation drivers/tee/Makefile | 1 + drivers/tee/optee/Makefile | 1 + drivers/tee/optee/call.c | 10 +- drivers/tee/optee/core.c | 1 + drivers/tee/optee/ffa_abi.c | 179 +++++++++++++- drivers/tee/optee/optee_ffa.h | 27 ++- drivers/tee/optee/optee_msg.h | 65 ++++- drivers/tee/optee/optee_private.h | 75 ++++-- drivers/tee/optee/optee_smc.h | 71 +++++- drivers/tee/optee/rpc.c | 31 ++- drivers/tee/optee/rstmem.c | 389 ++++++++++++++++++++++++++++++ drivers/tee/optee/smc_abi.c | 215 +++++++++++++++-- drivers/tee/tee_core.c | 69 +++++- drivers/tee/tee_private.h | 4 + drivers/tee/tee_rstmem.c | 188 +++++++++++++++ drivers/tee/tee_shm.c | 2 + drivers/tee/tee_shm_pool.c | 69 +++++- include/linux/tee_core.h | 15 ++ include/linux/tee_drv.h | 2 + include/uapi/linux/tee.h | 71 +++++- 20 files changed, 1409 insertions(+), 76 deletions(-) create mode 100644 drivers/tee/optee/rstmem.c create mode 100644 drivers/tee/tee_rstmem.c base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 -- 2.43.0

1 year, 4 months

Re: [PATCH 4/4] dma-buf: drop caching of sg_tables

by Christian König

Am 17.02.25 um 17:28 schrieb Simona Vetter: > On Tue, Feb 11, 2025 at 05:31:09PM +0100, Christian König wrote: >> That was purely for the transition from static to dynamic dma-buf >> handling and can be removed again now. >> >> Signed-off-by: Christian König <christian.koenig(a)amd.com> > Yay! > > Might uncover some fun if people have meanwhile started to rely on this > for perf or something. But we'll figure that out when it happens. Yeah that was exactly the reason why I made this a separate patch and didn't mixed it into patch #3. On the other hand I'm fine to keep it a bit longer if anybody complains. Christian. > > Reviewed-by: Simona Vetter <simona.vetter(a)ffwll.ch> > > >> --- >> drivers/dma-buf/dma-buf.c | 34 -------------------------- >> drivers/dma-buf/udmabuf.c | 1 - >> drivers/gpu/drm/drm_prime.c | 1 - >> drivers/gpu/drm/virtio/virtgpu_prime.c | 1 - >> include/linux/dma-buf.h | 13 ---------- >> 5 files changed, 50 deletions(-) >> >> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >> index 357b94a3dbaa..35221c4ddbf5 100644 >> --- a/drivers/dma-buf/dma-buf.c >> +++ b/drivers/dma-buf/dma-buf.c >> @@ -636,10 +636,6 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) >> || !exp_info->ops->release)) >> return ERR_PTR(-EINVAL); >> >> - if (WARN_ON(exp_info->ops->cache_sgt_mapping && >> - (exp_info->ops->pin || exp_info->ops->unpin))) >> - return ERR_PTR(-EINVAL); >> - >> if (WARN_ON(!exp_info->ops->pin != !exp_info->ops->unpin)) >> return ERR_PTR(-EINVAL); >> >> @@ -964,17 +960,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) >> return; >> >> dma_resv_lock(dmabuf->resv, NULL); >> - >> - if (attach->sgt) { >> - mangle_sg_table(attach->sgt); >> - attach->dmabuf->ops->unmap_dma_buf(attach, attach->sgt, >> - attach->dir); >> - >> - if (dma_buf_pin_on_map(attach)) >> - dma_buf_unpin(attach); >> - } >> list_del(&attach->node); >> - >> dma_resv_unlock(dmabuf->resv); >> >> if (dmabuf->ops->detach) >> @@ -1069,18 +1055,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, >> >> dma_resv_assert_held(attach->dmabuf->resv); >> >> - if (attach->sgt) { >> - /* >> - * Two mappings with different directions for the same >> - * attachment are not allowed. >> - */ >> - if (attach->dir != direction && >> - attach->dir != DMA_BIDIRECTIONAL) >> - return ERR_PTR(-EBUSY); >> - >> - return attach->sgt; >> - } >> - >> if (dma_buf_pin_on_map(attach)) { >> ret = attach->dmabuf->ops->pin(attach); >> if (ret) >> @@ -1105,11 +1079,6 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, >> } >> mangle_sg_table(sg_table); >> >> - if (attach->dmabuf->ops->cache_sgt_mapping) { >> - attach->sgt = sg_table; >> - attach->dir = direction; >> - } >> - >> #ifdef CONFIG_DMA_API_DEBUG >> { >> struct scatterlist *sg; >> @@ -1190,9 +1159,6 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, >> >> dma_resv_assert_held(attach->dmabuf->resv); >> >> - if (attach->sgt == sg_table) >> - return; >> - >> mangle_sg_table(sg_table); >> attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); >> >> diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c >> index cc7398cc17d6..2fa2c9135eac 100644 >> --- a/drivers/dma-buf/udmabuf.c >> +++ b/drivers/dma-buf/udmabuf.c >> @@ -285,7 +285,6 @@ static int end_cpu_udmabuf(struct dma_buf *buf, >> } >> >> static const struct dma_buf_ops udmabuf_ops = { >> - .cache_sgt_mapping = true, >> .map_dma_buf = map_udmabuf, >> .unmap_dma_buf = unmap_udmabuf, >> .release = release_udmabuf, >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index 32a8781cfd67..c284f306d597 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -810,7 +810,6 @@ int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma) >> EXPORT_SYMBOL(drm_gem_dmabuf_mmap); >> >> static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { >> - .cache_sgt_mapping = true, >> .attach = drm_gem_map_attach, >> .detach = drm_gem_map_detach, >> .map_dma_buf = drm_gem_map_dma_buf, >> diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c >> index fe6a0b018571..c6f3be3cb914 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_prime.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c >> @@ -75,7 +75,6 @@ static void virtgpu_gem_unmap_dma_buf(struct dma_buf_attachment *attach, >> >> static const struct virtio_dma_buf_ops virtgpu_dmabuf_ops = { >> .ops = { >> - .cache_sgt_mapping = true, >> .attach = virtio_dma_buf_attach, >> .detach = drm_gem_map_detach, >> .map_dma_buf = virtgpu_gem_map_dma_buf, >> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h >> index c54ff2dda8cb..544f8f8c3f44 100644 >> --- a/include/linux/dma-buf.h >> +++ b/include/linux/dma-buf.h >> @@ -34,15 +34,6 @@ struct dma_buf_attachment; >> * @vunmap: [optional] unmaps a vmap from the buffer >> */ >> struct dma_buf_ops { >> - /** >> - * @cache_sgt_mapping: >> - * >> - * If true the framework will cache the first mapping made for each >> - * attachment. This avoids creating mappings for attachments multiple >> - * times. >> - */ >> - bool cache_sgt_mapping; >> - >> /** >> * @attach: >> * >> @@ -493,8 +484,6 @@ struct dma_buf_attach_ops { >> * @dmabuf: buffer for this attachment. >> * @dev: device attached to the buffer. >> * @node: list of dma_buf_attachment, protected by dma_resv lock of the dmabuf. >> - * @sgt: cached mapping. >> - * @dir: direction of cached mapping. >> * @peer2peer: true if the importer can handle peer resources without pages. >> * @priv: exporter specific attachment data. >> * @importer_ops: importer operations for this attachment, if provided >> @@ -514,8 +503,6 @@ struct dma_buf_attachment { >> struct dma_buf *dmabuf; >> struct device *dev; >> struct list_head node; >> - struct sg_table *sgt; >> - enum dma_data_direction dir; >> bool peer2peer; >> const struct dma_buf_attach_ops *importer_ops; >> void *importer_priv; >> -- >> 2.34.1 >>

1 year, 4 months

Bunch of DMA-buf cleanup patches

by Christian König

Hello everyone, just a few DMA-buf cleanup patches. The first one is fixing an incorrect documentation which has annoyed me for quite a while. The rest basically just removes stuff we no longer need or which was just added as abstraction which was never used. Please review and/or voice objection if some of that is till needed for something. Regards, Christian.

1 year, 4 months

Re: [PATCH v4 0/6] TEE subsystem for restricted dma-buf allocations

by Daniel Stone

Hi, On Thu, 13 Feb 2025 at 12:40, Boris Brezillon <boris.brezillon(a)collabora.com> wrote: > On Thu, 13 Feb 2025 14:46:01 +0530 Sumit Garg <sumit.garg(a)linaro.org> wrote: > > Yeah but all the prior vendor specific secure/restricted DMA heaps > > relied on DT information. > > Right, but there's nothing in the DMA heap provider API forcing that. Yeah. DMA heaps are just a way to allocate memory from a specific place. It allows people to settle on having a single way to do allocations from weird platform-specific places; the only weird platform-specific part userspace needs to deal with is figuring out the name to use. The rest is at least a unified API: the point of dma-heaps was exactly to have a single coherent API for userspace, not to create one API for ZONE_CMA and DT ranges and everyone else doing their own thing. > > Rather than that it's better > > for the user to directly ask the TEE device to allocate restricted > > memory without worrying about how the memory restriction gets > > enforced. > > If the consensus is that restricted/protected memory allocation should > always be routed to the TEE, sure, but I had the feeling this wasn't as > clear as that. OTOH, using a dma-heap to expose the TEE-SDP > implementation provides the same benefits, without making potential > future non-TEE based implementations a pain for users. The dma-heap > ioctl being common to all implementations, it just becomes a > configuration matter if we want to change the heap we rely on for > protected/restricted buffer allocation. And because heaps have > unique/well-known names, users can still default to (or rely solely on) > the TEE-SPD implementation if they want. > > > There have been several attempts with DMA heaps in the past which all > > resulted in a very vendor specific vertically integrated solution. But > > the solution with TEE subsystem aims to make it generic and vendor > > agnostic. > > Just because all previous protected/restricted dma-heap effort > failed to make it upstream, doesn't mean dma-heap is the wrong way of > exposing this feature IMHO. To be fair, having a TEE implementation does give us a much better chance of having a sensible cross-vendor plan. And the fact it's already (sort of accidentally and only on one platform AFAICT) ready for a 'test' interface, where we can still exercise protected allocation paths but without having to go through all the platform-specific setup that is inaccessible to most people, is also really great! That's probably been the biggest barrier to having this tested outside of IHVs and OEMs. But just because TEE is one good backend implementation, doesn't mean it should be the userspace ABI. Why should userspace care that TEE has mediated the allocation instead of it being a predefined range within DT? How does userspace pick which TEE device to use? What advantage does userspace get from having to have a different codepath to get a different handle to memory? What about x86? I think this proposal is looking at it from the wrong direction. Instead of working upwards from the implementation to userspace, start with userspace and work downwards. The interesting property to focus on is allocating memory, not that EL1 is involved behind the scenes. Cheers, Daniel

1 year, 4 months

Re: [PATCH v4 0/6] TEE subsystem for restricted dma-buf allocations

by Sumit Garg

On Thu, 13 Feb 2025 at 14:06, Boris Brezillon <boris.brezillon(a)collabora.com> wrote: > > On Thu, 13 Feb 2025 12:11:52 +0530 > Sumit Garg <sumit.garg(a)linaro.org> wrote: > > > Hi Boris, > > > > On Thu, 13 Feb 2025 at 01:26, Boris Brezillon > > <boris.brezillon(a)collabora.com> wrote: > > > > > > +Florent, who's working on protected-mode support in Panthor. > > > > > > Hi Jens, > > > > > > On Tue, 17 Dec 2024 11:07:36 +0100 > > > Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > > > > > Hi, > > > > > > > > This patch set allocates the restricted DMA-bufs via the TEE subsystem. > > > > > > We're currently working on protected-mode support for Panthor [1] and it > > > looks like your series (and the OP-TEE implementation that goes with > > > it) would allow us to have a fully upstream/open solution for the > > > protected content use case we're trying to support. I need a bit more > > > time to play with the implementation but this looks very promising > > > (especially the lend rstmem feature, which might help us allocate our > > > FW sections that are supposed to execute code accessing protected > > > content). > > > > Glad to hear that, if you can demonstrate an open source use case > > based on this series then it will help to land it. We really would > > love to see support for restricted DMA-buf consumers be it GPU, crypto > > accelerator, media pipeline etc. > > > > > > > > > > > > > The TEE subsystem handles the DMA-buf allocations since it is the TEE > > > > (OP-TEE, AMD-TEE, TS-TEE, or perhaps a future QCOMTEE) which sets up the > > > > restrictions for the memory used for the DMA-bufs. > > > > > > > > I've added a new IOCTL, TEE_IOC_RSTMEM_ALLOC, to allocate the restricted > > > > DMA-bufs. This IOCTL reaches the backend TEE driver, allowing it to choose > > > > how to allocate the restricted physical memory. > > > > > > I'll probably have more questions soon, but here's one to start: any > > > particular reason you didn't go for a dma-heap to expose restricted > > > buffer allocation to userspace? I see you already have a cdev you can > > > take ioctl()s from, but my understanding was that dma-heap was the > > > standard solution for these device-agnostic/central allocators. > > > > This series started with the DMA heap approach only here [1] but later > > discussions [2] lead us here. To point out specifically: > > > > - DMA heaps require reliance on DT to discover static restricted > > regions carve-outs whereas via the TEE implementation driver (eg. > > OP-TEE) those can be discovered dynamically. > > Hm, the system heap [1] doesn't rely on any DT information AFAICT. Yeah but all the prior vendor specific secure/restricted DMA heaps relied on DT information. > The dynamic allocation scheme, where the TEE implementation allocates a > chunk of protected memory for us would have a similar behavior, I guess. In a dynamic scheme, the allocation will still be from CMA or system heap depending on TEE implementation capabilities but the restriction will be enforced via interaction with TEE. > > > - Dynamic allocation of buffers and making them restricted requires > > vendor specific driver hooks with DMA heaps whereas the TEE subsystem > > abstracts that out with underlying TEE implementation (eg. OP-TEE) > > managing the dynamic buffer restriction. > > Yeah, the lend rstmem feature is clearly something tee specific, and I > think that's okay to assume the user knows the protection request > should go through the tee subsystem in that case. Yeah but how will the user discover that? Rather than that it's better for the user to directly ask the TEE device to allocate restricted memory without worrying about how the memory restriction gets enforced. > > > - TEE subsystem already has a well defined user-space interface for > > managing shared memory buffers with TEE and restricted DMA buffers > > will be yet another interface managed along similar lines. > > Okay, so the very reason I'm asking about the dma-buf heap interface is > because there might be cases where the protected/restricted allocation > doesn't go through the TEE (Mediatek has a TEE-free implementation > for instance, but I realize vendor implementations are probably not the > best selling point :-/). You can always have a system with memory and peripheral access permissions setup during boot (or even have a pre-configured hardware as a special case) prior to booting up the kernel too. But that even gets somehow configured by a TEE implementation during boot, so calling it a TEE-free implementation seems over-simplified and not a scalable solution. However, this patchset [1] from Mediatek requires runtime TEE interaction too. [1] https://lore.kernel.org/linux-arm-kernel/20240515112308.10171-1-yong.wu@med… > If we expose things as a dma-heap, we have > a solution where integrators can pick the dma-heap they think is > relevant for protected buffer allocations without the various drivers > (GPU, video codec, ...) having to implement a dispatch function for all > possible implementations. The same goes for userspace allocations, > where passing a dma-heap name, is simpler than supporting different > ioctl()s based on the allocation backend. There have been several attempts with DMA heaps in the past which all resulted in a very vendor specific vertically integrated solution. But the solution with TEE subsystem aims to make it generic and vendor agnostic. > > [1]https://elixir.bootlin.com/linux/v6.13.2/source/drivers/dma-buf/heaps/sys… -Sumit

1 year, 4 months

Re: [PATCH v4 0/6] TEE subsystem for restricted dma-buf allocations

by Sumit Garg

Hi Boris, On Thu, 13 Feb 2025 at 01:26, Boris Brezillon <boris.brezillon(a)collabora.com> wrote: > > +Florent, who's working on protected-mode support in Panthor. > > Hi Jens, > > On Tue, 17 Dec 2024 11:07:36 +0100 > Jens Wiklander <jens.wiklander(a)linaro.org> wrote: > > > Hi, > > > > This patch set allocates the restricted DMA-bufs via the TEE subsystem. > > We're currently working on protected-mode support for Panthor [1] and it > looks like your series (and the OP-TEE implementation that goes with > it) would allow us to have a fully upstream/open solution for the > protected content use case we're trying to support. I need a bit more > time to play with the implementation but this looks very promising > (especially the lend rstmem feature, which might help us allocate our > FW sections that are supposed to execute code accessing protected > content). Glad to hear that, if you can demonstrate an open source use case based on this series then it will help to land it. We really would love to see support for restricted DMA-buf consumers be it GPU, crypto accelerator, media pipeline etc. > > > > > The TEE subsystem handles the DMA-buf allocations since it is the TEE > > (OP-TEE, AMD-TEE, TS-TEE, or perhaps a future QCOMTEE) which sets up the > > restrictions for the memory used for the DMA-bufs. > > > > I've added a new IOCTL, TEE_IOC_RSTMEM_ALLOC, to allocate the restricted > > DMA-bufs. This IOCTL reaches the backend TEE driver, allowing it to choose > > how to allocate the restricted physical memory. > > I'll probably have more questions soon, but here's one to start: any > particular reason you didn't go for a dma-heap to expose restricted > buffer allocation to userspace? I see you already have a cdev you can > take ioctl()s from, but my understanding was that dma-heap was the > standard solution for these device-agnostic/central allocators. This series started with the DMA heap approach only here [1] but later discussions [2] lead us here. To point out specifically: - DMA heaps require reliance on DT to discover static restricted regions carve-outs whereas via the TEE implementation driver (eg. OP-TEE) those can be discovered dynamically. - Dynamic allocation of buffers and making them restricted requires vendor specific driver hooks with DMA heaps whereas the TEE subsystem abstracts that out with underlying TEE implementation (eg. OP-TEE) managing the dynamic buffer restriction. - TEE subsystem already has a well defined user-space interface for managing shared memory buffers with TEE and restricted DMA buffers will be yet another interface managed along similar lines. [1] https://lore.kernel.org/lkml/mzur3odofwwrdqnystozjgf3qtvb73wqjm6g2vf5dfsqie… [2] https://lore.kernel.org/lkml/CAFA6WYPtp3H5JhxzgH9=z2EvNL7Kdku3EmG1aDkTS-gjF… -Sumit > > Regards, > > Boris > > [1]https://lwn.net/ml/all/cover.1738228114.git.florent.tomasin@arm.com/#t

1 year, 4 months

Re: [RFC PATCH 1/5] dt-bindings: dma: Add CMA Heap bindings

by Maxime Ripard

On Wed, Feb 12, 2025 at 10:29:32AM +0000, Florent Tomasin wrote: > > > On 12/02/2025 10:01, Maxime Ripard wrote: > > On Wed, Feb 12, 2025 at 09:49:56AM +0000, Florent Tomasin wrote: > >> Note that the CMA patches were initially shared to help reproduce my > >> environment of development, I can isolate them in a separate patch > >> series and include a reference or "base-commit:" tag to it in the > >> Panthor protected mode RFC, to help progress this review in another > >> thread. It will avoid overlapping these two topics: > >> > >> - Multiple standalone CMA heaps support > >> - Panthor protected mode handling > > > > You keep insisting on using CMA here, but it's really not clear to me > > why you would need CMA in the first place. > > > > By CMA, do you mean the CMA allocator, and thus would provide buffers > > through the usual dma_alloc_* API, or would any allocator providing > > physically contiguous memory work? > > You are correct only the CMA allocator is relevant. I needed a way to > sub-allocate from a carved-out memory. I'm still confused, sorry. You're saying that you require CMA but... > > In the latter case, would something like this work: > > https://lore.kernel.org/all/20240515-dma-buf-ecc-heap-v1-1-54cbbd049511@ker… > > Thanks for sharing this link, I was not aware previous work was done > on this aspect. The new carveout heap introduced in the series could > probably be a good alternative. I will play-around with it and share > some updates. ... you seem to be ok with a driver that doesn't use it? Maxime

1 year, 4 months

Jump to page:

2026

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig