From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7 Date: Sat, 14 Dec 2013 11:44:06 +0100 Message-ID: <20131214104406.GH9804@phenom.ffwll.local> References: <1386880917-2951-1-git-send-email-jbarnes@virtuousgeek.org> <1386880917-2951-3-git-send-email-jbarnes@virtuousgeek.org> <20131212225437.GT9804@phenom.ffwll.local> <20131213110905.06046c4e@jbarnes-desktop> <20131213164350.49c244c8@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f171.google.com (mail-ea0-f171.google.com [209.85.215.171]) by gabe.freedesktop.org (Postfix) with ESMTP id 10D92FACDE for ; Sat, 14 Dec 2013 02:43:15 -0800 (PST) Received: by mail-ea0-f171.google.com with SMTP id h10so1313791eak.30 for ; Sat, 14 Dec 2013 02:43:15 -0800 (PST) Content-Disposition: inline In-Reply-To: <20131213164350.49c244c8@jbarnes-desktop> 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: Jesse Barnes Cc: intel-gfx List-Id: intel-gfx@lists.freedesktop.org On Fri, Dec 13, 2013 at 04:43:50PM -0800, Jesse Barnes wrote: > On Fri, 13 Dec 2013 21:47:45 +0100 > Daniel Vetter wrote: > > > On Fri, Dec 13, 2013 at 8:09 PM, Jesse Barnes wrote: > > > On Thu, 12 Dec 2013 23:54:37 +0100 > > > Daniel Vetter wrote: > > > > > >> > @@ -258,8 +357,102 @@ static void intel_fbdev_destroy(struct drm_device *dev, > > >> > > > >> > drm_fb_helper_fini(&ifbdev->helper); > > >> > > > >> > - drm_framebuffer_unregister_private(&ifbdev->ifb.base); > > >> > - intel_framebuffer_fini(&ifbdev->ifb); > > >> > + drm_framebuffer_unregister_private(&ifbdev->fb->base); > > >> > + intel_framebuffer_fini(ifbdev->fb); > > >> > + kfree(ifbdev->fb); > > >> > > >> No need to go the private fb route here anymore since now the fb is > > >> free-standing. Normal refcounting should work. But a separate prep/cleanup > > >> patch (prep since switching ifbdev->fb from struct to point would look > > >> neat as a separate patch). > > > > > > Oh and can you explain this? I wouldn't be surprised if I got the > > > refcounting wrong, but given how tricky it can be, can you explain > > > where we'll take the ref here, and show that the right thing will > > > happen if/when we mode set away from this buffer? > > > > > > I haven't actually seen a bug here with or without this patch (no > > > crashes or warns), but I thought I needed this to make sure the obj > > > didn't get a negative count after a mode set... > > > > There's no bug here, and if you underrun the the refcount the current > > logic here won't help. It's just that the explicit call to _fini was > > an artifact of the old logic with embedding the framebuffer into the > > fbdev structure. But now that the fbdev framebuffer is freestanding > > there's no need for that - you exactly duplicate > > intel_user_framebuffer_destroy now. > > > > So a simple drm_framebuffer_unreference will do the trick and makes it > > clearer that this is now just another fb object with normal lifetime > > rules. > > > > I guess I score points for unclear review here ;-) > > Oh! I had this mixed up with the refs I take in the init_bios code... > I thought you were saying those weren't necessary and I was getting > really confused. > > This is just fixing up existing code to use the new field name, so no > functional change. I see what you mean about splitting out the field > change, but now that would be a pain :/ Yeah, the switch from struct to pointer for ifbdev->fb would be neat as a separate patch, but also real pain to split out now. > Do you want the above removed as a separate patch regardless of where > the rest of the patches go? Imo it should go with the switch to pointers, so this patch here is fine. Maybe a quick mention in the commit message about it. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch