diff mbox series

[v7,3/7] drm/vkms: Switch to managed for crtc

Message ID 20250113-google-vkms-managed-v7-3-4f81d1893e0b@bootlin.com (mailing list archive)
State New
Headers show
Series drm/vkms: Switch all vkms object to DRM managed objects | expand

Commit Message

Louis Chauvet Jan. 13, 2025, 5:09 p.m. UTC 
The current VKMS driver uses managed function to create crtc, but
don't use it to properly clean the crtc workqueue. It is not an
issue yet, but in order to support multiple devices easily,
convert this code to use drm and device managed helpers.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 14 ++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.c  |  9 ---------
 2 files changed, 14 insertions(+), 9 deletions(-)

Comments

Thomas Zimmermann Jan. 14, 2025, 9:06 a.m. UTC | #1 
Hi


Am 13.01.25 um 18:09 schrieb Louis Chauvet:
> The current VKMS driver uses managed function to create crtc, but
> don't use it to properly clean the crtc workqueue. It is not an
> issue yet, but in order to support multiple devices easily,
> convert this code to use drm and device managed helpers.
>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Reviewed-by: Maíra Canal <mcanal@igalia.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_crtc.c | 14 ++++++++++++++
>   drivers/gpu/drm/vkms/vkms_drv.c  |  9 ---------
>   2 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 28a57ae109fcc05af3fe74f94518c462c09119e3..ace8d293f7da611110c1e117b6cf2f3c9e9b4381 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -6,6 +6,7 @@
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_probe_helper.h>
>   #include <drm/drm_vblank.h>
> +#include <drm/drm_managed.h>

Alphabetical order please.

>   
>   #include "vkms_drv.h"
>   
> @@ -270,6 +271,14 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
>   	.atomic_disable	= vkms_crtc_atomic_disable,
>   };
>   
> +static void vkms_crtc_destroy_workqueue(struct drm_device *dev,
> +					void *raw_vkms_out)
> +{
> +	struct vkms_output *vkms_out = raw_vkms_out;
> +
> +	destroy_workqueue(vkms_out->composer_workq);
> +}
> +

This could be implemented in drm_managed.c. 
drmm_alloc_ordered_workqueue() would call alloc_ordered_workqueue() and 
set up the managed callback as well.

Best regards
Thomas

