[PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The current code is trying to be lazy with fences on scanout buffers.
That looks broken for several reasons:
* gen2/3 always need a fence for tiled scanout
* the unpin doesn't know whether we pinned the fence or not so it
  may unpin something we don't own
* FBC GTT tracking needs a fence (not sure we have proper fallback
  for when there is no fence)

So to fix this always use a fence for gen2/3, and for primary planes on
other platforms (for FBC). For sprites and cursor we never need a fence
so don't even try to get one.  Since we now know whether a fence was
pinned we can safely unpin it too.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 +-
 drivers/gpu/drm/i915/i915_gem.c      |  4 +--
 drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_drv.h     |  7 +++--
 drivers/gpu/drm/i915/intel_fbdev.c   | 17 +++++++++--
 drivers/gpu/drm/i915/intel_overlay.c |  2 +-
 6 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2158a758a17d..8c6d0b7ac9a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3783,7 +3783,7 @@ int __must_check
 i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
 struct i915_vma * __must_check
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
-				     u32 alignment,
+				     u32 alignment, bool needs_fence,
 				     const struct i915_ggtt_view *view);
 void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
 int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 61ba321e9970..af18168e48e3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
  */
 struct i915_vma *
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
-				     u32 alignment,
+				     u32 alignment, bool needs_fence,
 				     const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
@@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 		 * happy to scanout from anywhere within its global aperture.
 		 */
 		flags = 0;
-		if (HAS_GMCH_DISPLAY(i915))
+		if (HAS_GMCH_DISPLAY(i915) || needs_fence)
 			flags = PIN_MAPPABLE;
 		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e6fcbc5abc75..0657e03e871a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
 	}
 }
 
+static bool intel_plane_needs_fence(const struct intel_plane_state *plane_state)
+{
+	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
+
+	if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
+		return false;
+
+	return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
+}
+
 struct i915_vma *
-intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
+intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
+			   unsigned int rotation, bool needs_fence)
 {
 	struct drm_device *dev = fb->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
@@ -2189,11 +2202,16 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
 
 	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
 
-	vma = i915_gem_object_pin_to_display_plane(obj, alignment, &view);
+	vma = i915_gem_object_pin_to_display_plane(obj, alignment,
+						   needs_fence, &view);
 	if (IS_ERR(vma))
 		goto err;
 
-	if (i915_vma_is_map_and_fenceable(vma)) {
+	if (needs_fence) {
+		int ret;
+
+		WARN_ON(!i915_vma_is_map_and_fenceable(vma));
+
 		/* 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
@@ -2210,7 +2228,11 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
 		 * something and try to run the system in a "less than optimal"
 		 * mode that matches the user configuration.
 		 */
-		i915_vma_pin_fence(vma);
+		ret = i915_vma_pin_fence(vma);
+		if (ret) {
+			vma = ERR_PTR(ret);
+			goto err;
+		}
 	}
 
 	i915_vma_get(vma);
@@ -2221,11 +2243,12 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
 	return vma;
 }
 
-void intel_unpin_fb_vma(struct i915_vma *vma)
+void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence)
 {
 	lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
 
-	i915_vma_unpin_fence(vma);
+	if (needs_fence)
+		i915_vma_unpin_fence(vma);
 	i915_gem_object_unpin_from_display_plane(vma);
 	i915_vma_put(vma);
 }
@@ -2816,6 +2839,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	struct intel_plane_state *intel_state =
 		to_intel_plane_state(plane_state);
 	struct drm_framebuffer *fb;
+	bool needs_fence;
 
 	if (!plane_config->fb)
 		return;
@@ -2869,8 +2893,10 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 
 valid_fb:
 	mutex_lock(&dev->struct_mutex);
+	needs_fence = intel_plane_needs_fence(intel_state);
 	intel_state->vma =
-		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
+		intel_pin_and_fence_fb_obj(fb, primary->state->rotation,
+					   needs_fence);
 	mutex_unlock(&dev->struct_mutex);
 	if (IS_ERR(intel_state->vma)) {
 		DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
@@ -12753,9 +12779,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 
 		ret = i915_gem_object_attach_phys(obj, align);
 	} else {
+		bool needs_fence =
+			intel_plane_needs_fence(to_intel_plane_state(new_state));
 		struct i915_vma *vma;
 
-		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
+		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation,
+						 needs_fence);
 		if (!IS_ERR(vma))
 			to_intel_plane_state(new_state)->vma = vma;
 		else
@@ -12809,8 +12838,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 	/* Should only be called after a successful intel_prepare_plane_fb()! */
 	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
 	if (vma) {
+		bool needs_fence =
+			intel_plane_needs_fence(to_intel_plane_state(old_state));
+
 		mutex_lock(&plane->dev->struct_mutex);
-		intel_unpin_fb_vma(vma);
+		intel_unpin_fb_vma(vma, needs_fence);
 		mutex_unlock(&plane->dev->struct_mutex);
 	}
 }
@@ -13152,7 +13184,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 			goto out_unlock;
 		}
 	} else {
-		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
+		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation,
+						 false);
 		if (IS_ERR(vma)) {
 			DRM_DEBUG_KMS("failed to pin object\n");
 
@@ -13183,7 +13216,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 
 	old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma);
 	if (old_vma)
-		intel_unpin_fb_vma(old_vma);
+		intel_unpin_fb_vma(old_vma, false);
 
 out_unlock:
 	mutex_unlock(&dev_priv->drm.struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e9b66e0cb647..c13f15ef342b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -905,7 +905,7 @@ struct cxsr_latency {
 #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
 #define to_intel_plane(x) container_of(x, struct intel_plane, base)
 #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
-#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
+#define intel_fb_obj(x) ((x) ? to_intel_framebuffer(x)->obj : NULL)
 
 struct intel_hdmi {
 	i915_reg_t hdmi_reg;
@@ -1423,8 +1423,9 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
 				    struct intel_load_detect_pipe *old,
 				    struct drm_modeset_acquire_ctx *ctx);
 struct i915_vma *
-intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
-void intel_unpin_fb_vma(struct i915_vma *vma);
+intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
+			   unsigned int rotation, bool needs_fence);
+void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence);
 struct drm_framebuffer *
 intel_framebuffer_create(struct drm_i915_gem_object *obj,
 			 struct drm_mode_fb_cmd2 *mode_cmd);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index b8af35187d22..4cbc7fde5e30 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -165,6 +165,13 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
 	return ret;
 }
 
+static bool intel_fbdev_needs_fence(struct intel_fbdev *ifbdev)
+{
+	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
+
+	return i915_gem_object_get_tiling(obj) != I915_TILING_NONE;
+}
+
 static int intelfb_create(struct drm_fb_helper *helper,
 			  struct drm_fb_helper_surface_size *sizes)
 {
@@ -180,6 +187,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct i915_vma *vma;
 	bool prealloc = false;
 	void __iomem *vaddr;
+	bool needs_fence;
 	int ret;
 
 	if (intel_fb &&
@@ -212,7 +220,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	 * This also validates that any existing fb inherited from the
 	 * BIOS is suitable for own access.
 	 */
-	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base, DRM_MODE_ROTATE_0);
+	needs_fence = intel_fbdev_needs_fence(ifbdev);
+	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base,
+					 DRM_MODE_ROTATE_0, needs_fence);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
 		goto out_unlock;
@@ -276,7 +286,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	return 0;
 
 out_unpin:
-	intel_unpin_fb_vma(vma);
+	intel_unpin_fb_vma(vma, needs_fence);
 out_unlock:
 	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
@@ -514,7 +524,8 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
 
 	if (ifbdev->vma) {
 		mutex_lock(&ifbdev->helper.dev->struct_mutex);
-		intel_unpin_fb_vma(ifbdev->vma);
+		intel_unpin_fb_vma(ifbdev->vma,
+				   intel_fbdev_needs_fence(ifbdev));
 		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 1b397b41cb4f..2b54526fbf3c 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -801,7 +801,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 
 	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
 
-	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
+	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, false, NULL);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
 		goto out_pin_section;
-- 
2.13.6

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

[PATCH 2/5] drm/i915: Clean up fbc vs. plane checks

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Let's record the information whether a plane can do fbc or not under
struct inte_plane.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 36 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_fbc.c     | 29 +++++++----------------------
 drivers/gpu/drm/i915/intel_pm.c      |  2 --
 4 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0657e03e871a..f4b773a4caf7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13244,6 +13244,31 @@ static const struct drm_plane_funcs intel_cursor_plane_funcs = {
 	.format_mod_supported = intel_cursor_plane_format_mod_supported,
 };
 
