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

[Thomas Zimmermann]

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