Message-ID: <e76a94cc-3102-4327-9868-b5af5706cecd@suse.de>
Date: Wed, 19 Feb 2025 09:02:16 +0100
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Aditya Garg <gargaditya08@...e.com>
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

Am 19.02.25 um 08:57 schrieb Thomas Zimmermann:
> 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@...e.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

To add to this: you also have to set dmadev. See [2] for the respective 
code in udl. And please git-grep udl for dmadev and look for the places 
were it handles device ref-counting.

[2] 
https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/udl/udl_main.c#L314

Best regards
Thomas

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

Powered by blists - more mailing lists