[PATCH] drm: Don't grab an fb reference for the idr

[Daniel Vetter]

On Wed, Aug 06, 2014 at 09:12:42AM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Wed, Aug 06, 2014 at 07:11:28AM -0400, Rob Clark wrote:
> >> On Wed, Aug 6, 2014 at 3:10 AM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> >> > The current refcounting scheme is that the fb lookup idr also holds a
> >> > reference. This works out nicely bacause thus far we've always
> >> > explicitly cleaned up idr entries for framebuffers:
> >> > - Userspace fbs get removed in the rmfb ioctl or when the drm file
> >> >   gets closed.
> >> > - Kernel fbs (for fbdev emulation) get cleaned up by the driver code
> >> >   at module unload time.
> >> >
> >> > But now i915 also reconstructs the bios fbs for a smooth transition.
> >> > And that fb is purely transitional and should get removed immmediately
> >> > once all crtcs stop using it. Of course if the i915 fbdev code decides
> >> > to reuse it as the main fbdev fb then it shouldn't be cleaned up, but
> >> > in that case the fbdev code will grab it's own reference.
> >> >
> >> > The problem is now that we also want to register that takeover fb in
> >> > the idr, so that userspace can do a smooth transition (animated maybe
> >> > even!) itself. But currently we have no one who will clean up the idr
> >> > reference once that fb isn't useful any more, and so essentially leak
> >> > it.
> >>
> >> ewww..  couldn't you do some scheme on lastclose to check if no more
> >> crtc's are scanning out that fb, and if not then remove the idr?
> >
> > There's no natural point really but when we drop the last reference for
> > it. Going the weak reference route looked the most natural. And I honestly
> > expect other drivers to eventually do the same - forcing a modeset on
> > boot-up is kinda not too pretty, and permanently reserving a big
> > framebuffer just for the bios doesn't sound good either. This approach
> > would nicely solve it for everyone.
> 
> hmm, maybe somebody switched my coffee with decaf, but why isn't
> lastclose a natural point?

There is no lastclose for the bios ;-)

Let me elaborate on what happens:

1. BIOS sets up an initial config with a framebuffer in stolen.

2. i915 takes over and reconstructs all the state, so now we have all the
crtcs enabled using a framebuffer for all of them which wraps the bios
allocation.

2b. (optional) reuse that framebuffer for fbdev.

-> That special bios fb has the following references:
- 1 reference for each crtc that's using it
- 1 optional reference if it's reused as the fbdev fb
- 1 reference for the idr

3. Userspace takes over, potentially doing a getfb on the current
(bios-inherited) fb for smooth transition, but then does a modeset to its
own fb.

-> After this all the we've dropped the crtc references and we also want
to drop the idr reference (since no one will ever use this framebuffer
again). But there's simply no good place to do that. Lastclose might only
happen before we shut down the system again, which is a bit too late.

Note that the getfb call creates a gem handle for the fb object, so the
backing storage might survive for a lot longer than the fb.

> ofc if that really doesn't work, the weak-ref thing seems like it
> would solve it nicely.  But if there were a simple solution that
> didn't involve making fb refcnting more complex, I guess I would
> prefer that

Well I didn't come up with anything else really. Plan b would be to add
hooks after any plane updates and manually check whether that special fb
has lost all but its idr reference, and if so clean it up. That seems to
be a lot more fragile and convoluted than converting the idr to a weak
reference.

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