DRM framebuffer refcounting bug... still present

[Russell King - ARM Linux]

Daniel,

A while back you had a go at fixing the framebuffer reference counting.
Unfortunately, I'm still seeing leaks, particularly with the framebuffer
used with a mode set which is subsequently flipped.  It is only this
framebuffer which is leaked, flips of an already flipped framebuffer do
not.

Looking at what happens to this framebuffer, I see these refcount changes:

ADDFB
- 0->1 - kref_init
- 1->2 - drm_framebuffer_reference() creation

SETCRTC
- 2->3 - drm_framebuffer_lookup() in drm_mode_setcrtc()
- 3->4 - drm_framebuffer_reference() in armada_drm_crtc_mode_set()
- 4->5 - drm_framebuffer_reference() in drm_mode_set_config_internal()
- 5->4 - drm_framebuffer_unreference() in drm_mode_setcrtc()
* current scanout framebuffer has refcount of 4

PAGE_FLIP
- 4->5 - drm_framebuffer_reference() in armada_drm_crtc_page_flip()
- 5->4 - drm_framebuffer_unreference(old_fb) in drm_mode_page_flip_ioctl()
- 4->3 - drm_framebuffer_unreference() in armada_drm_unref_work()

RMFB
- 3->2 - __drm_framebuffer_unreference()
- 2->1 - drm_framebuffer_unreference() - *LEAK*

For the framebuffer being flipped in:

ADDFB
- 0->1 - kref_init
- 1->2 - drm_framebuffer_reference() creation

PAGE_FLIP (incoming)
- 2->3 - drm_framebuffer_lookup() in drm_mode_page_flip_ioctl()
* current scanout framebuffer has refcount of 3 - different from setcrtc.

PAGE_FLIP (outgoing)
- 3->4 - drm_framebuffer_reference() in armada_drm_crtc_page_flip()
- 4->3 - drm_framebuffer_unreference(old_fb) in drm_mode_page_flip_ioctl()
- 3->2 - drm_framebuffer_unreference() in armada_drm_unref_work()

RMFB
- 2->1 - __drm_framebuffer_unreference()
- 1->0 - drm_framebuffer_unreference() - *free*

Of course, this isn't going to be nice given appropriate timing of a
subsequent SETCRTC removing the FB and dropping its refcounts.




If I get rid of the referencing of the old framebuffer in PAGE_FLIP,
then:

ADDFB
- 0->1 - kref_init
- 1->2 - drm_framebuffer_reference() creation

SETCRTC
- 2->3 - drm_framebuffer_lookup() in drm_mode_setcrtc()
- 3->4 - drm_framebuffer_reference() in armada_drm_crtc_mode_set()
- 4->5 - drm_framebuffer_reference() in drm_mode_set_config_internal()
- 5->4 - drm_framebuffer_unreference() in drm_mode_setcrtc()
* current scanout framebuffer has refcount of 4

PAGE_FLIP
- 4->3 - drm_framebuffer_unreference(old_fb) in drm_mode_page_flip_ioctl()
- 3->2 - drm_framebuffer_unreference() in armada_drm_unref_work()

RMFB
- 2->1 - __drm_framebuffer_unreference()
- 1->0 - drm_framebuffer_unreference() - *free*

So that looks good, but let's look at what happens to the framebuffer
being flipped in:

ADDFB
- 0->1 - kref_init
- 1->2 - drm_framebuffer_reference() creation

PAGE_FLIP (incoming)
- 2->3 - drm_framebuffer_lookup() in drm_mode_page_flip_ioctl()
* current scanout framebuffer has refcount of 3 - different from setcrtc.

PAGE_FLIP (outgoing)
- 3->2 - drm_framebuffer_unreference(old_fb) in drm_mode_page_flip_ioctl()
- 2->1 - drm_framebuffer_unreference() in armada_drm_unref_work()

RMFB
- 1->0 - __drm_framebuffer_unreference() - *OOPS*
- 0->-1 - drm_framebuffer_unreference() - *broken*

So that doesn't work - and it still isn't good because the framebuffer
which was flipped in has a different refcount to the one going out.




If I instead try taking a reference on the new framebuffer and dropping
the previous ref in PAGE_FLIP:

ADDFB
- 0->1 - kref_init
- 1->2 - drm_framebuffer_reference() creation

SETCRTC
- 2->3 - drm_framebuffer_lookup() in drm_mode_setcrtc()
- 3->4 - drm_framebuffer_reference() in armada_drm_crtc_mode_set()
- 4->5 - drm_framebuffer_reference() in drm_mode_set_config_internal()
- 5->4 - drm_framebuffer_unreference() in drm_mode_setcrtc()
* current scanout framebuffer has refcount of 4

PAGE_FLIP
- 4->3 - drm_framebuffer_unreference() in armada_drm_crtc_page_flip()
- 3->2 - drm_framebuffer_unreference(old_fb) in drm_mode_page_flip_ioctl()
- 2->1 - drm_framebuffer_unreference() in armada_drm_unref_work()

RMFB
- 1->0 - __drm_framebuffer_unreference() - *OOPS*
- 0->-1 - drm_framebuffer_unreference() - (never happens)

I can see no way to solve this in driver code, other than tracking where
the framebuffer came from, and conditionally adjusting the refcount to
keep things sane.  The real problem here is the different behaviour in
the DRM core code between the refcounts resulting on the active scanout
framebuffer.

Any other thoughts how to solve this without having such refcount hacks
in drivers?

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.