From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f43.google.com ([74.125.82.43]:34434 "EHLO mail-wm0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751090AbbLPKwV (ORCPT ); Wed, 16 Dec 2015 05:52:21 -0500 Received: by mail-wm0-f43.google.com with SMTP id l126so33748290wml.1 for ; Wed, 16 Dec 2015 02:52:20 -0800 (PST) Date: Wed, 16 Dec 2015 11:52:17 +0100 From: Daniel Vetter To: Lukas Wunner Cc: Chris Wilson , intel-gfx@lists.freedesktop.org, "Goel, Akash" , Daniel Vetter , Jesse Barnes , stable@vger.kernel.org Subject: Re: [PATCH v5] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping Message-ID: <20151216105217.GE30437@phenom.ffwll.local> References: <20151124212049.GA26526@wunner.de> <1449245126-26158-1-git-send-email-chris@chris-wilson.co.uk> <20151206203320.GA29029@wunner.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151206203320.GA29029@wunner.de> Sender: stable-owner@vger.kernel.org List-ID: On Sun, Dec 06, 2015 at 09:33:20PM +0100, Lukas Wunner wrote: > Hi Chris, > > On Fri, Dec 04, 2015 at 04:05:26PM +0000, Chris Wilson wrote: > > A long time ago (before 3.14) we relied on a permanent pinning of the > > ifbdev to lock the fb in place inside the GGTT. However, the > > introduction of stealing the BIOS framebuffer and reusing its address in > > the GGTT for the fbdev has muddied waters and we use an inherited fb. > > However, the inherited fb is only pinned whilst it is active and we no > > longer have an explicit pin for the info->system_base mmapping used by > > the fbdev. The result is that after some aperture pressure the fbdev may > > be evicted, but we continue to write the fbcon into the same GGTT > > address - overwriting anything else that may be put into that offset. > > The effect is most pronounced across suspend/resume as > > intel_fbdev_set_suspend() does a full clear over the whole scanout. > > > > v2: Only unpin the intel_fb is we allocate it. If we inherit the fb from > > the BIOS, we do not own the pinned vma (except for the reference we add > > in this patch for our access via info->screen_base). > > > > v3: Finish balancing the vma pinning for the normal !preallocated case. > > > > v4: Try to simplify the pinning even further. > > v5: Leak the VMA (cleaned up by object-free) to avoid complicated error paths. > > It's beautiful how little code is needed to fix this. The only remaining > thing I noticed now while looking over the error paths is that these > lines in intelfb_alloc() become obsolete with your patch: > > out: > mutex_unlock(&dev->struct_mutex); > - if (!IS_ERR_OR_NULL(fb)) > - drm_framebuffer_unreference(fb); > return ret; > } > > Because at each of the remaining "goto out" in the function, > fb can be only either an ERR_PTR or NULL. > > Also, further up in the function, the declaration of fb can then be > changed thus: > > - struct drm_framebuffer *fb = NULL; > + struct drm_framebuffer *fb; > > Kind regards, Yeah there's room for follow-up polish, but this seems good enough at least for -fixes. Reviewed-by: Daniel Vetter Lukas, feel like supplying a patch to apply the polish you've spotted on top? Thanks, Daniel > > Lukas > > > > > Signed-off-by: Chris Wilson > > Cc: "Goel, Akash" > > Cc: Daniel Vetter > > Cc: Jesse Barnes > > Cc: Lukas Wunner > > Cc: stable@vger.kernel.org > > --- > > drivers/gpu/drm/i915/intel_fbdev.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > > index 7ccde58f8c98..bea75cafc623 100644 > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > @@ -163,13 +163,6 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > goto out; > > } > > > > - /* Flush everything out, we'll be doing GTT only from now on */ > > - ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL); > > - if (ret) { > > - DRM_ERROR("failed to pin obj: %d\n", ret); > > - goto out; > > - } > > - > > mutex_unlock(&dev->struct_mutex); > > > > ifbdev->fb = to_intel_framebuffer(fb); > > @@ -225,6 +218,14 @@ static int intelfb_create(struct drm_fb_helper *helper, > > > > mutex_lock(&dev->struct_mutex); > > > > + /* Pin the GGTT vma for our access via info->screen_base. > > + * This also validates that any existing fb inherited from the > > + * BIOS is suitable for own access. > > + */ > > + ret = intel_pin_and_fence_fb_obj(NULL, &ifbdev->fb->base, NULL); > > + if (ret) > > + goto out_unlock; > > + > > info = drm_fb_helper_alloc_fbi(helper); > > if (IS_ERR(info)) { > > DRM_ERROR("Failed to allocate fb_info\n"); > > @@ -287,6 +288,7 @@ out_destroy_fbi: > > drm_fb_helper_release_fbi(helper); > > out_unpin: > > i915_gem_object_ggtt_unpin(obj); > > +out_unlock: > > mutex_unlock(&dev->struct_mutex); > > return ret; > > } > > @@ -524,6 +526,10 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = { > > static void intel_fbdev_destroy(struct drm_device *dev, > > struct intel_fbdev *ifbdev) > > { > > + /* We rely on the object-free to release the VMA pinning for > > + * the info->screen_base mmaping. Leaking the VMA is simpler than > > + * trying to rectify all the possible error paths leading here. > > + */ > > > > drm_fb_helper_unregister_fbi(&ifbdev->helper); > > drm_fb_helper_release_fbi(&ifbdev->helper); > > -- > > 2.6.2 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch