Message-ID: <8051F1F7-C1B0-428E-BE12-353C242EA650@live.com>
Date: Tue, 18 Feb 2025 13:49:32 +0000
From: Aditya Garg <gargaditya08@...e.com>
To: Thomas Zimmermann <tzimmermann@...e.de>
CC: "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
	"mripard@...nel.org" <mripard@...nel.org>, "airlied@...il.com"
	<airlied@...il.com>, "simona@...ll.ch" <simona@...ll.ch>, Kerem Karabay
	<kekrby@...il.com>, Atharva Tiwari <evepolonium@...il.com>, Linux Kernel
 Mailing List <linux-kernel@...r.kernel.org>,
	"dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/tiny: add driver for Apple Touch Bars in x86 Macs


Hi

>>  drivers/gpu/drm/tiny/appletbdrm.c | 702 ++++++++++++++++++++++++++++++
> 
> You also have to add the driver to MAINTAINERS.

Sorry, forgot about that

> 
>>  3 files changed, 716 insertions(+)
>>  create mode 100644 drivers/gpu/drm/tiny/appletbdrm.c
>> 
>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>> index 94cbdb133..76d1a2f9f 100644
>> --- a/drivers/gpu/drm/tiny/Kconfig
>> +++ b/drivers/gpu/drm/tiny/Kconfig
>> @@ -1,5 +1,18 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  +config DRM_APPLETBDRM
>> + tristate "DRM support for Apple Touch Bars"
>> + depends on DRM && USB && MMU
>> + select DRM_GEM_SHMEM_HELPER
>> + select DRM_KMS_HELPER
>> + select HID_APPLETB_BL
>> + help
>> +   Say Y here if you want support for the display of Touch Bars on x86
>> +   MacBook Pros.
>> +
>> +   To compile this driver as a module, choose M here: the
>> +   module will be called appletbdrm.
>> +
>>  config DRM_ARCPGU
>>   tristate "ARC PGU"
>>   depends on DRM && OF
>> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
>> index 60816d2eb..0a3a7837a 100644
>> --- a/drivers/gpu/drm/tiny/Makefile
>> +++ b/drivers/gpu/drm/tiny/Makefile
>> @@ -1,5 +1,6 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>>  +obj-$(CONFIG_DRM_APPLETBDRM) += appletbdrm.o
>>  obj-$(CONFIG_DRM_ARCPGU) += arcpgu.o
>>  obj-$(CONFIG_DRM_BOCHS) += bochs.o
>>  obj-$(CONFIG_DRM_CIRRUS_QEMU) += cirrus-qemu.o
>> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
>> new file mode 100644
>> index 000000000..f056f6280
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
>> @@ -0,0 +1,702 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Apple Touch Bar DRM Driver
>> + *
>> + * Copyright (c) 2023 Kerem Karabay <kekrby@...il.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_gem_atomic_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/drm_plane.h>
>> +#include <drm/drm_probe_helper.h>
>> +
>> +#define __APPLETBDRM_MSG_STR4(str4) ((__le32 __force)((str4[0] << 24) | (str4[1] << 16) | (str4[2] << 8) | str4[3]))
>> +#define __APPLETBDRM_MSG_TOK4(tok4) __APPLETBDRM_MSG_STR4(#tok4)
>> +
>> +#define APPLETBDRM_PIXEL_FORMAT __APPLETBDRM_MSG_TOK4(RGBA) /* The actual format is BGR888 */
>> +#define APPLETBDRM_BITS_PER_PIXEL 24
>> +
>> +#define APPLETBDRM_MSG_CLEAR_DISPLAY __APPLETBDRM_MSG_TOK4(CLRD)
>> +#define APPLETBDRM_MSG_GET_INFORMATION __APPLETBDRM_MSG_TOK4(GINF)
>> +#define APPLETBDRM_MSG_UPDATE_COMPLETE __APPLETBDRM_MSG_TOK4(UDCL)
>> +#define APPLETBDRM_MSG_SIGNAL_READINESS __APPLETBDRM_MSG_TOK4(REDY)
>> +
>> +#define APPLETBDRM_BULK_MSG_TIMEOUT 1000
>> +
>> +#define drm_to_adev(_drm) container_of(_drm, struct appletbdrm_device, drm)
>> +#define adev_to_udev(adev) interface_to_usbdev(to_usb_interface(adev->dev))
>> +
>> +struct appletbdrm_msg_request_header {
>> + __le16 unk_00;
>> + __le16 unk_02;
>> + __le32 unk_04;
>> + __le32 unk_08;
>> + __le32 size;
>> +} __packed;
>> +
>> +struct appletbdrm_msg_response_header {
>> + u8 unk_00[16];
>> + __le32 msg;
>> +} __packed;
>> +
>> +struct appletbdrm_msg_simple_request {
>> + struct appletbdrm_msg_request_header header;
>> + __le32 msg;
>> + u8 unk_14[8];
>> + __le32 size;
>> +} __packed;
>> +
>> +struct appletbdrm_msg_information {
>> + struct appletbdrm_msg_response_header header;
>> + u8 unk_14[12];
>> + __le32 width;
>> + __le32 height;
>> + u8 bits_per_pixel;
>> + __le32 bytes_per_row;
>> + __le32 orientation;
>> + __le32 bitmap_info;
>> + __le32 pixel_format;
>> + __le32 width_inches; /* floating point */
>> + __le32 height_inches; /* floating point */
>> +} __packed;
>> +
>> +struct appletbdrm_frame {
>> + __le16 begin_x;
>> + __le16 begin_y;
>> + __le16 width;
>> + __le16 height;
>> + __le32 buf_size;
>> + u8 buf[];
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request_footer {
>> + u8 unk_00[12];
>> + __le32 unk_0c;
>> + u8 unk_10[12];
>> + __le32 unk_1c;
>> + __le64 timestamp;
>> + u8 unk_28[12];
>> + __le32 unk_34;
>> + u8 unk_38[20];
>> + __le32 unk_4c;
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request {
>> + struct appletbdrm_msg_request_header header;
>> + __le16 unk_10;
>> + u8 msg_id;
>> + u8 unk_13[29];
>> + /*
>> +  * Contents of `data`:
>> +  * - struct appletbdrm_frame frames[];
>> +  * - struct appletbdrm_fb_request_footer footer;
>> +  * - padding to make the total size a multiple of 16
>> +  */
>> + u8 data[];
>> +} __packed;
>> +
>> +struct appletbdrm_fb_request_response {
>> + struct appletbdrm_msg_response_header header;
>> + u8 unk_14[12];
>> + __le64 timestamp;
>> +} __packed;
>> +
>> +struct appletbdrm_device {
>> + struct device *dev;
>> +
>> + unsigned int in_ep;
>> + unsigned int out_ep;
>> +
>> + unsigned int width;
>> + unsigned int height;
>> +
>> + struct drm_device drm;
>> + struct drm_display_mode mode;
>> + struct drm_connector connector;
>> + struct drm_plane primary_plane;
>> + struct drm_crtc crtc;
>> + struct drm_encoder encoder;
>> +};
>> +
>> +static int appletbdrm_send_request(struct appletbdrm_device *adev,
>> +    struct appletbdrm_msg_request_header *request, size_t size)
>> +{
>> + struct usb_device *udev = adev_to_udev(adev);
>> + struct drm_device *drm = &adev->drm;
>> + int ret, actual_size;
>> +
>> + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, adev->out_ep),
>> +    request, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
>> + if (ret) {
>> + drm_err(drm, "Failed to send message (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + if (actual_size != size) {
>> + drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
>> + actual_size, size);
>> + return -EIO;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_read_response(struct appletbdrm_device *adev,
>> +     struct appletbdrm_msg_response_header *response,
>> +     size_t size, u32 expected_response)
>> +{
>> + struct usb_device *udev = adev_to_udev(adev);
>> + struct drm_device *drm = &adev->drm;
>> + int ret, actual_size;
>> + bool readiness_signal_received = false;
>> +
>> +retry:
>> + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, adev->in_ep),
>> +    response, size, &actual_size, APPLETBDRM_BULK_MSG_TIMEOUT);
>> + if (ret) {
>> + drm_err(drm, "Failed to read response (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + /*
>> +  * The device responds to the first request sent in a particular
>> +  * timeframe after the USB device configuration is set with a readiness
>> +  * signal, in which case the response should be read again
>> +  */
>> + if (response->msg == APPLETBDRM_MSG_SIGNAL_READINESS) {
>> + if (!readiness_signal_received) {
>> + readiness_signal_received = true;
>> + goto retry;
>> + }
>> +
>> + drm_err(drm, "Encountered unexpected readiness signal\n");
>> + return -EIO;
>> + }
>> +
>> + if (actual_size != size) {
>> + drm_err(drm, "Actual size (%d) doesn't match expected size (%lu)\n",
>> + actual_size, size);
>> + return -EIO;
>> + }
>> +
>> + if (response->msg != expected_response) {
>> + drm_err(drm, "Unexpected response from device (expected %p4ch found %p4ch)\n",
>> + &expected_response, &response->msg);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_send_msg(struct appletbdrm_device *adev, u32 msg)
>> +{
>> + struct appletbdrm_msg_simple_request *request;
>> + int ret;
>> +
>> + request = kzalloc(sizeof(*request), GFP_KERNEL);
>> + if (!request)
>> + return -ENOMEM;
>> +
>> + request->header.unk_00 = cpu_to_le16(2);
>> + request->header.unk_02 = cpu_to_le16(0x1512);
>> + request->header.size = cpu_to_le32(sizeof(*request) - sizeof(request->header));
>> + request->msg = msg;
>> + request->size = request->header.size;
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, sizeof(*request));
>> +
>> + kfree(request);
> 
> This is temporary data for the send operation and save to free here?
> 
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_clear_display(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_CLEAR_DISPLAY);
>> +}
>> +
>> +static int appletbdrm_signal_readiness(struct appletbdrm_device *adev)
>> +{
>> + return appletbdrm_send_msg(adev, APPLETBDRM_MSG_SIGNAL_READINESS);
>> +}
>> +
>> +static int appletbdrm_get_information(struct appletbdrm_device *adev)
>> +{
>> + struct appletbdrm_msg_information *info;
>> + struct drm_device *drm = &adev->drm;
>> + u8 bits_per_pixel;
>> + u32 pixel_format;
>> + int ret;
>> +
>> + info = kzalloc(sizeof(*info), GFP_KERNEL);
>> + if (!info)
>> + return -ENOMEM;
>> +
>> + ret = appletbdrm_send_msg(adev, APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + return ret;
>> +
>> + ret = appletbdrm_read_response(adev, &info->header, sizeof(*info),
>> +        APPLETBDRM_MSG_GET_INFORMATION);
>> + if (ret)
>> + goto free_info;
>> +
>> + bits_per_pixel = info->bits_per_pixel;
>> + pixel_format = get_unaligned(&info->pixel_format);
>> +
>> + adev->width = get_unaligned_le32(&info->width);
>> + adev->height = get_unaligned_le32(&info->height);
>> +
>> + if (bits_per_pixel != APPLETBDRM_BITS_PER_PIXEL) {
>> + drm_err(drm, "Encountered unexpected bits per pixel value (%d)\n", bits_per_pixel);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> + if (pixel_format != APPLETBDRM_PIXEL_FORMAT) {
>> + drm_err(drm, "Encountered unknown pixel format (%p4ch)\n", &pixel_format);
>> + ret = -EINVAL;
>> + goto free_info;
>> + }
>> +
>> +free_info:
>> + kfree(info);
>> +
>> + return ret;
>> +}
>> +
>> +static u32 rect_size(struct drm_rect *rect)
>> +{
>> + return drm_rect_width(rect) * drm_rect_height(rect) * (APPLETBDRM_BITS_PER_PIXEL / 8);
>> +}
>> +
>> +static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
>> +    struct drm_plane_state *old_state,
>> +    struct drm_plane_state *state)
>> +{
>> + struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(state);
>> + struct appletbdrm_fb_request_response *response;
>> + struct appletbdrm_fb_request_footer *footer;
>> + struct drm_atomic_helper_damage_iter iter;
>> + struct drm_framebuffer *fb = state->fb;
>> + struct appletbdrm_fb_request *request;
>> + struct drm_device *drm = &adev->drm;
>> + struct appletbdrm_frame *frame;
>> + u64 timestamp = ktime_get_ns();
>> + struct drm_rect damage;
>> + size_t frames_size = 0;
>> + size_t request_size;
>> + int ret;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + frames_size += struct_size(frame, buf, rect_size(&damage));
>> + }
>> +
>> + if (!frames_size)
>> + return 0;
>> +
>> + request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
>> +
>> + request = kzalloc(request_size, GFP_KERNEL);
>> + if (!request)
>> + return -ENOMEM;
>> +
>> + response = kzalloc(sizeof(*response), GFP_KERNEL);
>> + if (!response) {
>> + ret = -ENOMEM;
>> + goto free_request;
>> + }
> 
> This code runs in the middle of the atomic update. It's then too late to do error handling. If these allocs fail, the display hardware will remain in undefined state.
> 
> It is much preferable to allocate this memory in the atomic_check. To do so, you need a plane-state structure. Allocate the memory for request and response in the plane's atomic-check helper. If that fails, you can return an error there. Store the pointers and the request size in your plane-state structure and fetch them here.  The memory should later be freed in the plane's destroy callback. For an example, see how the ssd130x driver treats a buffer for a similar use case at [1].
> 
> [1] https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/solomon/ssd130x.c#L1136

I’ve tried these changes, seem to be breaking the driver:

—>8—
From 16c920cabf65ec664663ebe1611c0ccf6e81de4a Mon Sep 17 00:00:00 2001
From: Aditya Garg <gargaditya08@...e.com>
Date: Tue, 18 Feb 2025 18:54:10 +0530
Subject: [PATCH] better error handling

---
 .../apple-touchbar-advanced-0.1/appletbdrm.c  | 68 +++++++++++++------
 1 file changed, 46 insertions(+), 22 deletions(-)

diff --git a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
index f2d9113..cb13b36 100644
--- a/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
+++ b/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
@@ -133,6 +133,17 @@ struct appletbdrm_device {
 	struct drm_encoder encoder;
 };
 
+struct appletbdrm_plane_state {
+	struct drm_shadow_plane_state base;
+	u8 *request_buffer;
+	u8 *response_buffer;
+};
+
+static inline struct appletbdrm_plane_state *to_appletbdrm_plane_state(struct drm_plane_state *state)
+{
+	return container_of(state, struct appletbdrm_plane_state, base.base);
+}
+
 static int appletbdrm_send_request(struct appletbdrm_device *adev,
 				   struct appletbdrm_msg_request_header *request, size_t size)
 {
@@ -311,24 +322,6 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
 	if (!frames_size)
 		return 0;
 
-	request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
-
-	request = kzalloc(request_size, GFP_KERNEL);
-	if (!request)
-		return -ENOMEM;
-
-	response = kzalloc(sizeof(*response), GFP_KERNEL);
-	if (!response) {
-		ret = -ENOMEM;
-		goto free_request;
-	}
-
-	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
-	if (ret) {
-		drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret);
-		goto free_response;
-	}
-
 	request->header.unk_00 = cpu_to_le16(2);
 	request->header.unk_02 = cpu_to_le16(0x12);
 	request->header.unk_04 = cpu_to_le32(9);
@@ -389,10 +382,6 @@ static int appletbdrm_flush_damage(struct appletbdrm_device *adev,
 
 end_fb_cpu_access:
 	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-free_response:
-	kfree(response);
-free_request:
-	kfree(request);
 
 	return ret;
 }
@@ -415,6 +404,15 @@ static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
 	struct drm_crtc *new_crtc = new_plane_state->crtc;
 	struct drm_crtc_state *new_crtc_state = NULL;
+	struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(new_plane_state);
+	struct drm_device *drm = plane->dev;
+	struct drm_plane_state *plane_state = plane->state;
+	struct appletbdrm_fb_request_response *response;
+	struct appletbdrm_fb_request_footer *footer;
+	struct drm_framebuffer *fb = plane_state->fb;
+	struct appletbdrm_fb_request *request;
+	size_t frames_size = 0;
+	size_t request_size;
 	int ret;
 
 	if (new_crtc)
@@ -429,6 +427,22 @@ static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	else if (!new_plane_state->visible)
 		return 0;
 
+	request_size = ALIGN(sizeof(*request) + frames_size + sizeof(*footer), 16);
+
+	appletbdrm_state->request_buffer = kzalloc(request_size, GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	appletbdrm_state->response_buffer = kzalloc(sizeof(*response), GFP_KERNEL);
+	if (!response) {
+		ret = -ENOMEM;
+	}
+
+	ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+	if (ret) {
+		drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret);
+	}
+
 	return 0;
 }
 
@@ -464,6 +478,15 @@ static void appletbdrm_primary_plane_helper_atomic_disable(struct drm_plane *pla
 	drm_dev_exit(idx);
 }
 
+static void appletbdrm_primary_plane_destroy_state(struct drm_plane *plane,
+						struct drm_plane_state *state)
+{
+	struct appletbdrm_plane_state *appletbdrm_state = to_appletbdrm_plane_state(state);
+
+	kfree(appletbdrm_state->request_buffer);
+	kfree(appletbdrm_state->response_buffer);
+}
+
 static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs = {
 	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 	.atomic_check = appletbdrm_primary_plane_helper_atomic_check,
@@ -474,6 +497,7 @@ static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs
 static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
+	.atomic_destroy_state = appletbdrm_primary_plane_destroy_state,
 	.destroy = drm_plane_cleanup,
 	DRM_GEM_SHADOW_PLANE_FUNCS,
 };
-- 
2.43.0

—>8--
> 
> 
>> +
>> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> + if (ret) {
>> + drm_err(drm, "Failed to start CPU framebuffer access (%d)\n", ret);
>> + goto free_response;
>> + }
>> +
>> + request->header.unk_00 = cpu_to_le16(2);
>> + request->header.unk_02 = cpu_to_le16(0x12);
>> + request->header.unk_04 = cpu_to_le32(9);
>> + request->header.size = cpu_to_le32(request_size - sizeof(request->header));
>> + request->unk_10 = cpu_to_le16(1);
>> + request->msg_id = timestamp & 0xff;
>> +
>> + frame = (struct appletbdrm_frame *)request->data;
>> +
>> + drm_atomic_helper_damage_iter_init(&iter, old_state, state);
>> + drm_atomic_for_each_plane_damage(&iter, &damage) {
>> + struct iosys_map dst = IOSYS_MAP_INIT_VADDR(frame->buf);
>> + u32 buf_size = rect_size(&damage);
>> +
>> + /*
>> +  * The coordinates need to be translated to the coordinate
>> +  * system the device expects, see the comment in
>> +  * appletbdrm_setup_mode_config
>> +  */
>> + frame->begin_x = cpu_to_le16(damage.y1);
>> + frame->begin_y = cpu_to_le16(adev->height - damage.x2);
>> + frame->width = cpu_to_le16(drm_rect_height(&damage));
>> + frame->height = cpu_to_le16(drm_rect_width(&damage));
>> + frame->buf_size = cpu_to_le32(buf_size);
>> +
>> + ret = drm_fb_blit(&dst, NULL, DRM_FORMAT_BGR888,
>> +   &shadow_plane_state->data[0], fb, &damage, &shadow_plane_state->fmtcnv_state);
> 
> Can we void the use of drm_fb_blit()? Since you know all formats in advance, just do
> 
> switch (format)
> case XRGB8888: drm_fb_xrgb888_to_bgr888() break default:
>     drm_fb_memcpy() break }We use blit in simpledrm and ofdrm, where we don't know the formats and output buffers in advance. But it's really not so great in other drivers, I think.
>> + if (ret) {
>> + drm_err(drm, "Failed to copy damage clip (%d)\n", ret);
>> + goto end_fb_cpu_access;
>> + }
>> +
>> + frame = (void *)frame + struct_size(frame, buf, buf_size);
>> + }
>> +
>> + footer = (struct appletbdrm_fb_request_footer *)&request->data[frames_size];
>> +
>> + footer->unk_0c = cpu_to_le32(0xfffe);
>> + footer->unk_1c = cpu_to_le32(0x80001);
>> + footer->unk_34 = cpu_to_le32(0x80002);
>> + footer->unk_4c = cpu_to_le32(0xffff);
>> + footer->timestamp = cpu_to_le64(timestamp);
>> +
>> + ret = appletbdrm_send_request(adev, &request->header, request_size);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + ret = appletbdrm_read_response(adev, &response->header, sizeof(*response),
>> +        APPLETBDRM_MSG_UPDATE_COMPLETE);
>> + if (ret)
>> + goto end_fb_cpu_access;
>> +
>> + if (response->timestamp != footer->timestamp) {
>> + drm_err(drm, "Response timestamp (%llu) doesn't match request timestamp (%llu)\n",
>> + le64_to_cpu(response->timestamp), timestamp);
>> + goto end_fb_cpu_access;
>> + }
>> +
>> +end_fb_cpu_access:
>> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> +free_response:
>> + kfree(response);
>> +free_request:
>> + kfree(request);
>> +
>> + return ret;
>> +}
>> +
>> +static int appletbdrm_connector_helper_get_modes(struct drm_connector *connector)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(connector->dev);
>> +
>> + return drm_connector_helper_get_modes_fixed(connector, &adev->mode);
>> +}
>> +
>> +static const u32 appletbdrm_primary_plane_formats[] = {
>> + DRM_FORMAT_BGR888,
>> + DRM_FORMAT_XRGB8888, /* emulated */
>> +};
>> +
>> +static int appletbdrm_primary_plane_helper_atomic_check(struct drm_plane *plane,
>> +    struct drm_atomic_state *state)
>> +{
>> + struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state, plane);
>> + struct drm_crtc *new_crtc = new_plane_state->crtc;
>> + struct drm_crtc_state *new_crtc_state = NULL;
>> + int ret;
>> +
>> + if (new_crtc)
>> + new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
>> +
>> + ret = drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
>> +   DRM_PLANE_NO_SCALING,
>> +   DRM_PLANE_NO_SCALING,
>> +   false, false);
>> + if (ret)
>> + return ret;
>> + else if (!new_plane_state->visible)
>> + return 0;
>> +
>> + return 0;
>> +}
>> +
>> +static void appletbdrm_primary_plane_helper_atomic_update(struct drm_plane *plane,
>> +      struct drm_atomic_state *old_state)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(plane->dev);
>> + struct drm_device *drm = plane->dev;
>> + struct drm_plane_state *plane_state = plane->state;
>> + struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(old_state, plane);
>> + int idx;
> 
> plane_state->fb can be NULL if the plane's atomic_disable is not set. At a minimum you should check this first and return if so.
> 
> A better idea is to implement atomic_disable. You can try to move the code from appletbdrm_crtc_helper_atomic_disable() to the plane's atomic disable and remove the CRTC helper. That would put all drawing code into the plane. The CRTC is more about programming a display mode.

