[PATCH] drm/gud: move format init from gud_probe()

[PATCH] drm/gud: move format init from gud_probe()

[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@aol.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)
 {
 	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);
 
 		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;
+}
+
+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);
 
-- 
2.51.0

Re: [PATCH] drm/gud: move format init from gud_probe()

[Thomas Zimmermann]

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@aol.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@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)

Re: [PATCH] drm/gud: move format init from gud_probe()

[Ruben Wauters]

[-- Attachment #1: Type: text/plain, Size: 9382 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]