Re: [Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
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 > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: David Airlie > Cc: Daniel Vetter > Cc: Eric Anholt > Cc: David Lechner > Cc: Kamlesh Gurudasani > Cc: "Noralf Trønnes" > Cc: Sam Ravnborg > Signed-off-by: Daniel Vetter > --- 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 I have tested the whole series on tiny/mi0283qt: Tested-by: Noralf Trønnes ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
On Wed, Feb 19, 2020 at 2:29 PM Thomas Zimmermann wrote:
>
> Hi
>
> Am 19.02.20 um 14:23 schrieb Daniel Vetter:
> > On Wed, Feb 19, 2020 at 12:47 PM Thomas Zimmermann
> > 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
> >>> Cc: Maxime Ripard
> >>> Cc: Thomas Zimmermann
> >>> Cc: David Airlie
> >>> Cc: Daniel Vetter
> >>> Cc: Eric Anholt
> >>> Cc: David Lechner
> >>> Cc: Kamlesh Gurudasani
> >>> Cc: "Noralf Trønnes"
> >>> Cc: Sam Ravnborg
> >>> Signed-off-by: Daniel Vetter
> >>> ---
> >>> 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
> >>> #include
> >>> #include
> >>> +#include
> >>> #include
> >>> #include
> >>> #include
> >>> @@ -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
>
>
Re: [Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
Hi
Am 19.02.20 um 14:23 schrieb Daniel Vetter:
> On Wed, Feb 19, 2020 at 12:47 PM Thomas Zimmermann
> 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
>>> Cc: Maxime Ripard
>>> Cc: Thomas Zimmermann
>>> Cc: David Airlie
>>> Cc: Daniel Vetter
>>> Cc: Eric Anholt
>>> Cc: David Lechner
>>> Cc: Kamlesh Gurudasani
>>> Cc: "Noralf Trønnes"
>>> Cc: Sam Ravnborg
>>> Signed-off-by: Daniel Vetter
>>> ---
>>> 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
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -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
>>> ---
Re: [Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
On Wed, Feb 19, 2020 at 12:47 PM Thomas Zimmermann 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
> > Cc: Maxime Ripard
> > Cc: Thomas Zimmermann
> > Cc: David Airlie
> > Cc: Daniel Vetter
> > Cc: Eric Anholt
> > Cc: David Lechner
> > Cc: Kamlesh Gurudasani
> > Cc: "Noralf Trønnes"
> > Cc: Sam Ravnborg
> > Signed-off-by: Daniel Vetter
> > ---
> > 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
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -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
> > #include
> > #include
> > +#include
> > #include
> > #include
> >
> > @@ -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
> > #include
> > #include
Re: [Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
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
>> Cc: Maxime Ripard
>> Cc: Thomas Zimmermann
>> Cc: David Airlie
>> Cc: Daniel Vetter
>> Cc: Eric Anholt
>> Cc: David Lechner
>> Cc: Kamlesh Gurudasani
>> Cc: "Noralf Trønnes"
>> Cc: Sam Ravnborg
>> Signed-off-by: Daniel Vetter
>> ---
>> 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
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -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
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -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
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -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
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -208,6 +209,7 @@ static int ili9486_probe(struct spi_device *spi)
>>
Re: [Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
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
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> Cc: Eric Anholt
> Cc: David Lechner
> Cc: Kamlesh Gurudasani
> Cc: "Noralf Trønnes"
> Cc: Sam Ravnborg
> Signed-off-by: Daniel Vetter
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
> #include
> #include
> +#include
> #include
> #include
>
> @@ -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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -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
> #include
> #include
> +#include
> #include
> #include
>
> @@ -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
> ---
[Intel-gfx] [PATCH 05/52] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers
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
Cc: Maxime Ripard
Cc: Thomas Zimmermann
Cc: David Airlie
Cc: Daniel Vetter
Cc: Eric Anholt
Cc: David Lechner
Cc: Kamlesh Gurudasani
Cc: "Noralf Trønnes"
Cc: Sam Ravnborg
Signed-off-by: Daniel Vetter
---
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
#include
#include
+#include
#include
#include
#include
@@ -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
#include
#include
+#include
#include
#include
@@ -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
#include
#include
+#include
#include
#include
#include
@@ -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
#include
#include
+#include
#include
#include
@@ -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
#include
#include
+#include
#include
#include
#include
@@ -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
#include
#include
+#include
#include
#include
@@ -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
#include
#include
+#include
#include
#define ST7735R_FRMCTR10xb1
@@ -209,6 +210,7 @@ static int st7735r_probe(struct spi_device *spi)
kfree(dbidev);
return ret;
}
+ drmm_add_final_kfree(drm, dbidev);
