Re: [PATCH 4/5] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 26 Nov 2013 18:54:04 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Nov 26, 2013 at 02:09:48PM +0000, Chris Wilson wrote:
> > On Mon, Nov 25, 2013 at 03:51:18PM -0800, Jesse Barnes wrote:
> > > Retrieve current framebuffer config info from the regs and create an fb
> > > object for the buffer the BIOS or boot loader left us.  This should
> > > allow for smooth transitions to userspace apps once we finish the
> > > initial configuration construction.
> > > 
> > > v2: check for non-native modes and adjust (Jesse)
> > >     fixup aperture and cmap frees (Imre)
> > >     use unlocked unref if init_bios fails (Jesse)
> > >     fix curly brace around DSPADDR check (Imre)
> > >     comment failure path for pin_and_fence (Imre)
> > > v3: fixup fixup of aperture frees (Chris)
> > > v4: update to current bits (locking & pin_and_fence hack) (Jesse)
> > > v5: move fb config fetch to display code (Jesse)
> > >     re-order hw state readout on initial load to suit fb inherit (Jesse)
> > >     re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
> > > v6: rename to plane_config (Daniel)
> > >     check for valid object when initializing BIOS fb (Jesse)
> > >     split from plane_config readout and other display changes (Jesse)
> > >     drop use_bios_fb option (Chris)
> > >     update comments (Jesse)
> > >     rework fbdev_init_bios for clarity (Jesse)
> > >     drop fb obj ref under lock (Chris)
> > > v7: use fb object from plane_config instead (Ville)
> > > 
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > Hmm, quietly steals plane_config->fb you mean. Other than bikeshedding
> > the kzalloc(intel_fbdev) and the clarity of
> > intel_fb_init/intel_fb_init_bios, I don't see anything else. The fb
> > lifetime of plane_config->fb is extremely ugly though (the theft could
> > be made a little more obvious for instance) and still leaked upon failure?
> 
> I think the lifetime stuff for the stolen fb is actually ok. But there's
> other stuff that will probably gives us some good fireworks:
> - intel_crtc->plane_config seems to be left hanging in the air. Imo
>   duplicating the crtc->fb pointer isnt' really all that good. I'd much
>   prefer when we just shovel the invented fb into the crtc->fb pointer. Of
>   course that requires us to properly adjust the refcount.

plane_config is just part of intel_crtc.  No hanging, the struct is
just bigger.  Saves me from dealing with alloc/free of it.

> - fb->obj has a very high chance to cause utter havoc on multi-pipe
>   configs, since the bios loves to set up shared framebuffers. I guess
>   this is untested? For now it's probably simplest to just not bother with
>   the 2nd/3rd pipe.

There should be no havoc.  The first allocation will succeed, and
following ones will fail since they overlap.  The BIOS takeover code
will then use the one that was allocated (or in the unlikely case of
multiple fbs, pick the biggest one).

Or did you see some other fail there?

> - We need to clean up the fbs we've created somehow - intel_framebuffer_init
>   will at least register the framebuffer with the drm core. But since it's a
>   driver-private fb and since we hold a reference already it'll never disappear
>   I think.

I don't think so... it should look just like a user allocated buffer
from the drm core POV.  But generally our fbdev allocations do tend to
live forever, so in that sense you're right, but it's not different
than what we have today.

-- 
Jesse Barnes, Intel Open Source Technology Center