>   int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>   		   struct drm_plane *primary, struct drm_plane *cursor)
>   {
> @@ -300,5 +309,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>   	if (!vkms_out->composer_workq)
>   		return -ENOMEM;
>   
> +	ret = drmm_add_action_or_reset(dev, vkms_crtc_destroy_workqueue,
> +				       vkms_out);
> +	if (ret)
> +		return ret;
> +
>   	return ret;
>   }
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index e0409aba93496932b32a130ebb608ee53b1a9c59..7c142bfc3bd9de9556621db3e7f570dc0a4fab3a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -53,14 +53,6 @@ MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
>   
>   DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>   
> -static void vkms_release(struct drm_device *dev)
> -{
> -	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> -
> -	if (vkms->output.composer_workq)
> -		destroy_workqueue(vkms->output.composer_workq);
> -}
> -
>   static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>   {
>   	struct drm_device *dev = old_state->dev;
> @@ -108,7 +100,6 @@ static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
>   
>   static const struct drm_driver vkms_driver = {
>   	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> -	.release		= vkms_release,
>   	.fops			= &vkms_driver_fops,
>   	DRM_GEM_SHMEM_DRIVER_OPS,
>   	DRM_FBDEV_SHMEM_DRIVER_OPS,
>
Louis Chauvet Jan. 14, 2025, 1:19 p.m. UTC | #2 
On 14/01/25 - 10:06, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 13.01.25 um 18:09 schrieb Louis Chauvet:
> > The current VKMS driver uses managed function to create crtc, but
> > don't use it to properly clean the crtc workqueue. It is not an
> > issue yet, but in order to support multiple devices easily,
> > convert this code to use drm and device managed helpers.
> > 
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > Reviewed-by: Maíra Canal <mcanal@igalia.com>
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_crtc.c | 14 ++++++++++++++
> >   drivers/gpu/drm/vkms/vkms_drv.c  |  9 ---------
> >   2 files changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> > index 28a57ae109fcc05af3fe74f94518c462c09119e3..ace8d293f7da611110c1e117b6cf2f3c9e9b4381 100644
> > --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> > +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> > @@ -6,6 +6,7 @@
> >   #include <drm/drm_atomic_helper.h>
> >   #include <drm/drm_probe_helper.h>
> >   #include <drm/drm_vblank.h>
> > +#include <drm/drm_managed.h>
> 
> Alphabetical order please.
> 
> >   #include "vkms_drv.h"
> > @@ -270,6 +271,14 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
> >   	.atomic_disable	= vkms_crtc_atomic_disable,
> >   };
> > +static void vkms_crtc_destroy_workqueue(struct drm_device *dev,
> > +					void *raw_vkms_out)
> > +{
> > +	struct vkms_output *vkms_out = raw_vkms_out;
> > +
> > +	destroy_workqueue(vkms_out->composer_workq);
> > +}
> > +
> 
> This could be implemented in drm_managed.c. drmm_alloc_ordered_workqueue()
> would call alloc_ordered_workqueue() and set up the managed callback as
> well.

Hello Thomas,

Thanks for this review. For the next iteration, I will add the macro 
drmm_alloc_ordered_workqueue:

void __drmm_destroy_workqueue(struct drm_device *device, void* res)
{
	struct workqueue_struct *wq = res;

	destroy_workqueue(wq);
}
EXPORT_SYMBOL(__drmm_destroy_workqueue);

#define drmm_alloc_ordered_workqueue(dev, fmt, flags, args...)					\
	({											\
		struct workqueue_struct *wq = alloc_ordered_workqueue(fmt, flags, ##args);	\
		wq ? ({										\
			int ret = drmm_add_action_or_reset(dev, __drmm_destroy_workqueue, wq);	\
			ret ? ERR_PTR(ret) : wq;						\
		}) :										\
			wq;									\
	})

Besides this, is there anything else you noticed that I should change 
for the next iteration in the remaining patches?

Thanks,
Louis Chauvet

> Best regards
> Thomas
> 
> >   int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> >   		   struct drm_plane *primary, struct drm_plane *cursor)
> >   {
> > @@ -300,5 +309,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> >   	if (!vkms_out->composer_workq)
> >   		return -ENOMEM;
> > +	ret = drmm_add_action_or_reset(dev, vkms_crtc_destroy_workqueue,
> > +				       vkms_out);
> > +	if (ret)
> > +		return ret;
> > +
> >   	return ret;
> >   }
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> > index e0409aba93496932b32a130ebb608ee53b1a9c59..7c142bfc3bd9de9556621db3e7f570dc0a4fab3a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.c
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> > @@ -53,14 +53,6 @@ MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
> >   DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
> > -static void vkms_release(struct drm_device *dev)
> > -{
> > -	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
> > -
> > -	if (vkms->output.composer_workq)
> > -		destroy_workqueue(vkms->output.composer_workq);
> > -}
> > -
> >   static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
> >   {
> >   	struct drm_device *dev = old_state->dev;
> > @@ -108,7 +100,6 @@ static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
> >   static const struct drm_driver vkms_driver = {
> >   	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
> > -	.release		= vkms_release,
> >   	.fops			= &vkms_driver_fops,
> >   	DRM_GEM_SHMEM_DRIVER_OPS,
> >   	DRM_FBDEV_SHMEM_DRIVER_OPS,
> > 
> 
> -- 
> --
> 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)
>
Thomas Zimmermann Jan. 14, 2025, 1:25 p.m. UTC | #3 
Hi


Am 14.01.25 um 14:19 schrieb Louis Chauvet:
> On 14/01/25 - 10:06, Thomas Zimmermann wrote:
>> Hi
>>
>>
>> Am 13.01.25 um 18:09 schrieb Louis Chauvet:
>>> The current VKMS driver uses managed function to create crtc, but
>>> don't use it to properly clean the crtc workqueue. It is not an
>>> issue yet, but in order to support multiple devices easily,
>>> convert this code to use drm and device managed helpers.
>>>
>>> Reviewed-by: Maxime Ripard <mripard@kernel.org>
>>> Reviewed-by: Maíra Canal <mcanal@igalia.com>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> ---
>>>    drivers/gpu/drm/vkms/vkms_crtc.c | 14 ++++++++++++++
>>>    drivers/gpu/drm/vkms/vkms_drv.c  |  9 ---------
>>>    2 files changed, 14 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
>>> index 28a57ae109fcc05af3fe74f94518c462c09119e3..ace8d293f7da611110c1e117b6cf2f3c9e9b4381 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
>>> @@ -6,6 +6,7 @@
>>>    #include <drm/drm_atomic_helper.h>
>>>    #include <drm/drm_probe_helper.h>
>>>    #include <drm/drm_vblank.h>
>>> +#include <drm/drm_managed.h>
>> Alphabetical order please.
>>
>>>    #include "vkms_drv.h"
>>> @@ -270,6 +271,14 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
>>>    	.atomic_disable	= vkms_crtc_atomic_disable,
>>>    };
>>> +static void vkms_crtc_destroy_workqueue(struct drm_device *dev,
>>> +					void *raw_vkms_out)
>>> +{
>>> +	struct vkms_output *vkms_out = raw_vkms_out;
>>> +
>>> +	destroy_workqueue(vkms_out->composer_workq);
>>> +}
>>> +
>> This could be implemented in drm_managed.c. drmm_alloc_ordered_workqueue()
>> would call alloc_ordered_workqueue() and set up the managed callback as
>> well.
> Hello Thomas,
>
> Thanks for this review. For the next iteration, I will add the macro
> drmm_alloc_ordered_workqueue:
>
> void __drmm_destroy_workqueue(struct drm_device *device, void* res)
> {
> 	struct workqueue_struct *wq = res;
>
> 	destroy_workqueue(wq);
> }
> EXPORT_SYMBOL(__drmm_destroy_workqueue);
>
> #define drmm_alloc_ordered_workqueue(dev, fmt, flags, args...)					\
> 	({											\
> 		struct workqueue_struct *wq = alloc_ordered_workqueue(fmt, flags, ##args);	\
> 		wq ? ({										\
> 			int ret = drmm_add_action_or_reset(dev, __drmm_destroy_workqueue, wq);	\
> 			ret ? ERR_PTR(ret) : wq;						\
> 		}) :										\
> 			wq;									\
> 	})

Great. There are quite a few work queues in DRM drivers. Hopefully 
soemone else will find this useful. With this change and the fixed 
include sorting, you can add my R-b to this patch.

>
> Besides this, is there anything else you noticed that I should change
> for the next iteration in the remaining patches?

I've looked through the other patches and they look good to me. Feel 
free to add Acked-by: Thomas Zimmermann <tzimmermann@suse.de> to patches 
4 to 7.

Best regards
Thomas

>
> Thanks,
> Louis Chauvet
>
>> Best regards
>> Thomas
>>
>>>    int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>>>    		   struct drm_plane *primary, struct drm_plane *cursor)
>>>    {
>>> @@ -300,5 +309,10 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>>>    	if (!vkms_out->composer_workq)
>>>    		return -ENOMEM;
>>> +	ret = drmm_add_action_or_reset(dev, vkms_crtc_destroy_workqueue,
>>> +				       vkms_out);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>>    	return ret;
>>>    }
>>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
>>> index e0409aba93496932b32a130ebb608ee53b1a9c59..7c142bfc3bd9de9556621db3e7f570dc0a4fab3a 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_drv.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>>> @@ -53,14 +53,6 @@ MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
>>>    DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
>>> -static void vkms_release(struct drm_device *dev)
>>> -{
>>> -	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
>>> -
>>> -	if (vkms->output.composer_workq)
>>> -		destroy_workqueue(vkms->output.composer_workq);
>>> -}
>>> -
>>>    static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>>>    {
>>>    	struct drm_device *dev = old_state->dev;
>>> @@ -108,7 +100,6 @@ static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
>>>    static const struct drm_driver vkms_driver = {
>>>    	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
>>> -	.release		= vkms_release,
>>>    	.fops			= &vkms_driver_fops,
>>>    	DRM_GEM_SHMEM_DRIVER_OPS,
>>>    	DRM_FBDEV_SHMEM_DRIVER_OPS,
>>>
>> -- 
>> --
>> 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)
>>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 28a57ae109fcc05af3fe74f94518c462c09119e3..ace8d293f7da611110c1e117b6cf2f3c9e9b4381 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -6,6 +6,7 @@ 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_vblank.h>
+#include <drm/drm_managed.h>
 
 #include "vkms_drv.h"
 
@@ -270,6 +271,14 @@  static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
 	.atomic_disable	= vkms_crtc_atomic_disable,
 };
 
+static void vkms_crtc_destroy_workqueue(struct drm_device *dev,
+					void *raw_vkms_out)
+{
+	struct vkms_output *vkms_out = raw_vkms_out;
+
+	destroy_workqueue(vkms_out->composer_workq);
+}
+
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 		   struct drm_plane *primary, struct drm_plane *cursor)
 {
@@ -300,5 +309,10 @@  int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
 	if (!vkms_out->composer_workq)
 		return -ENOMEM;
 
+	ret = drmm_add_action_or_reset(dev, vkms_crtc_destroy_workqueue,
+				       vkms_out);
+	if (ret)
+		return ret;
+
 	return ret;
 }
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index e0409aba93496932b32a130ebb608ee53b1a9c59..7c142bfc3bd9de9556621db3e7f570dc0a4fab3a 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -53,14 +53,6 @@  MODULE_PARM_DESC(enable_overlay, "Enable/Disable overlay support");
 
 DEFINE_DRM_GEM_FOPS(vkms_driver_fops);
 
-static void vkms_release(struct drm_device *dev)
-{
-	struct vkms_device *vkms = drm_device_to_vkms_device(dev);
-
-	if (vkms->output.composer_workq)
-		destroy_workqueue(vkms->output.composer_workq);
-}
-
 static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
@@ -108,7 +100,6 @@  static const struct drm_debugfs_info vkms_config_debugfs_list[] = {
 
 static const struct drm_driver vkms_driver = {
 	.driver_features	= DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM,
-	.release		= vkms_release,
 	.fops			= &vkms_driver_fops,
 	DRM_GEM_SHMEM_DRIVER_OPS,
 	DRM_FBDEV_SHMEM_DRIVER_OPS,