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

[Dave Gordon <david.s.gordon@intel.com>]

On 08/10/15 21:50, Wayne Boyer wrote:
> From: Chris Wilson <chris@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@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: stable@vger.kernel.org
> Reviewed-by: Deepak S <deepak.s@linux.intel.com>
> Signed-off-by: Wayne Boyer <wayne.boyer@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).

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx