[05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
diff mbox series

Message ID 20200219102122.1607365-6-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • drm_device managed resources
Related show

Commit Message

Daniel Vetter Feb. 19, 2020, 10:20 a.m. UTC
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(-)

Comments

Thomas Zimmermann Feb. 19, 2020, 11:47 a.m. UTC | #1
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);
>  
>
Thomas Zimmermann Feb. 19, 2020, 12:45 p.m. UTC | #2
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
>
Daniel Vetter Feb. 19, 2020, 1:23 p.m. UTC | #3
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
>
Thomas Zimmermann Feb. 19, 2020, 1:29 p.m. UTC | #4
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
>>
> 
>
Daniel Vetter Feb. 19, 2020, 2:32 p.m. UTC | #5
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
>
Noralf Trønnes Feb. 20, 2020, 4:18 p.m. UTC | #6
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>

Patch
diff mbox series

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