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

[Rob Clark]

On Wed, Aug 6, 2014 at 10:07 AM, Daniel Vetter <daniel at 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 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.

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..

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

BR,
-R

> 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