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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 18.02.25 um 21:12 schrieb Aditya Garg:
> Hi
>
> In continuation to my previous mail.
>
>>> +
>>> +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?
> Probably yes. If I understand correctly, it’s needed to make the touchbar go into the display mode, from the hid keyboard mode.
>
> We here are doing the same as the Windows driver [1] for this does.
>
> [1] https://github.com/imbushuo/DFRDisplayKm/blob/master/src/DFRDisplayKm/include/Dfr.h#L3

Yeah. My concern was that request is being freed while the USB send 
operation is still using it. But in the USB code, it doesn't look like that.

[...]
>> 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.
> I think you mean this:
>
> #include <drm/drm_framebuffer.h>
>
> 		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;
> 		}

Yes.

[...]
>> 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
> Disregard my previous reply for this. I believe you meant by this?:
>
> —>8—
>  From b6fda730995b7f28374c1ff38778a6f3e6da65da Mon Sep 17 00:00:00 2001
> From: Aditya Garg <gargaditya08@live.com>
> Date: Tue, 18 Feb 2025 22:47:44 +0530
> Subject: [PATCH] prime
>
> ---
> drivers/gpu/drm/tiny/appletbdrm.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/tiny/appletbdrm.c b/drivers/gpu/drm/tiny/appletbdrm.c
> index f2d911325..b835063c2 100644
> --- a/drivers/gpu/drm/tiny/appletbdrm.c
> +++ b/drivers/gpu/drm/tiny/appletbdrm.c
> @@ -118,6 +118,7 @@ struct appletbdrm_fb_request_response {
>
> struct appletbdrm_device {
> 	struct device *dev;
> +	struct device *dmadev;
>
> 	unsigned int in_ep;
> 	unsigned int out_ep;
> @@ -521,10 +522,22 @@ 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,

Exactly. The TODO item for this problem is at [1], but there's quite a 
bit of change involved to fix it. Setting a dedicated DMA device is the 
next best thing.

[1] 
https://elixir.bootlin.com/linux/v6.13.3/source/Documentation/gpu/todo.rst#L615

Best regards
Thomas

> 	.name			= "appletbdrm",
> 	.desc			= "Apple Touch Bar DRM Driver",
> 	.major			= 1,

-- 
--
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)