[Intel-gfx] [PATCH v4] drm/i915: Fallback to using unmappable memory for scanout

[Daniel Vetter]

On Wed, Mar 25, 2015 at 12:44:47PM +0000, Chris Wilson wrote:
> The existing ABI says that scanouts are pinned into the mappable region
> so that legacy clients (e.g. old Xorg or plymouthd) can write directly
> into the scanout through a GTT mapping. However if the surface does not
> fit into the mappable region, we are better off just trying to fit it
> anywhere and hoping for the best. (Any userspace that is cappable of
> using ginormous scanouts is also likely not to rely on pure GTT
> updates.) In the future, there may even be a kernel mediated method for
> the legacy clients.
> 
> v2: Skip fence pinning when not mappable.
> v3: Add a comment to explain the possible rammifactions of not being
>     able to use fences for unmappable scanouts.
> v4: Rebase to skip over some local patches
> 
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Satyanantha, Rama Gopal M <rama.gopal.m.satyanantha at intel.com>
> Cc: Deepak S <deepak.s at linux.intel.com>
> Cc: Damien Lespiau <damien.lespiau at intel.com>
> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>

I'm a bit uncomfortable merging this before Joonas has the partial mmap
ready. I fear that if we end up with a framebuffer stuck in unmappable
that we'll then have a cascade of crashing compositors because they can't
cope. Yes SNA can, but last time I've checked that's the only one.

Earmarked for merging as soon as the partial view stuff is in.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c      |  7 ++++++-
>  drivers/gpu/drm/i915/intel_display.c | 34 ++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_fbc.c     |  4 ++++
>  3 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 3300963ca716..dd3b42dafae2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3910,12 +3910,17 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  
>  	/* As the user may map the buffer once pinned in the display plane
>  	 * (e.g. libkms for the bootup splash), we have to ensure that we
> -	 * always use map_and_fenceable for all scanout buffers.
> +	 * always use map_and_fenceable for all scanout buffers. However,
> +	 * it may simply be too big to fit into mappable, in which case
> +	 * put it anyway and hope that userspace can cope (but always first
> +	 * try to preserve the existing ABI).
>  	 */
>  	ret = i915_gem_object_ggtt_pin(obj, view, alignment,
>  				       view->type == I915_GGTT_VIEW_NORMAL ?
>  				       PIN_MAPPABLE : 0);
>  	if (ret)
> +		ret = i915_gem_obj_ggtt_pin(obj, alignment, 0);
> +	if (ret)
>  		goto err_unpin_display;
>  
>  	i915_gem_object_flush_cpu_write_domain(obj);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f9dc5babcf27..cf348e954ff1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2394,16 +2394,29 @@ intel_pin_and_fence_fb_obj(struct drm_plane *plane,
>  	if (ret)
>  		goto err_interruptible;
>  
> -	/* Install a fence for tiled scan-out. Pre-i965 always needs a
> -	 * fence, whereas 965+ only requires a fence if using
> -	 * framebuffer compression.  For simplicity, we always install
> -	 * a fence as the cost is not that onerous.
> -	 */
> -	ret = i915_gem_object_get_fence(obj);
> -	if (ret)
> -		goto err_unpin;
> +	if (obj->map_and_fenceable) {
> +		/* Install a fence for tiled scan-out. Pre-i965 always needs a
> +		 * fence, whereas 965+ only requires a fence if using
> +		 * framebuffer compression.  For simplicity, we always, when
> +		 * possible, install a fence as the cost is not that onerous.
> +		 *
> +		 * If we fail to fence the tiled scanout, then either the
> +		 * modeset will reject the change (which is highly unlikely as
> +		 * the affected systems, all but one, do not have unmappable
> +		 * space) or we will not be able to enable full powersaving
> +		 * techniques (also likely not to apply due to various limits
> +		 * FBC and the like impose on the size of the buffer, which
> +		 * presumably we violated anyway with this unmappable buffer).
> +		 * Anyway, it is presumably better to stumble onwards with
> +		 * something and try to run the system in a "less than optimal"
> +		 * mode that matches the user configuration.
> +		 */
> +		ret = i915_gem_object_get_fence(obj);
> +		if (ret)
> +			goto err_unpin;
>  
> -	i915_gem_object_pin_fence(obj);
> +		i915_gem_object_pin_fence(obj);
> +	}
>  
>  	dev_priv->mm.interruptible = true;
>  	intel_runtime_pm_put(dev_priv);
> @@ -2429,7 +2442,8 @@ static void intel_unpin_fb_obj(struct drm_framebuffer *fb,
>  	ret = intel_fill_fb_ggtt_view(&view, fb, plane_state);
>  	WARN_ONCE(ret, "Couldn't get view from plane state!");
>  
> -	i915_gem_object_unpin_fence(obj);
> +	if (obj->map_and_fenceable)
> +		i915_gem_object_unpin_fence(obj);
>  	i915_gem_object_unpin_from_display_plane(obj, &view);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9fcf446e95f5..7ee1baade095 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -578,6 +578,10 @@ void intel_fbc_update(struct drm_device *dev)
>  
>  	/* The use of a CPU fence is mandatory in order to detect writes
>  	 * by the CPU to the scanout and trigger updates to the FBC.
> +	 *
> +	 * Note that is possible for a tiled surface to be unmappable (and
> +	 * so have no fence associated with it) due to aperture constaints
> +	 * at the time of pinning.
>  	 */
>  	if (obj->tiling_mode != I915_TILING_X ||
>  	    obj->fence_reg == I915_FENCE_REG_NONE) {
> -- 
> 2.1.4
> 

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