On Tue, Mar 12, 2013 at 03:31:11PM +0100, Laurent Pinchart wrote:
> Property blob objects need to be destroyed when cleaning up to avoid
> memory leaks. Go through the list of all blobs in the
> drm_mode_config_cleanup() function and destroy them.
>
> The drm_mode_config_cleanup() function needs to be moved after the
> drm_property_destroy_blob() declaration. Move drm_mode_config_init() as
> well to keep the functions together.
Imo moving drm_mode_config_init looks a bit superflous in this patch,
since there's still some other init code left around at the old place.
Drop that code movement?
Otherwise Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_crtc.c | 208 +++++++++++++++++++++++----------------------
> 1 file changed, 107 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index cf4ce3d..b597734 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1120,44 +1120,6 @@ int drm_mode_create_dirty_info_property(struct drm_device *dev)
> }
> EXPORT_SYMBOL(drm_mode_create_dirty_info_property);
>
> -/**
> - * drm_mode_config_init - initialize DRM mode_configuration structure
> - * @dev: DRM device
> - *
> - * Initialize @dev's mode_config structure, used for tracking the graphics
> - * configuration of @dev.
> - *
> - * Since this initializes the modeset locks, no locking is possible. Which is no
> - * problem, since this should happen single threaded at init time. It is the
> - * driver's problem to ensure this guarantee.
> - *
> - */
> -void drm_mode_config_init(struct drm_device *dev)
> -{
> - mutex_init(&dev->mode_config.mutex);
> - mutex_init(&dev->mode_config.idr_mutex);
> - mutex_init(&dev->mode_config.fb_lock);
> - INIT_LIST_HEAD(&dev->mode_config.fb_list);
> - INIT_LIST_HEAD(&dev->mode_config.crtc_list);
> - INIT_LIST_HEAD(&dev->mode_config.connector_list);
> - INIT_LIST_HEAD(&dev->mode_config.encoder_list);
> - INIT_LIST_HEAD(&dev->mode_config.property_list);
> - INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
> - INIT_LIST_HEAD(&dev->mode_config.plane_list);
> - idr_init(&dev->mode_config.crtc_idr);
> -
> - drm_modeset_lock_all(dev);
> - drm_mode_create_standard_connector_properties(dev);
> - drm_modeset_unlock_all(dev);
> -
> - /* Just to be sure */
> - dev->mode_config.num_fb = 0;
> - dev->mode_config.num_connector = 0;
> - dev->mode_config.num_crtc = 0;
> - dev->mode_config.num_encoder = 0;
> -}
> -EXPORT_SYMBOL(drm_mode_config_init);
> -
> int drm_mode_group_init(struct drm_device *dev, struct drm_mode_group *group)
> {
> uint32_t total_objects = 0;
> @@ -1203,69 +1165,6 @@ int drm_mode_group_init_legacy_group(struct drm_device *dev,
> EXPORT_SYMBOL(drm_mode_group_init_legacy_group);
>
> /**
> - * drm_mode_config_cleanup - free up DRM mode_config info
> - * @dev: DRM device
> - *
> - * Free up all the connectors and CRTCs associated with this DRM device, then
> - * free up the framebuffers and associated buffer objects.
> - *
> - * Note that since this /should/ happen single-threaded at driver/device
> - * teardown time, no locking is required. It's the driver's job to ensure that
> - * this guarantee actually holds true.
> - *
> - * FIXME: cleanup any dangling user buffer objects too
> - */
> -void drm_mode_config_cleanup(struct drm_device *dev)
> -{
> - struct drm_connector *connector, *ot;
> - struct drm_crtc *crtc, *ct;
> - struct drm_encoder *encoder, *enct;
> - struct drm_framebuffer *fb, *fbt;
> - struct drm_property *property, *pt;
> - struct drm_plane *plane, *plt;
> -
> - list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
> - head) {
> - encoder->funcs->destroy(encoder);
> - }
> -
> - list_for_each_entry_safe(connector, ot,
> - &dev->mode_config.connector_list, head) {
> - connector->funcs->destroy(connector);
> - }
> -
> - list_for_each_entry_safe(property, pt, &dev->mode_config.property_list,
> - head) {
> - drm_property_destroy(dev, property);
> - }
> -
> - /*
> - * Single-threaded teardown context, so it's not required to grab the
> - * fb_lock to protect against concurrent fb_list access. Contrary, it
> - * would actually deadlock with the drm_framebuffer_cleanup function.
> - *
> - * Also, if there are any framebuffers left, that's a driver leak now,
> - * so politely WARN about this.
> - */
> - WARN_ON(!list_empty(&dev->mode_config.fb_list));
> - list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
> - drm_framebuffer_remove(fb);
> - }
> -
> - list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
> - head) {
> - plane->funcs->destroy(plane);
> - }
> -
> - list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> - crtc->funcs->destroy(crtc);
> - }
> -
> - idr_destroy(&dev->mode_config.crtc_idr);
> -}
> -EXPORT_SYMBOL(drm_mode_config_cleanup);
> -
> -/**
> * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo
> * @out: drm_mode_modeinfo struct to return to the user
> * @in: drm_display_mode to use
> @@ -4074,3 +3973,110 @@ int drm_format_vert_chroma_subsampling(uint32_t format)
> }
> }
> EXPORT_SYMBOL(drm_format_vert_chroma_subsampling);
> +
> +/**
> + * drm_mode_config_init - initialize DRM mode_configuration structure
> + * @dev: DRM device
> + *
> + * Initialize @dev's mode_config structure, used for tracking the graphics
> + * configuration of @dev.
> + *
> + * Since this initializes the modeset locks, no locking is possible. Which is no
> + * problem, since this should happen single threaded at init time. It is the
> + * driver's problem to ensure this guarantee.
> + *
> + */
> +void drm_mode_config_init(struct drm_device *dev)
> +{
> + mutex_init(&dev->mode_config.mutex);
> + mutex_init(&dev->mode_config.idr_mutex);
> + mutex_init(&dev->mode_config.fb_lock);
> + INIT_LIST_HEAD(&dev->mode_config.fb_list);
> + INIT_LIST_HEAD(&dev->mode_config.crtc_list);
> + INIT_LIST_HEAD(&dev->mode_config.connector_list);
> + INIT_LIST_HEAD(&dev->mode_config.encoder_list);
> + INIT_LIST_HEAD(&dev->mode_config.property_list);
> + INIT_LIST_HEAD(&dev->mode_config.property_blob_list);
> + INIT_LIST_HEAD(&dev->mode_config.plane_list);
> + idr_init(&dev->mode_config.crtc_idr);
> +
> + drm_modeset_lock_all(dev);
> + drm_mode_create_standard_connector_properties(dev);
> + drm_modeset_unlock_all(dev);
> +
> + /* Just to be sure */
> + dev->mode_config.num_fb = 0;
> + dev->mode_config.num_connector = 0;
> + dev->mode_config.num_crtc = 0;
> + dev->mode_config.num_encoder = 0;
> +}
> +EXPORT_SYMBOL(drm_mode_config_init);
> +
> +/**
> + * drm_mode_config_cleanup - free up DRM mode_config info
> + * @dev: DRM device
> + *
> + * Free up all the connectors and CRTCs associated with this DRM device, then
> + * free up the framebuffers and associated buffer objects.
> + *
> + * Note that since this /should/ happen single-threaded at driver/device
> + * teardown time, no locking is required. It's the driver's job to ensure that
> + * this guarantee actually holds true.
> + *
> + * FIXME: cleanup any dangling user buffer objects too
> + */
> +void drm_mode_config_cleanup(struct drm_device *dev)
> +{
> + struct drm_connector *connector, *ot;
> + struct drm_crtc *crtc, *ct;
> + struct drm_encoder *encoder, *enct;
> + struct drm_framebuffer *fb, *fbt;
> + struct drm_property *property, *pt;
> + struct drm_property_blob *blob, *bt;
> + struct drm_plane *plane, *plt;
> +
> + list_for_each_entry_safe(encoder, enct, &dev->mode_config.encoder_list,
> + head) {
> + encoder->funcs->destroy(encoder);
> + }
> +
> + list_for_each_entry_safe(connector, ot,
> + &dev->mode_config.connector_list, head) {
> + connector->funcs->destroy(connector);
> + }
> +
> + list_for_each_entry_safe(property, pt, &dev->mode_config.property_list,
> + head) {
> + drm_property_destroy(dev, property);
> + }
> +
> + list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list,
> + head) {
> + drm_property_destroy_blob(dev, blob);
> + }
> +
> + /*
> + * Single-threaded teardown context, so it's not required to grab the
> + * fb_lock to protect against concurrent fb_list access. Contrary, it
> + * would actually deadlock with the drm_framebuffer_cleanup function.
> + *
> + * Also, if there are any framebuffers left, that's a driver leak now,
> + * so politely WARN about this.
> + */
> + WARN_ON(!list_empty(&dev->mode_config.fb_list));
> + list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
> + drm_framebuffer_remove(fb);
> + }
> +
> + list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
> + head) {
> + plane->funcs->destroy(plane);
> + }
> +
> + list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> + crtc->funcs->destroy(crtc);
> + }
> +
> + idr_destroy(&dev->mode_config.crtc_idr);
> +}
> +EXPORT_SYMBOL(drm_mode_config_cleanup);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
