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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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