+static bool i9xx_plane_has_fbc(struct drm_i915_private *dev_priv,
+			       enum plane plane_id)
+{
+	if (!HAS_FBC(dev_priv))
+		return false;
+
+	if (IS_G4X(dev_priv) || IS_GEN5(dev_priv) ||
+	    IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
+		return true;
+
+	if (IS_GEN4(dev_priv))
+		return plane_id == PLANE_A || plane_id == PLANE_B;
+	else
+		return plane_id == PLANE_A;
+}
+
+static bool skl_plane_has_fbc(struct drm_i915_private *dev_priv,
+			      enum pipe pipe, enum plane_id plane_id)
+{
+	if (!HAS_FBC(dev_priv))
+		return false;
+
+	return pipe == PIPE_A && plane_id == PLANE_PRIMARY;
+}
+
 static struct intel_plane *
 intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 {
@@ -13286,6 +13311,15 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe)
 		primary->plane = (enum plane) pipe;
 	primary->id = PLANE_PRIMARY;
 	primary->frontbuffer_bit = INTEL_FRONTBUFFER_PRIMARY(pipe);
+
+	if (INTEL_GEN(dev_priv) >= 9)
+		primary->has_fbc = skl_plane_has_fbc(dev_priv,
+						     primary->pipe,
+						     primary->id);
+	else
+		primary->has_fbc = i9xx_plane_has_fbc(dev_priv,
+						      primary->plane);
+
 	primary->check_plane = intel_check_primary_plane;
 
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
@@ -14654,6 +14688,8 @@ int intel_modeset_init(struct drm_device *dev)
 		}
 	}
 
+	intel_fbc_init(dev_priv);
+
 	intel_shared_dpll_init(dev);
 	intel_update_fdi_pll_freq(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c13f15ef342b..472e37f00402 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -855,6 +855,7 @@ struct intel_plane {
 	enum plane_id id;
 	enum pipe pipe;
 	bool can_scale;
+	bool has_fbc;
 	int max_downscale;
 	uint32_t frontbuffer_bit;
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1a0f5e0c8d10..81a2526c445e 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -46,16 +46,6 @@ static inline bool fbc_supported(struct drm_i915_private *dev_priv)
 	return HAS_FBC(dev_priv);
 }
 
-static inline bool fbc_on_pipe_a_only(struct drm_i915_private *dev_priv)
-{
-	return IS_HASWELL(dev_priv) || INTEL_GEN(dev_priv) >= 8;
-}
-
-static inline bool fbc_on_plane_a_only(struct drm_i915_private *dev_priv)
-{
-	return INTEL_GEN(dev_priv) < 4;
-}
-
 static inline bool no_fbc_on_multiple_pipes(struct drm_i915_private *dev_priv)
 {
 	return INTEL_GEN(dev_priv) <= 3;
@@ -1082,13 +1072,10 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv,
 		struct intel_crtc_state *intel_crtc_state;
 		struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc);
 
-		if (!intel_plane_state->base.visible)
-			continue;
-
-		if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A)
+		if (!to_intel_plane(plane)->has_fbc)
 			continue;
 
