Re: [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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

Attachment: signature.asc
Description: OpenPGP digital signature

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux