From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH 2/6] drm/i915: retrieve current fb config into new plane_config structure at init Date: Mon, 16 Dec 2013 16:01:41 -0800 Message-ID: <20131216160141.64ef8685@jbarnes-desktop> References: <1386880917-2951-1-git-send-email-jbarnes@virtuousgeek.org> <1386880917-2951-2-git-send-email-jbarnes@virtuousgeek.org> <20131214110147.GJ9804@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from alt-proxy33.mail.unifiedlayer.com (alt-proxy33.mail.unifiedlayer.com [70.40.209.146]) by gabe.freedesktop.org (Postfix) with SMTP id 6DC4AFA31B for ; Mon, 16 Dec 2013 15:59:53 -0800 (PST) In-Reply-To: <20131214110147.GJ9804@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Sat, 14 Dec 2013 12:01:47 +0100 Daniel Vetter 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