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

[Louis Chauvet <louis.chauvet@xxxxxxxxxxx>]

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@xxxxxxxxxx>
> > Reviewed-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
> > ---
> >   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)
>