On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> wrote: > On Thu, 12 Dec 2013 23:54:37 +0100 > Daniel Vetter <daniel@xxxxxxxx> wrote: > >> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev, >> > >> > drm_fb_helper_fini(&ifbdev->helper); >> > >> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base); >> > - intel_framebuffer_fini(&ifbdev->ifb); >> > + drm_framebuffer_unregister_private(&ifbdev->fb->base); >> > + intel_framebuffer_fini(ifbdev->fb); >> > + kfree(ifbdev->fb); >> >> No need to go the private fb route here anymore since now the fb is >> free-standing. Normal refcounting should work. But a separate prep/cleanup >> patch (prep since switching ifbdev->fb from struct to point would look >> neat as a separate patch). > > Oh and can you explain this? I wouldn't be surprised if I got the > refcounting wrong, but given how tricky it can be, can you explain > where we'll take the ref here, and show that the right thing will > happen if/when we mode set away from this buffer? > > I haven't actually seen a bug here with or without this patch (no > crashes or warns), but I thought I needed this to make sure the obj > didn't get a negative count after a mode set... There's no bug here, and if you underrun the the refcount the current logic here won't help. It's just that the explicit call to _fini was an artifact of the old logic with embedding the framebuffer into the fbdev structure. But now that the fbdev framebuffer is freestanding there's no need for that - you exactly duplicate intel_user_framebuffer_destroy now. So a simple drm_framebuffer_unreference will do the trick and makes it clearer that this is now just another fb object with normal lifetime rules. I guess I score points for unclear review here ;-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
