Re: [Intel-gfx] [PATCH v5] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	stable@vger.kernel.org, "Goel, Akash" <akash.goel@intel.com>,
	Takashi Iwai <tiwai@suse.de>
Subject: Re: [Intel-gfx] [PATCH v5] drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping
Date: Thu, 10 Dec 2015 18:36:04 +0200	[thread overview]
Message-ID: <20151210163604.GC4437@intel.com> (raw)
In-Reply-To: <1449245126-26158-1-git-send-email-chris@chris-wilson.co.uk>

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.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: "Goel, Akash" <akash.goel@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Cc: Lukas Wunner <lukas@wunner.de>
> Cc: stable@vger.kernel.org

This seems to have fixed my garbled text+fbcon dead after
suspend/hibernate issues. Well, only had the patch in for a day or so,
but so far so good.

Tested-by: Ville Syrjï¿½lï¿½ <ville.syrjala@linux.intel.com>

Takashi, don't know if you already found this patch, but it's definitely
something you should try as well.

> ---
>  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
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjï¿½lï¿½
Intel OTC