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.
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);
--
--
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)
