| 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: <dbf71996-c470-4875-82e0-19f139fb830a@suse.de>
Date: Fri, 14 Nov 2025 14:34:56 +0100
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: move format init from gud_probe()
Hi,
sorry for being late with the review.
Am 10.11.25 um 17:03 schrieb Ruben Wauters:
> As part of the cleanup of gud_probe(), this patch splits off format init
> code from gud_probe into it's own function. This makes the code more
> compartmentalised and easier to follow, and makes gud_probe() smaller
> and also easier to read.
>
> Signed-off-by: Ruben Wauters <rubenru09@....com>
> ---
> drivers/gpu/drm/gud/gud_drv.c | 167 +++++++++++++++++++---------------
> 1 file changed, 93 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
> index 42135a48d92e..22524601d463 100644
> --- a/drivers/gpu/drm/gud/gud_drv.c
> +++ b/drivers/gpu/drm/gud/gud_drv.c
> @@ -427,84 +427,18 @@ static void gud_free_buffers_and_mutex(void *data)
> mutex_destroy(&gdrm->ctrl_lock);
> }
>
> -static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +static int gud_get_formats(struct gud_device *gdrm, struct device *usb_device, u32 *formats,
> + size_t *max_buffer_size, unsigned int *num_formats_ret)
Please don't pass the usb device here. You can get the device from
gdrm->drm.dev.
> {
> const struct drm_format_info *xrgb8888_emulation_format = NULL;
> bool rgb565_supported = false, xrgb8888_supported = false;
> unsigned int num_formats_dev, num_formats = 0;
> - struct usb_endpoint_descriptor *bulk_out;
> - struct gud_display_descriptor_req desc;
> - struct device *dev = &intf->dev;
> - size_t max_buffer_size = 0;
> - struct gud_device *gdrm;
> - struct drm_device *drm;
> - struct device *dma_dev;
> u8 *formats_dev;
> - u32 *formats;
> + struct drm_device *drm = &gdrm->drm;
> int ret, i;
>
> - ret = usb_find_bulk_out_endpoint(intf->cur_altsetting, &bulk_out);
> - if (ret)
> - return ret;
> -
> - ret = gud_get_display_descriptor(intf, &desc);
> - if (ret) {
> - DRM_DEV_DEBUG_DRIVER(dev, "Not a display interface: ret=%d\n", ret);
> - return -ENODEV;
> - }
> -
> - if (desc.version > 1) {
> - dev_err(dev, "Protocol version %u is not supported\n", desc.version);
> - return -ENODEV;
> - }
> -
> - gdrm = devm_drm_dev_alloc(dev, &gud_drm_driver, struct gud_device, drm);
> - if (IS_ERR(gdrm))
> - return PTR_ERR(gdrm);
> -
> - drm = &gdrm->drm;
> -
> - gdrm->flags = le32_to_cpu(desc.flags);
> - gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4;
> -
> - if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE && gdrm->compression)
> - return -EINVAL;
> -
> - mutex_init(&gdrm->ctrl_lock);
> - mutex_init(&gdrm->damage_lock);
> - INIT_WORK(&gdrm->work, gud_flush_work);
> - gud_clear_damage(gdrm);
> -
> - ret = devm_add_action(dev, gud_free_buffers_and_mutex, gdrm);
> - if (ret)
> - return ret;
> -
> - usb_set_intfdata(intf, gdrm);
> -
> - dma_dev = usb_intf_get_dma_device(intf);
> - if (dma_dev) {
> - drm_dev_set_dma_dev(drm, dma_dev);
> - put_device(dma_dev);
> - } else {
> - dev_warn(dev, "buffer sharing not supported"); /* not an error */
> - }
> -
> - /* 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);
> - if (!formats_dev || !formats)
> + formats_dev = devm_kmalloc(usb_device, GUD_FORMATS_MAX_NUM, GFP_KERNEL);
> + if (!formats_dev)
> return -ENOMEM;
>
> ret = gud_usb_get(gdrm, GUD_REQ_GET_FORMATS, 0, formats_dev, GUD_FORMATS_MAX_NUM);
> @@ -555,7 +489,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>
> fmt_buf_size = drm_format_info_min_pitch(info, 0, drm->mode_config.max_width) *
> drm->mode_config.max_height;
> - max_buffer_size = max(max_buffer_size, fmt_buf_size);
> + *max_buffer_size = max(*max_buffer_size, fmt_buf_size);
Rather do this in a separate helper, where you pass in the array of
supported formats and it computes max_buffer_size. This way you don't
need an output parameter for max_buffer_size.
>
> if (format == GUD_DRM_FORMAT_R1 || format == GUD_DRM_FORMAT_XRGB1111)
> continue; /* Internal not for userspace */
> @@ -564,7 +498,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> }
>
> if (!num_formats && !xrgb8888_emulation_format) {
> - dev_err(dev, "No supported pixel formats found\n");
> + dev_err(usb_device, "No supported pixel formats found\n");
> return -EINVAL;
> }
>
> @@ -577,6 +511,92 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> formats[num_formats++] = DRM_FORMAT_XRGB8888;
> }
>
> + devm_kfree(usb_device, formats_dev);
> + *num_formats_ret = num_formats;
> +
> + return 0;
Instead of 0, you can return num_formats on success. Together with the
change to max_buffer_size, no output parameters will be required.
Best regards
Thomas
> +}
> +
> +static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +{
> + unsigned int num_formats = 0;
> + struct usb_endpoint_descriptor *bulk_out;
> + struct gud_display_descriptor_req desc;
> + struct device *dev = &intf->dev;
> + size_t max_buffer_size = 0;
> + struct gud_device *gdrm;
> + struct drm_device *drm;
> + struct device *dma_dev;
> + u32 *formats;
> + int ret;
> +
> + ret = usb_find_bulk_out_endpoint(intf->cur_altsetting, &bulk_out);
> + if (ret)
> + return ret;
> +
> + ret = gud_get_display_descriptor(intf, &desc);
> + if (ret) {
> + DRM_DEV_DEBUG_DRIVER(dev, "Not a display interface: ret=%d\n", ret);
> + return -ENODEV;
> + }
> +
> + if (desc.version > 1) {
> + dev_err(dev, "Protocol version %u is not supported\n", desc.version);
> + return -ENODEV;
> + }
> +
> + gdrm = devm_drm_dev_alloc(dev, &gud_drm_driver, struct gud_device, drm);
> + if (IS_ERR(gdrm))
> + return PTR_ERR(gdrm);
> +
> + drm = &gdrm->drm;
> +
> + gdrm->flags = le32_to_cpu(desc.flags);
> + gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4;
> +
> + if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE && gdrm->compression)
> + return -EINVAL;
> +
> + mutex_init(&gdrm->ctrl_lock);
> + mutex_init(&gdrm->damage_lock);
> + INIT_WORK(&gdrm->work, gud_flush_work);
> + gud_clear_damage(gdrm);
> +
> + ret = devm_add_action(dev, gud_free_buffers_and_mutex, gdrm);
> + if (ret)
> + return ret;
> +
> + usb_set_intfdata(intf, gdrm);
> +
> + dma_dev = usb_intf_get_dma_device(intf);
> + if (dma_dev) {
> + drm_dev_set_dma_dev(drm, dma_dev);
> + put_device(dma_dev);
> + } else {
> + dev_warn(dev, "buffer sharing not supported"); /* not an error */
> + }
> +
> + /* 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 */
> + /* Add room for emulated XRGB8888 */
> + formats = devm_kmalloc_array(dev, GUD_FORMATS_MAX_NUM + 1, sizeof(*formats), GFP_KERNEL);
> + if (!formats)
> + return -ENOMEM;
> +
> + ret = gud_get_formats(gdrm, dev, formats, &max_buffer_size, &num_formats);
> + if (ret)
> + return ret;
> +
> if (desc.max_buffer_size)
> max_buffer_size = le32_to_cpu(desc.max_buffer_size);
> /* Prevent a misbehaving device from allocating loads of RAM. 4096x4096@...B8888 = 64 MB */
> @@ -641,7 +661,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
> return ret;
>
> devm_kfree(dev, formats);
> - devm_kfree(dev, formats_dev);
>
> drm_client_setup(drm, NULL);
>
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)
Powered by blists - more mailing lists