-		if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A)
+		if (!intel_plane_state->base.visible)
 			continue;
 
 		intel_crtc_state = to_intel_crtc_state(
@@ -1346,7 +1333,7 @@ static bool need_fbc_vtd_wa(struct drm_i915_private *dev_priv)
 void intel_fbc_init(struct drm_i915_private *dev_priv)
 {
 	struct intel_fbc *fbc = &dev_priv->fbc;
-	enum pipe pipe;
+	struct intel_plane *plane;
 
 	INIT_WORK(&fbc->work.work, intel_fbc_work_fn);
 	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
@@ -1367,12 +1354,10 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	for_each_pipe(dev_priv, pipe) {
-		fbc->possible_framebuffer_bits |=
-				INTEL_FRONTBUFFER_PRIMARY(pipe);
-
-		if (fbc_on_pipe_a_only(dev_priv))
-			break;
+	for_each_intel_plane(&dev_priv->drm, plane) {
+		if (plane->has_fbc)
+			fbc->possible_framebuffer_bits |=
+				INTEL_FRONTBUFFER_PRIMARY(plane->pipe);
 	}
 
 	/* This value was pulled out of someone's hat */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 8c69ec9eb6ee..286d1613a9d0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -9012,8 +9012,6 @@ void intel_init_clock_gating_hooks(struct drm_i915_private *dev_priv)
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_i915_private *dev_priv)
 {
-	intel_fbc_init(dev_priv);
-
 	/* For cxsr */
 	if (IS_PINEVIEW(dev_priv))
 		i915_pineview_get_mem_freq(dev_priv);
-- 
2.13.6

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

[PATCH 3/5] drm/i915: Require fence only for FBC capable planes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since only a subset of primary planes are FBC capable there's no need to
waste fences on all of them. So let's skip the fence if the plane
isn't even fbc capable.

In the future we might extend this to skip the fence even for FBC
capable planes if the crtc and/or plane state isn't suitable
for FBC.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f4b773a4caf7..3b7021b39fdf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2163,7 +2163,7 @@ static bool intel_plane_needs_fence(const struct intel_plane_state *plane_state)
 	if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
 		return false;
 
-	return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
+	return INTEL_GEN(dev_priv) < 4 || plane->has_fbc;
 }
 
 struct i915_vma *
-- 
2.13.6

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

[PATCH 4/5] drm/i915: Add a FIXME about FBC vs. fence. 90/270 degree rotation

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently we're pinning the fence for the rotated GTT view. That doesn't
acually make any sense since the fence is used only for the FBC GTT
tracking. For that we would want the fence for the normal GTT view
instead of the rotated view. Too lazy to fix this now so just add a
FIXME.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3b7021b39fdf..25050bfce5d1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2202,6 +2202,12 @@ intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
 
 	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
 
+	/*
+	 * FIXME presumably with FBC and 90/270 degree rotation we
+	 * should pin the fence on the normal GTT view, and tell FBC
+	 * to monitor that one instead of the rotated view. Would
+	 * need changes to the FBC fence Y offset as well.
+	 */
 	vma = i915_gem_object_pin_to_display_plane(obj, alignment,
 						   needs_fence, &view);
 	if (IS_ERR(vma))
-- 
2.13.6

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

[PATCH 5/5] drm/i915: Extract intel_plane_{pin, unpin}_fb()

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We've replicated the fb pin/unpin code in a few places. Pull it into
convenint helpers.

This results in a slight change in behaviour on account of the cursor
path now dropping struct_mutex and reacquiring it later.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 105 +++++++++++++++++------------------
 1 file changed, 52 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 25050bfce5d1..8e1e1dd7f4af 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12705,6 +12705,51 @@ static void add_rps_boost_after_vblank(struct drm_crtc *crtc,
 	add_wait_queue(drm_crtc_vblank_waitqueue(crtc), &wait->wait);
 }
 
+static int intel_plane_pin_fb(struct intel_plane_state *plane_state)
+{
+	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	const struct drm_framebuffer *fb = plane_state->base.fb;
+	struct i915_vma *vma;
+	bool needs_fence;
+
+	if (plane->id == PLANE_CURSOR &&
+	    INTEL_INFO(dev_priv)->cursor_needs_physical) {
+		struct drm_i915_gem_object *obj = intel_fb_obj(fb);
+		int align = intel_cursor_alignment(dev_priv);
+
+		return i915_gem_object_attach_phys(obj, align);
+	}
+
+	needs_fence = intel_plane_needs_fence(plane_state);
+	vma = intel_pin_and_fence_fb_obj(fb, plane_state->base.rotation,
+					 needs_fence);
+	if (IS_ERR(vma))
+		return PTR_ERR(vma);
+
+	plane_state->vma = vma;
+
+	return 0;
+}
+
+static void intel_plane_unpin_fb(struct intel_plane_state *old_plane_state)
+{
+	struct intel_plane *plane = to_intel_plane(old_plane_state->base.plane);
+	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
+	struct i915_vma *vma;
+	bool needs_fence;
+
+	vma = fetch_and_zero(&old_plane_state->vma);
+	if (!vma)
+		return;
+
+	needs_fence = intel_plane_needs_fence(old_plane_state);
+
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	intel_unpin_fb_vma(vma, needs_fence);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+}
+
 /**
  * intel_prepare_plane_fb - Prepare fb for usage on plane
  * @plane: drm plane to prepare for
@@ -12779,23 +12824,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		return ret;
 	}
 
-	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
-	    INTEL_INFO(dev_priv)->cursor_needs_physical) {
-		const int align = intel_cursor_alignment(dev_priv);
-
-		ret = i915_gem_object_attach_phys(obj, align);
-	} else {
-		bool needs_fence =
-			intel_plane_needs_fence(to_intel_plane_state(new_state));
-		struct i915_vma *vma;
-
-		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation,
-						 needs_fence);
-		if (!IS_ERR(vma))
-			to_intel_plane_state(new_state)->vma = vma;
-		else
-			ret =  PTR_ERR(vma);
-	}
+	ret = intel_plane_pin_fb(to_intel_plane_state(new_state));
 
 	i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
 
@@ -12839,18 +12868,8 @@ void
 intel_cleanup_plane_fb(struct drm_plane *plane,
 		       struct drm_plane_state *old_state)
 {
-	struct i915_vma *vma;
-
 	/* Should only be called after a successful intel_prepare_plane_fb()! */
-	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
-	if (vma) {
-		bool needs_fence =
-			intel_plane_needs_fence(to_intel_plane_state(old_state));
-
-		mutex_lock(&plane->dev->struct_mutex);
-		intel_unpin_fb_vma(vma, needs_fence);
-		mutex_unlock(&plane->dev->struct_mutex);
-	}
+	intel_plane_unpin_fb(to_intel_plane_state(old_state));
 }
 
 int
@@ -13122,7 +13141,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	struct intel_plane *intel_plane = to_intel_plane(plane);
 	struct drm_framebuffer *old_fb;
 	struct drm_crtc_state *crtc_state = crtc->state;
-	struct i915_vma *old_vma, *vma;
 
 	/*
 	 * When crtc is inactive or there is a modeset pending,
@@ -13181,26 +13199,11 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	if (ret)
 		goto out_free;
 
-	if (INTEL_INFO(dev_priv)->cursor_needs_physical) {
-		int align = intel_cursor_alignment(dev_priv);
-
-		ret = i915_gem_object_attach_phys(intel_fb_obj(fb), align);
-		if (ret) {
-			DRM_DEBUG_KMS("failed to attach phys object\n");
-			goto out_unlock;
-		}
-	} else {
-		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation,
-						 false);
-		if (IS_ERR(vma)) {
-			DRM_DEBUG_KMS("failed to pin object\n");
-
-			ret = PTR_ERR(vma);
-			goto out_unlock;
-		}
+	ret = intel_plane_pin_fb(to_intel_plane_state(new_plane_state));
 
-		to_intel_plane_state(new_plane_state)->vma = vma;
-	}
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+	if (ret)
+		goto out_free;
 
 	old_fb = old_plane_state->fb;
 
@@ -13220,12 +13223,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 		intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
 	}
 
-	old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma);
-	if (old_vma)
-		intel_unpin_fb_vma(old_vma, false);
+	intel_plane_unpin_fb(to_intel_plane_state(old_plane_state));
 
-out_unlock:
-	mutex_unlock(&dev_priv->drm.struct_mutex);
 out_free:
 	if (ret)
 		intel_plane_destroy_state(plane, new_plane_state);
-- 
2.13.6

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

✗ Fi.CI.BAT: failure for series starting with [1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes
URL   : https://patchwork.freedesktop.org/series/33960/
State : failure

== Summary ==

Series 33960v1 series starting with [1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes
https://patchwork.freedesktop.org/api/1.0/series/33960/revisions/1/mbox/

Test chamelium:
        Subgroup dp-hpd-fast:
                skip       -> INCOMPLETE (fi-ivb-3520m)
                skip       -> INCOMPLETE (fi-ivb-3770)
                skip       -> INCOMPLETE (fi-byt-j1900)
                skip       -> INCOMPLETE (fi-hsw-4770)
                skip       -> INCOMPLETE (fi-hsw-4770r) fdo#102332
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test drv_module_reload:
        Subgroup basic-no-display:
                pass       -> DMESG-WARN (fi-gdg-551) fdo#102707
                pass       -> DMESG-WARN (fi-blb-e6850)
                pass       -> DMESG-WARN (fi-pnv-d510)
                pass       -> DMESG-WARN (fi-bwr-2160)
                pass       -> DMESG-WARN (fi-elk-e7500)
                pass       -> DMESG-WARN (fi-ilk-650)
                pass       -> DMESG-WARN (fi-snb-2600)
                pass       -> DMESG-WARN (fi-bdw-5557u)
                pass       -> DMESG-WARN (fi-bdw-gvtdvm)
                pass       -> DMESG-WARN (fi-bsw-n3050) fdo#103479
                pass       -> DMESG-WARN (fi-skl-6260u)
                pass       -> DMESG-WARN (fi-skl-6600u)
                pass       -> DMESG-WARN (fi-skl-6700hq)
                pass       -> DMESG-WARN (fi-skl-6700k)
                pass       -> DMESG-WARN (fi-skl-6770hq)
                pass       -> DMESG-WARN (fi-skl-gvtdvm)
                pass       -> DMESG-WARN (fi-bxt-dsi)
                pass       -> DMESG-WARN (fi-bxt-j4205)
                pass       -> DMESG-WARN (fi-kbl-7500u)
                pass       -> DMESG-WARN (fi-kbl-7560u)
                pass       -> DMESG-WARN (fi-kbl-7567u)
                pass       -> DMESG-WARN (fi-kbl-r)
                pass       -> DMESG-WARN (fi-glk-1)

fdo#102332 https://bugs.freedesktop.org/show_bug.cgi?id=102332
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707
fdo#103479 https://bugs.freedesktop.org/show_bug.cgi?id=103479

fi-bdw-5557u     total:289  pass:267  dwarn:1   dfail:0   fail:0   skip:21  time:452s
fi-bdw-gvtdvm    total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:461s
fi-blb-e6850     total:289  pass:222  dwarn:2   dfail:0   fail:0   skip:65  time:382s
fi-bsw-n3050     total:289  pass:242  dwarn:1   dfail:0   fail:0   skip:46  time:541s
fi-bwr-2160      total:289  pass:182  dwarn:1   dfail:0   fail:0   skip:106 time:278s
fi-bxt-dsi       total:289  pass:258  dwarn:1   dfail:0   fail:0   skip:30  time:507s
fi-bxt-j4205     total:289  pass:259  dwarn:1   dfail:0   fail:0   skip:29  time:505s
fi-byt-j1900     total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
fi-elk-e7500     total:289  pass:228  dwarn:1   dfail:0   fail:0   skip:60  time:438s
fi-gdg-551       total:289  pass:177  dwarn:2   dfail:0   fail:1   skip:109 time:264s
fi-glk-1         total:289  pass:260  dwarn:1   dfail:0   fail:0   skip:28  time:542s
fi-hsw-4770      total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
fi-hsw-4770r     total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
fi-ilk-650       total:289  pass:227  dwarn:1   dfail:0   fail:0   skip:61  time:430s
fi-ivb-3520m     total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
fi-ivb-3770      total:1    pass:0    dwarn:0   dfail:0   fail:0   skip:0  
fi-kbl-7500u     total:289  pass:263  dwarn:2   dfail:0   fail:0   skip:24  time:491s
fi-kbl-7560u     total:289  pass:269  dwarn:1   dfail:0   fail:0   skip:19  time:534s
fi-kbl-7567u     total:289  pass:268  dwarn:1   dfail:0   fail:0   skip:20  time:481s
fi-kbl-r         total:289  pass:261  dwarn:1   dfail:0   fail:0   skip:27  time:534s
fi-pnv-d510      total:289  pass:221  dwarn:2   dfail:0   fail:0   skip:66  time:582s
fi-skl-6260u     total:289  pass:268  dwarn:1   dfail:0   fail:0   skip:20  time:456s
fi-skl-6600u     total:289  pass:261  dwarn:1   dfail:0   fail:0   skip:27  time:561s
fi-skl-6700hq    total:289  pass:262  dwarn:1   dfail:0   fail:0   skip:26  time:573s
fi-skl-6700k     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:521s
fi-skl-6770hq    total:289  pass:268  dwarn:1   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:289  pass:265  dwarn:1   dfail:0   fail:0   skip:23  time:464s
fi-snb-2520m     total:246  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:289  pass:248  dwarn:1   dfail:0   fail:0   skip:40  time:426s
Blacklisted hosts:
fi-cfl-s2        total:289  pass:260  dwarn:1   dfail:0   fail:0   skip:28  time:480s
fi-cnl-y         total:289  pass:261  dwarn:1   dfail:0   fail:0   skip:27  time:572s

3878ad19ccd41428b5533eee0baabddcf19adc96 drm-tip: 2017y-11m-16d-15h-48m-14s UTC integration manifest
2e76e4522fd0 drm/i915: Extract intel_plane_{pin, unpin}_fb()
089078c4d269 drm/i915: Add a FIXME about FBC vs. fence. 90/270 degree rotation
bdbf1a074ba4 drm/i915: Require fence only for FBC capable planes
ae8114c4f6d7 drm/i915: Clean up fbc vs. plane checks
648c71fa59f1 drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7161/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Rodrigo Vivi]

On Thu, Nov 16, 2017 at 07:14:47PM +0000, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current code is trying to be lazy with fences on scanout buffers.
> That looks broken for several reasons:
> * gen2/3 always need a fence for tiled scanout
> * the unpin doesn't know whether we pinned the fence or not so it
>   may unpin something we don't own
> * FBC GTT tracking needs a fence (not sure we have proper fallback
>   for when there is no fence)

Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
FBC GTT tracking...

And "fallback" for both is the frontbuffer_tracking

> 
> So to fix this always use a fence for gen2/3, and for primary planes on
> other platforms (for FBC). For sprites and cursor we never need a fence
> so don't even try to get one.  Since we now know whether a fence was
> pinned we can safely unpin it too.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>  drivers/gpu/drm/i915/i915_gem.c      |  4 +--
>  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++--------
>  drivers/gpu/drm/i915/intel_drv.h     |  7 +++--
>  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +++++++++--
>  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
>  6 files changed, 66 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2158a758a17d..8c6d0b7ac9a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3783,7 +3783,7 @@ int __must_check
>  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
>  struct i915_vma * __must_check
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> -				     u32 alignment,
> +				     u32 alignment, bool needs_fence,
>  				     const struct i915_ggtt_view *view);
>  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
>  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61ba321e9970..af18168e48e3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
>   */
>  struct i915_vma *
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> -				     u32 alignment,
> +				     u32 alignment, bool needs_fence,
>  				     const struct i915_ggtt_view *view)
>  {
>  	struct i915_vma *vma;
> @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  		 * happy to scanout from anywhere within its global aperture.
>  		 */
>  		flags = 0;
> -		if (HAS_GMCH_DISPLAY(i915))
> +		if (HAS_GMCH_DISPLAY(i915) || needs_fence)
>  			flags = PIN_MAPPABLE;
>  		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e6fcbc5abc75..0657e03e871a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
>  	}
>  }
>  
> +static bool intel_plane_needs_fence(const struct intel_plane_state *plane_state)
> +{
> +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> +	struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
> +
> +	if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
> +		return false;
> +
> +	return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
> +}
> +
>  struct i915_vma *
> -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> +			   unsigned int rotation, bool needs_fence)
>  {
>  	struct drm_device *dev = fb->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -2189,11 +2202,16 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>  
>  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
>  
> -	vma = i915_gem_object_pin_to_display_plane(obj, alignment, &view);
> +	vma = i915_gem_object_pin_to_display_plane(obj, alignment,
> +						   needs_fence, &view);
>  	if (IS_ERR(vma))
>  		goto err;
>  
> -	if (i915_vma_is_map_and_fenceable(vma)) {
> +	if (needs_fence) {
> +		int ret;
> +
> +		WARN_ON(!i915_vma_is_map_and_fenceable(vma));
> +
>  		/* 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
> @@ -2210,7 +2228,11 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>  		 * something and try to run the system in a "less than optimal"
>  		 * mode that matches the user configuration.
>  		 */
> -		i915_vma_pin_fence(vma);
> +		ret = i915_vma_pin_fence(vma);
> +		if (ret) {
> +			vma = ERR_PTR(ret);
> +			goto err;
> +		}
>  	}
>  
>  	i915_vma_get(vma);
> @@ -2221,11 +2243,12 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
>  	return vma;
>  }
>  
> -void intel_unpin_fb_vma(struct i915_vma *vma)
> +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence)
>  {
>  	lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
>  
> -	i915_vma_unpin_fence(vma);
> +	if (needs_fence)
> +		i915_vma_unpin_fence(vma);
>  	i915_gem_object_unpin_from_display_plane(vma);
>  	i915_vma_put(vma);
>  }
> @@ -2816,6 +2839,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  	struct intel_plane_state *intel_state =
>  		to_intel_plane_state(plane_state);
>  	struct drm_framebuffer *fb;
> +	bool needs_fence;
>  
>  	if (!plane_config->fb)
>  		return;
> @@ -2869,8 +2893,10 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  
>  valid_fb:
>  	mutex_lock(&dev->struct_mutex);
> +	needs_fence = intel_plane_needs_fence(intel_state);
>  	intel_state->vma =
> -		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> +		intel_pin_and_fence_fb_obj(fb, primary->state->rotation,
> +					   needs_fence);
>  	mutex_unlock(&dev->struct_mutex);
>  	if (IS_ERR(intel_state->vma)) {
>  		DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
> @@ -12753,9 +12779,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  
>  		ret = i915_gem_object_attach_phys(obj, align);
>  	} else {
> +		bool needs_fence =
> +			intel_plane_needs_fence(to_intel_plane_state(new_state));
>  		struct i915_vma *vma;
>  
> -		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> +		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation,
> +						 needs_fence);
>  		if (!IS_ERR(vma))
>  			to_intel_plane_state(new_state)->vma = vma;
>  		else
> @@ -12809,8 +12838,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	/* Should only be called after a successful intel_prepare_plane_fb()! */
>  	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
>  	if (vma) {
> +		bool needs_fence =
> +			intel_plane_needs_fence(to_intel_plane_state(old_state));
> +
>  		mutex_lock(&plane->dev->struct_mutex);
> -		intel_unpin_fb_vma(vma);
> +		intel_unpin_fb_vma(vma, needs_fence);
>  		mutex_unlock(&plane->dev->struct_mutex);
>  	}
>  }
> @@ -13152,7 +13184,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  			goto out_unlock;
>  		}
>  	} else {
> -		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> +		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation,
> +						 false);
>  		if (IS_ERR(vma)) {
>  			DRM_DEBUG_KMS("failed to pin object\n");
>  
> @@ -13183,7 +13216,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  
>  	old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma);
>  	if (old_vma)
> -		intel_unpin_fb_vma(old_vma);
> +		intel_unpin_fb_vma(old_vma, false);
>  
>  out_unlock:
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e9b66e0cb647..c13f15ef342b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -905,7 +905,7 @@ struct cxsr_latency {
>  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
>  #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
> -#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
> +#define intel_fb_obj(x) ((x) ? to_intel_framebuffer(x)->obj : NULL)
>  
>  struct intel_hdmi {
>  	i915_reg_t hdmi_reg;
> @@ -1423,8 +1423,9 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  				    struct intel_load_detect_pipe *old,
>  				    struct drm_modeset_acquire_ctx *ctx);
>  struct i915_vma *
> -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
> -void intel_unpin_fb_vma(struct i915_vma *vma);
> +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> +			   unsigned int rotation, bool needs_fence);
> +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence);
>  struct drm_framebuffer *
>  intel_framebuffer_create(struct drm_i915_gem_object *obj,
>  			 struct drm_mode_fb_cmd2 *mode_cmd);
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index b8af35187d22..4cbc7fde5e30 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -165,6 +165,13 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
>  	return ret;
>  }
>  
> +static bool intel_fbdev_needs_fence(struct intel_fbdev *ifbdev)
> +{
> +	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
> +
> +	return i915_gem_object_get_tiling(obj) != I915_TILING_NONE;
> +}
> +
>  static int intelfb_create(struct drm_fb_helper *helper,
>  			  struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -180,6 +187,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	struct i915_vma *vma;
>  	bool prealloc = false;
>  	void __iomem *vaddr;
> +	bool needs_fence;
>  	int ret;
>  
>  	if (intel_fb &&
> @@ -212,7 +220,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	 * This also validates that any existing fb inherited from the
>  	 * BIOS is suitable for own access.
>  	 */
> -	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base, DRM_MODE_ROTATE_0);
> +	needs_fence = intel_fbdev_needs_fence(ifbdev);
> +	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base,
> +					 DRM_MODE_ROTATE_0, needs_fence);
>  	if (IS_ERR(vma)) {
>  		ret = PTR_ERR(vma);
>  		goto out_unlock;
> @@ -276,7 +286,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  	return 0;
>  
>  out_unpin:
> -	intel_unpin_fb_vma(vma);
> +	intel_unpin_fb_vma(vma, needs_fence);
>  out_unlock:
>  	intel_runtime_pm_put(dev_priv);
>  	mutex_unlock(&dev->struct_mutex);
> @@ -514,7 +524,8 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
>  
>  	if (ifbdev->vma) {
>  		mutex_lock(&ifbdev->helper.dev->struct_mutex);
> -		intel_unpin_fb_vma(ifbdev->vma);
> +		intel_unpin_fb_vma(ifbdev->vma,
> +				   intel_fbdev_needs_fence(ifbdev));
>  		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 1b397b41cb4f..2b54526fbf3c 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -801,7 +801,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
>  
>  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
>  
> -	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
> +	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, false, NULL);
>  	if (IS_ERR(vma)) {
>  		ret = PTR_ERR(vma);
>  		goto out_pin_section;
> -- 
> 2.13.6
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Add a FIXME about FBC vs. fence. 90/270 degree rotation

[Chris Wilson]

Quoting Ville Syrjala (2017-11-16 19:14:50)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently we're pinning the fence for the rotated GTT view. That doesn't
> acually make any sense since the fence is used only for the FBC GTT
> tracking. For that we would want the fence for the normal GTT view
> instead of the rotated view. Too lazy to fix this now so just add a
> FIXME.

The rotated view can't have a fence as its vma is not fenceable.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Chris Wilson]

Quoting Ville Syrjala (2017-11-16 19:14:47)
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current code is trying to be lazy with fences on scanout buffers.
> That looks broken for several reasons:
> * gen2/3 always need a fence for tiled scanout

Which it already gets. All gen2-gen4 are given a fenceable vma.

> * the unpin doesn't know whether we pinned the fence or not so it
>   may unpin something we don't own

Then track it correctly.

> * FBC GTT tracking needs a fence (not sure we have proper fallback
>   for when there is no fence)

Debatable as whether that is worth forcing a fence; the argument being
that you don't want to stall your flip upon eviction which is the
situation you are already in if you didn't get a fence in the first
place.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Add a FIXME about FBC vs. fence. 90/270 degree rotation

[Ville Syrjälä]

On Thu, Nov 16, 2017 at 09:01:03PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-16 19:14:50)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Currently we're pinning the fence for the rotated GTT view. That doesn't
> > acually make any sense since the fence is used only for the FBC GTT
> > tracking. For that we would want the fence for the normal GTT view
> > instead of the rotated view. Too lazy to fix this now so just add a
> > FIXME.
> 
> The rotated view can't have a fence as its vma is not fenceable.

I see. Doesn't change the fact that it's broken though. I'll see about
rewording this, or maybe I'll even try to fix it.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Ville Syrjälä]

On Thu, Nov 16, 2017 at 09:06:08PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-16 19:14:47)
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The current code is trying to be lazy with fences on scanout buffers.
> > That looks broken for several reasons:
> > * gen2/3 always need a fence for tiled scanout
> 
> Which it already gets. All gen2-gen4 are given a fenceable vma.

Fenceable yes, but we still allow the fence_pin() to fail so
there's no guarantee that we actually have a fence on the vma
AFAICS.

I did managed to trigger an oops in the FBC code on my i85x where
the vma didn't have a fence. I actually don't know how it managed
to scan out anything in that case. Maybe it didn't and I just wasn't
looking at the screen when it failed. 

It's also slightly odd that it even got that far as the FBC code has
a vma->fence check earlier. My current thinking is that we didn't
have a fence when we were supposed to pin it, and then when FBC did
its check a fence happened to be present, and later on the fence
disappeared once more. Either that or FBC was being enabled when
the fb was no longer pinned, which would be a clear bug in itself.

> 
> > * the unpin doesn't know whether we pinned the fence or not so it
> >   may unpin something we don't own
> 
> Then track it correctly.

Wasn't convinced that it's worth it. After this series on most platforms
we would have just the one plane that would need a fence. And the total
number of fences being 32 on modern platforms it seems somewhat
unlikely to me that we couldn't get the fence here. I've not tested that
theory though.

Although I guess for the 90/270 case we really would need to track the
other vma and its fence correctly.

> 
> > * FBC GTT tracking needs a fence (not sure we have proper fallback
> >   for when there is no fence)
> 
> Debatable as whether that is worth forcing a fence; the argument being
> that you don't want to stall your flip upon eviction which is the
> situation you are already in if you didn't get a fence in the first
> place.

My thinking was again that it's very unlikely that would can't get a
fence.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Ville Syrjälä]

On Thu, Nov 16, 2017 at 12:49:23PM -0800, Rodrigo Vivi wrote:
> On Thu, Nov 16, 2017 at 07:14:47PM +0000, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The current code is trying to be lazy with fences on scanout buffers.
> > That looks broken for several reasons:
> > * gen2/3 always need a fence for tiled scanout
> > * the unpin doesn't know whether we pinned the fence or not so it
> >   may unpin something we don't own
> > * FBC GTT tracking needs a fence (not sure we have proper fallback
> >   for when there is no fence)
> 
> Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
> FBC GTT tracking...

That whole concept seems a bit broken. AFAICS we have no "is FBC enabled
on the appropriate plane?" checks in the PSR code. I'm not quite sure
how it would handle multiple planes either. I guess we should be
disabling PSR when multiple planes are enabled?

> 
> And "fallback" for both is the frontbuffer_tracking

I'm not sure how fronbuffer tracking handles GTT mmaps. I thought it
didn't even try. If I'm mistaken then I'm thinking we should perhaps
even stop using the GTT tracking entirely just to make the whole thing
more consistent. Having two ways to do the same thing doesn't appeal to
me.

> 
> > 
> > So to fix this always use a fence for gen2/3, and for primary planes on
> > other platforms (for FBC). For sprites and cursor we never need a fence
> > so don't even try to get one.  Since we now know whether a fence was
> > pinned we can safely unpin it too.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> >  drivers/gpu/drm/i915/i915_gem.c      |  4 +--
> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++--------
> >  drivers/gpu/drm/i915/intel_drv.h     |  7 +++--
> >  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +++++++++--
> >  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
> >  6 files changed, 66 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 2158a758a17d..8c6d0b7ac9a5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3783,7 +3783,7 @@ int __must_check
> >  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
> >  struct i915_vma * __must_check
> >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > -				     u32 alignment,
> > +				     u32 alignment, bool needs_fence,
> >  				     const struct i915_ggtt_view *view);
> >  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
> >  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 61ba321e9970..af18168e48e3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> >   */
> >  struct i915_vma *
> >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > -				     u32 alignment,
> > +				     u32 alignment, bool needs_fence,
> >  				     const struct i915_ggtt_view *view)
> >  {
> >  	struct i915_vma *vma;
> > @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >  		 * happy to scanout from anywhere within its global aperture.
> >  		 */
> >  		flags = 0;
> > -		if (HAS_GMCH_DISPLAY(i915))
> > +		if (HAS_GMCH_DISPLAY(i915) || needs_fence)
> >  			flags = PIN_MAPPABLE;
> >  		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
> >  	}
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e6fcbc5abc75..0657e03e871a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> >  	}
> >  }
> >  
> > +static bool intel_plane_needs_fence(const struct intel_plane_state *plane_state)
> > +{
> > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > +	struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
> > +
> > +	if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
> > +		return false;
> > +
> > +	return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
> > +}
> > +
> >  struct i915_vma *
> > -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> > +			   unsigned int rotation, bool needs_fence)
> >  {
> >  	struct drm_device *dev = fb->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -2189,11 +2202,16 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> >  
> >  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
> >  
> > -	vma = i915_gem_object_pin_to_display_plane(obj, alignment, &view);
> > +	vma = i915_gem_object_pin_to_display_plane(obj, alignment,
> > +						   needs_fence, &view);
> >  	if (IS_ERR(vma))
> >  		goto err;
> >  
> > -	if (i915_vma_is_map_and_fenceable(vma)) {
> > +	if (needs_fence) {
> > +		int ret;
> > +
> > +		WARN_ON(!i915_vma_is_map_and_fenceable(vma));
> > +
> >  		/* 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
> > @@ -2210,7 +2228,11 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> >  		 * something and try to run the system in a "less than optimal"
> >  		 * mode that matches the user configuration.
> >  		 */
> > -		i915_vma_pin_fence(vma);
> > +		ret = i915_vma_pin_fence(vma);
> > +		if (ret) {
> > +			vma = ERR_PTR(ret);
> > +			goto err;
> > +		}
> >  	}
> >  
> >  	i915_vma_get(vma);
> > @@ -2221,11 +2243,12 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> >  	return vma;
> >  }
> >  
> > -void intel_unpin_fb_vma(struct i915_vma *vma)
> > +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence)
> >  {
> >  	lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
> >  
> > -	i915_vma_unpin_fence(vma);
> > +	if (needs_fence)
> > +		i915_vma_unpin_fence(vma);
> >  	i915_gem_object_unpin_from_display_plane(vma);
> >  	i915_vma_put(vma);
> >  }
> > @@ -2816,6 +2839,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  	struct intel_plane_state *intel_state =
> >  		to_intel_plane_state(plane_state);
> >  	struct drm_framebuffer *fb;
> > +	bool needs_fence;
> >  
> >  	if (!plane_config->fb)
> >  		return;
> > @@ -2869,8 +2893,10 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  
> >  valid_fb:
> >  	mutex_lock(&dev->struct_mutex);
> > +	needs_fence = intel_plane_needs_fence(intel_state);
> >  	intel_state->vma =
> > -		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> > +		intel_pin_and_fence_fb_obj(fb, primary->state->rotation,
> > +					   needs_fence);
> >  	mutex_unlock(&dev->struct_mutex);
> >  	if (IS_ERR(intel_state->vma)) {
> >  		DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
> > @@ -12753,9 +12779,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >  
> >  		ret = i915_gem_object_attach_phys(obj, align);
> >  	} else {
> > +		bool needs_fence =
> > +			intel_plane_needs_fence(to_intel_plane_state(new_state));
> >  		struct i915_vma *vma;
> >  
> > -		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> > +		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation,
> > +						 needs_fence);
> >  		if (!IS_ERR(vma))
> >  			to_intel_plane_state(new_state)->vma = vma;
> >  		else
> > @@ -12809,8 +12838,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> >  	/* Should only be called after a successful intel_prepare_plane_fb()! */
> >  	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
> >  	if (vma) {
> > +		bool needs_fence =
> > +			intel_plane_needs_fence(to_intel_plane_state(old_state));
> > +
> >  		mutex_lock(&plane->dev->struct_mutex);
> > -		intel_unpin_fb_vma(vma);
> > +		intel_unpin_fb_vma(vma, needs_fence);
> >  		mutex_unlock(&plane->dev->struct_mutex);
> >  	}
> >  }
> > @@ -13152,7 +13184,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> >  			goto out_unlock;
> >  		}
> >  	} else {
> > -		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> > +		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation,
> > +						 false);
> >  		if (IS_ERR(vma)) {
> >  			DRM_DEBUG_KMS("failed to pin object\n");
> >  
> > @@ -13183,7 +13216,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> >  
> >  	old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma);
> >  	if (old_vma)
> > -		intel_unpin_fb_vma(old_vma);
> > +		intel_unpin_fb_vma(old_vma, false);
> >  
> >  out_unlock:
> >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index e9b66e0cb647..c13f15ef342b 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -905,7 +905,7 @@ struct cxsr_latency {
> >  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
> >  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
> >  #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
> > -#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
> > +#define intel_fb_obj(x) ((x) ? to_intel_framebuffer(x)->obj : NULL)
> >  
> >  struct intel_hdmi {
> >  	i915_reg_t hdmi_reg;
> > @@ -1423,8 +1423,9 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
> >  				    struct intel_load_detect_pipe *old,
> >  				    struct drm_modeset_acquire_ctx *ctx);
> >  struct i915_vma *
> > -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
> > -void intel_unpin_fb_vma(struct i915_vma *vma);
> > +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> > +			   unsigned int rotation, bool needs_fence);
> > +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence);
> >  struct drm_framebuffer *
> >  intel_framebuffer_create(struct drm_i915_gem_object *obj,
> >  			 struct drm_mode_fb_cmd2 *mode_cmd);
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index b8af35187d22..4cbc7fde5e30 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -165,6 +165,13 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> >  	return ret;
> >  }
> >  
> > +static bool intel_fbdev_needs_fence(struct intel_fbdev *ifbdev)
> > +{
> > +	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
> > +
> > +	return i915_gem_object_get_tiling(obj) != I915_TILING_NONE;
> > +}
> > +
> >  static int intelfb_create(struct drm_fb_helper *helper,
> >  			  struct drm_fb_helper_surface_size *sizes)
> >  {
> > @@ -180,6 +187,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	struct i915_vma *vma;
> >  	bool prealloc = false;
> >  	void __iomem *vaddr;
> > +	bool needs_fence;
> >  	int ret;
> >  
> >  	if (intel_fb &&
> > @@ -212,7 +220,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	 * This also validates that any existing fb inherited from the
> >  	 * BIOS is suitable for own access.
> >  	 */
> > -	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base, DRM_MODE_ROTATE_0);
> > +	needs_fence = intel_fbdev_needs_fence(ifbdev);
> > +	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base,
> > +					 DRM_MODE_ROTATE_0, needs_fence);
> >  	if (IS_ERR(vma)) {
> >  		ret = PTR_ERR(vma);
> >  		goto out_unlock;
> > @@ -276,7 +286,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  	return 0;
> >  
> >  out_unpin:
> > -	intel_unpin_fb_vma(vma);
> > +	intel_unpin_fb_vma(vma, needs_fence);
> >  out_unlock:
> >  	intel_runtime_pm_put(dev_priv);
> >  	mutex_unlock(&dev->struct_mutex);
> > @@ -514,7 +524,8 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> >  
> >  	if (ifbdev->vma) {
> >  		mutex_lock(&ifbdev->helper.dev->struct_mutex);
> > -		intel_unpin_fb_vma(ifbdev->vma);
> > +		intel_unpin_fb_vma(ifbdev->vma,
> > +				   intel_fbdev_needs_fence(ifbdev));
> >  		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > index 1b397b41cb4f..2b54526fbf3c 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -801,7 +801,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> >  
> >  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
> >  
> > -	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
> > +	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, false, NULL);
> >  	if (IS_ERR(vma)) {
> >  		ret = PTR_ERR(vma);
> >  		goto out_pin_section;
> > -- 
> > 2.13.6
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Rodrigo Vivi]

On Fri, Nov 17, 2017 at 01:21:35PM +0000, Ville Syrjälä wrote:
> On Thu, Nov 16, 2017 at 12:49:23PM -0800, Rodrigo Vivi wrote:
> > On Thu, Nov 16, 2017 at 07:14:47PM +0000, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The current code is trying to be lazy with fences on scanout buffers.
> > > That looks broken for several reasons:
> > > * gen2/3 always need a fence for tiled scanout
> > > * the unpin doesn't know whether we pinned the fence or not so it
> > >   may unpin something we don't own
> > > * FBC GTT tracking needs a fence (not sure we have proper fallback
> > >   for when there is no fence)
> > 
> > Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
> > FBC GTT tracking...
> 
> That whole concept seems a bit broken. AFAICS we have no "is FBC enabled
> on the appropriate plane?" checks in the PSR code. I'm not quite sure
> how it would handle multiple planes either. I guess we should be
> disabling PSR when multiple planes are enabled?

Ironically this case is good on PSR afai can remember...
The old problem with PSR that is back to picture is just primary plane with
fbdev/fbcon... :/

> 
> > 
> > And "fallback" for both is the frontbuffer_tracking
> 
> I'm not sure how fronbuffer tracking handles GTT mmaps. I thought it
> didn't even try. If I'm mistaken then I'm thinking we should perhaps
> even stop using the GTT tracking entirely just to make the whole thing
> more consistent. Having two ways to do the same thing doesn't appeal to
> me.

Well... the frontbuffer tracking would kill the benefits of PSR2.
So if we could make that HW tracking really working I would prefer.
Otherwise we will have to have both... at least one for cases where
hw tracking works and other for the cases hw tracking doesn't work... :/

> 
> > 
> > > 
> > > So to fix this always use a fence for gen2/3, and for primary planes on
> > > other platforms (for FBC). For sprites and cursor we never need a fence
> > > so don't even try to get one.  Since we now know whether a fence was
> > > pinned we can safely unpin it too.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> > >  drivers/gpu/drm/i915/i915_gem.c      |  4 +--
> > >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++--------
> > >  drivers/gpu/drm/i915/intel_drv.h     |  7 +++--
> > >  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +++++++++--
> > >  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
> > >  6 files changed, 66 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 2158a758a17d..8c6d0b7ac9a5 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -3783,7 +3783,7 @@ int __must_check
> > >  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
> > >  struct i915_vma * __must_check
> > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > -				     u32 alignment,
> > > +				     u32 alignment, bool needs_fence,
> > >  				     const struct i915_ggtt_view *view);
> > >  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
> > >  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 61ba321e9970..af18168e48e3 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> > >   */
> > >  struct i915_vma *
> > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > -				     u32 alignment,
> > > +				     u32 alignment, bool needs_fence,
> > >  				     const struct i915_ggtt_view *view)
> > >  {
> > >  	struct i915_vma *vma;
> > > @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > >  		 * happy to scanout from anywhere within its global aperture.
> > >  		 */
> > >  		flags = 0;
> > > -		if (HAS_GMCH_DISPLAY(i915))
> > > +		if (HAS_GMCH_DISPLAY(i915) || needs_fence)
> > >  			flags = PIN_MAPPABLE;
> > >  		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
> > >  	}
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index e6fcbc5abc75..0657e03e871a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> > >  	}
> > >  }
> > >  
> > > +static bool intel_plane_needs_fence(const struct intel_plane_state *plane_state)
> > > +{
> > > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > +	struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
> > > +
> > > +	if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
> > > +		return false;
> > > +
> > > +	return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
> > > +}
> > > +
> > >  struct i915_vma *
> > > -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > > +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> > > +			   unsigned int rotation, bool needs_fence)
> > >  {
> > >  	struct drm_device *dev = fb->dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > @@ -2189,11 +2202,16 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > >  
> > >  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
> > >  
> > > -	vma = i915_gem_object_pin_to_display_plane(obj, alignment, &view);
> > > +	vma = i915_gem_object_pin_to_display_plane(obj, alignment,
> > > +						   needs_fence, &view);
> > >  	if (IS_ERR(vma))
> > >  		goto err;
> > >  
> > > -	if (i915_vma_is_map_and_fenceable(vma)) {
> > > +	if (needs_fence) {
> > > +		int ret;
> > > +
> > > +		WARN_ON(!i915_vma_is_map_and_fenceable(vma));
> > > +
> > >  		/* 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
> > > @@ -2210,7 +2228,11 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > >  		 * something and try to run the system in a "less than optimal"
> > >  		 * mode that matches the user configuration.
> > >  		 */
> > > -		i915_vma_pin_fence(vma);
> > > +		ret = i915_vma_pin_fence(vma);
> > > +		if (ret) {
> > > +			vma = ERR_PTR(ret);
> > > +			goto err;
> > > +		}
> > >  	}
> > >  
> > >  	i915_vma_get(vma);
> > > @@ -2221,11 +2243,12 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > >  	return vma;
> > >  }
> > >  
> > > -void intel_unpin_fb_vma(struct i915_vma *vma)
> > > +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence)
> > >  {
> > >  	lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
> > >  
> > > -	i915_vma_unpin_fence(vma);
> > > +	if (needs_fence)
> > > +		i915_vma_unpin_fence(vma);
> > >  	i915_gem_object_unpin_from_display_plane(vma);
> > >  	i915_vma_put(vma);
> > >  }
> > > @@ -2816,6 +2839,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > >  	struct intel_plane_state *intel_state =
> > >  		to_intel_plane_state(plane_state);
> > >  	struct drm_framebuffer *fb;
> > > +	bool needs_fence;
> > >  
> > >  	if (!plane_config->fb)
> > >  		return;
> > > @@ -2869,8 +2893,10 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > >  
> > >  valid_fb:
> > >  	mutex_lock(&dev->struct_mutex);
> > > +	needs_fence = intel_plane_needs_fence(intel_state);
> > >  	intel_state->vma =
> > > -		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> > > +		intel_pin_and_fence_fb_obj(fb, primary->state->rotation,
> > > +					   needs_fence);
> > >  	mutex_unlock(&dev->struct_mutex);
> > >  	if (IS_ERR(intel_state->vma)) {
> > >  		DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
> > > @@ -12753,9 +12779,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > >  
> > >  		ret = i915_gem_object_attach_phys(obj, align);
> > >  	} else {
> > > +		bool needs_fence =
> > > +			intel_plane_needs_fence(to_intel_plane_state(new_state));
> > >  		struct i915_vma *vma;
> > >  
> > > -		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> > > +		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation,
> > > +						 needs_fence);
> > >  		if (!IS_ERR(vma))
> > >  			to_intel_plane_state(new_state)->vma = vma;
> > >  		else
> > > @@ -12809,8 +12838,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> > >  	/* Should only be called after a successful intel_prepare_plane_fb()! */
> > >  	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
> > >  	if (vma) {
> > > +		bool needs_fence =
> > > +			intel_plane_needs_fence(to_intel_plane_state(old_state));
> > > +
> > >  		mutex_lock(&plane->dev->struct_mutex);
> > > -		intel_unpin_fb_vma(vma);
> > > +		intel_unpin_fb_vma(vma, needs_fence);
> > >  		mutex_unlock(&plane->dev->struct_mutex);
> > >  	}
> > >  }
> > > @@ -13152,7 +13184,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > >  			goto out_unlock;
> > >  		}
> > >  	} else {
> > > -		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> > > +		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation,
> > > +						 false);
> > >  		if (IS_ERR(vma)) {
> > >  			DRM_DEBUG_KMS("failed to pin object\n");
> > >  
> > > @@ -13183,7 +13216,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > >  
> > >  	old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma);
> > >  	if (old_vma)
> > > -		intel_unpin_fb_vma(old_vma);
> > > +		intel_unpin_fb_vma(old_vma, false);
> > >  
> > >  out_unlock:
> > >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index e9b66e0cb647..c13f15ef342b 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -905,7 +905,7 @@ struct cxsr_latency {
> > >  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
> > >  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
> > >  #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
> > > -#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
> > > +#define intel_fb_obj(x) ((x) ? to_intel_framebuffer(x)->obj : NULL)
> > >  
> > >  struct intel_hdmi {
> > >  	i915_reg_t hdmi_reg;
> > > @@ -1423,8 +1423,9 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
> > >  				    struct intel_load_detect_pipe *old,
> > >  				    struct drm_modeset_acquire_ctx *ctx);
> > >  struct i915_vma *
> > > -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
> > > -void intel_unpin_fb_vma(struct i915_vma *vma);
> > > +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> > > +			   unsigned int rotation, bool needs_fence);
> > > +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence);
> > >  struct drm_framebuffer *
> > >  intel_framebuffer_create(struct drm_i915_gem_object *obj,
> > >  			 struct drm_mode_fb_cmd2 *mode_cmd);
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index b8af35187d22..4cbc7fde5e30 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -165,6 +165,13 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > >  	return ret;
> > >  }
> > >  
> > > +static bool intel_fbdev_needs_fence(struct intel_fbdev *ifbdev)
> > > +{
> > > +	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
> > > +
> > > +	return i915_gem_object_get_tiling(obj) != I915_TILING_NONE;
> > > +}
> > > +
> > >  static int intelfb_create(struct drm_fb_helper *helper,
> > >  			  struct drm_fb_helper_surface_size *sizes)
> > >  {
> > > @@ -180,6 +187,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	struct i915_vma *vma;
> > >  	bool prealloc = false;
> > >  	void __iomem *vaddr;
> > > +	bool needs_fence;
> > >  	int ret;
> > >  
> > >  	if (intel_fb &&
> > > @@ -212,7 +220,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	 * This also validates that any existing fb inherited from the
> > >  	 * BIOS is suitable for own access.
> > >  	 */
> > > -	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base, DRM_MODE_ROTATE_0);
> > > +	needs_fence = intel_fbdev_needs_fence(ifbdev);
> > > +	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base,
> > > +					 DRM_MODE_ROTATE_0, needs_fence);
> > >  	if (IS_ERR(vma)) {
> > >  		ret = PTR_ERR(vma);
> > >  		goto out_unlock;
> > > @@ -276,7 +286,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  	return 0;
> > >  
> > >  out_unpin:
> > > -	intel_unpin_fb_vma(vma);
> > > +	intel_unpin_fb_vma(vma, needs_fence);
> > >  out_unlock:
> > >  	intel_runtime_pm_put(dev_priv);
> > >  	mutex_unlock(&dev->struct_mutex);
> > > @@ -514,7 +524,8 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> > >  
> > >  	if (ifbdev->vma) {
> > >  		mutex_lock(&ifbdev->helper.dev->struct_mutex);
> > > -		intel_unpin_fb_vma(ifbdev->vma);
> > > +		intel_unpin_fb_vma(ifbdev->vma,
> > > +				   intel_fbdev_needs_fence(ifbdev));
> > >  		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
> > >  	}
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > > index 1b397b41cb4f..2b54526fbf3c 100644
> > > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > > @@ -801,7 +801,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> > >  
> > >  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
> > >  
> > > -	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
> > > +	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, false, NULL);
> > >  	if (IS_ERR(vma)) {
> > >  		ret = PTR_ERR(vma);
> > >  		goto out_pin_section;
> > > -- 
> > > 2.13.6
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/5] drm/i915: Always pin the fence for scanout on gen2/3 and primary planes

