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

[Daniel Vetter]

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 at 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.
- 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.
- 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.
- Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch