linux-kernel - Re: [RFC PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in x86 Macs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PNZPR01MB447845E0E272252D11BDB60DB8FB2@PNZPR01MB4478.INDPRD01.PROD.OUTLOOK.COM>
Date: Mon, 17 Feb 2025 11:43:21 +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>, "daniel@...ll.ch" <daniel@...ll.ch>, Jiri Kosina
	<jikos@...nel.org>, "bentiss@...nel.org" <bentiss@...nel.org>,
	Thomas Weißschuh <thomas@...ch.de>, Orlando Chamberlain
	<orlandoch.dev@...il.com>, Kerem Karabay <kekrby@...il.com>, Linux Kernel
 Mailing List <linux-kernel@...r.kernel.org>, "linux-input@...r.kernel.org"
	<linux-input@...r.kernel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>
Subject: Re: [RFC PATCH v5 10/10] drm/tiny: add driver for Apple Touch Bars in
 x86 Macs

Hi Thomas

> On 17 Feb 2025, at 1:34 PM, Thomas Zimmermann <tzimmermann@...e.de> wrote:
> 
> Hi Aditya,
> 
> the code looks correct overall. There's one place where I think it fails. See below.
> 
> Am 15.02.25 um 14:43 schrieb Aditya Garg:
> [...]
>> DEFINE_DRM_GEM_FOPS(appletbdrm_drm_fops);
>> @@ -484,10 +537,38 @@ static const struct drm_driver appletbdrm_drm_driver = {
>> 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;
>> 
>> +    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 ret;
>> +    drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
>> +
>> +    crtc = &adev->crtc;
>> +    ret = drm_crtc_init_with_planes(drm, crtc, primary_plane, NULL,
>> +                    &appletbdrm_crtc_funcs, NULL);
>> +    if (ret)
>> +        return ret;
>> +    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 ret;
>> +    encoder->possible_crtcs = drm_crtc_mask(crtc);
>> +
>>    ret = drmm_mode_config_init(drm);
> 
> Try to do drmm_mode_config_init() first. The initialization of planes, crtcs and encoders requires it. See [1] for how other drivers order these calls.

Indeed that was the issue. Thanks a lot for giving me time to fix this. You are a lifesaver!
> 
>>    if (ret)
>>        return dev_err_probe(dev, ret, "Failed to initialize mode configuration\n");
>> @@ -530,13 +611,13 @@ static int appletbdrm_setup_mode_config(struct appletbdrm_device *adev)
>>    if (ret)
>>        return dev_err_probe(dev, ret, "Failed to set non-desktop property\n");
>> 
>> -    ret = drm_simple_display_pipe_init(drm, &adev->pipe, &appletbdrm_pipe_funcs,
>> -                       appletbdrm_formats, ARRAY_SIZE(appletbdrm_formats),
>> -                       NULL, &adev->connector);
>> +    ret = drm_connector_attach_encoder(connector, encoder);
>> +
>>    if (ret)
>>        return dev_err_probe(dev, ret, "Failed to initialize simple display pipe\n");
>> 
>> -    drm_plane_enable_fb_damage_clips(&adev->pipe.plane);
> 
>> +    drm_plane_helper_add(primary_plane, &appletbdrm_primary_plane_helper_funcs);
> 
> This line can be removed. You've already set the plane helpers a few lines above.
> 
>> +    drm_plane_enable_fb_damage_clips(&adev->primary_plane);
> 
> And this call should better be done next to the plane init. The code at [1] again gives you an example of the preferable order.
> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.13.2/source/drivers/gpu/drm/tiny/bochs.c#L606
> 
>> 
>>    drm_mode_config_reset(drm);
>> 
>> 
>> The commit history having both old and new revisions of the driver is here:
>> 
>> https://github.com/AdityaGarg8/apple-touchbar-drv/blob/atomic/usr/src/apple-touchbar-advanced-0.1/appletbdrm.c
>> 
>> Thanks
>> Aditya
> 
> --
> --
> 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)
>