* [PATCH] drm/gud: rearrange gud_probe() to prepare for function splitting [not found] <20251019185642.14266-2-rubenru09.ref@aol.com> @ 2025-10-19 18:53 ` Ruben Wauters 2025-10-20 7:00 ` Thomas Zimmermann 0 siblings, 1 reply; 4+ messages in thread From: Ruben Wauters @ 2025-10-19 18:53 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter Cc: Ruben Wauters, dri-devel, linux-kernel gud_prove() is currently very large and does many things, including pipeline setup and feature detection, as well as having USB functions. This patch re-orders the code in gud_probe() to make it more organised and easier to split apart in the future. Signed-off-by: Ruben Wauters <rubenru09@aol.com> --- I wanted to move mode config to just before pipeline init, however mode config is edited in feature detection so I was unsure how to go about it exactly. Further untangling of this may be required before splitting it out --- drivers/gpu/drm/gud/gud_drv.c | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c index b7345c8d823d..583f7f8f4c00 100644 --- a/drivers/gpu/drm/gud/gud_drv.c +++ b/drivers/gpu/drm/gud/gud_drv.c @@ -463,10 +463,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) return PTR_ERR(gdrm); drm = &gdrm->drm; - drm->mode_config.funcs = &gud_mode_config_funcs; - ret = drmm_mode_config_init(drm); - if (ret) - return ret; gdrm->flags = le32_to_cpu(desc.flags); gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4; @@ -483,11 +479,18 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) if (ret) return ret; + /* 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); @@ -587,6 +590,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) return -ENOMEM; } + /*Pipeline init*/ ret = drm_universal_plane_init(drm, &gdrm->plane, 0, &gud_plane_funcs, formats, num_formats, @@ -598,15 +602,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) drm_plane_helper_add(&gdrm->plane, &gud_plane_helper_funcs); drm_plane_enable_fb_damage_clips(&gdrm->plane); - devm_kfree(dev, formats); - devm_kfree(dev, formats_dev); - - ret = gud_get_properties(gdrm); - if (ret) { - dev_err(dev, "Failed to get properties (error=%d)\n", ret); - return ret; - } - ret = drm_crtc_init_with_planes(drm, &gdrm->crtc, &gdrm->plane, NULL, &gud_crtc_funcs, NULL); if (ret) @@ -621,6 +616,13 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) } drm_mode_config_reset(drm); + drm_kms_helper_poll_init(drm); + + ret = gud_get_properties(gdrm); + if (ret) { + dev_err(dev, "Failed to get properties (error=%d)\n", ret); + return ret; + } usb_set_intfdata(intf, gdrm); @@ -638,7 +640,8 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) if (ret) return ret; - drm_kms_helper_poll_init(drm); + devm_kfree(dev, formats); + devm_kfree(dev, formats_dev); drm_client_setup(drm, NULL); -- 2.51.0 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/gud: rearrange gud_probe() to prepare for function splitting 2025-10-19 18:53 ` [PATCH] drm/gud: rearrange gud_probe() to prepare for function splitting Ruben Wauters @ 2025-10-20 7:00 ` Thomas Zimmermann 2025-10-20 13:45 ` Ruben Wauters 0 siblings, 1 reply; 4+ messages in thread From: Thomas Zimmermann @ 2025-10-20 7:00 UTC (permalink / raw) To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard, David Airlie, Simona Vetter Cc: dri-devel, linux-kernel Hi Am 19.10.25 um 20:53 schrieb Ruben Wauters: > gud_prove() is currently very large and does many things, including 'gud_probe' > pipeline setup and feature detection, as well as having USB functions. > > This patch re-orders the code in gud_probe() to make it more organised > and easier to split apart in the future. > > Signed-off-by: Ruben Wauters <rubenru09@aol.com> > --- > I wanted to move mode config to just before pipeline init, however mode > config is edited in feature detection so I was unsure how to go about it > exactly. > Further untangling of this may be required before splitting it out > --- > drivers/gpu/drm/gud/gud_drv.c | 31 +++++++++++++++++-------------- > 1 file changed, 17 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c > index b7345c8d823d..583f7f8f4c00 100644 > --- a/drivers/gpu/drm/gud/gud_drv.c > +++ b/drivers/gpu/drm/gud/gud_drv.c > @@ -463,10 +463,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > return PTR_ERR(gdrm); > > drm = &gdrm->drm; > - drm->mode_config.funcs = &gud_mode_config_funcs; > - ret = drmm_mode_config_init(drm); > - if (ret) > - return ret; > > gdrm->flags = le32_to_cpu(desc.flags); > gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4; > @@ -483,11 +479,18 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > if (ret) > return ret; > > + /* 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); > @@ -587,6 +590,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > return -ENOMEM; > } > > + /*Pipeline init*/ > ret = drm_universal_plane_init(drm, &gdrm->plane, 0, > &gud_plane_funcs, > formats, num_formats, > @@ -598,15 +602,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > drm_plane_helper_add(&gdrm->plane, &gud_plane_helper_funcs); > drm_plane_enable_fb_damage_clips(&gdrm->plane); > > - devm_kfree(dev, formats); > - devm_kfree(dev, formats_dev); > - > - ret = gud_get_properties(gdrm); > - if (ret) { > - dev_err(dev, "Failed to get properties (error=%d)\n", ret); > - return ret; > - } > - > ret = drm_crtc_init_with_planes(drm, &gdrm->crtc, &gdrm->plane, NULL, > &gud_crtc_funcs, NULL); > if (ret) > @@ -621,6 +616,13 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > } > > drm_mode_config_reset(drm); > + drm_kms_helper_poll_init(drm); > + > + ret = gud_get_properties(gdrm); This function installs plane properties. So it need to be in its old location after drm_universal_plane_init() and before drm_mode_config_reset(). It should be renamed to gud_plane_add_properties(). See [1] for the connector equivalent. [1] https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_connector.c#L474 > + if (ret) { > + dev_err(dev, "Failed to get properties (error=%d)\n", ret); > + return ret; > + } > > usb_set_intfdata(intf, gdrm); After you've called drm_kms_helper_poll_init(), DRM could query the connector for attached displays at any time. This might require the usb interface to point to the gud device. And the DRM driver also needs to know about the DMA device. I quick look through the driver code suggest that it works without, but better not count on it. Best would be to move this line and the block with the DMA setup [2] just after where the gud device got allocated with devmdrm_dev_alloc(). [3] [2] https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L627 [3] https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L464 Best regards Thomas > > @@ -638,7 +640,8 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > if (ret) > return ret; > > - drm_kms_helper_poll_init(drm); > + devm_kfree(dev, formats); > + devm_kfree(dev, formats_dev); > > drm_client_setup(drm, NULL); > -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/gud: rearrange gud_probe() to prepare for function splitting 2025-10-20 7:00 ` Thomas Zimmermann @ 2025-10-20 13:45 ` Ruben Wauters 2025-10-20 14:19 ` Thomas Zimmermann 0 siblings, 1 reply; 4+ messages in thread From: Ruben Wauters @ 2025-10-20 13:45 UTC (permalink / raw) To: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, David Airlie, Simona Vetter Cc: dri-devel, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5325 bytes --] On Mon, 2025-10-20 at 09:00 +0200, Thomas Zimmermann wrote: > Hi > > Am 19.10.25 um 20:53 schrieb Ruben Wauters: > > gud_prove() is currently very large and does many things, including > > 'gud_probe' Unfortunate typo that I only realised after sending > > > pipeline setup and feature detection, as well as having USB functions. > > > > This patch re-orders the code in gud_probe() to make it more organised > > and easier to split apart in the future. > > > > Signed-off-by: Ruben Wauters <rubenru09@aol.com> > > --- > > I wanted to move mode config to just before pipeline init, however mode > > config is edited in feature detection so I was unsure how to go about it > > exactly. > > Further untangling of this may be required before splitting it out > > --- > > drivers/gpu/drm/gud/gud_drv.c | 31 +++++++++++++++++-------------- > > 1 file changed, 17 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c > > index b7345c8d823d..583f7f8f4c00 100644 > > --- a/drivers/gpu/drm/gud/gud_drv.c > > +++ b/drivers/gpu/drm/gud/gud_drv.c > > @@ -463,10 +463,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > > return PTR_ERR(gdrm); > > > > drm = &gdrm->drm; > > - drm->mode_config.funcs = &gud_mode_config_funcs; > > - ret = drmm_mode_config_init(drm); > > - if (ret) > > - return ret; > > > > gdrm->flags = le32_to_cpu(desc.flags); > > gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4; > > @@ -483,11 +479,18 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > > if (ret) > > return ret; > > > > + /* 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); > > @@ -587,6 +590,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > > return -ENOMEM; > > } > > > > + /*Pipeline init*/ > > ret = drm_universal_plane_init(drm, &gdrm->plane, 0, > > &gud_plane_funcs, > > formats, num_formats, > > @@ -598,15 +602,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > > drm_plane_helper_add(&gdrm->plane, &gud_plane_helper_funcs); > > drm_plane_enable_fb_damage_clips(&gdrm->plane); > > > > - devm_kfree(dev, formats); > > - devm_kfree(dev, formats_dev); > > - > > - ret = gud_get_properties(gdrm); > > - if (ret) { > > - dev_err(dev, "Failed to get properties (error=%d)\n", ret); > > - return ret; > > - } > > - > > ret = drm_crtc_init_with_planes(drm, &gdrm->crtc, &gdrm->plane, NULL, > > &gud_crtc_funcs, NULL); > > if (ret) > > @@ -621,6 +616,13 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > > } > > > > drm_mode_config_reset(drm); > > + drm_kms_helper_poll_init(drm); > > + > > + ret = gud_get_properties(gdrm); > > This function installs plane properties. So it need to be in its old > location after drm_universal_plane_init() and before > drm_mode_config_reset(). It should be renamed to > gud_plane_add_properties(). See [1] for the connector equivalent This does make sense, though I do wonder why it worked on testing, either way, will change it. > > [1] > https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_connector.c#L474 > > > + if (ret) { > > + dev_err(dev, "Failed to get properties (error=%d)\n", ret); > > + return ret; > > + } > > > > > usb_set_intfdata(intf, gdrm); > > After you've called drm_kms_helper_poll_init(), DRM could query the > connector for attached displays at any time. This might require the usb > interface to point to the gud device. And the DRM driver also needs to > know about the DMA device. I quick look through the driver code suggest > that it works without, but better not count on it. > > Best would be to move this line and the block with the DMA setup [2] > just after where the gud device got allocated with devmdrm_dev_alloc(). [3] > > [2] > https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L627 > [3] > https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L464 Will do, I'll send a v2 patch with the requested changes. > > Best regards > Thomas > > > > > @@ -638,7 +640,8 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) > > if (ret) > > return ret; > > > > - drm_kms_helper_poll_init(drm); > > + 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 --] ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/gud: rearrange gud_probe() to prepare for function splitting 2025-10-20 13:45 ` Ruben Wauters @ 2025-10-20 14:19 ` Thomas Zimmermann 0 siblings, 0 replies; 4+ messages in thread From: Thomas Zimmermann @ 2025-10-20 14:19 UTC (permalink / raw) To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard, David Airlie, Simona Vetter Cc: dri-devel, linux-kernel Hi Am 20.10.25 um 15:45 schrieb Ruben Wauters: > On Mon, 2025-10-20 at 09:00 +0200, Thomas Zimmermann wrote: >> Hi >> >> Am 19.10.25 um 20:53 schrieb Ruben Wauters: >>> gud_prove() is currently very large and does many things, including >> 'gud_probe' > Unfortunate typo that I only realised after sending >>> pipeline setup and feature detection, as well as having USB functions. >>> >>> This patch re-orders the code in gud_probe() to make it more organised >>> and easier to split apart in the future. >>> >>> Signed-off-by: Ruben Wauters <rubenru09@aol.com> >>> --- >>> I wanted to move mode config to just before pipeline init, however mode >>> config is edited in feature detection so I was unsure how to go about it >>> exactly. >>> Further untangling of this may be required before splitting it out >>> --- >>> drivers/gpu/drm/gud/gud_drv.c | 31 +++++++++++++++++-------------- >>> 1 file changed, 17 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c >>> index b7345c8d823d..583f7f8f4c00 100644 >>> --- a/drivers/gpu/drm/gud/gud_drv.c >>> +++ b/drivers/gpu/drm/gud/gud_drv.c >>> @@ -463,10 +463,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) >>> return PTR_ERR(gdrm); >>> >>> drm = &gdrm->drm; >>> - drm->mode_config.funcs = &gud_mode_config_funcs; >>> - ret = drmm_mode_config_init(drm); >>> - if (ret) >>> - return ret; >>> >>> gdrm->flags = le32_to_cpu(desc.flags); >>> gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4; >>> @@ -483,11 +479,18 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) >>> if (ret) >>> return ret; >>> >>> + /* 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); >>> @@ -587,6 +590,7 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) >>> return -ENOMEM; >>> } >>> >>> + /*Pipeline init*/ >>> ret = drm_universal_plane_init(drm, &gdrm->plane, 0, >>> &gud_plane_funcs, >>> formats, num_formats, >>> @@ -598,15 +602,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) >>> drm_plane_helper_add(&gdrm->plane, &gud_plane_helper_funcs); >>> drm_plane_enable_fb_damage_clips(&gdrm->plane); >>> >>> - devm_kfree(dev, formats); >>> - devm_kfree(dev, formats_dev); >>> - >>> - ret = gud_get_properties(gdrm); >>> - if (ret) { >>> - dev_err(dev, "Failed to get properties (error=%d)\n", ret); >>> - return ret; >>> - } >>> - >>> ret = drm_crtc_init_with_planes(drm, &gdrm->crtc, &gdrm->plane, NULL, >>> &gud_crtc_funcs, NULL); >>> if (ret) >>> @@ -621,6 +616,13 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) >>> } >>> >>> drm_mode_config_reset(drm); >>> + drm_kms_helper_poll_init(drm); >>> + >>> + ret = gud_get_properties(gdrm); >> This function installs plane properties. So it need to be in its old >> location after drm_universal_plane_init() and before >> drm_mode_config_reset(). It should be renamed to >> gud_plane_add_properties(). See [1] for the connector equivalent > This does make sense, though I do wonder why it worked on testing, > either way, will change it. It could be that you can add it afterwards, but that certainly creates timing issues with user space. There's a related note at [1]. [1] https://elixir.bootlin.com/linux/v6.17.3/source/drivers/gpu/drm/drm_mode_object.c#L231 Best regards Thomas >> [1] >> https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_connector.c#L474 >> >>> + if (ret) { >>> + dev_err(dev, "Failed to get properties (error=%d)\n", ret); >>> + return ret; >>> + } >>> >>> usb_set_intfdata(intf, gdrm); >> After you've called drm_kms_helper_poll_init(), DRM could query the >> connector for attached displays at any time. This might require the usb >> interface to point to the gud device. And the DRM driver also needs to >> know about the DMA device. I quick look through the driver code suggest >> that it works without, but better not count on it. >> >> Best would be to move this line and the block with the DMA setup [2] >> just after where the gud device got allocated with devmdrm_dev_alloc(). [3] >> >> [2] >> https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L627 >> [3] >> https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L464 > Will do, I'll send a v2 patch with the requested changes. >> Best regards >> Thomas >> >>> >>> @@ -638,7 +640,8 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id) >>> if (ret) >>> return ret; >>> >>> - drm_kms_helper_poll_init(drm); >>> + devm_kfree(dev, formats); >>> + devm_kfree(dev, formats_dev); >>> >>> drm_client_setup(drm, NULL); >>> -- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-10-20 14:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20251019185642.14266-2-rubenru09.ref@aol.com>
2025-10-19 18:53 ` [PATCH] drm/gud: rearrange gud_probe() to prepare for function splitting Ruben Wauters
2025-10-20 7:00 ` Thomas Zimmermann
2025-10-20 13:45 ` Ruben Wauters
2025-10-20 14:19 ` Thomas Zimmermann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®