Code will be moved to plane’s atomic disable in next version

>> +
>> + if (!drm_dev_enter(drm, &idx))
>> + return;
>> +
>> + appletbdrm_flush_damage(adev, old_plane_state, plane_state);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +static const struct drm_plane_helper_funcs appletbdrm_primary_plane_helper_funcs = {
>> + DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>> + .atomic_check = appletbdrm_primary_plane_helper_atomic_check,
>> + .atomic_update = appletbdrm_primary_plane_helper_atomic_update,
>> +};
>> +
>> +static const struct drm_plane_funcs appletbdrm_primary_plane_funcs = {
>> + .update_plane = drm_atomic_helper_update_plane,
>> + .disable_plane = drm_atomic_helper_disable_plane,
>> + .destroy = drm_plane_cleanup,
>> + DRM_GEM_SHADOW_PLANE_FUNCS,
>> +};
>> +
>> +static enum drm_mode_status appletbdrm_crtc_helper_mode_valid(struct drm_crtc *crtc,
>> +   const struct drm_display_mode *mode)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> +
>> + return drm_crtc_helper_mode_valid_fixed(crtc, mode, &adev->mode);
>> +}
>> +
>> +static void appletbdrm_crtc_helper_atomic_disable(struct drm_crtc *crtc,
>> +      struct drm_atomic_state *crtc_state)
>> +{
>> + struct appletbdrm_device *adev = drm_to_adev(crtc->dev);
>> + int idx;
>> +
>> + if (!drm_dev_enter(&adev->drm, &idx))
>> + return;
>> +
>> + appletbdrm_clear_display(adev);
>> +
>> + drm_dev_exit(idx);
>> +}
>> +
>> +static const struct drm_mode_config_funcs appletbdrm_mode_config_funcs = {
>> + .fb_create = drm_gem_fb_create_with_dirty,
>> + .atomic_check = drm_atomic_helper_check,
>> + .atomic_commit = drm_atomic_helper_commit,
>> +};
>> +
>> +static const struct drm_connector_funcs appletbdrm_connector_funcs = {
>> + .reset = drm_atomic_helper_connector_reset,
>> + .destroy = drm_connector_cleanup,
>> + .fill_modes = drm_helper_probe_single_connector_modes,
>> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
>> +};
>> +
>> +static const struct drm_connector_helper_funcs appletbdrm_connector_helper_funcs = {
>> + .get_modes = appletbdrm_connector_helper_get_modes,
>> +};
>> +
>> +static const struct drm_crtc_helper_funcs appletbdrm_crtc_helper_funcs = {
>> + .mode_valid = appletbdrm_crtc_helper_mode_valid,
>> + .atomic_disable = appletbdrm_crtc_helper_atomic_disable,
>> +};
>> +
>> +static const struct drm_crtc_funcs appletbdrm_crtc_funcs = {
>> + .reset = drm_atomic_helper_crtc_reset,
>> + .destroy = drm_crtc_cleanup,
>> + .set_config = drm_atomic_helper_set_config,
>> + .page_flip = drm_atomic_helper_page_flip,
>> + .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>> +};
>> +
>> +static const struct drm_encoder_funcs appletbdrm_encoder_funcs = {
>> + .destroy = drm_encoder_cleanup,
>> +};
>> +
>> +DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>> +
>> +static const struct drm_driver appletbdrm_drm_driver = {
>> + DRM_GEM_SHMEM_DRIVER_OPS,
> 
> For USB devices, we need special wiring to make PRIME work. The PRIME device must support DMA, but a USB device doesn't. So we pass the USB controller device instead. See [2] for what udl does and how it obtains dmadev.
> 
> [2] https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/udl/udl_drv.c#L76

Basically you want me to simply add .gem_prime_import = udl_driver_gem_prime_import, here right?
> 
>> + .name = "appletbdrm",
>> + .desc = "Apple Touch Bar DRM Driver",
>> + .date = "20230910",
> 
> The date field has been removed recently.
Will be removed
> 
>> + .major = 1,
>> + .minor = 0,
>> + .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>> + .fops = &appletbdrm_drm_fops,
>> +};
>> +
>> +static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>> +{
>> + struct drm_connector *connector = &adev->connector;
>> + struct drm_plane *primary_plane;
>> + struct drm_crtc *crtc;
>> + struct drm_encoder *encoder;
>> + struct drm_device *drm = &adev->drm;
>> + struct device *dev = adev->dev;
>> + int ret;
>> +
>> + ret = drmm_mode_config_init(drm);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
>> +
>> + primary_plane = &adev->primary_plane;
>> + ret = drm_universal_plane_init(drm, primary_plane, 0,
>> +        &appletbdrm_primary_plane_funcs,
>> +        appletbdrm_primary_plane_formats,
>> +        ARRAY_SIZE(appletbdrm_primary_plane_formats),
>> +        NULL,
>> +        DRM_PLANE_TYPE_PRIMARY, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize universal plane object\n");
>> + drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
>> + drm_plane_enable_fb_damage_clips(primary_plane);
>> +
>> + crtc = &adev->crtc;
>> + ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
>> + &appletbdrm_crtc_funcs, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize CRTC object\n");
>> + drm_crtc_helper_add(crtc, &appletbdrm_crtc_helper_funcs);
>> +
>> + encoder = &adev->encoder;
>> + ret = drm_encoder_init(drm, encoder, &appletbdrm_encoder_funcs,
>> +        DRM_MODE_ENCODER_DAC, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize encoder\n");
>> + encoder->possible_crtcs = drm_crtc_mask(crtc);
>> +
>> + /*
>> +  * The coordinate system used by the device is different from the
>> +  * coordinate system of the framebuffer in that the x and y axes are
>> +  * swapped, and that the y axis is inverted; so what the device reports
>> +  * as the height is actually the width of the framebuffer and vice
>> +  * versa
>> +  */
>> + drm->mode_config.min_width = 0;
>> + drm->mode_config.min_height = 0;
> 
>> + drm->mode_config.max_width = max(adev->height, DRM_SHADOW_PLANE_MAX_WIDTH);
>> + drm->mode_config.max_height = max(adev->width, DRM_SHADOW_PLANE_MAX_HEIGHT);
> 
> This might not work correctly. To use those SHADOW_PLANE_MAX constants, you have to fixup the damage coordinates to account for changes to the plane coordinates. Simpledrm does that by intersecting damage with dst_clip. [3] You also have to put the result at the right place within the display.
> 
> [3] https://elixir.bootlin.com/linux/v6.14-rc3/source/drivers/gpu/drm/tiny/simpledrm.c#L646
> 
>> + drm->mode_config.preferred_depth = APPLETBDRM_BITS_PER_PIXEL;
>> + drm->mode_config.funcs = &appletbdrm_mode_config_funcs;
>> +
>> + adev->mode = (struct drm_display_mode) {
>> + DRM_MODE_INIT(60, adev->height, adev->width,
>> +       DRM_MODE_RES_MM(adev->height, 218),
>> +       DRM_MODE_RES_MM(adev->width, 218))
>> + };
>> +
>> + ret = drm_connector_init(drm, connector,
>> +  &appletbdrm_connector_funcs, DRM_MODE_CONNECTOR_USB);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize connector\n");
>> +
>> + drm_connector_helper_add(connector, &appletbdrm_connector_helper_funcs);
>> +
>> + ret = drm_connector_set_panel_orientation(connector,
>> +   DRM_MODE_PANEL_ORIENTATION_RIGHT_UP);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set panel orientation\n");
>> +
>> + connector->display_info.non_desktop = true;
>> + ret = drm_object_property_set_value(&connector->base,
>> +     drm->mode_config.non_desktop_property, true);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>> +
>> + ret = drm_connector_attach_encoder(connector, encoder);
>> +
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>> +
>> + drm_mode_config_reset(drm);
>> +
> 
> 
>> + ret = drm_dev_register(drm, 0);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to register DRM device\n");
> 
> This call does not belong to the mode-setting pipeline and belongs into appletbdrm_probe().
> 
>> +
>> + return 0;
>> +}
>> +
>> +static int appletbdrm_probe(struct usb_interface *intf,
>> +     const struct usb_device_id *id)
>> +{
>> + struct usb_endpoint_descriptor *bulk_in, *bulk_out;
>> + struct device *dev = &intf->dev;
>> + struct appletbdrm_device *adev;
>> + int ret;
>> +
>> + ret = usb_find_common_endpoints(intf->cur_altsetting, &bulk_in, &bulk_out, NULL, NULL);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to find bulk endpoints\n");
>> +
>> + adev = devm_drm_dev_alloc(dev, &appletbdrm_drm_driver, struct appletbdrm_device, drm);
>> + if (IS_ERR(adev))
>> + return PTR_ERR(adev);
>> +
>> + adev->dev = dev;
>> + adev->in_ep = bulk_in->bEndpointAddress;
>> + adev->out_ep = bulk_out->bEndpointAddress;
>> +
>> + usb_set_intfdata(intf, adev);
> 
> Rather set the DRM device here and fetch it in the callbacks below. At some point, we might be able to share some of those helpers among drivers.
> 
>> +
>> + ret = appletbdrm_get_information(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to get display information\n");
>> +
>> + ret = appletbdrm_signal_readiness(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to signal readiness\n");
>> +
> 
>> + ret = appletbdrm_clear_display(adev);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to clear display\n");
> 
> Clearing the display is not something usually done in probe. But I guess there's no better place. I'd do this as the final call in probe; after registering the device.  That way, it acts a bit like an in-kernel DRM client.
> 
> Best regards
> Thomas
> 
>> +
>> + return appletbdrm_setup_mode_config(adev);
>> +}
>> +
>> +static void appletbdrm_disconnect(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> + struct drm_device *drm = &adev->drm;
>> +
>> + drm_dev_unplug(drm);
>> + drm_atomic_helper_shutdown(drm);
>> +}
>> +
>> +static void appletbdrm_shutdown(struct usb_interface *intf)
>> +{
>> + struct appletbdrm_device *adev = usb_get_intfdata(intf);
>> +
>> + /*
>> +  * The framebuffer needs to be cleared on shutdown since its content
>> +  * persists across boots
>> +  */
>> + drm_atomic_helper_shutdown(&adev->drm);
>> +}
>> +
>> +static const struct usb_device_id appletbdrm_usb_id_table[] = {
>> + { USB_DEVICE_INTERFACE_CLASS(0x05ac, 0x8302, USB_CLASS_AUDIO_VIDEO) },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(usb, appletbdrm_usb_id_table);
>> +
>> +static struct usb_driver appletbdrm_usb_driver = {
>> + .name = "appletbdrm",
>> + .probe = appletbdrm_probe,
>> + .disconnect = appletbdrm_disconnect,
>> + .shutdown = appletbdrm_shutdown,
>> + .id_table = appletbdrm_usb_id_table,
>> +};
>> +module_usb_driver(appletbdrm_usb_driver);
>> +
>> +MODULE_AUTHOR("Kerem Karabay <kekrby@...il.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)

Powered by blists - more mailing lists