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

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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.

> 3. fbdev needs to grab one more reference (if it decides to steal the bios
> framebuffer) to make sure the fbdev survives. But besides that I don't
> think we need anything else - any subsequent modeset will update
> references correctly. And for the fbdev config we can rely on the fastboot
> modeset paths to ellide any real updates when fbcon sets its desired
> config (which hopefully matches what the bios has set up already).

I thought this was correct already... I'll post with the CONFIG_FB
changes and you can check again.  But I take a ref in the fbdev layer
on both the GEM object and the fb that we end up using, so it should
have the appropriate ref in that case.

> > -
> > -	drm_modeset_lock_all(dev);
> > -	drm_mode_config_reset(dev);
> > -	intel_modeset_setup_hw_state(dev, false);
> > -	drm_modeset_unlock_all(dev);
> 
> As mention in the dpio fixup patch I'd like this code block move to be
> split out in a small prep patch.

Ok will do.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center