| Message ID | 20220630070607.858766-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2] drm/i915: free crtc on driver remove | expand |
On Thu, 30 Jun 2022, Arun R Murthy <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. BR, Jani. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_crtc.c | 2 +- > drivers/gpu/drm/i915/display/intel_crtc.h | 1 + > drivers/gpu/drm/i915/display/intel_display.c | 6 ++++++ > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c > index 4442aa355f86..c90b2854c772 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -193,7 +193,7 @@ static struct intel_crtc *intel_crtc_alloc(void) > return crtc; > } > > -static void intel_crtc_free(struct intel_crtc *crtc) > +void intel_crtc_free(struct intel_crtc *crtc) > { > intel_crtc_destroy_state(&crtc->base, crtc->base.state); > kfree(crtc); > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h > index 73077137fb99..d20200a2c33b 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.h > +++ b/drivers/gpu/drm/i915/display/intel_crtc.h > @@ -35,5 +35,6 @@ struct intel_crtc *intel_crtc_for_pipe(struct drm_i915_private *i915, > void intel_wait_for_vblank_if_active(struct drm_i915_private *i915, > enum pipe pipe); > void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc); > +void intel_crtc_free(struct intel_crtc *crtc); > > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index a0f84cbe974f..33e29455fe56 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -9046,6 +9046,8 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915) > /* part #3: call after gem init */ > void intel_modeset_driver_remove_nogem(struct drm_i915_private *i915) > { > + struct intel_crtc *crtc; > + > intel_dmc_ucode_fini(i915); > > intel_power_domains_driver_remove(i915); > @@ -9053,6 +9055,10 @@ void intel_modeset_driver_remove_nogem(struct drm_i915_private *i915) > intel_vga_unregister(i915); > > intel_bios_driver_remove(i915); > + > + /* Free the allocated crtc */ > + for_each_intel_crtc(&i915->drm, crtc) > + intel_crtc_free(crtc); > } > > bool intel_modeset_probe_defer(struct pci_dev *pdev)
> > 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 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. Also, looking into the func intel_modeset_init_nogem(), the func intel_modeset_driver_remove_nogem should be renamed as intel_modeset_deinit_nogem(). Thanks and Regards, Arun R Murthy --------------------
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. 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. 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. BR, Jani.
> 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. 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 --------------------
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.
> >> >> > 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... > I am also trying to bring in the same reverse ordering, considering the dependencies so as to make the code more readable. While getting to this patch, I didn't notice the .destroy part, after considering that I agree this patch makes no sense, but it just opens up opportunity to cleanup remove so as to follow the reverse ordering to that being done in probe. If maintainers feel that its not required and would survive with what exists, I will just drop this patch. Thanks and Regards, Arun R Murthy --------------------
diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index 4442aa355f86..c90b2854c772 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -193,7 +193,7 @@ static struct intel_crtc *intel_crtc_alloc(void) return crtc; } -static void intel_crtc_free(struct intel_crtc *crtc) +void intel_crtc_free(struct intel_crtc *crtc) { intel_crtc_destroy_state(&crtc->base, crtc->base.state); kfree(crtc); diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h index 73077137fb99..d20200a2c33b 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.h +++ b/drivers/gpu/drm/i915/display/intel_crtc.h @@ -35,5 +35,6 @@ struct intel_crtc *intel_crtc_for_pipe(struct drm_i915_private *i915, void intel_wait_for_vblank_if_active(struct drm_i915_private *i915, enum pipe pipe); void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc); +void intel_crtc_free(struct intel_crtc *crtc); #endif diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index a0f84cbe974f..33e29455fe56 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -9046,6 +9046,8 @@ void intel_modeset_driver_remove_noirq(struct drm_i915_private *i915) /* part #3: call after gem init */ void intel_modeset_driver_remove_nogem(struct drm_i915_private *i915) { + struct intel_crtc *crtc; + intel_dmc_ucode_fini(i915); intel_power_domains_driver_remove(i915); @@ -9053,6 +9055,10 @@ void intel_modeset_driver_remove_nogem(struct drm_i915_private *i915) intel_vga_unregister(i915); intel_bios_driver_remove(i915); + + /* Free the allocated crtc */ + for_each_intel_crtc(&i915->drm, crtc) + intel_crtc_free(crtc); } bool intel_modeset_probe_defer(struct pci_dev *pdev)
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. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_crtc.c | 2 +- drivers/gpu/drm/i915/display/intel_crtc.h | 1 + drivers/gpu/drm/i915/display/intel_display.c | 6 ++++++ 3 files changed, 8 insertions(+), 1 deletion(-)