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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 06, 2014 at 02:59:29PM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 10:07 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > 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@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@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.
> 
> Well, you could still cleanup your idr reference on current
> userspace's lastclose.. that doesn't do much good, I suppose, if
> current userspace never exits.  But if first userspace is plymouth or
> something like that, you would get cleaned up on the
> splash->{x11/wayland} transition..

So plymouth starts, but it doesn't exit until X starts (since otherwise
the fb gets removed and the crtc shut off and we switch to fbcon). So
again no lastclose to link into. So I don't think any check in a close
callback will cut it.

> if that isn't sufficient, then yeah I guess the more fancy weak-ref
> stuff needed..

Trust me I don't like it either ;-) But for this we really have no natural
point to reap the lookup reference like with gem objects, or normal
userspace framebuffers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch