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

[Daniel Vetter]

On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> On Thu, 12 Dec 2013 23:54:37 +0100
> Daniel Vetter <daniel at 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 ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch