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

[Thomas Zimmermann]

Hi

Am 19.02.25 um 10:37 schrieb Aditya Garg:
> Hi
>
> > > 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@xxxxxxxx>
> > > 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,
> > You don't allocate struct appletbdrm_plane_state. Instead of this macro, you also have to set your own helpers for the plane's .reset and .atomic_duplicate_state There's again example code in the ssd130x driver.
> Any attempt make to allocate request and response outside appletdrm_flush_damage seems to be breaking the driver.
>
> If I understand correctly, you want me to allocate them outside appletdrm_flush_damage, in appletbdrm_primary_plane_helper_atomic_check, return -ENOMEM if they fail. After that add kfree(return) and kfree(response) in appletbdrm_primary_plane_destroy_state.
>
> The ssd130x driver example isn’t really helping me. Could you please help me out here?

What's the exact error message?

Best regards
Thomas


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