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

[Daniel Vetter]

On Fri, Oct 02, 2015 at 10:16:26PM +0000, Boyer, Wayne wrote:
> On 8/26/15, 1:23 AM, Deepak <deepak.s at linux.intel.com> wrote:
> 
> 
> >
> >
> >On 08/25/2015 10:18 PM, 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.
> >
> >Yup this is a critical fix :) by keeping the internal FB pinned we avoid
> >alloc of buffer within same FB GTT offset
> >Reviewed-by: Deepak S <deepak.s at linux.intel.com>
> 
> Daniel,
> 
> Is there anything else that needs to happen before this gets pulled in?

Someone needs to send out the rebased-on-upstream version. The patches
have been merged but then had to be dropped again since they where based
on top of Chris' tree.
-Daniel

> 
> Thanks,
> 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
> >> ---
> >>   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 96476d7d7ed2..082f2938ec97 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_object_ggtt_pin(obj, NULL, 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);
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx at lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch