| 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: <79ad192f1444daeeaad8543b652cd49f13e95223.camel@aol.com>
Date: Fri, 14 Nov 2025 17:50:51 +0000
From: Ruben Wauters <rubenru09@....com>
To: Thomas Zimmermann <tzimmermann@...e.de>, 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,
On Fri, 2025-11-14 at 14:34 +0100, Thomas Zimmermann wrote:
> 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.
Doing it that way doesn't take into account GUD_DRM_FORMAT_R1 and
GUD_DRM_FORMAT_XRGB1111, since they are not passed into the final
formats array. I'm not sure it really matters though? I need to dig a
bit more into the use of it. Currently they are taken into account in
the max_buffer_size but I'm not 100% sure they actually need to be. If
it's not an issue I can move the computation into another function
quite easily.
Ruben
> >
> > 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);
> >
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists