On 11/20/2015 06:34 AM, 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.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: "Goel, Akash" <akash.goel@xxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 7ccde58f8c98..7a415fe31299 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -225,6 +225,16 @@ static int intelfb_create(struct drm_fb_helper *helper,
>
> mutex_lock(&dev->struct_mutex);
>
> + /* 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)) {
> DRM_ERROR("Failed to allocate fb_info\n");
> @@ -279,6 +289,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
> fb->width, fb->height,
> i915_gem_obj_ggtt_offset(obj), obj);
>
> + /* We pin the vma for our access through info->screen_base, so
> + * we can drop the pin we took if we created the intel_fb.
> + */
> + if (!prealloc)
> + i915_gem_object_ggtt_unpin(obj);
> +
> mutex_unlock(&dev->struct_mutex);
> vga_switcheroo_client_fb_set(dev->pdev, info);
> return 0;
> @@ -286,7 +302,12 @@ 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);
> +out_unlock:
> + if (!prealloc)
> + /* ...and once for the intel_fb */
> + i915_gem_object_ggtt_unpin(obj);
> mutex_unlock(&dev->struct_mutex);
> return ret;
> }
> @@ -524,6 +545,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);
>
Now you're making me look at the pin/unpin handling... Could probably
make the prealloc vs non-prealloc cases a bit clearer, but it looks
correct. In the prealloc case we need the additional pin, since
create_stolen_for_preallocated just pins the pages and doesn't up the
pin count, right? But in the non-prealloc case we'll have done a
regular fb alloc, which does a pin & fence, so we can drop the extra pin
count. And I think the page unpin is already taken care of? ISTR bugs
there when we first landed the initial plane allocation stuff.
Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html