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: Tue, 17 Dec 2013 13:04:04 -0800 Message-ID: <20131217130404.435cbc66@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> <20131216160141.64ef8685@jbarnes-desktop> <20131217083836.GK9804@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from alt-proxy1.mail.unifiedlayer.com (unknown [70.40.203.67]) by gabe.freedesktop.org (Postfix) with SMTP id 5028BFD253 for ; Tue, 17 Dec 2013 13:02:13 -0800 (PST) In-Reply-To: <20131217083836.GK9804@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 Tue, 17 Dec 2013 09:38:36 +0100 Daniel Vetter 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 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