[Intel-gfx] [PATCHv2] drm/i915: free crtc on driver remove

[Murthy, Arun R]

> On Thu, 30 Jun 2022, "Murthy, Arun R" <arun.r.murthy at intel.com> wrote:
> >> > intel_crtc is being allocated as part of intel_modeset_init_nogem
> >> > and not freed as part of driver remove. This will lead to memory leak.
> >> > Hence free up the allocated crtc on driver remove as part of
> >> > intel_modeset_driver_remove_nogem.
> >>
> >> No, there's no leak and this is not needed.
> >>
> >> See drm_mode_config_cleanup() calling crtc->funcs->destroy() on each
> crtc.
> >
> > Sorry, I didn't notice this function.
> >
> > intel_crtc_alloc() is called as part of
> > probe->intel_modeset_init_nogem->intel_crtc_init
> > on similar basis cleanup/free should be done in  driver
> > remove->intel_modeset_driver_remove_nogem->intel_crtc_free
> 
> It's just an error prone extra burden for the drivers to take care of this
> manually, so we have drm_mode_config_cleanup(). Which also cleans up
> encoders and encoders etc.
> 
> > Does this look cleaner?
> >
> > Kfree(crtc) which is called in crtc->funcs->destroy is intended for
> > cleanup and hence
> > drm_crtc_cleanup() is being called from intel_crtc_destroy(). The
> > comments added in drm_crtc_funcs say that cleanup resources on destroy.
> >
> > Again looking at the driver design, intel_crtc_alloc is not done as
> > part of any drm_crtc_funcs, rather on probe->modeset_init_nogem, so
> > calling intel_crtc_free from remove->modeset_driver_remove_nogem
> would make more sence.
> 
> That would add another ordering dependency between calling
> drm_mode_config_cleanup() and freeing the individual crtcs separately
> afterwards. And looking at the cleanup code, I'm sure that's not what we
> want.
> 
This is for sure ordering and not dependency. This ordering is being followed
all over the drivers across the kernel. Just like simple probe and remove.
So the usual tendency would be to see as whatever initialization/allocation
being done in probe in the same order deinitialization/deallocation being
done in remove.
Usually in probe multiple initializations are done and so on each failures a goto
will be used to cleanup the respective initialized stuff. Eventually these functions
mentioned in the cleanup under the goto will be the one called in remove.

On similar basis I just tried to depict the above flow for crtc alloc/free.

> Moreover, drm is moving towards managed initialization, which means not
> having to call drm_mode_config_cleanup() explicitly at all. It'll get called as
> part of drmm managed release action tied to the drm device. So really,
> calling kfree as part of the callback is the natural thing to do. Indeed it would
> be difficult to do it anywhere else, for no benefit.
> 
> > Also, looking into the func intel_modeset_init_nogem(), the func
> > intel_modeset_driver_remove_nogem should be renamed as
> intel_modeset_deinit_nogem().
> 
> The cleanup naming comes from them being called as part of struct
> pci_driver .remove callback chain, and it's a useful reminder.
> 
Yes agree even in that case, the function name should look like intel_modeset_remove_nogem()

> Also, the intel_modeset_driver_remove{,noirq,nogem} functions should
> *not* be considered 1:1 counterparts of intel_modeset_init{noirq,nogem,}
> as the init/remove are asymmetric around irq and gem.
> 
> Sure, there's work to be done in cleaning up the probe and remove paths,
> and further trying to separate the gem and display parts, but that's way more
> involved than simple renames, really.
> 
I agree and understand that, but wouldn't small cleanups like this put together
make the driver cleaner?

I am not trying to debate, I have added a new allocation function in intel_crtc_init() so
the deallocation of that should be done as part of crtc deinit/remove which doesn't exist.
I just rolled back to the caller of intel_crtc_init() which is modeset_init_nogem and also
found modeset_driver_remove_nogem.
So though of cleaning up this part.

Thanks and Regards,
Arun R Murthy
--------------------