On Wed, Dec 16, 2015 at 11:52:17AM +0100, Daniel Vetter wrote:
> 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.
I just tested when we started to fail things, and for me it only happens
with 4.4-rc releases (tested with [1]). 4.3.3 OTOH still had the fbcon fb
pinned while X was running.
[1] airlied/drm-fixes 4655a12b81ed ("drm: Don't overwrite UNVERFIED mode status
to OK")
>
> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
>
> Lukas, feel like supplying a patch to apply the polish you've spotted on
> top?
>
> Thanks, Daniel
>
> >
> > Lukas
> >
> > >
> > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > > Cc: "Goel, Akash" <akash.g...@intel.com>
> > > Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > Cc: Jesse Barnes <jbar...@virtuousgeek.org>
> > > Cc: Lukas Wunner <lu...@wunner.de>
> > > Cc: sta...@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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
