From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.hostsharing.net ([83.223.95.204]:51585 "EHLO mailout1.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752459AbbKXVUz (ORCPT ); Tue, 24 Nov 2015 16:20:55 -0500 Date: Tue, 24 Nov 2015 22:20:49 +0100 From: Lukas Wunner To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org, Daniel Vetter , "Goel, Akash" , stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH v4] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping Message-ID: <20151124212049.GA26526@wunner.de> References: <564F4709.5070909@virtuousgeek.org> <1448036992-11749-1-git-send-email-chris@chris-wilson.co.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1448036992-11749-1-git-send-email-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org List-ID: Hi Chris, On Fri, Nov 20, 2015 at 04:29:52PM +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. > > Signed-off-by: Chris Wilson > Cc: "Goel, Akash" > Cc: Daniel Vetter > Cc: Jesse Barnes > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/intel_fbdev.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c > index 7ccde58f8c98..79f02e72da8a 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,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = { > static void intel_fbdev_destroy(struct drm_device *dev, > struct intel_fbdev *ifbdev) > { > + /* Release the pinning for the info->screen_base mmaping. */ > + i915_gem_object_ggtt_unpin(ifbdev->fb->obj); If the call to intelfb_alloc() failed, ifbdev->fb will be NULL and intelfb_create() will return a non-zero value. Further up in the call stack, intel_fbdev_initial_config() will then clobber the fbdev by calling intel_fbdev_destroy(). This will oops because you dereference ifbdev->fb here. So you need to add: if (ifbdev->fb) If intel_pin_and_fence_fb_obj() failed, intelfb_create() will likewise return a non-zero value and the fbdev gets clobbered. This will WARN because you're calling i915_gem_object_ggtt_unpin() even though the pin_count is 0. Best regards, Lukas > > drm_fb_helper_unregister_fbi(&ifbdev->helper); > drm_fb_helper_release_fbi(&ifbdev->helper); > -- > 2.6.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx