[RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Laurent Pinchart]

Hi Daniel,

On Tuesday 16 April 2013 21:06:43 Daniel Vetter wrote:
> On Tue, Apr 16, 2013 at 01:12:21PM +1000, Dave Airlie wrote:
> > On Mon, Apr 15, 2013 at 11:37 PM, 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.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > 
> > > Reviewed-by: Daniel Vetter 
> > 
> > I've applied this one, the other two I don't mind, but I'm not sure
> > they aren't a bit restrictive in the generic code,
> 
> Patch 2 is just for the crtc helpers, and matches what we've just done in
> the i915 code. I think it fits in with the general design of the crtc
> helpers.
> 
> Patch 1 matches a check in the intel code, too. So since applications
> can't really know whether a flip to a different pixel format works I don't
> think that capability is useful at all. And at least for i915 it's just
> broken ;-) On patch 1 itself though: Just checking fb->pixel_format should
> be good enough.

I've fixed that and I'll resubmit after we reach an agreement on 1/3 and 2/3.

-- 
Regards,

Laurent Pinchart

[RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Daniel Vetter]

On Tue, Apr 16, 2013 at 01:12:21PM +1000, Dave Airlie wrote:
> On Mon, Apr 15, 2013 at 11:37 PM, 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.
> >
> > Signed-off-by: Laurent Pinchart  > ideasonboard.com>
> > Reviewed-by: Daniel Vetter 
> 
> I've applied this one, the other two I don't mind, but I'm not sure
> they aren't a bit restrictive in the generic code,

Patch 2 is just for the crtc helpers, and matches what we've just done in
the i915 code. I think it fits in with the general design of the crtc
helpers.

Patch 1 matches a check in the intel code, too. So since applications
can't really know whether a flip to a different pixel format works I don't
think that capability is useful at all. And at least for i915 it's just
broken ;-) On patch 1 itself though: Just checking fb->pixel_format should
be good enough.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Dave Airlie]

On Mon, Apr 15, 2013 at 11:37 PM, 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.
>
> Signed-off-by: Laurent Pinchart 
> Reviewed-by: Daniel Vetter 

I've applied this one, the other two I don't mind, but I'm not sure
they aren't a bit restrictive in the generic code,

Dave.

[RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Laurent Pinchart]

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+rene...@ideasonboard.com
Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
---
 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 ef247d5..db9707e 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: 

Re: [RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Daniel Vetter]

On Tue, Apr 16, 2013 at 01:12:21PM +1000, Dave Airlie wrote:
 On Mon, Apr 15, 2013 at 11:37 PM, Laurent Pinchart
 laurent.pinchart+rene...@ideasonboard.com 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.
 
  Signed-off-by: Laurent Pinchart laurent.pinchart+rene...@ideasonboard.com
  Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
 
 I've applied this one, the other two I don't mind, but I'm not sure
 they aren't a bit restrictive in the generic code,

Patch 2 is just for the crtc helpers, and matches what we've just done in
the i915 code. I think it fits in with the general design of the crtc
helpers.

Patch 1 matches a check in the intel code, too. So since applications
can't really know whether a flip to a different pixel format works I don't
think that capability is useful at all. And at least for i915 it's just
broken ;-) On patch 1 itself though: Just checking fb-pixel_format should
be good enough.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Laurent Pinchart]

Hi Daniel,

On Tuesday 16 April 2013 21:06:43 Daniel Vetter wrote:
 On Tue, Apr 16, 2013 at 01:12:21PM +1000, Dave Airlie wrote:
  On Mon, Apr 15, 2013 at 11:37 PM, 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.
   
   Signed-off-by: Laurent Pinchart
   laurent.pinchart+rene...@ideasonboard.com
   Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch
  
  I've applied this one, the other two I don't mind, but I'm not sure
  they aren't a bit restrictive in the generic code,
 
 Patch 2 is just for the crtc helpers, and matches what we've just done in
 the i915 code. I think it fits in with the general design of the crtc
 helpers.
 
 Patch 1 matches a check in the intel code, too. So since applications
 can't really know whether a flip to a different pixel format works I don't
 think that capability is useful at all. And at least for i915 it's just
 broken ;-) On patch 1 itself though: Just checking fb-pixel_format should
 be good enough.

I've fixed that and I'll resubmit after we reach an agreement on 1/3 and 2/3.

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Laurent Pinchart]

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 
Reviewed-by: Daniel Vetter 
---
 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 ef247d5..db9707e 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(>mode_config.mutex);
-   mutex_init(>mode_config.idr_mutex);
-   mutex_init(>mode_config.fb_lock);
-   INIT_LIST_HEAD(>mode_config.fb_list);
-   INIT_LIST_HEAD(>mode_config.crtc_list);
-   INIT_LIST_HEAD(>mode_config.connector_list);
-   INIT_LIST_HEAD(>mode_config.encoder_list);
-   INIT_LIST_HEAD(>mode_config.property_list);
-   INIT_LIST_HEAD(>mode_config.property_blob_list);
-   INIT_LIST_HEAD(>mode_config.plane_list);
-   idr_init(>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, >mode_config.encoder_list,
-head) {
-   encoder->funcs->destroy(encoder);
-   }
-
-   list_for_each_entry_safe(connector, ot,
->mode_config.connector_list, head) {
-   connector->funcs->destroy(connector);
-   }
-
-   list_for_each_entry_safe(property, pt, >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(>mode_config.fb_list));
-   list_for_each_entry_safe(fb, fbt, >mode_config.fb_list, head) {
-   drm_framebuffer_remove(fb);
-   }
-
-   list_for_each_entry_safe(plane, plt, >mode_config.plane_list,
-head) {
-   plane->funcs->destroy(plane);
-   }
-
-   list_for_each_entry_safe(crtc, ct, >mode_config.crtc_list, head) {
-   crtc->funcs->destroy(crtc);
-   }
-
-   idr_destroy(>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: 

Re: [RESEND PATCH 3/3] drm: Destroy property blobs at mode config cleanup time

[Dave Airlie]

On Mon, Apr 15, 2013 at 11:37 PM, Laurent Pinchart
laurent.pinchart+rene...@ideasonboard.com 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.

 Signed-off-by: Laurent Pinchart laurent.pinchart+rene...@ideasonboard.com
 Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch

I've applied this one, the other two I don't mind, but I'm not sure
they aren't a bit restrictive in the generic code,

Dave.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel