Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Dec 13, 2013 at 04:43:50PM -0800, Jesse Barnes wrote:
> On Fri, 13 Dec 2013 21:47:45 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > On Thu, 12 Dec 2013 23:54:37 +0100
> > > Daniel Vetter <daniel@ffwll.ch> 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 ;-)
> 
> Oh!  I had this mixed up with the refs I take in the init_bios code...
> I thought you were saying those weren't necessary and I was getting
> really confused.
> 
> This is just fixing up existing code to use the new field name, so no
> functional change.  I see what you mean about splitting out the field
> change, but now that would be a pain :/

Yeah, the switch from struct to pointer for ifbdev->fb would be neat as a
separate patch, but also real pain to split out now.

> Do you want the above removed as a separate patch regardless of where
> the rest of the patches go?

Imo it should go with the switch to pointers, so this patch here is fine.
Maybe a quick mention in the commit message about it.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch