Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 17 Dec 2013 09:38:36 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Dec 16, 2013 at 04:01:41PM -0800, Jesse Barnes wrote:
> > On Sat, 14 Dec 2013 12:01:47 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > > But I still think the fb lifetime management is a bit broken here and we
> > > need a few small changes:
> > > 
> > > 1. Right here in this loop we need to assign the fb from the plane_config
> > > ot the crtc->fb pointer and grab an fb reference for that.
> > > 
> > > If we don't do that we'll fall over for CONFIG_FB=n
> > > 
> > > A side-effect of that is that plane_config is now fairly redundant and we
> > > have the problem of cleaning up the fb referenced in there somehow
> > > (especially for CONFIG_FB=n). That's kinda the reason why I don't like it
> > > very much ...
> > > 
> > > The below points are for the next patch, just noting them here for the
> > > full picture. I haven't read carefully through that patch yet, so might
> > > all be correct already.
> > > 
> > > 2. We need to clean up fb reference in the plane config. Iirc your current
> > > patch 3 fails that for CONFIG_FB=n
> > 
> > Hm yeah the ownership is less clear in the CONFIG_FB=n case.  I think
> > the driver will own the buffer, and it'll get dropped on the first mode
> > set with a new buffer.  But even then there will be no process to deref
> > the object finally, so it'll stick around.  Hm... maybe just disable it
> > if CONFIG_FB=n is the right answer for now.
> 
> If you switch the fbdev code to look at crtc->fb instead of
> crtc->plane_config.fb and just drop the plane_config.fb pointer (and it's
> reference) it should pan out. Then the only reference+pointers we have are
> the ones in crtc->fb, and the drm core will take care of those.

How can I switch to looking at crtc->fb?  Or do you mean
get_plane_config should stuff a full fb into crtc->fb instead of the
plane_config struct?

> fbdev then needs to grab an additional reference for ifbdev->fb, but it
> needs to do that anyway. Your current code seems to just steal the initial
> reference from creating the framebuffer in ->get_plane_config.

Right.

-- 
Jesse Barnes, Intel Open Source Technology Center