[Ville Syrjälä]

On Fri, Nov 17, 2017 at 09:29:33AM -0800, Rodrigo Vivi wrote:
> On Fri, Nov 17, 2017 at 01:21:35PM +0000, Ville Syrjälä wrote:
> > On Thu, Nov 16, 2017 at 12:49:23PM -0800, Rodrigo Vivi wrote:
> > > On Thu, Nov 16, 2017 at 07:14:47PM +0000, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > The current code is trying to be lazy with fences on scanout buffers.
> > > > That looks broken for several reasons:
> > > > * gen2/3 always need a fence for tiled scanout
> > > > * the unpin doesn't know whether we pinned the fence or not so it
> > > >   may unpin something we don't own
> > > > * FBC GTT tracking needs a fence (not sure we have proper fallback
> > > >   for when there is no fence)
> > > 
> > > Ohh! I wonder if this would also solve few of old PSR cases... PSR re-uses
> > > FBC GTT tracking...
> > 
> > That whole concept seems a bit broken. AFAICS we have no "is FBC enabled
> > on the appropriate plane?" checks in the PSR code. I'm not quite sure
> > how it would handle multiple planes either. I guess we should be
> > disabling PSR when multiple planes are enabled?
> 
> Ironically this case is good on PSR afai can remember...

OK, so it works by luck then :)

