| 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
| ||
|
Message-ID: <dd443c76-b68d-4a1c-9b36-d88dd6c95998@suse.de>
Date: Mon, 20 Oct 2025 16:19:50 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Ruben Wauters <rubenru09@....com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/gud: rearrange gud_probe() to prepare for function
splitting
Hi
Am 20.10.25 um 15:45 schrieb Ruben Wauters:
> On Mon, 2025-10-20 at 09:00 +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 19.10.25 um 20:53 schrieb Ruben Wauters:
>>> gud_prove() is currently very large and does many things, including
>> 'gud_probe'
> Unfortunate typo that I only realised after sending
>>> pipeline setup and feature detection, as well as having USB functions.
>>>
>>> This patch re-orders the code in gud_probe() to make it more organised
>>> and easier to split apart in the future.
>>>
>>> Signed-off-by: Ruben Wauters <rubenru09@....com>
>>> ---
>>> I wanted to move mode config to just before pipeline init, however mode
>>> config is edited in feature detection so I was unsure how to go about it
>>> exactly.
>>> Further untangling of this may be required before splitting it out
>>> ---
>>> drivers/gpu/drm/gud/gud_drv.c | 31 +++++++++++++++++--------------
>>> 1 file changed, 17 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
>>> index b7345c8d823d..583f7f8f4c00 100644
>>> --- a/drivers/gpu/drm/gud/gud_drv.c
>>> +++ b/drivers/gpu/drm/gud/gud_drv.c
>>> @@ -463,10 +463,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> return PTR_ERR(gdrm);
>>>
>>> drm = &gdrm->drm;
>>> - drm->mode_config.funcs = &gud_mode_config_funcs;
>>> - ret = drmm_mode_config_init(drm);
>>> - if (ret)
>>> - return ret;
>>>
>>> gdrm->flags = le32_to_cpu(desc.flags);
>>> gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4;
>>> @@ -483,11 +479,18 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> if (ret)
>>> return ret;
>>>
>>> + /* Mode config init*/
>>> + ret = drmm_mode_config_init(drm);
>>> + if (ret)
>>> + return ret;
>>> +
>>> drm->mode_config.min_width = le32_to_cpu(desc.min_width);
>>> drm->mode_config.max_width = le32_to_cpu(desc.max_width);
>>> drm->mode_config.min_height = le32_to_cpu(desc.min_height);
>>> drm->mode_config.max_height = le32_to_cpu(desc.max_height);
>>> + drm->mode_config.funcs = &gud_mode_config_funcs;
>>>
>>> + /*Format init*/
>>> formats_dev = devm_kmalloc(dev, GUD_FORMATS_MAX_NUM, GFP_KERNEL);
>>> /* Add room for emulated XRGB8888 */
>>> formats = devm_kmalloc_array(dev, GUD_FORMATS_MAX_NUM + 1, sizeof(*formats), GFP_KERNEL);
>>> @@ -587,6 +590,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> return -ENOMEM;
>>> }
>>>
>>> + /*Pipeline init*/
>>> ret = drm_universal_plane_init(drm, &gdrm->plane, 0,
>>> &gud_plane_funcs,
>>> formats, num_formats,
>>> @@ -598,15 +602,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> drm_plane_helper_add(&gdrm->plane, &gud_plane_helper_funcs);
>>> drm_plane_enable_fb_damage_clips(&gdrm->plane);
>>>
>>> - devm_kfree(dev, formats);
>>> - devm_kfree(dev, formats_dev);
>>> -
>>> - ret = gud_get_properties(gdrm);
>>> - if (ret) {
>>> - dev_err(dev, "Failed to get properties (error=%d)\n", ret);
>>> - return ret;
>>> - }
>>> -
>>> ret = drm_crtc_init_with_planes(drm, &gdrm->crtc, &gdrm->plane, NULL,
>>> &gud_crtc_funcs, NULL);
>>> if (ret)
>>> @@ -621,6 +616,13 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> }
>>>
>>> drm_mode_config_reset(drm);
>>> + drm_kms_helper_poll_init(drm);
>>> +
>>> + ret = gud_get_properties(gdrm);
>> This function installs plane properties. So it need to be in its old
>> location after drm_universal_plane_init() and before
>> drm_mode_config_reset(). It should be renamed to
>> gud_plane_add_properties(). See [1] for the connector equivalent
> This does make sense, though I do wonder why it worked on testing,
> either way, will change it.
It could be that you can add it afterwards, but that certainly creates
timing issues with user space. There's a related note at [1].
[1]
https://elixir.bootlin.com/linux/v6.17.3/source/drivers/gpu/drm/drm_mode_object.c#L231
Best regards
Thomas
>> [1]
>> https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_connector.c#L474
>>
>>> + if (ret) {
>>> + dev_err(dev, "Failed to get properties (error=%d)\n", ret);
>>> + return ret;
>>> + }
>>>
>>> usb_set_intfdata(intf, gdrm);
>> After you've called drm_kms_helper_poll_init(), DRM could query the
>> connector for attached displays at any time. This might require the usb
>> interface to point to the gud device. And the DRM driver also needs to
>> know about the DMA device. I quick look through the driver code suggest
>> that it works without, but better not count on it.
>>
>> Best would be to move this line and the block with the DMA setup [2]
>> just after where the gud device got allocated with devmdrm_dev_alloc(). [3]
>>
>> [2]
>> https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L627
>> [3]
>> https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L464
> Will do, I'll send a v2 patch with the requested changes.
>> Best regards
>> Thomas
>>
>>>
>>> @@ -638,7 +640,8 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>> if (ret)
>>> return ret;
>>>
>>> - drm_kms_helper_poll_init(drm);
>>> + devm_kfree(dev, formats);
>>> + devm_kfree(dev, formats_dev);
>>>
>>> drm_client_setup(drm, NULL);
>>>
--
--
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