On Wed, Feb 19, 2020 at 2:29 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 19.02.20 um 14:23 schrieb Daniel Vetter:
> > On Wed, Feb 19, 2020 at 12:47 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxxxxxxx>
> >>> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> >>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >>> Cc: David Airlie <airlied@xxxxxxxx>
> >>> Cc: Daniel Vetter <daniel@xxxxxxxx>
> >>> Cc: Eric Anholt <eric@xxxxxxxxxx>
> >>> Cc: David Lechner <david@xxxxxxxxxxxxxx>
> >>> Cc: Kamlesh Gurudasani <kamlesh.gurudasani@xxxxxxxxx>
> >>> Cc: "Noralf Trønnes" <noralf@xxxxxxxxxxx>
> >>> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> >>> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> >>> ---
> >>> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