> The old problem with PSR that is back to picture is just primary plane with
> fbdev/fbcon... :/
> 
> > 
> > > 
> > > And "fallback" for both is the frontbuffer_tracking
> > 
> > I'm not sure how fronbuffer tracking handles GTT mmaps. I thought it
> > didn't even try. If I'm mistaken then I'm thinking we should perhaps
> > even stop using the GTT tracking entirely just to make the whole thing
> > more consistent. Having two ways to do the same thing doesn't appeal to
> > me.
> 
> Well... the frontbuffer tracking would kill the benefits of PSR2.

Which benefit is that? Partial updates or something? I don't think FBC
tracking would help much there because modern platforms just nuke the
entire compressed buffer on any modification. Well, maybe GTT tracking
still has smaller granularity, but that seems like a niche use case so
not much benefit I think.

> So if we could make that HW tracking really working I would prefer.

All it would take is someone resurrecting my old FBC work. I had full
HW tracking implemented there, but at the time no one wanted it.

> Otherwise we will have to have both... at least one for cases where
> hw tracking works and other for the cases hw tracking doesn't work... :/
> 
> > 
> > > 
> > > > 
> > > > So to fix this always use a fence for gen2/3, and for primary planes on
> > > > other platforms (for FBC). For sprites and cursor we never need a fence
> > > > so don't even try to get one.  Since we now know whether a fence was
> > > > pinned we can safely unpin it too.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> > > >  drivers/gpu/drm/i915/i915_gem.c      |  4 +--
> > > >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++--------
> > > >  drivers/gpu/drm/i915/intel_drv.h     |  7 +++--
> > > >  drivers/gpu/drm/i915/intel_fbdev.c   | 17 +++++++++--
> > > >  drivers/gpu/drm/i915/intel_overlay.c |  2 +-
> > > >  6 files changed, 66 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 2158a758a17d..8c6d0b7ac9a5 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -3783,7 +3783,7 @@ int __must_check
> > > >  i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write);
> > > >  struct i915_vma * __must_check
> > > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > > -				     u32 alignment,
> > > > +				     u32 alignment, bool needs_fence,
> > > >  				     const struct i915_ggtt_view *view);
> > > >  void i915_gem_object_unpin_from_display_plane(struct i915_vma *vma);
> > > >  int i915_gem_object_attach_phys(struct drm_i915_gem_object *obj,
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index 61ba321e9970..af18168e48e3 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -3944,7 +3944,7 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> > > >   */
> > > >  struct i915_vma *
> > > >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > > -				     u32 alignment,
> > > > +				     u32 alignment, bool needs_fence,
> > > >  				     const struct i915_ggtt_view *view)
> > > >  {
> > > >  	struct i915_vma *vma;
> > > > @@ -3997,7 +3997,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > > >  		 * happy to scanout from anywhere within its global aperture.
> > > >  		 */
> > > >  		flags = 0;
> > > > -		if (HAS_GMCH_DISPLAY(i915))
> > > > +		if (HAS_GMCH_DISPLAY(i915) || needs_fence)
> > > >  			flags = PIN_MAPPABLE;
> > > >  		vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment, flags);
> > > >  	}
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index e6fcbc5abc75..0657e03e871a 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -2154,8 +2154,21 @@ static unsigned int intel_surf_alignment(const struct drm_framebuffer *fb,
> > > >  	}
> > > >  }
> > > >  
> > > > +static bool intel_plane_needs_fence(const struct intel_plane_state *plane_state)
> > > > +{
> > > > +	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > > > +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > > > +	struct drm_i915_gem_object *obj = intel_fb_obj(plane_state->base.fb);
> > > > +
> > > > +	if (i915_gem_object_get_tiling(obj) == I915_TILING_NONE)
> > > > +		return false;
> > > > +
> > > > +	return INTEL_GEN(dev_priv) < 4 || plane->id == PLANE_PRIMARY;
> > > > +}
> > > > +
> > > >  struct i915_vma *
> > > > -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > > > +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> > > > +			   unsigned int rotation, bool needs_fence)
> > > >  {
> > > >  	struct drm_device *dev = fb->dev;
> > > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > > > @@ -2189,11 +2202,16 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > > >  
> > > >  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
> > > >  
> > > > -	vma = i915_gem_object_pin_to_display_plane(obj, alignment, &view);
> > > > +	vma = i915_gem_object_pin_to_display_plane(obj, alignment,
> > > > +						   needs_fence, &view);
> > > >  	if (IS_ERR(vma))
> > > >  		goto err;
> > > >  
> > > > -	if (i915_vma_is_map_and_fenceable(vma)) {
> > > > +	if (needs_fence) {
> > > > +		int ret;
> > > > +
> > > > +		WARN_ON(!i915_vma_is_map_and_fenceable(vma));
> > > > +
> > > >  		/* 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
> > > > @@ -2210,7 +2228,11 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > > >  		 * something and try to run the system in a "less than optimal"
> > > >  		 * mode that matches the user configuration.
> > > >  		 */
> > > > -		i915_vma_pin_fence(vma);
> > > > +		ret = i915_vma_pin_fence(vma);
> > > > +		if (ret) {
> > > > +			vma = ERR_PTR(ret);
> > > > +			goto err;
> > > > +		}
> > > >  	}
> > > >  
> > > >  	i915_vma_get(vma);
> > > > @@ -2221,11 +2243,12 @@ intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation)
> > > >  	return vma;
> > > >  }
> > > >  
> > > > -void intel_unpin_fb_vma(struct i915_vma *vma)
> > > > +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence)
> > > >  {
> > > >  	lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
> > > >  
> > > > -	i915_vma_unpin_fence(vma);
> > > > +	if (needs_fence)
> > > > +		i915_vma_unpin_fence(vma);
> > > >  	i915_gem_object_unpin_from_display_plane(vma);
> > > >  	i915_vma_put(vma);
> > > >  }
> > > > @@ -2816,6 +2839,7 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > > >  	struct intel_plane_state *intel_state =
> > > >  		to_intel_plane_state(plane_state);
> > > >  	struct drm_framebuffer *fb;
> > > > +	bool needs_fence;
> > > >  
> > > >  	if (!plane_config->fb)
> > > >  		return;
> > > > @@ -2869,8 +2893,10 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > > >  
> > > >  valid_fb:
> > > >  	mutex_lock(&dev->struct_mutex);
> > > > +	needs_fence = intel_plane_needs_fence(intel_state);
> > > >  	intel_state->vma =
> > > > -		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> > > > +		intel_pin_and_fence_fb_obj(fb, primary->state->rotation,
> > > > +					   needs_fence);
> > > >  	mutex_unlock(&dev->struct_mutex);
> > > >  	if (IS_ERR(intel_state->vma)) {
> > > >  		DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
> > > > @@ -12753,9 +12779,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > > >  
> > > >  		ret = i915_gem_object_attach_phys(obj, align);
> > > >  	} else {
> > > > +		bool needs_fence =
> > > > +			intel_plane_needs_fence(to_intel_plane_state(new_state));
> > > >  		struct i915_vma *vma;
> > > >  
> > > > -		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> > > > +		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation,
> > > > +						 needs_fence);
> > > >  		if (!IS_ERR(vma))
> > > >  			to_intel_plane_state(new_state)->vma = vma;
> > > >  		else
> > > > @@ -12809,8 +12838,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> > > >  	/* Should only be called after a successful intel_prepare_plane_fb()! */
> > > >  	vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
> > > >  	if (vma) {
> > > > +		bool needs_fence =
> > > > +			intel_plane_needs_fence(to_intel_plane_state(old_state));
> > > > +
> > > >  		mutex_lock(&plane->dev->struct_mutex);
> > > > -		intel_unpin_fb_vma(vma);
> > > > +		intel_unpin_fb_vma(vma, needs_fence);
> > > >  		mutex_unlock(&plane->dev->struct_mutex);
> > > >  	}
> > > >  }
> > > > @@ -13152,7 +13184,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > > >  			goto out_unlock;
> > > >  		}
> > > >  	} else {
> > > > -		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> > > > +		vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation,
> > > > +						 false);
> > > >  		if (IS_ERR(vma)) {
> > > >  			DRM_DEBUG_KMS("failed to pin object\n");
> > > >  
> > > > @@ -13183,7 +13216,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > > >  
> > > >  	old_vma = fetch_and_zero(&to_intel_plane_state(old_plane_state)->vma);
> > > >  	if (old_vma)
> > > > -		intel_unpin_fb_vma(old_vma);
> > > > +		intel_unpin_fb_vma(old_vma, false);
> > > >  
> > > >  out_unlock:
> > > >  	mutex_unlock(&dev_priv->drm.struct_mutex);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index e9b66e0cb647..c13f15ef342b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -905,7 +905,7 @@ struct cxsr_latency {
> > > >  #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
> > > >  #define to_intel_plane(x) container_of(x, struct intel_plane, base)
> > > >  #define to_intel_plane_state(x) container_of(x, struct intel_plane_state, base)
> > > > -#define intel_fb_obj(x) (x ? to_intel_framebuffer(x)->obj : NULL)
> > > > +#define intel_fb_obj(x) ((x) ? to_intel_framebuffer(x)->obj : NULL)
> > > >  
> > > >  struct intel_hdmi {
> > > >  	i915_reg_t hdmi_reg;
> > > > @@ -1423,8 +1423,9 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
> > > >  				    struct intel_load_detect_pipe *old,
> > > >  				    struct drm_modeset_acquire_ctx *ctx);
> > > >  struct i915_vma *
> > > > -intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, unsigned int rotation);
> > > > -void intel_unpin_fb_vma(struct i915_vma *vma);
> > > > +intel_pin_and_fence_fb_obj(const struct drm_framebuffer *fb,
> > > > +			   unsigned int rotation, bool needs_fence);
> > > > +void intel_unpin_fb_vma(struct i915_vma *vma, bool needs_fence);
> > > >  struct drm_framebuffer *
> > > >  intel_framebuffer_create(struct drm_i915_gem_object *obj,
> > > >  			 struct drm_mode_fb_cmd2 *mode_cmd);
> > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > index b8af35187d22..4cbc7fde5e30 100644
> > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > > @@ -165,6 +165,13 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +static bool intel_fbdev_needs_fence(struct intel_fbdev *ifbdev)
> > > > +{
> > > > +	struct drm_i915_gem_object *obj = ifbdev->fb->obj;
> > > > +
> > > > +	return i915_gem_object_get_tiling(obj) != I915_TILING_NONE;
> > > > +}
> > > > +
> > > >  static int intelfb_create(struct drm_fb_helper *helper,
> > > >  			  struct drm_fb_helper_surface_size *sizes)
> > > >  {
> > > > @@ -180,6 +187,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > >  	struct i915_vma *vma;
> > > >  	bool prealloc = false;
> > > >  	void __iomem *vaddr;
> > > > +	bool needs_fence;
> > > >  	int ret;
> > > >  
> > > >  	if (intel_fb &&
> > > > @@ -212,7 +220,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > >  	 * This also validates that any existing fb inherited from the
> > > >  	 * BIOS is suitable for own access.
> > > >  	 */
> > > > -	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base, DRM_MODE_ROTATE_0);
> > > > +	needs_fence = intel_fbdev_needs_fence(ifbdev);
> > > > +	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base,
> > > > +					 DRM_MODE_ROTATE_0, needs_fence);
> > > >  	if (IS_ERR(vma)) {
> > > >  		ret = PTR_ERR(vma);
> > > >  		goto out_unlock;
> > > > @@ -276,7 +286,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > > >  	return 0;
> > > >  
> > > >  out_unpin:
> > > > -	intel_unpin_fb_vma(vma);
> > > > +	intel_unpin_fb_vma(vma, needs_fence);
> > > >  out_unlock:
> > > >  	intel_runtime_pm_put(dev_priv);
> > > >  	mutex_unlock(&dev->struct_mutex);
> > > > @@ -514,7 +524,8 @@ static void intel_fbdev_destroy(struct intel_fbdev *ifbdev)
> > > >  
> > > >  	if (ifbdev->vma) {
> > > >  		mutex_lock(&ifbdev->helper.dev->struct_mutex);
> > > > -		intel_unpin_fb_vma(ifbdev->vma);
> > > > +		intel_unpin_fb_vma(ifbdev->vma,
> > > > +				   intel_fbdev_needs_fence(ifbdev));
> > > >  		mutex_unlock(&ifbdev->helper.dev->struct_mutex);
> > > >  	}
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> > > > index 1b397b41cb4f..2b54526fbf3c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > > > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > > > @@ -801,7 +801,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
> > > >  
> > > >  	atomic_inc(&dev_priv->gpu_error.pending_fb_pin);
> > > >  
> > > > -	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, NULL);
> > > > +	vma = i915_gem_object_pin_to_display_plane(new_bo, 0, false, NULL);
> > > >  	if (IS_ERR(vma)) {
> > > >  		ret = PTR_ERR(vma);
> > > >  		goto out_pin_section;
> > > > -- 
> > > > 2.13.6
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx