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 <[email protected]> > > --- > > 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@XRGB8888 = 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);
> >
signature.asc
Description: This is a digitally signed message part
