[24/52] drm: Manage drm_gem_init with drmm_
diff mbox series

Message ID 20200219102122.1607365-25-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
We might want to look into pushing this down into drm_mm_init, but
that would mean rolling out return codes to a pile of functions
unfortunately. So let's leave that for now.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_drv.c      |  8 +-------
 drivers/gpu/drm/drm_gem.c      | 21 ++++++++++-----------
 drivers/gpu/drm/drm_internal.h |  1 -
 3 files changed, 11 insertions(+), 19 deletions(-)

Comments

Laurent Pinchart Feb. 19, 2020, 2:22 p.m. UTC | #1
Hi Daniel,

Thank you for the patch.

On Wed, Feb 19, 2020 at 11:20:54AM +0100, Daniel Vetter wrote:
> We might want to look into pushing this down into drm_mm_init, but
> that would mean rolling out return codes to a pile of functions
> unfortunately. So let's leave that for now.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c      |  8 +-------
>  drivers/gpu/drm/drm_gem.c      | 21 ++++++++++-----------
>  drivers/gpu/drm/drm_internal.h |  1 -
>  3 files changed, 11 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 03a1fb377830..7b3df1188da9 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -688,13 +688,10 @@ int drm_dev_init(struct drm_device *dev,
>  
>  	ret = drm_dev_set_unique(dev, dev_name(parent));
>  	if (ret)
> -		goto err_setunique;
> +		goto err;
>  
>  	return 0;
>  
> -err_setunique:
> -	if (drm_core_check_feature(dev, DRIVER_GEM))
> -		drm_gem_destroy(dev);
>  err:
>  	drm_managed_release(dev);
>  
> @@ -756,9 +753,6 @@ EXPORT_SYMBOL(devm_drm_dev_init);
>  void drm_dev_fini(struct drm_device *dev)
>  {
>  	drm_vblank_cleanup(dev);
> -
> -	if (drm_core_check_feature(dev, DRIVER_GEM))
> -		drm_gem_destroy(dev);
>  }
>  EXPORT_SYMBOL(drm_dev_fini);
>  
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 0b6e6623735e..31095e0f6b9f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -44,6 +44,7 @@
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> +#include <drm/drm_managed.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_vma_manager.h>
>  
> @@ -77,6 +78,12 @@
>   * up at a later date, and as our interface with shmfs for memory allocation.
>   */
>  
> +static void
> +drm_gem_init_release(struct drm_device *dev, void *ptr)
> +{
> +	drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> +}
> +
>  /**
>   * drm_gem_init - Initialize the GEM device fields
>   * @dev: drm_devic structure to initialize
> @@ -89,7 +96,8 @@ drm_gem_init(struct drm_device *dev)
>  	mutex_init(&dev->object_name_lock);
>  	idr_init_base(&dev->object_name_idr, 1);
>  
> -	vma_offset_manager = kzalloc(sizeof(*vma_offset_manager), GFP_KERNEL);
> +	vma_offset_manager = drmm_kzalloc(dev, sizeof(*vma_offset_manager),
> +					  GFP_KERNEL);
>  	if (!vma_offset_manager) {
>  		DRM_ERROR("out of memory\n");
>  		return -ENOMEM;
> @@ -100,16 +108,7 @@ drm_gem_init(struct drm_device *dev)
>  				    DRM_FILE_PAGE_OFFSET_START,
>  				    DRM_FILE_PAGE_OFFSET_SIZE);
>  
> -	return 0;
> -}
> -
> -void
> -drm_gem_destroy(struct drm_device *dev)
> -{
> -
> -	drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> -	kfree(dev->vma_offset_manager);
> -	dev->vma_offset_manager = NULL;
> +	return drmm_add_action(dev, drm_gem_init_release, NULL);

This looks fine as such (although I'm not sure if the managed API
overhead is really worth it for core code), but it leads to a potential
issue: if we handle more of the cleanup through the managed API, how do
we ensure that the cleanup functions are called in the right order (when
order matters) ?

>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 8c2628dfc6c7..cb09e95a795e 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -144,7 +144,6 @@ void drm_sysfs_lease_event(struct drm_device *dev);
>  /* drm_gem.c */
>  struct drm_gem_object;
>  int drm_gem_init(struct drm_device *dev);
> -void drm_gem_destroy(struct drm_device *dev);
>  int drm_gem_handle_create_tail(struct drm_file *file_priv,
>  			       struct drm_gem_object *obj,
>  			       u32 *handlep);
Daniel Vetter Feb. 19, 2020, 2:37 p.m. UTC | #2
On Wed, Feb 19, 2020 at 3:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> On Wed, Feb 19, 2020 at 11:20:54AM +0100, Daniel Vetter wrote:
> > We might want to look into pushing this down into drm_mm_init, but
> > that would mean rolling out return codes to a pile of functions
> > unfortunately. So let's leave that for now.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_drv.c      |  8 +-------
> >  drivers/gpu/drm/drm_gem.c      | 21 ++++++++++-----------
> >  drivers/gpu/drm/drm_internal.h |  1 -
> >  3 files changed, 11 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 03a1fb377830..7b3df1188da9 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -688,13 +688,10 @@ int drm_dev_init(struct drm_device *dev,
> >
> >       ret = drm_dev_set_unique(dev, dev_name(parent));
> >       if (ret)
> > -             goto err_setunique;
> > +             goto err;
> >
> >       return 0;
> >
> > -err_setunique:
> > -     if (drm_core_check_feature(dev, DRIVER_GEM))
> > -             drm_gem_destroy(dev);
> >  err:
> >       drm_managed_release(dev);
> >
> > @@ -756,9 +753,6 @@ EXPORT_SYMBOL(devm_drm_dev_init);
> >  void drm_dev_fini(struct drm_device *dev)
> >  {
> >       drm_vblank_cleanup(dev);
> > -
> > -     if (drm_core_check_feature(dev, DRIVER_GEM))
> > -             drm_gem_destroy(dev);
> >  }
> >  EXPORT_SYMBOL(drm_dev_fini);
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 0b6e6623735e..31095e0f6b9f 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -44,6 +44,7 @@
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_file.h>
> >  #include <drm/drm_gem.h>
> > +#include <drm/drm_managed.h>
> >  #include <drm/drm_print.h>
> >  #include <drm/drm_vma_manager.h>
> >
> > @@ -77,6 +78,12 @@
> >   * up at a later date, and as our interface with shmfs for memory allocation.
> >   */
> >
> > +static void
> > +drm_gem_init_release(struct drm_device *dev, void *ptr)
> > +{
> > +     drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> > +}
> > +
> >  /**
> >   * drm_gem_init - Initialize the GEM device fields
> >   * @dev: drm_devic structure to initialize
> > @@ -89,7 +96,8 @@ drm_gem_init(struct drm_device *dev)
> >       mutex_init(&dev->object_name_lock);
> >       idr_init_base(&dev->object_name_idr, 1);
> >
> > -     vma_offset_manager = kzalloc(sizeof(*vma_offset_manager), GFP_KERNEL);
> > +     vma_offset_manager = drmm_kzalloc(dev, sizeof(*vma_offset_manager),
> > +                                       GFP_KERNEL);
> >       if (!vma_offset_manager) {
> >               DRM_ERROR("out of memory\n");
> >               return -ENOMEM;
> > @@ -100,16 +108,7 @@ drm_gem_init(struct drm_device *dev)
> >                                   DRM_FILE_PAGE_OFFSET_START,
> >                                   DRM_FILE_PAGE_OFFSET_SIZE);
> >
> > -     return 0;
> > -}
> > -
> > -void
> > -drm_gem_destroy(struct drm_device *dev)
> > -{
> > -
> > -     drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> > -     kfree(dev->vma_offset_manager);
> > -     dev->vma_offset_manager = NULL;
> > +     return drmm_add_action(dev, drm_gem_init_release, NULL);
>
> This looks fine as such (although I'm not sure if the managed API
> overhead is really worth it for core code), but it leads to a potential
> issue: if we handle more of the cleanup through the managed API, how do
> we ensure that the cleanup functions are called in the right order (when
> order matters) ?

KASAN essentially (already helped while developing this), plus review.
It's still the same problem like reviewing onion unwind code, it's
just less fragile for the normal case.

I also think that if you have ordering constraints in your drm_device
release functions, there's a more fundamental problem going on.
Unfortunately we have a lot of these, which is why converting
everything in drm, including drivers, is not going to be easy nor
quick. There's a lot of problems. E.g. naively converting all
drm_connector allocations from devm_kzalloc to drmm_kzalloc still
means they get released too early, since the drm_mode_config_init
happens before you set up the connectors. So you still have the
problem that your connector_funcs->destroy gets called on already
freed memory. Lots of work ahead.
-Daniel


> >  }
> >
> >  /**
> > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > index 8c2628dfc6c7..cb09e95a795e 100644
> > --- a/drivers/gpu/drm/drm_internal.h
> > +++ b/drivers/gpu/drm/drm_internal.h
> > @@ -144,7 +144,6 @@ void drm_sysfs_lease_event(struct drm_device *dev);
> >  /* drm_gem.c */
> >  struct drm_gem_object;
> >  int drm_gem_init(struct drm_device *dev);
> > -void drm_gem_destroy(struct drm_device *dev);
> >  int drm_gem_handle_create_tail(struct drm_file *file_priv,
> >                              struct drm_gem_object *obj,
> >                              u32 *handlep);
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart Feb. 19, 2020, 2:52 p.m. UTC | #3
Hi Daniel,

On Wed, Feb 19, 2020 at 03:37:46PM +0100, Daniel Vetter wrote:
> On Wed, Feb 19, 2020 at 3:22 PM Laurent Pinchart wrote:
> > On Wed, Feb 19, 2020 at 11:20:54AM +0100, Daniel Vetter wrote:
> > > We might want to look into pushing this down into drm_mm_init, but
> > > that would mean rolling out return codes to a pile of functions
> > > unfortunately. So let's leave that for now.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_drv.c      |  8 +-------
> > >  drivers/gpu/drm/drm_gem.c      | 21 ++++++++++-----------
> > >  drivers/gpu/drm/drm_internal.h |  1 -
> > >  3 files changed, 11 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index 03a1fb377830..7b3df1188da9 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -688,13 +688,10 @@ int drm_dev_init(struct drm_device *dev,
> > >
> > >       ret = drm_dev_set_unique(dev, dev_name(parent));
> > >       if (ret)
> > > -             goto err_setunique;
> > > +             goto err;
> > >
> > >       return 0;
> > >
> > > -err_setunique:
> > > -     if (drm_core_check_feature(dev, DRIVER_GEM))
> > > -             drm_gem_destroy(dev);
> > >  err:
> > >       drm_managed_release(dev);
> > >
> > > @@ -756,9 +753,6 @@ EXPORT_SYMBOL(devm_drm_dev_init);
> > >  void drm_dev_fini(struct drm_device *dev)
> > >  {
> > >       drm_vblank_cleanup(dev);
> > > -
> > > -     if (drm_core_check_feature(dev, DRIVER_GEM))
> > > -             drm_gem_destroy(dev);
> > >  }
> > >  EXPORT_SYMBOL(drm_dev_fini);
> > >
> > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > index 0b6e6623735e..31095e0f6b9f 100644
> > > --- a/drivers/gpu/drm/drm_gem.c
> > > +++ b/drivers/gpu/drm/drm_gem.c
> > > @@ -44,6 +44,7 @@
> > >  #include <drm/drm_drv.h>
> > >  #include <drm/drm_file.h>
> > >  #include <drm/drm_gem.h>
> > > +#include <drm/drm_managed.h>
> > >  #include <drm/drm_print.h>
> > >  #include <drm/drm_vma_manager.h>
> > >
> > > @@ -77,6 +78,12 @@
> > >   * up at a later date, and as our interface with shmfs for memory allocation.
> > >   */
> > >
> > > +static void
> > > +drm_gem_init_release(struct drm_device *dev, void *ptr)
> > > +{
> > > +     drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> > > +}
> > > +
> > >  /**
> > >   * drm_gem_init - Initialize the GEM device fields
> > >   * @dev: drm_devic structure to initialize
> > > @@ -89,7 +96,8 @@ drm_gem_init(struct drm_device *dev)
> > >       mutex_init(&dev->object_name_lock);
> > >       idr_init_base(&dev->object_name_idr, 1);
> > >
> > > -     vma_offset_manager = kzalloc(sizeof(*vma_offset_manager), GFP_KERNEL);
> > > +     vma_offset_manager = drmm_kzalloc(dev, sizeof(*vma_offset_manager),
> > > +                                       GFP_KERNEL);
> > >       if (!vma_offset_manager) {
> > >               DRM_ERROR("out of memory\n");
> > >               return -ENOMEM;
> > > @@ -100,16 +108,7 @@ drm_gem_init(struct drm_device *dev)
> > >                                   DRM_FILE_PAGE_OFFSET_START,
> > >                                   DRM_FILE_PAGE_OFFSET_SIZE);
> > >
> > > -     return 0;
> > > -}
> > > -
> > > -void
> > > -drm_gem_destroy(struct drm_device *dev)
> > > -{
> > > -
> > > -     drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> > > -     kfree(dev->vma_offset_manager);
> > > -     dev->vma_offset_manager = NULL;
> > > +     return drmm_add_action(dev, drm_gem_init_release, NULL);
> >
> > This looks fine as such (although I'm not sure if the managed API
> > overhead is really worth it for core code), but it leads to a potential
> > issue: if we handle more of the cleanup through the managed API, how do
> > we ensure that the cleanup functions are called in the right order (when
> > order matters) ?
> 
> KASAN essentially (already helped while developing this), plus review.
> It's still the same problem like reviewing onion unwind code, it's
> just less fragile for the normal case.

That wasn't really my question though. If there are ordering
constraints, and if we want to honour them, the ordering of cleanups
need to be documented in the API (and of course implemented). We may for
instance want to always do cleanups in the reverse order of the
allocations.

> I also think that if you have ordering constraints in your drm_device
> release functions, there's a more fundamental problem going on.
> Unfortunately we have a lot of these, which is why converting
> everything in drm, including drivers, is not going to be easy nor
> quick. There's a lot of problems. E.g. naively converting all
> drm_connector allocations from devm_kzalloc to drmm_kzalloc still
> means they get released too early, since the drm_mode_config_init
> happens before you set up the connectors. So you still have the
> problem that your connector_funcs->destroy gets called on already
> freed memory. Lots of work ahead.

Yes that's the kind of issue I was thinking about. We have ordering
constraints, they will not go away. What's your idea on how to handle
this ?

> > >  }
> > >
> > >  /**
> > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > > index 8c2628dfc6c7..cb09e95a795e 100644
> > > --- a/drivers/gpu/drm/drm_internal.h
> > > +++ b/drivers/gpu/drm/drm_internal.h
> > > @@ -144,7 +144,6 @@ void drm_sysfs_lease_event(struct drm_device *dev);
> > >  /* drm_gem.c */
> > >  struct drm_gem_object;
> > >  int drm_gem_init(struct drm_device *dev);
> > > -void drm_gem_destroy(struct drm_device *dev);
> > >  int drm_gem_handle_create_tail(struct drm_file *file_priv,
> > >                              struct drm_gem_object *obj,
> > >                              u32 *handlep);
Daniel Vetter Feb. 19, 2020, 2:56 p.m. UTC | #4
On Wed, Feb 19, 2020 at 3:52 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Daniel,
>
> On Wed, Feb 19, 2020 at 03:37:46PM +0100, Daniel Vetter wrote:
> > On Wed, Feb 19, 2020 at 3:22 PM Laurent Pinchart wrote:
> > > On Wed, Feb 19, 2020 at 11:20:54AM +0100, Daniel Vetter wrote:
> > > > We might want to look into pushing this down into drm_mm_init, but
> > > > that would mean rolling out return codes to a pile of functions
> > > > unfortunately. So let's leave that for now.
> > > >
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_drv.c      |  8 +-------
> > > >  drivers/gpu/drm/drm_gem.c      | 21 ++++++++++-----------
> > > >  drivers/gpu/drm/drm_internal.h |  1 -
> > > >  3 files changed, 11 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > > index 03a1fb377830..7b3df1188da9 100644
> > > > --- a/drivers/gpu/drm/drm_drv.c
> > > > +++ b/drivers/gpu/drm/drm_drv.c
> > > > @@ -688,13 +688,10 @@ int drm_dev_init(struct drm_device *dev,
> > > >
> > > >       ret = drm_dev_set_unique(dev, dev_name(parent));
> > > >       if (ret)
> > > > -             goto err_setunique;
> > > > +             goto err;
> > > >
> > > >       return 0;
> > > >
> > > > -err_setunique:
> > > > -     if (drm_core_check_feature(dev, DRIVER_GEM))
> > > > -             drm_gem_destroy(dev);
> > > >  err:
> > > >       drm_managed_release(dev);
> > > >
> > > > @@ -756,9 +753,6 @@ EXPORT_SYMBOL(devm_drm_dev_init);
> > > >  void drm_dev_fini(struct drm_device *dev)
> > > >  {
> > > >       drm_vblank_cleanup(dev);
> > > > -
> > > > -     if (drm_core_check_feature(dev, DRIVER_GEM))
> > > > -             drm_gem_destroy(dev);
> > > >  }
> > > >  EXPORT_SYMBOL(drm_dev_fini);
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > > > index 0b6e6623735e..31095e0f6b9f 100644
> > > > --- a/drivers/gpu/drm/drm_gem.c
> > > > +++ b/drivers/gpu/drm/drm_gem.c
> > > > @@ -44,6 +44,7 @@
> > > >  #include <drm/drm_drv.h>
> > > >  #include <drm/drm_file.h>
> > > >  #include <drm/drm_gem.h>
> > > > +#include <drm/drm_managed.h>
> > > >  #include <drm/drm_print.h>
> > > >  #include <drm/drm_vma_manager.h>
> > > >
> > > > @@ -77,6 +78,12 @@
> > > >   * up at a later date, and as our interface with shmfs for memory allocation.
> > > >   */
> > > >
> > > > +static void
> > > > +drm_gem_init_release(struct drm_device *dev, void *ptr)
> > > > +{
> > > > +     drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> > > > +}
> > > > +
> > > >  /**
> > > >   * drm_gem_init - Initialize the GEM device fields
> > > >   * @dev: drm_devic structure to initialize
> > > > @@ -89,7 +96,8 @@ drm_gem_init(struct drm_device *dev)
> > > >       mutex_init(&dev->object_name_lock);
> > > >       idr_init_base(&dev->object_name_idr, 1);
> > > >
> > > > -     vma_offset_manager = kzalloc(sizeof(*vma_offset_manager), GFP_KERNEL);
> > > > +     vma_offset_manager = drmm_kzalloc(dev, sizeof(*vma_offset_manager),
> > > > +                                       GFP_KERNEL);
> > > >       if (!vma_offset_manager) {
> > > >               DRM_ERROR("out of memory\n");
> > > >               return -ENOMEM;
> > > > @@ -100,16 +108,7 @@ drm_gem_init(struct drm_device *dev)
> > > >                                   DRM_FILE_PAGE_OFFSET_START,
> > > >                                   DRM_FILE_PAGE_OFFSET_SIZE);
> > > >
> > > > -     return 0;
> > > > -}
> > > > -
> > > > -void
> > > > -drm_gem_destroy(struct drm_device *dev)
> > > > -{
> > > > -
> > > > -     drm_vma_offset_manager_destroy(dev->vma_offset_manager);
> > > > -     kfree(dev->vma_offset_manager);
> > > > -     dev->vma_offset_manager = NULL;
> > > > +     return drmm_add_action(dev, drm_gem_init_release, NULL);
> > >
> > > This looks fine as such (although I'm not sure if the managed API
> > > overhead is really worth it for core code), but it leads to a potential
> > > issue: if we handle more of the cleanup through the managed API, how do
> > > we ensure that the cleanup functions are called in the right order (when
> > > order matters) ?
> >
> > KASAN essentially (already helped while developing this), plus review.
> > It's still the same problem like reviewing onion unwind code, it's
> > just less fragile for the normal case.
>
> That wasn't really my question though. If there are ordering
> constraints, and if we want to honour them, the ordering of cleanups
> need to be documented in the API (and of course implemented). We may for
> instance want to always do cleanups in the reverse order of the
> allocations.
>
> > I also think that if you have ordering constraints in your drm_device
> > release functions, there's a more fundamental problem going on.
> > Unfortunately we have a lot of these, which is why converting
> > everything in drm, including drivers, is not going to be easy nor
> > quick. There's a lot of problems. E.g. naively converting all
> > drm_connector allocations from devm_kzalloc to drmm_kzalloc still
> > means they get released too early, since the drm_mode_config_init
> > happens before you set up the connectors. So you still have the
> > problem that your connector_funcs->destroy gets called on already
> > freed memory. Lots of work ahead.
>
> Yes that's the kind of issue I was thinking about. We have ordering
> constraints, they will not go away. What's your idea on how to handle
> this ?

drmm_ guarantees that release actions are executed in reverse order of
how they're added. That's the right thing to do in 99% of cases. For
the others you need manual unwind logic, maybe with a combined release
action. Or some some safety checks in your release hook. I think the
drm_minor_alloc conversion is a useful example of some of the problems
that can lurk, and options for handling it all.
-Daniel

>
> > > >  }
> > > >
> > > >  /**
> > > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> > > > index 8c2628dfc6c7..cb09e95a795e 100644
> > > > --- a/drivers/gpu/drm/drm_internal.h
> > > > +++ b/drivers/gpu/drm/drm_internal.h
> > > > @@ -144,7 +144,6 @@ void drm_sysfs_lease_event(struct drm_device *dev);
> > > >  /* drm_gem.c */
> > > >  struct drm_gem_object;
> > > >  int drm_gem_init(struct drm_device *dev);
> > > > -void drm_gem_destroy(struct drm_device *dev);
> > > >  int drm_gem_handle_create_tail(struct drm_file *file_priv,
> > > >                              struct drm_gem_object *obj,
> > > >                              u32 *handlep);
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 03a1fb377830..7b3df1188da9 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -688,13 +688,10 @@  int drm_dev_init(struct drm_device *dev,
 
 	ret = drm_dev_set_unique(dev, dev_name(parent));
 	if (ret)
-		goto err_setunique;
+		goto err;
 
 	return 0;
 
-err_setunique:
-	if (drm_core_check_feature(dev, DRIVER_GEM))
-		drm_gem_destroy(dev);
 err:
 	drm_managed_release(dev);
 
@@ -756,9 +753,6 @@  EXPORT_SYMBOL(devm_drm_dev_init);
 void drm_dev_fini(struct drm_device *dev)
 {
 	drm_vblank_cleanup(dev);
-
-	if (drm_core_check_feature(dev, DRIVER_GEM))
-		drm_gem_destroy(dev);
 }
 EXPORT_SYMBOL(drm_dev_fini);
 
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 0b6e6623735e..31095e0f6b9f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -44,6 +44,7 @@ 
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 #include <drm/drm_vma_manager.h>
 
@@ -77,6 +78,12 @@ 
  * up at a later date, and as our interface with shmfs for memory allocation.
  */
 
+static void
+drm_gem_init_release(struct drm_device *dev, void *ptr)
+{
+	drm_vma_offset_manager_destroy(dev->vma_offset_manager);
+}
+
 /**
  * drm_gem_init - Initialize the GEM device fields
  * @dev: drm_devic structure to initialize
@@ -89,7 +96,8 @@  drm_gem_init(struct drm_device *dev)
 	mutex_init(&dev->object_name_lock);
 	idr_init_base(&dev->object_name_idr, 1);
 
-	vma_offset_manager = kzalloc(sizeof(*vma_offset_manager), GFP_KERNEL);
+	vma_offset_manager = drmm_kzalloc(dev, sizeof(*vma_offset_manager),
+					  GFP_KERNEL);
 	if (!vma_offset_manager) {
 		DRM_ERROR("out of memory\n");
 		return -ENOMEM;
@@ -100,16 +108,7 @@  drm_gem_init(struct drm_device *dev)
 				    DRM_FILE_PAGE_OFFSET_START,
 				    DRM_FILE_PAGE_OFFSET_SIZE);
 
-	return 0;
-}
-
-void
-drm_gem_destroy(struct drm_device *dev)
-{
-
-	drm_vma_offset_manager_destroy(dev->vma_offset_manager);
-	kfree(dev->vma_offset_manager);
-	dev->vma_offset_manager = NULL;
+	return drmm_add_action(dev, drm_gem_init_release, NULL);
 }
 
 /**
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
index 8c2628dfc6c7..cb09e95a795e 100644
--- a/drivers/gpu/drm/drm_internal.h
+++ b/drivers/gpu/drm/drm_internal.h
@@ -144,7 +144,6 @@  void drm_sysfs_lease_event(struct drm_device *dev);
 /* drm_gem.c */
 struct drm_gem_object;
 int drm_gem_init(struct drm_device *dev);
-void drm_gem_destroy(struct drm_device *dev);
 int drm_gem_handle_create_tail(struct drm_file *file_priv,
 			       struct drm_gem_object *obj,
 			       u32 *handlep);