| Message ID | 1363098671-4747-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ffwll.ch> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Daniel, On Monday 18 March 2013 09:06:21 Daniel Vetter wrote: > 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. It's not mandatory indeed, but it's a step in the right direction in my opinion. Maybe a separate patch that just moves functions around in drm_crtc.c would be a better idea :-) > Drop that code movement? I have no strong opinion, I can drop it if that's preferred. > Otherwise Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Thank you.
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);
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. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/gpu/drm/drm_crtc.c | 208 +++++++++++++++++++++++---------------------- 1 file changed, 107 insertions(+), 101 deletions(-)