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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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 :/

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

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center