| Message ID | 20200219102122.1607365-6-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm_device managed resources | expand |
Hi Daniel, good idea. I guess it's the simple encoder's fault. :) I only read briefly over the whole thing. Am 19.02.20 um 11:20 schrieb Daniel Vetter: > They all share mipi_dbi_release so we need to switch them all > together. With this we can drop the final kfree from the release > function. > > Aside, I think we could perhaps have a tiny additional helper for > these mipi_dbi drivers, the first few lines around devm_drm_dev_init > are all the same (except for the drm_driver pointer). > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Eric Anholt <eric@anholt.net> > Cc: David Lechner <david@lechnology.com> > Cc: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com> > Cc: "Noralf Trønnes" <noralf@tronnes.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_mipi_dbi.c | 3 --- > drivers/gpu/drm/tiny/hx8357d.c | 2 ++ > drivers/gpu/drm/tiny/ili9225.c | 2 ++ > drivers/gpu/drm/tiny/ili9341.c | 2 ++ > drivers/gpu/drm/tiny/ili9486.c | 2 ++ > drivers/gpu/drm/tiny/mi0283qt.c | 2 ++ > drivers/gpu/drm/tiny/st7586.c | 2 ++ > drivers/gpu/drm/tiny/st7735r.c | 2 ++ > 8 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c > index 558baf989f5a..069603dfcd10 100644 > --- a/drivers/gpu/drm/drm_mipi_dbi.c > +++ b/drivers/gpu/drm/drm_mipi_dbi.c > @@ -588,13 +588,10 @@ EXPORT_SYMBOL(mipi_dbi_dev_init); > */ > void mipi_dbi_release(struct drm_device *drm) > { > - struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(drm); > - > DRM_DEBUG_DRIVER("\n"); > > drm_mode_config_cleanup(drm); > drm_dev_fini(drm); > - kfree(dbidev); > } > EXPORT_SYMBOL(mipi_dbi_release); > > diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c > index 9af8ff84974f..42bc5dadcb1c 100644 > --- a/drivers/gpu/drm/tiny/hx8357d.c > +++ b/drivers/gpu/drm/tiny/hx8357d.c > @@ -21,6 +21,7 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mipi_dbi.h> > #include <drm/drm_modeset_helper.h> > #include <video/mipi_display.h> > @@ -236,6 +237,7 @@ static int hx8357d_probe(struct spi_device *spi) > kfree(dbidev); > return ret; > } > + drmm_add_final_kfree(drm, dbidev); I'd prefer something else than drmm_add_final_kfree(). From what I understand, drmmadd_add_final_kfree() is required so that the device instance itself gets free. But wiring up garbage collection manually is easy to forget and a somewhat odd design. If we have to do that, there's little benefit over calling kfree in the release callback. Instead, could drivers rather be converted to drm_dev_alloc() where possible? For the other cases, could there be a dedicated allocator function that invokes drmm_add_final_kfree()? Like that void* __drmm_kzalloc_dev(size_t size, size_t dev_off) { void *parent = kzalloc(size) drm_device *dev = (parent + dev_off) __drmm_add_final_kfree(dev, parent); return parent; } /* * takes the name of driver's device structure and the * name of the drm device structure embedded within */ drmm_kzalloc(parent_type, base) (parent_type*)__drm_kzalloc_dev(sizeof(parent_type), offsetof(parent_type, base)); Best regards Thomas > > drm_mode_config_init(drm); > > diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c > index 802fb8dde1b6..aae88dc5b3f7 100644 > --- a/drivers/gpu/drm/tiny/ili9225.c > +++ b/drivers/gpu/drm/tiny/ili9225.c > @@ -24,6 +24,7 @@ > #include <drm/drm_fourcc.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mipi_dbi.h> > #include <drm/drm_rect.h> > > @@ -387,6 +388,7 @@ static int ili9225_probe(struct spi_device *spi) > kfree(dbidev); > return ret; > } > + drmm_add_final_kfree(drm, dbidev); > > drm_mode_config_init(drm); > > diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c > index 33b51dc7faa8..7d40cb4ff72b 100644 > --- a/drivers/gpu/drm/tiny/ili9341.c > +++ b/drivers/gpu/drm/tiny/ili9341.c > @@ -20,6 +20,7 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mipi_dbi.h> > #include <drm/drm_modeset_helper.h> > #include <video/mipi_display.h> > @@ -194,6 +195,7 @@ static int ili9341_probe(struct spi_device *spi) > kfree(dbidev); > return ret; > } > + drmm_add_final_kfree(drm, dbidev); > > drm_mode_config_init(drm); > > diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c > index 5084b38c1a71..7d735fc67498 100644 > --- a/drivers/gpu/drm/tiny/ili9486.c > +++ b/drivers/gpu/drm/tiny/ili9486.c > @@ -19,6 +19,7 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mipi_dbi.h> > #include <drm/drm_modeset_helper.h> > > @@ -208,6 +209,7 @@ static int ili9486_probe(struct spi_device *spi) > kfree(dbidev); > return ret; > } > + drmm_add_final_kfree(drm, dbidev); > > drm_mode_config_init(drm); > > diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c > index e2cfd9a17143..8555a56bce8c 100644 > --- a/drivers/gpu/drm/tiny/mi0283qt.c > +++ b/drivers/gpu/drm/tiny/mi0283qt.c > @@ -18,6 +18,7 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mipi_dbi.h> > #include <drm/drm_modeset_helper.h> > #include <video/mipi_display.h> > @@ -198,6 +199,7 @@ static int mi0283qt_probe(struct spi_device *spi) > kfree(dbidev); > return ret; > } > + drmm_add_final_kfree(drm, dbidev); > > drm_mode_config_init(drm); > > diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c > index 9ef559dd3191..427c2561f5f4 100644 > --- a/drivers/gpu/drm/tiny/st7586.c > +++ b/drivers/gpu/drm/tiny/st7586.c > @@ -21,6 +21,7 @@ > #include <drm/drm_format_helper.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mipi_dbi.h> > #include <drm/drm_rect.h> > > @@ -328,6 +329,7 @@ static int st7586_probe(struct spi_device *spi) > kfree(dbidev); > return ret; > } > + drmm_add_final_kfree(drm, dbidev); > > drm_mode_config_init(drm); > > diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c > index 18b925df6e51..b447235c3d47 100644 > --- a/drivers/gpu/drm/tiny/st7735r.c > +++ b/drivers/gpu/drm/tiny/st7735r.c > @@ -21,6 +21,7 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > +#include <drm/drm_managed.h> > #include <drm/drm_mipi_dbi.h> > > #define ST7735R_FRMCTR1 0xb1 > @@ -209,6 +210,7 @@ static int st7735r_probe(struct spi_device *spi) > kfree(dbidev); > return ret; > } > + drmm_add_final_kfree(drm, dbidev); > > drm_mode_config_init(drm); > >
Am 19.02.20 um 12:47 schrieb Thomas Zimmermann: > Hi Daniel, > > good idea. I guess it's the simple encoder's fault. :) I only read > briefly over the whole thing. > > Am 19.02.20 um 11:20 schrieb Daniel Vetter: >> They all share mipi_dbi_release so we need to switch them all >> together. With this we can drop the final kfree from the release >> function. >> >> Aside, I think we could perhaps have a tiny additional helper for >> these mipi_dbi drivers, the first few lines around devm_drm_dev_init >> are all the same (except for the drm_driver pointer). >> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: David Airlie <airlied@linux.ie> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: Eric Anholt <eric@anholt.net> >> Cc: David Lechner <david@lechnology.com> >> Cc: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com> >> Cc: "Noralf Trønnes" <noralf@tronnes.org> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> --- >> drivers/gpu/drm/drm_mipi_dbi.c | 3 --- >> drivers/gpu/drm/tiny/hx8357d.c | 2 ++ >> drivers/gpu/drm/tiny/ili9225.c | 2 ++ >> drivers/gpu/drm/tiny/ili9341.c | 2 ++ >> drivers/gpu/drm/tiny/ili9486.c | 2 ++ >> drivers/gpu/drm/tiny/mi0283qt.c | 2 ++ >> drivers/gpu/drm/tiny/st7586.c | 2 ++ >> drivers/gpu/drm/tiny/st7735r.c | 2 ++ >> 8 files changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c >> index 558baf989f5a..069603dfcd10 100644 >> --- a/drivers/gpu/drm/drm_mipi_dbi.c >> +++ b/drivers/gpu/drm/drm_mipi_dbi.c >> @@ -588,13 +588,10 @@ EXPORT_SYMBOL(mipi_dbi_dev_init); >> */ >> void mipi_dbi_release(struct drm_device *drm) >> { >> - struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(drm); >> - >> DRM_DEBUG_DRIVER("\n"); >> >> drm_mode_config_cleanup(drm); >> drm_dev_fini(drm); >> - kfree(dbidev); >> } >> EXPORT_SYMBOL(mipi_dbi_release); >> >> diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c >> index 9af8ff84974f..42bc5dadcb1c 100644 >> --- a/drivers/gpu/drm/tiny/hx8357d.c >> +++ b/drivers/gpu/drm/tiny/hx8357d.c >> @@ -21,6 +21,7 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mipi_dbi.h> >> #include <drm/drm_modeset_helper.h> >> #include <video/mipi_display.h> >> @@ -236,6 +237,7 @@ static int hx8357d_probe(struct spi_device *spi) >> kfree(dbidev); >> return ret; >> } >> + drmm_add_final_kfree(drm, dbidev); > > I'd prefer something else than drmm_add_final_kfree(). > > From what I understand, drmmadd_add_final_kfree() is required so that > the device instance itself gets free. But wiring up garbage collection > manually is easy to forget and a somewhat odd design. If we have to do > that, there's little benefit over calling kfree in the release callback. > > Instead, could drivers rather be converted to drm_dev_alloc() where > possible? > > For the other cases, could there be a dedicated allocator function that > invokes drmm_add_final_kfree()? Like that > > void* > __drmm_kzalloc_dev(size_t size, size_t dev_off) > { > void *parent = kzalloc(size) > > drm_device *dev = (parent + dev_off) > > __drmm_add_final_kfree(dev, parent); > > return parent; > } > > /* > * takes the name of driver's device structure and the > * name of the drm device structure embedded within > */ > drmm_kzalloc(parent_type, base) > (parent_type*)__drm_kzalloc_dev(sizeof(parent_type), > offsetof(parent_type, base)); This is supposed to be a #define statement > > Best regards > Thomas > >> >> drm_mode_config_init(drm); >> >> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c >> index 802fb8dde1b6..aae88dc5b3f7 100644 >> --- a/drivers/gpu/drm/tiny/ili9225.c >> +++ b/drivers/gpu/drm/tiny/ili9225.c >> @@ -24,6 +24,7 @@ >> #include <drm/drm_fourcc.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mipi_dbi.h> >> #include <drm/drm_rect.h> >> >> @@ -387,6 +388,7 @@ static int ili9225_probe(struct spi_device *spi) >> kfree(dbidev); >> return ret; >> } >> + drmm_add_final_kfree(drm, dbidev); >> >> drm_mode_config_init(drm); >> >> diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c >> index 33b51dc7faa8..7d40cb4ff72b 100644 >> --- a/drivers/gpu/drm/tiny/ili9341.c >> +++ b/drivers/gpu/drm/tiny/ili9341.c >> @@ -20,6 +20,7 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mipi_dbi.h> >> #include <drm/drm_modeset_helper.h> >> #include <video/mipi_display.h> >> @@ -194,6 +195,7 @@ static int ili9341_probe(struct spi_device *spi) >> kfree(dbidev); >> return ret; >> } >> + drmm_add_final_kfree(drm, dbidev); >> >> drm_mode_config_init(drm); >> >> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c >> index 5084b38c1a71..7d735fc67498 100644 >> --- a/drivers/gpu/drm/tiny/ili9486.c >> +++ b/drivers/gpu/drm/tiny/ili9486.c >> @@ -19,6 +19,7 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mipi_dbi.h> >> #include <drm/drm_modeset_helper.h> >> >> @@ -208,6 +209,7 @@ static int ili9486_probe(struct spi_device *spi) >> kfree(dbidev); >> return ret; >> } >> + drmm_add_final_kfree(drm, dbidev); >> >> drm_mode_config_init(drm); >> >> diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c >> index e2cfd9a17143..8555a56bce8c 100644 >> --- a/drivers/gpu/drm/tiny/mi0283qt.c >> +++ b/drivers/gpu/drm/tiny/mi0283qt.c >> @@ -18,6 +18,7 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mipi_dbi.h> >> #include <drm/drm_modeset_helper.h> >> #include <video/mipi_display.h> >> @@ -198,6 +199,7 @@ static int mi0283qt_probe(struct spi_device *spi) >> kfree(dbidev); >> return ret; >> } >> + drmm_add_final_kfree(drm, dbidev); >> >> drm_mode_config_init(drm); >> >> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c >> index 9ef559dd3191..427c2561f5f4 100644 >> --- a/drivers/gpu/drm/tiny/st7586.c >> +++ b/drivers/gpu/drm/tiny/st7586.c >> @@ -21,6 +21,7 @@ >> #include <drm/drm_format_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mipi_dbi.h> >> #include <drm/drm_rect.h> >> >> @@ -328,6 +329,7 @@ static int st7586_probe(struct spi_device *spi) >> kfree(dbidev); >> return ret; >> } >> + drmm_add_final_kfree(drm, dbidev); >> >> drm_mode_config_init(drm); >> >> diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c >> index 18b925df6e51..b447235c3d47 100644 >> --- a/drivers/gpu/drm/tiny/st7735r.c >> +++ b/drivers/gpu/drm/tiny/st7735r.c >> @@ -21,6 +21,7 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> +#include <drm/drm_managed.h> >> #include <drm/drm_mipi_dbi.h> >> >> #define ST7735R_FRMCTR1 0xb1 >> @@ -209,6 +210,7 @@ static int st7735r_probe(struct spi_device *spi) >> kfree(dbidev); >> return ret; >> } >> + drmm_add_final_kfree(drm, dbidev); >> >> drm_mode_config_init(drm); >> >> > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Wed, Feb 19, 2020 at 12:47 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi Daniel, > > good idea. I guess it's the simple encoder's fault. :) I only read > briefly over the whole thing. > > Am 19.02.20 um 11:20 schrieb Daniel Vetter: > > They all share mipi_dbi_release so we need to switch them all > > together. With this we can drop the final kfree from the release > > function. > > > > Aside, I think we could perhaps have a tiny additional helper for > > these mipi_dbi drivers, the first few lines around devm_drm_dev_init > > are all the same (except for the drm_driver pointer). > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > Cc: Eric Anholt <eric@anholt.net> > > Cc: David Lechner <david@lechnology.com> > > Cc: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com> > > Cc: "Noralf Trønnes" <noralf@tronnes.org> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_mipi_dbi.c | 3 --- > > drivers/gpu/drm/tiny/hx8357d.c | 2 ++ > > drivers/gpu/drm/tiny/ili9225.c | 2 ++ > > drivers/gpu/drm/tiny/ili9341.c | 2 ++ > > drivers/gpu/drm/tiny/ili9486.c | 2 ++ > > drivers/gpu/drm/tiny/mi0283qt.c | 2 ++ > > drivers/gpu/drm/tiny/st7586.c | 2 ++ > > drivers/gpu/drm/tiny/st7735r.c | 2 ++ > > 8 files changed, 14 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c > > index 558baf989f5a..069603dfcd10 100644 > > --- a/drivers/gpu/drm/drm_mipi_dbi.c > > +++ b/drivers/gpu/drm/drm_mipi_dbi.c > > @@ -588,13 +588,10 @@ EXPORT_SYMBOL(mipi_dbi_dev_init); > > */ > > void mipi_dbi_release(struct drm_device *drm) > > { > > - struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(drm); > > - > > DRM_DEBUG_DRIVER("\n"); > > > > drm_mode_config_cleanup(drm); > > drm_dev_fini(drm); > > - kfree(dbidev); > > } > > EXPORT_SYMBOL(mipi_dbi_release); > > > > diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c > > index 9af8ff84974f..42bc5dadcb1c 100644 > > --- a/drivers/gpu/drm/tiny/hx8357d.c > > +++ b/drivers/gpu/drm/tiny/hx8357d.c > > @@ -21,6 +21,7 @@ > > #include <drm/drm_fb_helper.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mipi_dbi.h> > > #include <drm/drm_modeset_helper.h> > > #include <video/mipi_display.h> > > @@ -236,6 +237,7 @@ static int hx8357d_probe(struct spi_device *spi) > > kfree(dbidev); > > return ret; > > } > > + drmm_add_final_kfree(drm, dbidev); > > I'd prefer something else than drmm_add_final_kfree(). > > From what I understand, drmmadd_add_final_kfree() is required so that > the device instance itself gets free. But wiring up garbage collection > manually is easy to forget and a somewhat odd design. If we have to do > that, there's little benefit over calling kfree in the release callback. > > Instead, could drivers rather be converted to drm_dev_alloc() where > possible? Uh, we want to get away from drm_dev_alloc because that doesn't allow embedding of struct drm_device. > For the other cases, could there be a dedicated allocator function that > invokes drmm_add_final_kfree()? Like that > > void* > __drmm_kzalloc_dev(size_t size, size_t dev_off) > { > void *parent = kzalloc(size) > > drm_device *dev = (parent + dev_off) > > __drmm_add_final_kfree(dev, parent); > > return parent; > } > > /* > * takes the name of driver's device structure and the > * name of the drm device structure embedded within > */ > drmm_kzalloc(parent_type, base) > (parent_type*)__drm_kzalloc_dev(sizeof(parent_type), > offsetof(parent_type, base)); drmm_kzalloc exists and works, and is used plenty in this series. The trouble is you can only use it once the drm_device is initialized, so there's a chicken/egg problem. But your idea is sound, and I already proposed it in the todo list of the final patch as devm_drm_dev_alloc. It's just at 50+ patches, there's a limit of what's reasonable to do in the first round. I think if you read the entire thing (skipping the driver patches you don't care about) it will make a lot more sense what's going on. Cheers, Daniel > > Best regards > Thomas > > > > > drm_mode_config_init(drm); > > > > diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c > > index 802fb8dde1b6..aae88dc5b3f7 100644 > > --- a/drivers/gpu/drm/tiny/ili9225.c > > +++ b/drivers/gpu/drm/tiny/ili9225.c > > @@ -24,6 +24,7 @@ > > #include <drm/drm_fourcc.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mipi_dbi.h> > > #include <drm/drm_rect.h> > > > > @@ -387,6 +388,7 @@ static int ili9225_probe(struct spi_device *spi) > > kfree(dbidev); > > return ret; > > } > > + drmm_add_final_kfree(drm, dbidev); > > > > drm_mode_config_init(drm); > > > > diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c > > index 33b51dc7faa8..7d40cb4ff72b 100644 > > --- a/drivers/gpu/drm/tiny/ili9341.c > > +++ b/drivers/gpu/drm/tiny/ili9341.c > > @@ -20,6 +20,7 @@ > > #include <drm/drm_fb_helper.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mipi_dbi.h> > > #include <drm/drm_modeset_helper.h> > > #include <video/mipi_display.h> > > @@ -194,6 +195,7 @@ static int ili9341_probe(struct spi_device *spi) > > kfree(dbidev); > > return ret; > > } > > + drmm_add_final_kfree(drm, dbidev); > > > > drm_mode_config_init(drm); > > > > diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c > > index 5084b38c1a71..7d735fc67498 100644 > > --- a/drivers/gpu/drm/tiny/ili9486.c > > +++ b/drivers/gpu/drm/tiny/ili9486.c > > @@ -19,6 +19,7 @@ > > #include <drm/drm_fb_helper.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mipi_dbi.h> > > #include <drm/drm_modeset_helper.h> > > > > @@ -208,6 +209,7 @@ static int ili9486_probe(struct spi_device *spi) > > kfree(dbidev); > > return ret; > > } > > + drmm_add_final_kfree(drm, dbidev); > > > > drm_mode_config_init(drm); > > > > diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c > > index e2cfd9a17143..8555a56bce8c 100644 > > --- a/drivers/gpu/drm/tiny/mi0283qt.c > > +++ b/drivers/gpu/drm/tiny/mi0283qt.c > > @@ -18,6 +18,7 @@ > > #include <drm/drm_fb_helper.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mipi_dbi.h> > > #include <drm/drm_modeset_helper.h> > > #include <video/mipi_display.h> > > @@ -198,6 +199,7 @@ static int mi0283qt_probe(struct spi_device *spi) > > kfree(dbidev); > > return ret; > > } > > + drmm_add_final_kfree(drm, dbidev); > > > > drm_mode_config_init(drm); > > > > diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c > > index 9ef559dd3191..427c2561f5f4 100644 > > --- a/drivers/gpu/drm/tiny/st7586.c > > +++ b/drivers/gpu/drm/tiny/st7586.c > > @@ -21,6 +21,7 @@ > > #include <drm/drm_format_helper.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mipi_dbi.h> > > #include <drm/drm_rect.h> > > > > @@ -328,6 +329,7 @@ static int st7586_probe(struct spi_device *spi) > > kfree(dbidev); > > return ret; > > } > > + drmm_add_final_kfree(drm, dbidev); > > > > drm_mode_config_init(drm); > > > > diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c > > index 18b925df6e51..b447235c3d47 100644 > > --- a/drivers/gpu/drm/tiny/st7735r.c > > +++ b/drivers/gpu/drm/tiny/st7735r.c > > @@ -21,6 +21,7 @@ > > #include <drm/drm_fb_helper.h> > > #include <drm/drm_gem_cma_helper.h> > > #include <drm/drm_gem_framebuffer_helper.h> > > +#include <drm/drm_managed.h> > > #include <drm/drm_mipi_dbi.h> > > > > #define ST7735R_FRMCTR1 0xb1 > > @@ -209,6 +210,7 @@ static int st7735r_probe(struct spi_device *spi) > > kfree(dbidev); > > return ret; > > } > > + drmm_add_final_kfree(drm, dbidev); > > > > drm_mode_config_init(drm); > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
Hi Am 19.02.20 um 14:23 schrieb Daniel Vetter: > On Wed, Feb 19, 2020 at 12:47 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi Daniel, >> >> good idea. I guess it's the simple encoder's fault. :) I only read >> briefly over the whole thing. >> >> Am 19.02.20 um 11:20 schrieb Daniel Vetter: >>> They all share mipi_dbi_release so we need to switch them all >>> together. With this we can drop the final kfree from the release >>> function. >>> >>> Aside, I think we could perhaps have a tiny additional helper for >>> these mipi_dbi drivers, the first few lines around devm_drm_dev_init >>> are all the same (except for the drm_driver pointer). >>> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Cc: Maxime Ripard <mripard@kernel.org> >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>> Cc: David Airlie <airlied@linux.ie> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> Cc: Eric Anholt <eric@anholt.net> >>> Cc: David Lechner <david@lechnology.com> >>> Cc: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com> >>> Cc: "Noralf Trønnes" <noralf@tronnes.org> >>> Cc: Sam Ravnborg <sam@ravnborg.org> >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >>> --- >>> drivers/gpu/drm/drm_mipi_dbi.c | 3 --- >>> drivers/gpu/drm/tiny/hx8357d.c | 2 ++ >>> drivers/gpu/drm/tiny/ili9225.c | 2 ++ >>> drivers/gpu/drm/tiny/ili9341.c | 2 ++ >>> drivers/gpu/drm/tiny/ili9486.c | 2 ++ >>> drivers/gpu/drm/tiny/mi0283qt.c | 2 ++ >>> drivers/gpu/drm/tiny/st7586.c | 2 ++ >>> drivers/gpu/drm/tiny/st7735r.c | 2 ++ >>> 8 files changed, 14 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c >>> index 558baf989f5a..069603dfcd10 100644 >>> --- a/drivers/gpu/drm/drm_mipi_dbi.c >>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c >>> @@ -588,13 +588,10 @@ EXPORT_SYMBOL(mipi_dbi_dev_init); >>> */ >>> void mipi_dbi_release(struct drm_device *drm) >>> { >>> - struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(drm); >>> - >>> DRM_DEBUG_DRIVER("\n"); >>> >>> drm_mode_config_cleanup(drm); >>> drm_dev_fini(drm); >>> - kfree(dbidev); >>> } >>> EXPORT_SYMBOL(mipi_dbi_release); >>> >>> diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c >>> index 9af8ff84974f..42bc5dadcb1c 100644 >>> --- a/drivers/gpu/drm/tiny/hx8357d.c >>> +++ b/drivers/gpu/drm/tiny/hx8357d.c >>> @@ -21,6 +21,7 @@ >>> #include <drm/drm_fb_helper.h> >>> #include <drm/drm_gem_cma_helper.h> >>> #include <drm/drm_gem_framebuffer_helper.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mipi_dbi.h> >>> #include <drm/drm_modeset_helper.h> >>> #include <video/mipi_display.h> >>> @@ -236,6 +237,7 @@ static int hx8357d_probe(struct spi_device *spi) >>> kfree(dbidev); >>> return ret; >>> } >>> + drmm_add_final_kfree(drm, dbidev); >> >> I'd prefer something else than drmm_add_final_kfree(). >> >> From what I understand, drmmadd_add_final_kfree() is required so that >> the device instance itself gets free. But wiring up garbage collection >> manually is easy to forget and a somewhat odd design. If we have to do >> that, there's little benefit over calling kfree in the release callback. >> >> Instead, could drivers rather be converted to drm_dev_alloc() where >> possible? > > Uh, we want to get away from drm_dev_alloc because that doesn't allow > embedding of struct drm_device. > >> For the other cases, could there be a dedicated allocator function that >> invokes drmm_add_final_kfree()? Like that >> >> void* >> __drmm_kzalloc_dev(size_t size, size_t dev_off) >> { >> void *parent = kzalloc(size) >> >> drm_device *dev = (parent + dev_off) >> >> __drmm_add_final_kfree(dev, parent); >> >> return parent; >> } >> >> /* >> * takes the name of driver's device structure and the >> * name of the drm device structure embedded within >> */ >> drmm_kzalloc(parent_type, base) >> (parent_type*)__drm_kzalloc_dev(sizeof(parent_type), >> offsetof(parent_type, base)); > > drmm_kzalloc exists and works, and is used plenty in this series. The > trouble is you can only use it once the drm_device is initialized, so > there's a chicken/egg problem. That line of the example was a total mess. Sorry. It actually was supposed to look something like this. /* * takes the name of driver's device structure and the * name of the drm device structure embedded within */ #define drmm_kzalloc_dev(parent_type, base) \ (parent_type*)__drm_kzalloc_dev(sizeof(parent_type), offsetof(parent_type, base)); __drm_kzalloc_dev() is the allocator function for device instances and drm_kzalloc_dev() is a macro that makes it convenient. Best regards Thomas > > But your idea is sound, and I already proposed it in the todo list of > the final patch as devm_drm_dev_alloc. It's just at 50+ patches, > there's a limit of what's reasonable to do in the first round. > > I think if you read the entire thing (skipping the driver patches you > don't care about) it will make a lot more sense what's going on. > > Cheers, Daniel > >> >> Best regards >> Thomas >> >>> >>> drm_mode_config_init(drm); >>> >>> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c >>> index 802fb8dde1b6..aae88dc5b3f7 100644 >>> --- a/drivers/gpu/drm/tiny/ili9225.c >>> +++ b/drivers/gpu/drm/tiny/ili9225.c >>> @@ -24,6 +24,7 @@ >>> #include <drm/drm_fourcc.h> >>> #include <drm/drm_gem_cma_helper.h> >>> #include <drm/drm_gem_framebuffer_helper.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mipi_dbi.h> >>> #include <drm/drm_rect.h> >>> >>> @@ -387,6 +388,7 @@ static int ili9225_probe(struct spi_device *spi) >>> kfree(dbidev); >>> return ret; >>> } >>> + drmm_add_final_kfree(drm, dbidev); >>> >>> drm_mode_config_init(drm); >>> >>> diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c >>> index 33b51dc7faa8..7d40cb4ff72b 100644 >>> --- a/drivers/gpu/drm/tiny/ili9341.c >>> +++ b/drivers/gpu/drm/tiny/ili9341.c >>> @@ -20,6 +20,7 @@ >>> #include <drm/drm_fb_helper.h> >>> #include <drm/drm_gem_cma_helper.h> >>> #include <drm/drm_gem_framebuffer_helper.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mipi_dbi.h> >>> #include <drm/drm_modeset_helper.h> >>> #include <video/mipi_display.h> >>> @@ -194,6 +195,7 @@ static int ili9341_probe(struct spi_device *spi) >>> kfree(dbidev); >>> return ret; >>> } >>> + drmm_add_final_kfree(drm, dbidev); >>> >>> drm_mode_config_init(drm); >>> >>> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c >>> index 5084b38c1a71..7d735fc67498 100644 >>> --- a/drivers/gpu/drm/tiny/ili9486.c >>> +++ b/drivers/gpu/drm/tiny/ili9486.c >>> @@ -19,6 +19,7 @@ >>> #include <drm/drm_fb_helper.h> >>> #include <drm/drm_gem_cma_helper.h> >>> #include <drm/drm_gem_framebuffer_helper.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mipi_dbi.h> >>> #include <drm/drm_modeset_helper.h> >>> >>> @@ -208,6 +209,7 @@ static int ili9486_probe(struct spi_device *spi) >>> kfree(dbidev); >>> return ret; >>> } >>> + drmm_add_final_kfree(drm, dbidev); >>> >>> drm_mode_config_init(drm); >>> >>> diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c >>> index e2cfd9a17143..8555a56bce8c 100644 >>> --- a/drivers/gpu/drm/tiny/mi0283qt.c >>> +++ b/drivers/gpu/drm/tiny/mi0283qt.c >>> @@ -18,6 +18,7 @@ >>> #include <drm/drm_fb_helper.h> >>> #include <drm/drm_gem_cma_helper.h> >>> #include <drm/drm_gem_framebuffer_helper.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mipi_dbi.h> >>> #include <drm/drm_modeset_helper.h> >>> #include <video/mipi_display.h> >>> @@ -198,6 +199,7 @@ static int mi0283qt_probe(struct spi_device *spi) >>> kfree(dbidev); >>> return ret; >>> } >>> + drmm_add_final_kfree(drm, dbidev); >>> >>> drm_mode_config_init(drm); >>> >>> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c >>> index 9ef559dd3191..427c2561f5f4 100644 >>> --- a/drivers/gpu/drm/tiny/st7586.c >>> +++ b/drivers/gpu/drm/tiny/st7586.c >>> @@ -21,6 +21,7 @@ >>> #include <drm/drm_format_helper.h> >>> #include <drm/drm_gem_cma_helper.h> >>> #include <drm/drm_gem_framebuffer_helper.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mipi_dbi.h> >>> #include <drm/drm_rect.h> >>> >>> @@ -328,6 +329,7 @@ static int st7586_probe(struct spi_device *spi) >>> kfree(dbidev); >>> return ret; >>> } >>> + drmm_add_final_kfree(drm, dbidev); >>> >>> drm_mode_config_init(drm); >>> >>> diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c >>> index 18b925df6e51..b447235c3d47 100644 >>> --- a/drivers/gpu/drm/tiny/st7735r.c >>> +++ b/drivers/gpu/drm/tiny/st7735r.c >>> @@ -21,6 +21,7 @@ >>> #include <drm/drm_fb_helper.h> >>> #include <drm/drm_gem_cma_helper.h> >>> #include <drm/drm_gem_framebuffer_helper.h> >>> +#include <drm/drm_managed.h> >>> #include <drm/drm_mipi_dbi.h> >>> >>> #define ST7735R_FRMCTR1 0xb1 >>> @@ -209,6 +210,7 @@ static int st7735r_probe(struct spi_device *spi) >>> kfree(dbidev); >>> return ret; >>> } >>> + drmm_add_final_kfree(drm, dbidev); >>> >>> drm_mode_config_init(drm); >>> >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > >
On Wed, Feb 19, 2020 at 2:29 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 19.02.20 um 14:23 schrieb Daniel Vetter: > > On Wed, Feb 19, 2020 at 12:47 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > >> > >> Hi Daniel, > >> > >> good idea. I guess it's the simple encoder's fault. :) I only read > >> briefly over the whole thing. > >> > >> Am 19.02.20 um 11:20 schrieb Daniel Vetter: > >>> They all share mipi_dbi_release so we need to switch them all > >>> together. With this we can drop the final kfree from the release > >>> function. > >>> > >>> Aside, I think we could perhaps have a tiny additional helper for > >>> these mipi_dbi drivers, the first few lines around devm_drm_dev_init > >>> are all the same (except for the drm_driver pointer). > >>> > >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >>> Cc: Maxime Ripard <mripard@kernel.org> > >>> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >>> Cc: David Airlie <airlied@linux.ie> > >>> Cc: Daniel Vetter <daniel@ffwll.ch> > >>> Cc: Eric Anholt <eric@anholt.net> > >>> Cc: David Lechner <david@lechnology.com> > >>> Cc: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com> > >>> Cc: "Noralf Trønnes" <noralf@tronnes.org> > >>> Cc: Sam Ravnborg <sam@ravnborg.org> > >>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >>> --- > >>> drivers/gpu/drm/drm_mipi_dbi.c | 3 --- > >>> drivers/gpu/drm/tiny/hx8357d.c | 2 ++ > >>> drivers/gpu/drm/tiny/ili9225.c | 2 ++ > >>> drivers/gpu/drm/tiny/ili9341.c | 2 ++ > >>> drivers/gpu/drm/tiny/ili9486.c | 2 ++ > >>> drivers/gpu/drm/tiny/mi0283qt.c | 2 ++ > >>> drivers/gpu/drm/tiny/st7586.c | 2 ++ > >>> drivers/gpu/drm/tiny/st7735r.c | 2 ++ > >>> 8 files changed, 14 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c > >>> index 558baf989f5a..069603dfcd10 100644 > >>> --- a/drivers/gpu/drm/drm_mipi_dbi.c > >>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c > >>> @@ -588,13 +588,10 @@ EXPORT_SYMBOL(mipi_dbi_dev_init); > >>> */ > >>> void mipi_dbi_release(struct drm_device *drm) > >>> { > >>> - struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(drm); > >>> - > >>> DRM_DEBUG_DRIVER("\n"); > >>> > >>> drm_mode_config_cleanup(drm); > >>> drm_dev_fini(drm); > >>> - kfree(dbidev); > >>> } > >>> EXPORT_SYMBOL(mipi_dbi_release); > >>> > >>> diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c > >>> index 9af8ff84974f..42bc5dadcb1c 100644 > >>> --- a/drivers/gpu/drm/tiny/hx8357d.c > >>> +++ b/drivers/gpu/drm/tiny/hx8357d.c > >>> @@ -21,6 +21,7 @@ > >>> #include <drm/drm_fb_helper.h> > >>> #include <drm/drm_gem_cma_helper.h> > >>> #include <drm/drm_gem_framebuffer_helper.h> > >>> +#include <drm/drm_managed.h> > >>> #include <drm/drm_mipi_dbi.h> > >>> #include <drm/drm_modeset_helper.h> > >>> #include <video/mipi_display.h> > >>> @@ -236,6 +237,7 @@ static int hx8357d_probe(struct spi_device *spi) > >>> kfree(dbidev); > >>> return ret; > >>> } > >>> + drmm_add_final_kfree(drm, dbidev); > >> > >> I'd prefer something else than drmm_add_final_kfree(). > >> > >> From what I understand, drmmadd_add_final_kfree() is required so that > >> the device instance itself gets free. But wiring up garbage collection > >> manually is easy to forget and a somewhat odd design. If we have to do > >> that, there's little benefit over calling kfree in the release callback. > >> > >> Instead, could drivers rather be converted to drm_dev_alloc() where > >> possible? > > > > Uh, we want to get away from drm_dev_alloc because that doesn't allow > > embedding of struct drm_device. > > > >> For the other cases, could there be a dedicated allocator function that > >> invokes drmm_add_final_kfree()? Like that > >> > >> void* > >> __drmm_kzalloc_dev(size_t size, size_t dev_off) > >> { > >> void *parent = kzalloc(size) > >> > >> drm_device *dev = (parent + dev_off) > >> > >> __drmm_add_final_kfree(dev, parent); > >> > >> return parent; > >> } > >> > >> /* > >> * takes the name of driver's device structure and the > >> * name of the drm device structure embedded within > >> */ > >> drmm_kzalloc(parent_type, base) > >> (parent_type*)__drm_kzalloc_dev(sizeof(parent_type), > >> offsetof(parent_type, base)); > > > > drmm_kzalloc exists and works, and is used plenty in this series. The > > trouble is you can only use it once the drm_device is initialized, so > > there's a chicken/egg problem. > > That line of the example was a total mess. Sorry. It actually was > supposed to look something like this. > > /* > * takes the name of driver's device structure and the > * name of the drm device structure embedded within > */ > #define drmm_kzalloc_dev(parent_type, base) \ > (parent_type*)__drm_kzalloc_dev(sizeof(parent_type), > offsetof(parent_type, base)); > > __drm_kzalloc_dev() is the allocator function for device instances and > drm_kzalloc_dev() is a macro that makes it convenient. Yup, exactly, except I had called it devm_drm_dev_alloc in the todo list in the last patch, combining the kzalloc() + devm_drm_dev_init() + drmm_add_final_kfree(). But exactly this idea behind slightly different choice of paint. I think a lot of drivers would benefit from such a macro. -Daniel > > Best regards > Thomas > > > > > But your idea is sound, and I already proposed it in the todo list of > > the final patch as devm_drm_dev_alloc. It's just at 50+ patches, > > there's a limit of what's reasonable to do in the first round. > > > > I think if you read the entire thing (skipping the driver patches you > > don't care about) it will make a lot more sense what's going on. > > > > Cheers, Daniel > > > >> > >> Best regards > >> Thomas > >> > >>> > >>> drm_mode_config_init(drm); > >>> > >>> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c > >>> index 802fb8dde1b6..aae88dc5b3f7 100644 > >>> --- a/drivers/gpu/drm/tiny/ili9225.c > >>> +++ b/drivers/gpu/drm/tiny/ili9225.c > >>> @@ -24,6 +24,7 @@ > >>> #include <drm/drm_fourcc.h> > >>> #include <drm/drm_gem_cma_helper.h> > >>> #include <drm/drm_gem_framebuffer_helper.h> > >>> +#include <drm/drm_managed.h> > >>> #include <drm/drm_mipi_dbi.h> > >>> #include <drm/drm_rect.h> > >>> > >>> @@ -387,6 +388,7 @@ static int ili9225_probe(struct spi_device *spi) > >>> kfree(dbidev); > >>> return ret; > >>> } > >>> + drmm_add_final_kfree(drm, dbidev); > >>> > >>> drm_mode_config_init(drm); > >>> > >>> diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c > >>> index 33b51dc7faa8..7d40cb4ff72b 100644 > >>> --- a/drivers/gpu/drm/tiny/ili9341.c > >>> +++ b/drivers/gpu/drm/tiny/ili9341.c > >>> @@ -20,6 +20,7 @@ > >>> #include <drm/drm_fb_helper.h> > >>> #include <drm/drm_gem_cma_helper.h> > >>> #include <drm/drm_gem_framebuffer_helper.h> > >>> +#include <drm/drm_managed.h> > >>> #include <drm/drm_mipi_dbi.h> > >>> #include <drm/drm_modeset_helper.h> > >>> #include <video/mipi_display.h> > >>> @@ -194,6 +195,7 @@ static int ili9341_probe(struct spi_device *spi) > >>> kfree(dbidev); > >>> return ret; > >>> } > >>> + drmm_add_final_kfree(drm, dbidev); > >>> > >>> drm_mode_config_init(drm); > >>> > >>> diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c > >>> index 5084b38c1a71..7d735fc67498 100644 > >>> --- a/drivers/gpu/drm/tiny/ili9486.c > >>> +++ b/drivers/gpu/drm/tiny/ili9486.c > >>> @@ -19,6 +19,7 @@ > >>> #include <drm/drm_fb_helper.h> > >>> #include <drm/drm_gem_cma_helper.h> > >>> #include <drm/drm_gem_framebuffer_helper.h> > >>> +#include <drm/drm_managed.h> > >>> #include <drm/drm_mipi_dbi.h> > >>> #include <drm/drm_modeset_helper.h> > >>> > >>> @@ -208,6 +209,7 @@ static int ili9486_probe(struct spi_device *spi) > >>> kfree(dbidev); > >>> return ret; > >>> } > >>> + drmm_add_final_kfree(drm, dbidev); > >>> > >>> drm_mode_config_init(drm); > >>> > >>> diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c > >>> index e2cfd9a17143..8555a56bce8c 100644 > >>> --- a/drivers/gpu/drm/tiny/mi0283qt.c > >>> +++ b/drivers/gpu/drm/tiny/mi0283qt.c > >>> @@ -18,6 +18,7 @@ > >>> #include <drm/drm_fb_helper.h> > >>> #include <drm/drm_gem_cma_helper.h> > >>> #include <drm/drm_gem_framebuffer_helper.h> > >>> +#include <drm/drm_managed.h> > >>> #include <drm/drm_mipi_dbi.h> > >>> #include <drm/drm_modeset_helper.h> > >>> #include <video/mipi_display.h> > >>> @@ -198,6 +199,7 @@ static int mi0283qt_probe(struct spi_device *spi) > >>> kfree(dbidev); > >>> return ret; > >>> } > >>> + drmm_add_final_kfree(drm, dbidev); > >>> > >>> drm_mode_config_init(drm); > >>> > >>> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c > >>> index 9ef559dd3191..427c2561f5f4 100644 > >>> --- a/drivers/gpu/drm/tiny/st7586.c > >>> +++ b/drivers/gpu/drm/tiny/st7586.c > >>> @@ -21,6 +21,7 @@ > >>> #include <drm/drm_format_helper.h> > >>> #include <drm/drm_gem_cma_helper.h> > >>> #include <drm/drm_gem_framebuffer_helper.h> > >>> +#include <drm/drm_managed.h> > >>> #include <drm/drm_mipi_dbi.h> > >>> #include <drm/drm_rect.h> > >>> > >>> @@ -328,6 +329,7 @@ static int st7586_probe(struct spi_device *spi) > >>> kfree(dbidev); > >>> return ret; > >>> } > >>> + drmm_add_final_kfree(drm, dbidev); > >>> > >>> drm_mode_config_init(drm); > >>> > >>> diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c > >>> index 18b925df6e51..b447235c3d47 100644 > >>> --- a/drivers/gpu/drm/tiny/st7735r.c > >>> +++ b/drivers/gpu/drm/tiny/st7735r.c > >>> @@ -21,6 +21,7 @@ > >>> #include <drm/drm_fb_helper.h> > >>> #include <drm/drm_gem_cma_helper.h> > >>> #include <drm/drm_gem_framebuffer_helper.h> > >>> +#include <drm/drm_managed.h> > >>> #include <drm/drm_mipi_dbi.h> > >>> > >>> #define ST7735R_FRMCTR1 0xb1 > >>> @@ -209,6 +210,7 @@ static int st7735r_probe(struct spi_device *spi) > >>> kfree(dbidev); > >>> return ret; > >>> } > >>> + drmm_add_final_kfree(drm, dbidev); > >>> > >>> drm_mode_config_init(drm); > >>> > >>> > >> > >> -- > >> Thomas Zimmermann > >> Graphics Driver Developer > >> SUSE Software Solutions Germany GmbH > >> Maxfeldstr. 5, 90409 Nürnberg, Germany > >> (HRB 36809, AG Nürnberg) > >> Geschäftsführer: Felix Imendörffer > >> > > > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
Den 19.02.2020 11.20, skrev Daniel Vetter: > They all share mipi_dbi_release so we need to switch them all > together. With this we can drop the final kfree from the release > function. > > Aside, I think we could perhaps have a tiny additional helper for > these mipi_dbi drivers, the first few lines around devm_drm_dev_init > are all the same (except for the drm_driver pointer). > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Eric Anholt <eric@anholt.net> > Cc: David Lechner <david@lechnology.com> > Cc: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com> > Cc: "Noralf Trønnes" <noralf@tronnes.org> > Cc: Sam Ravnborg <sam@ravnborg.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- I really would have preferred having devm_drm_dev_alloc() in this series, drmm_add_final_kfree() is rather odd. But I can wait: Reviewed-by: Noralf Trønnes <noralf@tronnes.org> I have tested the whole series on tiny/mi0283qt: Tested-by: Noralf Trønnes <noralf@tronnes.org>
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c index 558baf989f5a..069603dfcd10 100644 --- a/drivers/gpu/drm/drm_mipi_dbi.c +++ b/drivers/gpu/drm/drm_mipi_dbi.c @@ -588,13 +588,10 @@ EXPORT_SYMBOL(mipi_dbi_dev_init); */ void mipi_dbi_release(struct drm_device *drm) { - struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(drm); - DRM_DEBUG_DRIVER("\n"); drm_mode_config_cleanup(drm); drm_dev_fini(drm); - kfree(dbidev); } EXPORT_SYMBOL(mipi_dbi_release); diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c index 9af8ff84974f..42bc5dadcb1c 100644 --- a/drivers/gpu/drm/tiny/hx8357d.c +++ b/drivers/gpu/drm/tiny/hx8357d.c @@ -21,6 +21,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_modeset_helper.h> #include <video/mipi_display.h> @@ -236,6 +237,7 @@ static int hx8357d_probe(struct spi_device *spi) kfree(dbidev); return ret; } + drmm_add_final_kfree(drm, dbidev); drm_mode_config_init(drm); diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c index 802fb8dde1b6..aae88dc5b3f7 100644 --- a/drivers/gpu/drm/tiny/ili9225.c +++ b/drivers/gpu/drm/tiny/ili9225.c @@ -24,6 +24,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_rect.h> @@ -387,6 +388,7 @@ static int ili9225_probe(struct spi_device *spi) kfree(dbidev); return ret; } + drmm_add_final_kfree(drm, dbidev); drm_mode_config_init(drm); diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c index 33b51dc7faa8..7d40cb4ff72b 100644 --- a/drivers/gpu/drm/tiny/ili9341.c +++ b/drivers/gpu/drm/tiny/ili9341.c @@ -20,6 +20,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_modeset_helper.h> #include <video/mipi_display.h> @@ -194,6 +195,7 @@ static int ili9341_probe(struct spi_device *spi) kfree(dbidev); return ret; } + drmm_add_final_kfree(drm, dbidev); drm_mode_config_init(drm); diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c index 5084b38c1a71..7d735fc67498 100644 --- a/drivers/gpu/drm/tiny/ili9486.c +++ b/drivers/gpu/drm/tiny/ili9486.c @@ -19,6 +19,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_modeset_helper.h> @@ -208,6 +209,7 @@ static int ili9486_probe(struct spi_device *spi) kfree(dbidev); return ret; } + drmm_add_final_kfree(drm, dbidev); drm_mode_config_init(drm); diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c index e2cfd9a17143..8555a56bce8c 100644 --- a/drivers/gpu/drm/tiny/mi0283qt.c +++ b/drivers/gpu/drm/tiny/mi0283qt.c @@ -18,6 +18,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_modeset_helper.h> #include <video/mipi_display.h> @@ -198,6 +199,7 @@ static int mi0283qt_probe(struct spi_device *spi) kfree(dbidev); return ret; } + drmm_add_final_kfree(drm, dbidev); drm_mode_config_init(drm); diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c index 9ef559dd3191..427c2561f5f4 100644 --- a/drivers/gpu/drm/tiny/st7586.c +++ b/drivers/gpu/drm/tiny/st7586.c @@ -21,6 +21,7 @@ #include <drm/drm_format_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mipi_dbi.h> #include <drm/drm_rect.h> @@ -328,6 +329,7 @@ static int st7586_probe(struct spi_device *spi) kfree(dbidev); return ret; } + drmm_add_final_kfree(drm, dbidev); drm_mode_config_init(drm); diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c index 18b925df6e51..b447235c3d47 100644 --- a/drivers/gpu/drm/tiny/st7735r.c +++ b/drivers/gpu/drm/tiny/st7735r.c @@ -21,6 +21,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> +#include <drm/drm_managed.h> #include <drm/drm_mipi_dbi.h> #define ST7735R_FRMCTR1 0xb1 @@ -209,6 +210,7 @@ static int st7735r_probe(struct spi_device *spi) kfree(dbidev); return ret; } + drmm_add_final_kfree(drm, dbidev); drm_mode_config_init(drm);
They all share mipi_dbi_release so we need to switch them all together. With this we can drop the final kfree from the release function. Aside, I think we could perhaps have a tiny additional helper for these mipi_dbi drivers, the first few lines around devm_drm_dev_init are all the same (except for the drm_driver pointer). Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Eric Anholt <eric@anholt.net> Cc: David Lechner <david@lechnology.com> Cc: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com> Cc: "Noralf Trønnes" <noralf@tronnes.org> Cc: Sam Ravnborg <sam@ravnborg.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_mipi_dbi.c | 3 --- drivers/gpu/drm/tiny/hx8357d.c | 2 ++ drivers/gpu/drm/tiny/ili9225.c | 2 ++ drivers/gpu/drm/tiny/ili9341.c | 2 ++ drivers/gpu/drm/tiny/ili9486.c | 2 ++ drivers/gpu/drm/tiny/mi0283qt.c | 2 ++ drivers/gpu/drm/tiny/st7586.c | 2 ++ drivers/gpu/drm/tiny/st7735r.c | 2 ++ 8 files changed, 14 insertions(+), 3 deletions(-)