Re: [PATCH 24/52] drm: Manage drm_gem_init with drmm_

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