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

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 30 Jun 2022, "Murthy, Arun R" <arun.r.murthy@intel.com> wrote:
>> On Thu, 30 Jun 2022, "Murthy, Arun R" <arun.r.murthy@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.

Well, ordering is mostly driven by dependency, and as I said it's not
purely reverse ordering for cleanup, because of non-trivial
interdependencies...

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

Dunno, we have i915_driver_probe() and i915_driver_remove(), called from
i915_pci_probe() and i915_pci_remove(), respectively. And a bunch of the
calls from i915_driver_remove() are calls to functions named
_driver_remove().

Maybe the init functions should be intel_modeset_driver_probe*()
instead... or everything needs to be just probe/remove.

But mostly I've held off on the renames because really we'll want to
split up intel_display.c to smaller pieces, and one of the pieces is
going to have the high level probe/remove stuff and not much else. And
the functions will be named after whatever the file is going to be
named. In the mean time it would just be a somewhat temporary rename.

You might say "intel_display.c" or "intel_modeset.c" would be the
logical names... but unfortunately intel_display.c has proven that a
generic name like that leads to it being a dumping ground for anything
"display" that doesn't already have a specific place for it. It's an
awful mess. :(

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

The way I see it, we probably shouldn't have intel_crtc_free() at all,
because it's confusing. It's only used in the intel_crtc_init() failure
path. And that should probably open code the cleanup steps with
appropriate goto labels.

The real counterpart for intel_crtc_init() is intel_crtc_destroy(), and
it's called _destroy because for whatever reason the drm_crtc_funcs
callback was named .destroy.



BR,
Jani.


PS. Most often the counterpart for init functions is fini, not deinit,
although there are a few deinit functions around. A quick and simple git
grep popularity contest says the kernel is roughly 5:1 in favor of
deinit. Also, naming is hard.


-- 
Jani Nikula, Intel Open Source Graphics Center