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

[Chris Wilson]

On Fri, Oct 30, 2015 at 05:18:15PM +0100, Daniel Vetter wrote:
> On Fri, Oct 23, 2015 at 10:17:44PM +0100, Dave Gordon wrote:
> > On 08/10/15 21:50, Wayne Boyer wrote:
> > >From: Chris Wilson <chris at chris-wilson.co.uk>
> > >
> > >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: rebased on latest nightly (Wayne)
> > >v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based
> > >on Chris' review. (Wayne)
> > >
> > >Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > >Cc: "Goel, Akash" <akash.goel at intel.com>
> > >Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > >Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
> > >Cc: stable at vger.kernel.org
> > >Reviewed-by: Deepak S <deepak.s at linux.intel.com>
> > >Signed-off-by: Wayne Boyer <wayne.boyer at intel.com>
> > >---
> > >  drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > >diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > >index 6532912..0ad46521 100644
> > >--- a/drivers/gpu/drm/i915/intel_fbdev.c
> > >+++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > >@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	obj = intel_fb->obj;
> > >  	size = obj->base.size;
> > >
> > >+	/* The fb constructor will have already pinned us (or inherited a
> > >+	 * GGTT region from the BIOS) suitable for a scanout, so
> > >+	 * this should just be a no-op and increment the pin count for the
> > >+	 * fbdev mmapping. It does have a useful side-effect of validating
> > >+	 * the pin for fbdev's use via a GGTT mmapping.
> > >+	 */
> > >+	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > >+	if (ret)
> > >+		goto out_unlock;
> > >+
> > >  	info = drm_fb_helper_alloc_fbi(helper);
> > >  	if (IS_ERR(info)) {
> > >  		ret = PTR_ERR(info);
> > >@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  out_destroy_fbi:
> > >  	drm_fb_helper_release_fbi(helper);
> > >  out_unpin:
> > >+	/* Once for info->screen_base mmaping... */
> > >+	i915_gem_object_ggtt_unpin(obj);
> > >+	/* ...and once for the intel_fb */
> > >  	i915_gem_object_ggtt_unpin(obj);
> > >  	drm_gem_object_unreference(&obj->base);
> > >  out_unlock:
> > >@@ -514,6 +527,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);
> > >
> > >  	drm_fb_helper_unregister_fbi(&ifbdev->helper);
> > >  	drm_fb_helper_release_fbi(&ifbdev->helper);
> > 
> > Hmm .. pinning now done by i915_gem_obj_ggtt_pin(), but the unpinning
> > function is i915_gem_object_ggtt_unpin(). Just the sort of asymmetry that
> > helps everyone understand what's going on :(
> > 
> > Could we not have a mass rename of the various i915_gem_obj{ect} functions
> > to ONE consistent naming convention? (Personally I prefer 'obj' because it's
> > shorter, but consistency is more important than saving just 3 letters).
> 
> Of course, just needs someone to do it, and make sure to not step onto too
> many toes. I'd love if more people actually take charge of gem instead of
> piling more on top.

I have patches to remove as much of the nonsense as I could. I have sent
some of them before, but no one looked at them it seems. Now they are
about 150 patches from the top of the queue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre