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

[Thomas Zimmermann]

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@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;									\
> 	})

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