diff mbox

[15/25] drm/i915/fbc: rewrite the multiple_pipes_ok() code for locking

Message ID 1453210558-7875-16-git-send-email-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R Jan. 19, 2016, 1:35 p.m. UTC
Older FBC platforms have this restriction where FBC can't be enabled
if multiple pipes are enabled. In the current code, we disable FBC
before the second pipe becomes visible.

One of the problems with this code is that the current
multiple_pipes_ok() implementation just iterates through all CRTCs
looking at their states, but it doesn't make sure that the state
locks are grabbed. It also can't just grab the locks for every CRTC
since this would kill one of the biggest advantages of atomic
modesetting.

After the recent FBC changes, we now have the appropriate locks for
the given CRTC, so we can just try to maintain the state of each CRTC
and update it once intel_fbc_pre_update is called.

As a last note, I don't have gen 2/3 machines to test this code. My
current plan is to enable FBC on just the newer platforms, so this
patch is just an attempt to get the gen 2/3 code at least looking
sane, so if one day someone decide to fix FBC on these platforms, they
may have less work to do.

Not-tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (only on HSW+)
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c |  2 ++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_fbc.c     | 55 +++++++++++++++++++++++++++---------
 4 files changed, 45 insertions(+), 14 deletions(-)

Comments

Maarten Lankhorst Jan. 21, 2016, 1:29 p.m. UTC | #1
Op 19-01-16 om 14:35 schreef Paulo Zanoni:
> Older FBC platforms have this restriction where FBC can't be enabled
> if multiple pipes are enabled. In the current code, we disable FBC
> before the second pipe becomes visible.
>
> One of the problems with this code is that the current
> multiple_pipes_ok() implementation just iterates through all CRTCs
> looking at their states, but it doesn't make sure that the state
> locks are grabbed. It also can't just grab the locks for every CRTC
> since this would kill one of the biggest advantages of atomic
> modesetting.
>
> After the recent FBC changes, we now have the appropriate locks for
> the given CRTC, so we can just try to maintain the state of each CRTC
> and update it once intel_fbc_pre_update is called.
>
> As a last note, I don't have gen 2/3 machines to test this code. My
> current plan is to enable FBC on just the newer platforms, so this
> patch is just an attempt to get the gen 2/3 code at least looking
> sane, so if one day someone decide to fix FBC on these platforms, they
> may have less work to do.
>
It would still be nice if pre/post update took a crtc_state and plane_state so we wouldn't have to worry about lifetime issues.
Right now it's pulled from various places.

~Maarten
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 02e6869..65e5771 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -911,6 +911,7 @@  struct intel_fbc {
 	unsigned threshold;
 	unsigned int possible_framebuffer_bits;
 	unsigned int busy_bits;
+	unsigned int visible_pipes_mask;
 	struct intel_crtc *crtc;
 
 	struct drm_mm_node compressed_fb;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b7c0707..bfd5336 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15981,6 +15981,8 @@  intel_modeset_setup_hw_state(struct drm_device *dev)
 			modeset_put_power_domains(dev_priv, put_domains);
 	}
 	intel_display_set_init_power(dev_priv, false);
+
+	intel_fbc_init_pipe_state(dev_priv);
 }
 
 void intel_display_resume(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f227bf1..62a25c3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1356,6 +1356,7 @@  bool intel_fbc_is_active(struct drm_i915_private *dev_priv);
 void intel_fbc_pre_update(struct intel_crtc *crtc);
 void intel_fbc_post_update(struct intel_crtc *crtc);
 void intel_fbc_init(struct drm_i915_private *dev_priv);
+void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv);
 void intel_fbc_enable(struct intel_crtc *crtc);
 void intel_fbc_disable(struct intel_crtc *crtc);
 void intel_fbc_global_disable(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 1977176..fe0754c 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -56,6 +56,11 @@  static inline bool fbc_on_plane_a_only(struct drm_i915_private *dev_priv)
 	return INTEL_INFO(dev_priv)->gen < 4;
 }
 
+static inline bool no_fbc_on_multiple_pipes(struct drm_i915_private *dev_priv)
+{
+	return INTEL_INFO(dev_priv)->gen <= 3;
+}
+
 /*
  * In some platforms where the CRTC's x:0/y:0 coordinates doesn't match the
  * frontbuffer's x:0/y:0 coordinates we lie to the hardware about the plane's
@@ -479,25 +484,25 @@  static bool crtc_can_fbc(struct intel_crtc *crtc)
 	return true;
 }
 
-static bool multiple_pipes_ok(struct drm_i915_private *dev_priv)
+static bool multiple_pipes_ok(struct intel_crtc *crtc)
 {
-	enum pipe pipe;
-	int n_pipes = 0;
-	struct drm_crtc *crtc;
+	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_plane *primary = crtc->base.primary;
+	struct intel_fbc *fbc = &dev_priv->fbc;
+	enum pipe pipe = crtc->pipe;
 
-	if (INTEL_INFO(dev_priv)->gen > 4)
+	/* Don't even bother tracking anything we don't need. */
+	if (!no_fbc_on_multiple_pipes(dev_priv))
 		return true;
 
-	/* FIXME: we don't have the appropriate state locks to do this here. */
-	for_each_pipe(dev_priv, pipe) {
-		crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+	WARN_ON(!drm_modeset_is_locked(&primary->mutex));
 
-		if (intel_crtc_active(crtc) &&
-		    to_intel_plane_state(crtc->primary->state)->visible)
-			n_pipes++;
-	}
+	if (to_intel_plane_state(primary->state)->visible)
+		fbc->visible_pipes_mask |= (1 << pipe);
+	else
+		fbc->visible_pipes_mask &= ~(1 << pipe);
 
-	return (n_pipes < 2);
+	return (fbc->visible_pipes_mask & ~(1 << pipe)) != 0;
 }
 
 static int find_compression_threshold(struct drm_i915_private *dev_priv,
@@ -889,7 +894,7 @@  void intel_fbc_pre_update(struct intel_crtc *crtc)
 
 	mutex_lock(&fbc->lock);
 
-	if (!multiple_pipes_ok(dev_priv)) {
+	if (!multiple_pipes_ok(crtc)) {
 		set_no_fbc_reason(dev_priv, "more than one pipe active");
 		goto deactivate;
 	}
@@ -1122,6 +1127,28 @@  void intel_fbc_global_disable(struct drm_i915_private *dev_priv)
 }
 
 /**
+ * intel_fbc_init_pipe_state - initialize FBC's CRTC visibility tracking
+ * @dev_priv: i915 device instance
+ *
+ * The FBC code needs to track CRTC visibility since the older platforms can't
+ * have FBC enabled while multiple pipes are used. This function does the
+ * initial setup at driver load to make sure FBC is matching the real hardware.
+ */
+void intel_fbc_init_pipe_state(struct drm_i915_private *dev_priv)
+{
+	struct intel_crtc *crtc;
+
+	/* Don't even bother tracking anything if we don't need. */
+	if (!no_fbc_on_multiple_pipes(dev_priv))
+		return;
+
+	for_each_intel_crtc(dev_priv->dev, crtc)
+		if (intel_crtc_active(&crtc->base) &&
+		    to_intel_plane_state(crtc->base.primary->state)->visible)
+			dev_priv->fbc.visible_pipes_mask |= (1 << crtc->pipe);
+}
+
+/**
  * intel_fbc_init - Initialize FBC
  * @dev_priv: the i915 device
  *