| Message ID | 5031860caad67faa0f1be5965331ef048a311a01.1465383212.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 08, 2016 at 01:15:22PM +0200, Lukas Wunner wrote: > Calling drm_framebuffer_unregister_private() in intel_fbdev_destroy() is > superfluous because the framebuffer will subsequently be unregistered by > drm_framebuffer_free() when unreferenced in drm_framebuffer_remove(). > The call is a leftover, when it was introduced by commit 362063619cf6 > ("drm: revamp framebuffer cleanup interfaces"), struct intel_framebuffer > was still embedded in struct intel_fbdev rather than being a pointer as > it is today, and drm_framebuffer_remove() wasn't used yet. > > As a bonus, the ID of the framebuffer is no longer 0 in the debug log: > > Before: > [ 39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3) > [ 39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2) > [ 39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1) > > After: > [ 102.504649] [drm:drm_mode_object_unreference] OBJ ID: 45 (3) > [ 102.504651] [drm:drm_mode_object_unreference] OBJ ID: 45 (2) > [ 102.504654] [drm:drm_mode_object_unreference] OBJ ID: 45 (1) > > Signed-off-by: Lukas Wunner <lukas@wunner.de> Hm yeah. But there's a pile of that particaluar cargo-culting copied all over the place, would be really good to audit all callers of unregister_private and fix them all up. A few older drivers still embed the fbdev fb, but most don't but still use the unregister_private + cleanup combo. Nitpick in your subject: s/fbdev/fbdev's fb/ Cheers, Daniel > --- > drivers/gpu/drm/i915/intel_fbdev.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index ef8e676..4c7ea46 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -552,8 +552,6 @@ static void intel_fbdev_destroy(struct drm_device *dev, > drm_fb_helper_fini(&ifbdev->helper); > > if (ifbdev->fb) { > - drm_framebuffer_unregister_private(&ifbdev->fb->base); > - > mutex_lock(&dev->struct_mutex); > intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0)); > mutex_unlock(&dev->struct_mutex); > -- > 2.8.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jun 08, 2016 at 02:09:40PM +0200, Daniel Vetter wrote: > On Wed, Jun 08, 2016 at 01:15:22PM +0200, Lukas Wunner wrote: > > Calling drm_framebuffer_unregister_private() in intel_fbdev_destroy() is > > superfluous because the framebuffer will subsequently be unregistered by > > drm_framebuffer_free() when unreferenced in drm_framebuffer_remove(). > > The call is a leftover, when it was introduced by commit 362063619cf6 > > ("drm: revamp framebuffer cleanup interfaces"), struct intel_framebuffer > > was still embedded in struct intel_fbdev rather than being a pointer as > > it is today, and drm_framebuffer_remove() wasn't used yet. > > > > As a bonus, the ID of the framebuffer is no longer 0 in the debug log: > > > > Before: > > [ 39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3) > > [ 39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2) > > [ 39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1) > > > > After: > > [ 102.504649] [drm:drm_mode_object_unreference] OBJ ID: 45 (3) > > [ 102.504651] [drm:drm_mode_object_unreference] OBJ ID: 45 (2) > > [ 102.504654] [drm:drm_mode_object_unreference] OBJ ID: 45 (1) > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > Hm yeah. But there's a pile of that particaluar cargo-culting copied all > over the place, would be really good to audit all callers of > unregister_private and fix them all up. A few older drivers still embed > the fbdev fb, but most don't but still use the unregister_private + > cleanup combo. Yes, I noticed that but i915 was the only one that I could actually test, the others I can only compile test. So fixing those up requires very careful examination and takes more time, but I'll keep it on my todo list. > Nitpick in your subject: s/fbdev/fbdev's fb/ Right, should I post a v2 or are you going to fix it up if/when merging? Thanks, Lukas > > > --- > > drivers/gpu/drm/i915/intel_fbdev.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > > index ef8e676..4c7ea46 100644 > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > @@ -552,8 +552,6 @@ static void intel_fbdev_destroy(struct drm_device *dev, > > drm_fb_helper_fini(&ifbdev->helper); > > > > if (ifbdev->fb) { > > - drm_framebuffer_unregister_private(&ifbdev->fb->base); > > - > > mutex_lock(&dev->struct_mutex); > > intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0)); > > mutex_unlock(&dev->struct_mutex); > > -- > > 2.8.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On Wed, Jun 08, 2016 at 07:03:02PM +0200, Lukas Wunner wrote: > On Wed, Jun 08, 2016 at 02:09:40PM +0200, Daniel Vetter wrote: > > On Wed, Jun 08, 2016 at 01:15:22PM +0200, Lukas Wunner wrote: > > > Calling drm_framebuffer_unregister_private() in intel_fbdev_destroy() is > > > superfluous because the framebuffer will subsequently be unregistered by > > > drm_framebuffer_free() when unreferenced in drm_framebuffer_remove(). > > > The call is a leftover, when it was introduced by commit 362063619cf6 > > > ("drm: revamp framebuffer cleanup interfaces"), struct intel_framebuffer > > > was still embedded in struct intel_fbdev rather than being a pointer as > > > it is today, and drm_framebuffer_remove() wasn't used yet. > > > > > > As a bonus, the ID of the framebuffer is no longer 0 in the debug log: > > > > > > Before: > > > [ 39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3) > > > [ 39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2) > > > [ 39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1) > > > > > > After: > > > [ 102.504649] [drm:drm_mode_object_unreference] OBJ ID: 45 (3) > > > [ 102.504651] [drm:drm_mode_object_unreference] OBJ ID: 45 (2) > > > [ 102.504654] [drm:drm_mode_object_unreference] OBJ ID: 45 (1) > > > > > > Signed-off-by: Lukas Wunner <lukas@wunner.de> > > > > Hm yeah. But there's a pile of that particaluar cargo-culting copied all > > over the place, would be really good to audit all callers of > > unregister_private and fix them all up. A few older drivers still embed > > the fbdev fb, but most don't but still use the unregister_private + > > cleanup combo. > > Yes, I noticed that but i915 was the only one that I could actually test, > the others I can only compile test. So fixing those up requires very > careful examination and takes more time, but I'll keep it on my todo list. > > > > Nitpick in your subject: s/fbdev/fbdev's fb/ > > Right, should I post a v2 or are you going to fix it up if/when merging? Fixed up while applying - I just waited for CI to get around (and then w/e). Going through the other drivers to nuke the cargo-culting would still be awesome. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index ef8e676..4c7ea46 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -552,8 +552,6 @@ static void intel_fbdev_destroy(struct drm_device *dev, drm_fb_helper_fini(&ifbdev->helper); if (ifbdev->fb) { - drm_framebuffer_unregister_private(&ifbdev->fb->base); - mutex_lock(&dev->struct_mutex); intel_unpin_fb_obj(&ifbdev->fb->base, BIT(DRM_ROTATE_0)); mutex_unlock(&dev->struct_mutex);
Calling drm_framebuffer_unregister_private() in intel_fbdev_destroy() is superfluous because the framebuffer will subsequently be unregistered by drm_framebuffer_free() when unreferenced in drm_framebuffer_remove(). The call is a leftover, when it was introduced by commit 362063619cf6 ("drm: revamp framebuffer cleanup interfaces"), struct intel_framebuffer was still embedded in struct intel_fbdev rather than being a pointer as it is today, and drm_framebuffer_remove() wasn't used yet. As a bonus, the ID of the framebuffer is no longer 0 in the debug log: Before: [ 39.680874] [drm:drm_mode_object_unreference] OBJ ID: 0 (3) [ 39.680878] [drm:drm_mode_object_unreference] OBJ ID: 0 (2) [ 39.680884] [drm:drm_mode_object_unreference] OBJ ID: 0 (1) After: [ 102.504649] [drm:drm_mode_object_unreference] OBJ ID: 45 (3) [ 102.504651] [drm:drm_mode_object_unreference] OBJ ID: 45 (2) [ 102.504654] [drm:drm_mode_object_unreference] OBJ ID: 45 (1) Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/gpu/drm/i915/intel_fbdev.c | 2 -- 1 file changed, 2 deletions(-)