[PATCH 1/9] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[PATCH 1/9] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[Daniel Vetter]

The current/old frontbuffer might still have gpu frontbuffer rendering
pending. But once flipped it won't have the corresponding frontbuffer
bits any more and hence the request retire function won't ever clear
the corresponding busy bits. The async flip tracking (with the
flip_prepare and flip_complete functions) already does this, but
somehow I've forgotten to do this for synchronous flips.

Note that we don't track outstanding rendering of the new framebuffer
with busy_bits since all our plane update code waits for previous
rendering to complete before displaying a new buffer. Hence a new
buffer will never be busy.

Reported-by: Paulo Zanoni <przanoni@gmail.com>
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
busy buffer tests.
---
 drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bcafefcf048b..b7c69460fb20 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
 			     unsigned frontbuffer_bits);
-/**
- * intel_frontbuffer_flip - synchronous frontbuffer flip
- * @dev: DRM device
- * @frontbuffer_bits: frontbuffer plane tracking bits
- *
- * This function gets called after scheduling a flip on @obj. This is for
- * synchronous plane updates which will happen on the next vblank and which will
- * not get delayed by pending gpu rendering.
- *
- * Can be called without any locks held.
- */
-static inline
+inline
 void intel_frontbuffer_flip(struct drm_device *dev,
-			    unsigned frontbuffer_bits)
-{
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
-}
+			    unsigned frontbuffer_bits);
 
 unsigned int intel_fb_align_height(struct drm_device *dev,
 				   unsigned int height,
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 57095f54c1f2..2a1611a7ce1d 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 
 	intel_frontbuffer_flush(dev, frontbuffer_bits);
 }
+
+/**
+ * intel_frontbuffer_flip - synchronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *
+ * Can be called without any locks held.
+ */
+inline
+void intel_frontbuffer_flip(struct drm_device *dev,
+			    unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
+	/* Remove stale busy bits due to the old buffer. */
+	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
-- 
2.1.0

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

[PATCH 2/9] drm/i915: Filter out no-op frontbuffer tracking flushes

[Daniel Vetter]

Paulo noticed that the fbc frontbuffer tracking flush callback
occasionally gets a call without any bit set. This can happen when we
have to filter flush calls due to e.g. gpu rendering. Filter these
out.

Reported-by: Paulo Zanoni <przanoni@gmail.com>
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_frontbuffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 2a1611a7ce1d..899b6112bbe0 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -179,6 +179,9 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
+	if (!frontbuffer_bits)
+		return;
+
 	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
 
 	intel_edp_drrs_flush(dev, frontbuffer_bits);
-- 
2.1.0

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

Re: [PATCH 2/9] drm/i915: Filter out no-op frontbuffer tracking flushes

[Paulo Zanoni]

2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Paulo noticed that the fbc frontbuffer tracking flush callback
> occasionally gets a call without any bit set. This can happen when we
> have to filter flush calls due to e.g. gpu rendering. Filter these
> out.
>
> Reported-by: Paulo Zanoni <przanoni@gmail.com>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 2a1611a7ce1d..899b6112bbe0 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -179,6 +179,9 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>         frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
>         mutex_unlock(&dev_priv->fb_tracking.lock);
>
> +       if (!frontbuffer_bits)
> +               return;
> +
>         intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
>
>         intel_edp_drrs_flush(dev, frontbuffer_bits);
> --
> 2.1.0
>



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

[PATCH 3/9] drm/i915: debugfs for frontbuffer tracking

[Daniel Vetter]

Useful to figure out whether stuck bits are due to the frontbuffer
tracking code as opposed to individual consumers (who have their own
bitmask tracking).

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c49fe2afa223..f3b80627d5b7 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1581,6 +1581,21 @@ static int i915_drpc_info(struct seq_file *m, void *unused)
 		return ironlake_drpc_info(m);
 }
 
+static int i915_frontbuffer_tracking(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	seq_printf(m, "FB tracking busy bits: 0x%08x\n",
+		   dev_priv->fb_tracking.busy_bits);
+
+	seq_printf(m, "FB tracking flip bits: 0x%08x\n",
+		   dev_priv->fb_tracking.flip_bits);
+
+	return 0;
+}
+
 static int i915_fbc_status(struct seq_file *m, void *unused)
 {
 	struct drm_info_node *node = m->private;
@@ -5024,6 +5039,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_drpc_info", i915_drpc_info, 0},
 	{"i915_emon_status", i915_emon_status, 0},
 	{"i915_ring_freq_table", i915_ring_freq_table, 0},
+	{"i915_frontbuffer_tracking", i915_frontbuffer_tracking, 0},
 	{"i915_fbc_status", i915_fbc_status, 0},
 	{"i915_ips_status", i915_ips_status, 0},
 	{"i915_sr_status", i915_sr_status, 0},
-- 
2.1.0

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

Re: [PATCH 3/9] drm/i915: debugfs for frontbuffer tracking

[Paulo Zanoni]

2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Useful to figure out whether stuck bits are due to the frontbuffer
> tracking code as opposed to individual consumers (who have their own
> bitmask tracking).

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c49fe2afa223..f3b80627d5b7 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1581,6 +1581,21 @@ static int i915_drpc_info(struct seq_file *m, void *unused)
>                 return ironlake_drpc_info(m);
>  }
>
> +static int i915_frontbuffer_tracking(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       seq_printf(m, "FB tracking busy bits: 0x%08x\n",
> +                  dev_priv->fb_tracking.busy_bits);
> +
> +       seq_printf(m, "FB tracking flip bits: 0x%08x\n",
> +                  dev_priv->fb_tracking.flip_bits);
> +
> +       return 0;
> +}
> +
>  static int i915_fbc_status(struct seq_file *m, void *unused)
>  {
>         struct drm_info_node *node = m->private;
> @@ -5024,6 +5039,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_drpc_info", i915_drpc_info, 0},
>         {"i915_emon_status", i915_emon_status, 0},
>         {"i915_ring_freq_table", i915_ring_freq_table, 0},
> +       {"i915_frontbuffer_tracking", i915_frontbuffer_tracking, 0},
>         {"i915_fbc_status", i915_fbc_status, 0},
>         {"i915_ips_status", i915_ips_status, 0},
>         {"i915_sr_status", i915_sr_status, 0},
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

[PATCH 4/9] drm/i915: Nuke lvds downclock support

[Daniel Vetter]

With the new DRRS code it kinda sticks out, and we never managed to
get this to work well enough without causing issues. Time to wave
goodbye.

I've decided to keep the logic for programming the reduced clocks
intact, but everything else is gone. If anyone ever wants to resurrect
this we need to redo it all anyway on top of the frontbuffer tracking.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  4 --
 drivers/gpu/drm/i915/i915_params.c       |  6 ---
 drivers/gpu/drm/i915/intel_bios.c        | 62 +---------------------
 drivers/gpu/drm/i915/intel_display.c     | 90 +++-----------------------------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 57 --------------------
 drivers/gpu/drm/i915/intel_lvds.c        | 18 +------
 6 files changed, 8 insertions(+), 229 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 491ef0cfcb0b..b34ff2ba2cbc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1793,9 +1793,6 @@ struct drm_i915_private {
 
 	/* Reclocking support */
 	bool render_reclock_avail;
-	bool lvds_downclock_avail;
-	/* indicates the reduced downclock for LVDS*/
-	int lvds_downclock;
 
 	struct i915_frontbuffer_tracking fb_tracking;
 
@@ -2548,7 +2545,6 @@ struct i915_params {
 	int modeset;
 	int panel_ignore_lid;
 	int semaphores;
-	unsigned int lvds_downclock;
 	int lvds_channel_mode;
 	int panel_use_ssc;
 	int vbt_sdvo_panel_type;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8ac5a1b29ac0..566826d98c97 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -28,7 +28,6 @@ struct i915_params i915 __read_mostly = {
 	.modeset = -1,
 	.panel_ignore_lid = 1,
 	.semaphores = -1,
-	.lvds_downclock = 0,
 	.lvds_channel_mode = 0,
 	.panel_use_ssc = -1,
 	.vbt_sdvo_panel_type = -1,
@@ -84,11 +83,6 @@ MODULE_PARM_DESC(enable_fbc,
 	"Enable frame buffer compression for power savings "
 	"(default: -1 (use per-chip default))");
 
-module_param_named(lvds_downclock, i915.lvds_downclock, int, 0400);
-MODULE_PARM_DESC(lvds_downclock,
-	"Use panel (LVDS/eDP) downclocking for power savings "
-	"(default: false)");
-
 module_param_named(lvds_channel_mode, i915.lvds_channel_mode, int, 0600);
 MODULE_PARM_DESC(lvds_channel_mode,
 	 "Specify LVDS channel mode "
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 198fc3c3291b..2ff9eb00fdec 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -122,42 +122,6 @@ fill_detail_timing_data(struct drm_display_mode *panel_fixed_mode,
 	drm_mode_set_name(panel_fixed_mode);
 }
 
-static bool
-lvds_dvo_timing_equal_size(const struct lvds_dvo_timing *a,
-			   const struct lvds_dvo_timing *b)
-{
-	if (a->hactive_hi != b->hactive_hi ||
-	    a->hactive_lo != b->hactive_lo)
-		return false;
-
-	if (a->hsync_off_hi != b->hsync_off_hi ||
-	    a->hsync_off_lo != b->hsync_off_lo)
-		return false;
-
-	if (a->hsync_pulse_width != b->hsync_pulse_width)
-		return false;
-
-	if (a->hblank_hi != b->hblank_hi ||
-	    a->hblank_lo != b->hblank_lo)
-		return false;
-
-	if (a->vactive_hi != b->vactive_hi ||
-	    a->vactive_lo != b->vactive_lo)
-		return false;
-
-	if (a->vsync_off != b->vsync_off)
-		return false;
-
-	if (a->vsync_pulse_width != b->vsync_pulse_width)
-		return false;
-
-	if (a->vblank_hi != b->vblank_hi ||
-	    a->vblank_lo != b->vblank_lo)
-		return false;
-
-	return true;
-}
-
 static const struct lvds_dvo_timing *
 get_lvds_dvo_timing(const struct bdb_lvds_lfp_data *lvds_lfp_data,
 		    const struct bdb_lvds_lfp_data_ptrs *lvds_lfp_data_ptrs,
@@ -213,7 +177,7 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 	const struct lvds_dvo_timing *panel_dvo_timing;
 	const struct lvds_fp_timing *fp_timing;
 	struct drm_display_mode *panel_fixed_mode;
-	int i, downclock, drrs_mode;
+	int drrs_mode;
 
 	lvds_options = find_section(bdb, BDB_LVDS_OPTIONS);
 	if (!lvds_options)
@@ -272,30 +236,6 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 	DRM_DEBUG_KMS("Found panel mode in BIOS VBT tables:\n");
 	drm_mode_debug_printmodeline(panel_fixed_mode);
 
-	/*
-	 * Iterate over the LVDS panel timing info to find the lowest clock
-	 * for the native resolution.
-	 */
-	downclock = panel_dvo_timing->clock;
-	for (i = 0; i < 16; i++) {
-		const struct lvds_dvo_timing *dvo_timing;
-
-		dvo_timing = get_lvds_dvo_timing(lvds_lfp_data,
-						 lvds_lfp_data_ptrs,
-						 i);
-		if (lvds_dvo_timing_equal_size(dvo_timing, panel_dvo_timing) &&
-		    dvo_timing->clock < downclock)
-			downclock = dvo_timing->clock;
-	}
-
-	if (downclock < panel_dvo_timing->clock && i915.lvds_downclock) {
-		dev_priv->lvds_downclock_avail = 1;
-		dev_priv->lvds_downclock = downclock * 10;
-		DRM_DEBUG_KMS("LVDS downclock is found in VBT. "
-			      "Normal Clock %dKHz, downclock %dKHz\n",
-			      panel_fixed_mode->clock, 10*downclock);
-	}
-
 	fp_timing = get_lvds_fp_timing(bdb, lvds_lfp_data,
 				       lvds_lfp_data_ptrs,
 				       lvds_options->panel_type);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3f4891782cf6..45a93c87d48e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7827,9 +7827,9 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int refclk, num_connectors = 0;
-	intel_clock_t clock, reduced_clock;
-	bool ok, has_reduced_clock = false;
-	bool is_lvds = false, is_dsi = false;
+	intel_clock_t clock;
+	bool ok;
+	bool is_dsi = false;
 	struct intel_encoder *encoder;
 	const intel_limit_t *limit;
 	struct drm_atomic_state *state = crtc_state->base.state;
@@ -7847,9 +7847,6 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 		encoder = to_intel_encoder(connector_state->best_encoder);
 
 		switch (encoder->type) {
-		case INTEL_OUTPUT_LVDS:
-			is_lvds = true;
-			break;
 		case INTEL_OUTPUT_DSI:
 			is_dsi = true;
 			break;
@@ -7881,19 +7878,6 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 			return -EINVAL;
 		}
 
-		if (is_lvds && dev_priv->lvds_downclock_avail) {
-			/*
-			 * Ensure we match the reduced clock's P to the target
-			 * clock.  If the clocks don't match, we can't switch
-			 * the display clock by using the FP0/FP1. In such case
-			 * we will disable the LVDS downclock feature.
-			 */
-			has_reduced_clock =
-				dev_priv->display.find_dpll(limit, crtc_state,
-							    dev_priv->lvds_downclock,
-							    refclk, &clock,
-							    &reduced_clock);
-		}
 		/* Compat-code for transition, will disappear. */
 		crtc_state->dpll.n = clock.n;
 		crtc_state->dpll.m1 = clock.m1;
@@ -7903,16 +7887,14 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 	}
 
 	if (IS_GEN2(dev)) {
-		i8xx_update_pll(crtc, crtc_state,
-				has_reduced_clock ? &reduced_clock : NULL,
+		i8xx_update_pll(crtc, crtc_state, NULL,
 				num_connectors);
 	} else if (IS_CHERRYVIEW(dev)) {
 		chv_update_pll(crtc, crtc_state);
 	} else if (IS_VALLEYVIEW(dev)) {
 		vlv_update_pll(crtc, crtc_state);
 	} else {
-		i9xx_update_pll(crtc, crtc_state,
-				has_reduced_clock ? &reduced_clock : NULL,
+		i9xx_update_pll(crtc, crtc_state, NULL,
 				num_connectors);
 	}
 
@@ -8724,9 +8706,7 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int refclk;
 	const intel_limit_t *limit;
-	bool ret, is_lvds = false;
-
-	is_lvds = intel_pipe_will_have_type(crtc_state, INTEL_OUTPUT_LVDS);
+	bool ret;
 
 	refclk = ironlake_get_refclk(crtc_state);
 
@@ -8742,20 +8722,6 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	if (!ret)
 		return false;
 
-	if (is_lvds && dev_priv->lvds_downclock_avail) {
-		/*
-		 * Ensure we match the reduced clock's P to the target clock.
-		 * If the clocks don't match, we can't switch the display clock
-		 * by using the FP0/FP1. In such case we will disable the LVDS
-		 * downclock feature.
-		*/
-		*has_reduced_clock =
-			dev_priv->display.find_dpll(limit, crtc_state,
-						    dev_priv->lvds_downclock,
-						    refclk, clock,
-						    reduced_clock);
-	}
-
 	return true;
 }
 
@@ -10730,42 +10696,6 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
 	return mode;
 }
 
-static void intel_decrease_pllclock(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
-	if (!HAS_GMCH_DISPLAY(dev))
-		return;
-
-	if (!dev_priv->lvds_downclock_avail)
-		return;
-
-	/*
-	 * Since this is called by a timer, we should never get here in
-	 * the manual case.
-	 */
-	if (!HAS_PIPE_CXSR(dev) && intel_crtc->lowfreq_avail) {
-		int pipe = intel_crtc->pipe;
-		int dpll_reg = DPLL(pipe);
-		int dpll;
-
-		DRM_DEBUG_DRIVER("downclocking LVDS\n");
-
-		assert_panel_unlocked(dev_priv, pipe);
-
-		dpll = I915_READ(dpll_reg);
-		dpll |= DISPLAY_RATE_SELECT_FPA1;
-		I915_WRITE(dpll_reg, dpll);
-		intel_wait_for_vblank(dev, pipe);
-		dpll = I915_READ(dpll_reg);
-		if (!(dpll & DISPLAY_RATE_SELECT_FPA1))
-			DRM_DEBUG_DRIVER("failed to downclock LVDS!\n");
-	}
-
-}
-
 void intel_mark_busy(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -10783,20 +10713,12 @@ void intel_mark_busy(struct drm_device *dev)
 void intel_mark_idle(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc;
 
 	if (!dev_priv->mm.busy)
 		return;
 
 	dev_priv->mm.busy = false;
 
-	for_each_crtc(dev, crtc) {
-		if (!crtc->primary->fb)
-			continue;
-
-		intel_decrease_pllclock(crtc);
-	}
-
 	if (INTEL_INFO(dev)->gen >= 6)
 		gen6_rps_idle(dev->dev_private);
 
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 899b6112bbe0..3c4862c8629d 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -65,59 +65,6 @@
 #include "intel_drv.h"
 #include "i915_drv.h"
 
-static void intel_increase_pllclock(struct drm_device *dev,
-				    enum pipe pipe)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int dpll_reg = DPLL(pipe);
-	int dpll;
-
-	if (!HAS_GMCH_DISPLAY(dev))
-		return;
-
-	if (!dev_priv->lvds_downclock_avail)
-		return;
-
-	dpll = I915_READ(dpll_reg);
-	if (!HAS_PIPE_CXSR(dev) && (dpll & DISPLAY_RATE_SELECT_FPA1)) {
-		DRM_DEBUG_DRIVER("upclocking LVDS\n");
-
-		assert_panel_unlocked(dev_priv, pipe);
-
-		dpll &= ~DISPLAY_RATE_SELECT_FPA1;
-		I915_WRITE(dpll_reg, dpll);
-		intel_wait_for_vblank(dev, pipe);
-
-		dpll = I915_READ(dpll_reg);
-		if (dpll & DISPLAY_RATE_SELECT_FPA1)
-			DRM_DEBUG_DRIVER("failed to upclock LVDS!\n");
-	}
-}
-
-/**
- * intel_mark_fb_busy - mark given planes as busy
- * @dev: DRM device
- * @frontbuffer_bits: bits for the affected planes
- * @ring: optional ring for asynchronous commands
- *
- * This function gets called every time the screen contents change. It can be
- * used to keep e.g. the update rate at the nominal refresh rate with DRRS.
- */
-static void intel_mark_fb_busy(struct drm_device *dev,
-			       unsigned frontbuffer_bits,
-			       struct intel_engine_cs *ring)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum pipe pipe;
-
-	for_each_pipe(dev_priv, pipe) {
-		if (!(frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)))
-			continue;
-
-		intel_increase_pllclock(dev, pipe);
-	}
-}
-
 /**
  * intel_fb_obj_invalidate - invalidate frontbuffer object
  * @obj: GEM object to invalidate
@@ -151,8 +98,6 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 		mutex_unlock(&dev_priv->fb_tracking.lock);
 	}
 
-	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
-
 	intel_psr_invalidate(dev, obj->frontbuffer_bits);
 	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
 	intel_fbc_invalidate(dev_priv, obj->frontbuffer_bits, origin);
@@ -182,8 +127,6 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 	if (!frontbuffer_bits)
 		return;
 
-	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
-
 	intel_edp_drrs_flush(dev, frontbuffer_bits);
 	intel_psr_flush(dev, frontbuffer_bits);
 	intel_fbc_flush(dev_priv, frontbuffer_bits);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 161ab26f81fb..61dff9e8ba32 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1068,24 +1068,8 @@ void intel_lvds_init(struct drm_device *dev)
 			drm_mode_debug_printmodeline(scan);
 
 			fixed_mode = drm_mode_duplicate(dev, scan);
-			if (fixed_mode) {
-				downclock_mode =
-					intel_find_panel_downclock(dev,
-					fixed_mode, connector);
-				if (downclock_mode != NULL &&
-					i915.lvds_downclock) {
-					/* We found the downclock for LVDS. */
-					dev_priv->lvds_downclock_avail = true;
-					dev_priv->lvds_downclock =
-						downclock_mode->clock;
-					DRM_DEBUG_KMS("LVDS downclock is found"
-					" in EDID. Normal clock %dKhz, "
-					"downclock %dKhz\n",
-					fixed_mode->clock,
-					dev_priv->lvds_downclock);
-				}
+			if (fixed_mode)
 				goto out;
-			}
 		}
 	}
 
-- 
2.1.0

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

Re: [PATCH 4/9] drm/i915: Nuke lvds downclock support

[Chris Wilson]

On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote:
> With the new DRRS code it kinda sticks out, and we never managed to
> get this to work well enough without causing issues. Time to wave
> goodbye.
> 
> I've decided to keep the logic for programming the reduced clocks
> intact, but everything else is gone. If anyone ever wants to resurrect
> this we need to redo it all anyway on top of the frontbuffer tracking.

Can you nuke just the intel_mark_busy side? Keeping the mode finder
intact would be useful as the intrepid reader need only then implement
the intel_frontbuffer callbacks and have the harder part of matching
appropriate modes and switching routines ready to plug in. (Those latter
ones I expect to be tweaked over time, and so the reader's first step of
reverting this commit would conflict in such a way as to dissuade them.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Nuke lvds downclock support

[Daniel Vetter]

On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote:
> On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote:
> > With the new DRRS code it kinda sticks out, and we never managed to
> > get this to work well enough without causing issues. Time to wave
> > goodbye.
> > 
> > I've decided to keep the logic for programming the reduced clocks
> > intact, but everything else is gone. If anyone ever wants to resurrect
> > this we need to redo it all anyway on top of the frontbuffer tracking.
> 
> Can you nuke just the intel_mark_busy side? Keeping the mode finder
> intact would be useful as the intrepid reader need only then implement
> the intel_frontbuffer callbacks and have the harder part of matching
> appropriate modes and switching routines ready to plug in. (Those latter
> ones I expect to be tweaked over time, and so the reader's first step of
> reverting this commit would conflict in such a way as to dissuade them.)

Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured
getting rid of that is good - it really should be stored somewhere in
intel_lvds or in the pipe_config. Also given that no one ever really
bothered to fix this up since gen5 (where the bit to change frequency
moved around iirc) I don't think anyone will ever resurrect this. Hence
the much more eager delete.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Nuke lvds downclock support

[Chris Wilson]

On Thu, Jun 18, 2015 at 11:23:17AM +0200, Daniel Vetter wrote:
> On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote:
> > On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote:
> > > With the new DRRS code it kinda sticks out, and we never managed to
> > > get this to work well enough without causing issues. Time to wave
> > > goodbye.
> > > 
> > > I've decided to keep the logic for programming the reduced clocks
> > > intact, but everything else is gone. If anyone ever wants to resurrect
> > > this we need to redo it all anyway on top of the frontbuffer tracking.
> > 
> > Can you nuke just the intel_mark_busy side? Keeping the mode finder
> > intact would be useful as the intrepid reader need only then implement
> > the intel_frontbuffer callbacks and have the harder part of matching
> > appropriate modes and switching routines ready to plug in. (Those latter
> > ones I expect to be tweaked over time, and so the reader's first step of
> > reverting this commit would conflict in such a way as to dissuade them.)
> 
> Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured
> getting rid of that is good - it really should be stored somewhere in
> intel_lvds or in the pipe_config. Also given that no one ever really
> bothered to fix this up since gen5 (where the bit to change frequency
> moved around iirc) I don't think anyone will ever resurrect this. Hence
> the much more eager delete.

*shrug* we know that people do try to use it, I was just thinking of a
way that we could make it easier for them to refresh the code. (Ideally,
I would prefer that they wouldn't have to and the new framework provided
the impetus needed to solidify LVDS reclocking. All that was lacking was
the vblank task to do the reclocking on the refresh to hide any flicker
on transition. That and so few manufacturers supplied dual-mode panels.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Nuke lvds downclock support

[Daniel Vetter]

On Thu, Jun 18, 2015 at 10:30:36AM +0100, Chris Wilson wrote:
> On Thu, Jun 18, 2015 at 11:23:17AM +0200, Daniel Vetter wrote:
> > On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote:
> > > On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote:
> > > > With the new DRRS code it kinda sticks out, and we never managed to
> > > > get this to work well enough without causing issues. Time to wave
> > > > goodbye.
> > > > 
> > > > I've decided to keep the logic for programming the reduced clocks
> > > > intact, but everything else is gone. If anyone ever wants to resurrect
> > > > this we need to redo it all anyway on top of the frontbuffer tracking.
> > > 
> > > Can you nuke just the intel_mark_busy side? Keeping the mode finder
> > > intact would be useful as the intrepid reader need only then implement
> > > the intel_frontbuffer callbacks and have the harder part of matching
> > > appropriate modes and switching routines ready to plug in. (Those latter
> > > ones I expect to be tweaked over time, and so the reader's first step of
> > > reverting this commit would conflict in such a way as to dissuade them.)
> > 
> > Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured
> > getting rid of that is good - it really should be stored somewhere in
> > intel_lvds or in the pipe_config. Also given that no one ever really
> > bothered to fix this up since gen5 (where the bit to change frequency
> > moved around iirc) I don't think anyone will ever resurrect this. Hence
> > the much more eager delete.
> 
> *shrug* we know that people do try to use it, I was just thinking of a
> way that we could make it easier for them to refresh the code. (Ideally,
> I would prefer that they wouldn't have to and the new framework provided
> the impetus needed to solidify LVDS reclocking. All that was lacking was
> the vblank task to do the reclocking on the refresh to hide any flicker
> on transition. That and so few manufacturers supplied dual-mode panels.)

Not sure users trying to use it is a good argument - they enthusiastically
try it on gen5+ too (where we don't frob the right pipe bits to make the
frequency switch) and on machines without lvds panels. I expect a dummy
i915.save_me_some_power option would get enabled too ;-)

Btw for the vblank work I think the crucial bit is that we want seamless
DRRS (which edp drrs checks in the vbt) and lvds drrs never did check
that. At least that might explain why it flickered often badly.

Given that ack or nack?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/9] drm/i915: Nuke lvds downclock support

[Chris Wilson]

On Tue, Jun 23, 2015 at 11:18:05PM +0200, Daniel Vetter wrote:
> On Thu, Jun 18, 2015 at 10:30:36AM +0100, Chris Wilson wrote:
> > On Thu, Jun 18, 2015 at 11:23:17AM +0200, Daniel Vetter wrote:
> > > On Thu, Jun 18, 2015 at 10:00:51AM +0100, Chris Wilson wrote:
> > > > On Thu, Jun 18, 2015 at 10:30:23AM +0200, Daniel Vetter wrote:
> > > > > With the new DRRS code it kinda sticks out, and we never managed to
> > > > > get this to work well enough without causing issues. Time to wave
> > > > > goodbye.
> > > > > 
> > > > > I've decided to keep the logic for programming the reduced clocks
> > > > > intact, but everything else is gone. If anyone ever wants to resurrect
> > > > > this we need to redo it all anyway on top of the frontbuffer tracking.
> > > > 
> > > > Can you nuke just the intel_mark_busy side? Keeping the mode finder
> > > > intact would be useful as the intrepid reader need only then implement
> > > > the intel_frontbuffer callbacks and have the harder part of matching
> > > > appropriate modes and switching routines ready to plug in. (Those latter
> > > > ones I expect to be tweaked over time, and so the reader's first step of
> > > > reverting this commit would conflict in such a way as to dissuade them.)
> > > 
> > > Well I was also somewhat annoyed by the dev_priv->lvds_* stuff and figured
> > > getting rid of that is good - it really should be stored somewhere in
> > > intel_lvds or in the pipe_config. Also given that no one ever really
> > > bothered to fix this up since gen5 (where the bit to change frequency
> > > moved around iirc) I don't think anyone will ever resurrect this. Hence
> > > the much more eager delete.
> > 
> > *shrug* we know that people do try to use it, I was just thinking of a
> > way that we could make it easier for them to refresh the code. (Ideally,
> > I would prefer that they wouldn't have to and the new framework provided
> > the impetus needed to solidify LVDS reclocking. All that was lacking was
> > the vblank task to do the reclocking on the refresh to hide any flicker
> > on transition. That and so few manufacturers supplied dual-mode panels.)
> 
> Not sure users trying to use it is a good argument - they enthusiastically
> try it on gen5+ too (where we don't frob the right pipe bits to make the
> frequency switch) and on machines without lvds panels. I expect a dummy
> i915.save_me_some_power option would get enabled too ;-)
> 
> Btw for the vblank work I think the crucial bit is that we want seamless
> DRRS (which edp drrs checks in the vbt) and lvds drrs never did check
> that. At least that might explain why it flickered often badly.
> 
> Given that ack or nack?

Given that it has been buggy forever, and removes some interloping code,
ack.

I'm just disappointed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9] drm/i915: s/update/compute/ for gmch dpll register functions

[Daniel Vetter]

I was momentarily confused until I've double-checked that these
functions really only compute state and don't update the hardware
state. They once did that, but since Ander's rework of the dpll
computation flow that's no longer the case.

Rename them to avoid further confusion.

Note that the ilk code already follows the compute_dpll naming scheme
for computing the actual register value. DDI code goes with _calc_,
but that is close enough.

Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 45a93c87d48e..a8d9dbd5bd34 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7233,8 +7233,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
 		intel_cpu_transcoder_set_m_n(crtc, dp_m_n, dp_m2_n2);
 }
 
-static void vlv_update_pll(struct intel_crtc *crtc,
-			   struct intel_crtc_state *pipe_config)
+static void vlv_compute_dpll(struct intel_crtc *crtc,
+			     struct intel_crtc_state *pipe_config)
 {
 	u32 dpll, dpll_md;
 
@@ -7347,8 +7347,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
 	mutex_unlock(&dev_priv->sb_lock);
 }
 
-static void chv_update_pll(struct intel_crtc *crtc,
-			   struct intel_crtc_state *pipe_config)
+static void chv_compute_dpll(struct intel_crtc *crtc,
+			     struct intel_crtc_state *pipe_config)
 {
 	pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
 		DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
@@ -7487,11 +7487,11 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
 	};
 
 	if (IS_CHERRYVIEW(dev)) {
-		chv_update_pll(crtc, &pipe_config);
+		chv_compute_dpll(crtc, &pipe_config);
 		chv_prepare_pll(crtc, &pipe_config);
 		chv_enable_pll(crtc, &pipe_config);
 	} else {
-		vlv_update_pll(crtc, &pipe_config);
+		vlv_compute_dpll(crtc, &pipe_config);
 		vlv_prepare_pll(crtc, &pipe_config);
 		vlv_enable_pll(crtc, &pipe_config);
 	}
@@ -7513,10 +7513,10 @@ void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe)
 		vlv_disable_pll(to_i915(dev), pipe);
 }
 
-static void i9xx_update_pll(struct intel_crtc *crtc,
-			    struct intel_crtc_state *crtc_state,
-			    intel_clock_t *reduced_clock,
-			    int num_connectors)
+static void i9xx_compute_dpll(struct intel_crtc *crtc,
+			      struct intel_crtc_state *crtc_state,
+			      intel_clock_t *reduced_clock,
+			      int num_connectors)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -7590,10 +7590,10 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
 	}
 }
 
-static void i8xx_update_pll(struct intel_crtc *crtc,
-			    struct intel_crtc_state *crtc_state,
-			    intel_clock_t *reduced_clock,
-			    int num_connectors)
+static void i8xx_compute_dpll(struct intel_crtc *crtc,
+			      struct intel_crtc_state *crtc_state,
+			      intel_clock_t *reduced_clock,
+			      int num_connectors)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -7887,15 +7887,15 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
 	}
 
 	if (IS_GEN2(dev)) {
-		i8xx_update_pll(crtc, crtc_state, NULL,
-				num_connectors);
+		i8xx_compute_dpll(crtc, crtc_state, NULL,
+				  num_connectors);
 	} else if (IS_CHERRYVIEW(dev)) {
-		chv_update_pll(crtc, crtc_state);
+		chv_compute_dpll(crtc, crtc_state);
 	} else if (IS_VALLEYVIEW(dev)) {
-		vlv_update_pll(crtc, crtc_state);
+		vlv_compute_dpll(crtc, crtc_state);
 	} else {
-		i9xx_update_pll(crtc, crtc_state, NULL,
-				num_connectors);
+		i9xx_compute_dpll(crtc, crtc_state, NULL,
+				  num_connectors);
 	}
 
 	return 0;
-- 
2.1.0

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

Re: [PATCH 5/9] drm/i915: s/update/compute/ for gmch dpll register functions

[Paulo Zanoni]

2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> I was momentarily confused until I've double-checked that these
> functions really only compute state and don't update the hardware
> state. They once did that, but since Ander's rework of the dpll
> computation flow that's no longer the case.
>
> Rename them to avoid further confusion.
>
> Note that the ilk code already follows the compute_dpll naming scheme
> for computing the actual register value. DDI code goes with _calc_,
> but that is close enough.

My only bikeshed would be to point that we now have x_prepare_pll(),
x_enable_pll() and x_compute_dpll(), with the "d" to break the
standard. OTOH the ILK function also has the "d" letter, so meh...

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 45a93c87d48e..a8d9dbd5bd34 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7233,8 +7233,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
>                 intel_cpu_transcoder_set_m_n(crtc, dp_m_n, dp_m2_n2);
>  }
>
> -static void vlv_update_pll(struct intel_crtc *crtc,
> -                          struct intel_crtc_state *pipe_config)
> +static void vlv_compute_dpll(struct intel_crtc *crtc,
> +                            struct intel_crtc_state *pipe_config)
>  {
>         u32 dpll, dpll_md;
>
> @@ -7347,8 +7347,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
>         mutex_unlock(&dev_priv->sb_lock);
>  }
>
> -static void chv_update_pll(struct intel_crtc *crtc,
> -                          struct intel_crtc_state *pipe_config)
> +static void chv_compute_dpll(struct intel_crtc *crtc,
> +                            struct intel_crtc_state *pipe_config)
>  {
>         pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
>                 DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> @@ -7487,11 +7487,11 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
>         };
>
>         if (IS_CHERRYVIEW(dev)) {
> -               chv_update_pll(crtc, &pipe_config);
> +               chv_compute_dpll(crtc, &pipe_config);
>                 chv_prepare_pll(crtc, &pipe_config);
>                 chv_enable_pll(crtc, &pipe_config);
>         } else {
> -               vlv_update_pll(crtc, &pipe_config);
> +               vlv_compute_dpll(crtc, &pipe_config);
>                 vlv_prepare_pll(crtc, &pipe_config);
>                 vlv_enable_pll(crtc, &pipe_config);
>         }
> @@ -7513,10 +7513,10 @@ void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe)
>                 vlv_disable_pll(to_i915(dev), pipe);
>  }
>
> -static void i9xx_update_pll(struct intel_crtc *crtc,
> -                           struct intel_crtc_state *crtc_state,
> -                           intel_clock_t *reduced_clock,
> -                           int num_connectors)
> +static void i9xx_compute_dpll(struct intel_crtc *crtc,
> +                             struct intel_crtc_state *crtc_state,
> +                             intel_clock_t *reduced_clock,
> +                             int num_connectors)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -7590,10 +7590,10 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>         }
>  }
>
> -static void i8xx_update_pll(struct intel_crtc *crtc,
> -                           struct intel_crtc_state *crtc_state,
> -                           intel_clock_t *reduced_clock,
> -                           int num_connectors)
> +static void i8xx_compute_dpll(struct intel_crtc *crtc,
> +                             struct intel_crtc_state *crtc_state,
> +                             intel_clock_t *reduced_clock,
> +                             int num_connectors)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -7887,15 +7887,15 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
>         }
>
>         if (IS_GEN2(dev)) {
> -               i8xx_update_pll(crtc, crtc_state, NULL,
> -                               num_connectors);
> +               i8xx_compute_dpll(crtc, crtc_state, NULL,
> +                                 num_connectors);
>         } else if (IS_CHERRYVIEW(dev)) {
> -               chv_update_pll(crtc, crtc_state);
> +               chv_compute_dpll(crtc, crtc_state);
>         } else if (IS_VALLEYVIEW(dev)) {
> -               vlv_update_pll(crtc, crtc_state);
> +               vlv_compute_dpll(crtc, crtc_state);
>         } else {
> -               i9xx_update_pll(crtc, crtc_state, NULL,
> -                               num_connectors);
> +               i9xx_compute_dpll(crtc, crtc_state, NULL,
> +                                 num_connectors);
>         }
>
>         return 0;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 5/9] drm/i915: s/update/compute/ for gmch dpll register functions

[Daniel Vetter]

On Tue, Jun 23, 2015 at 05:26:40PM -0300, Paulo Zanoni wrote:
> 2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > I was momentarily confused until I've double-checked that these
> > functions really only compute state and don't update the hardware
> > state. They once did that, but since Ander's rework of the dpll
> > computation flow that's no longer the case.
> >
> > Rename them to avoid further confusion.
> >
> > Note that the ilk code already follows the compute_dpll naming scheme
> > for computing the actual register value. DDI code goes with _calc_,
> > but that is close enough.
> 
> My only bikeshed would be to point that we now have x_prepare_pll(),
> x_enable_pll() and x_compute_dpll(), with the "d" to break the
> standard. OTOH the ILK function also has the "d" letter, so meh...

Thanks for your review. I fixed up patch 1, pinged Chris again on the lvds
drrs removal and merged all the others.
-Daniel

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> >
> > Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++++++------------------
> >  1 file changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 45a93c87d48e..a8d9dbd5bd34 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -7233,8 +7233,8 @@ void intel_dp_set_m_n(struct intel_crtc *crtc, enum link_m_n_set m_n)
> >                 intel_cpu_transcoder_set_m_n(crtc, dp_m_n, dp_m2_n2);
> >  }
> >
> > -static void vlv_update_pll(struct intel_crtc *crtc,
> > -                          struct intel_crtc_state *pipe_config)
> > +static void vlv_compute_dpll(struct intel_crtc *crtc,
> > +                            struct intel_crtc_state *pipe_config)
> >  {
> >         u32 dpll, dpll_md;
> >
> > @@ -7347,8 +7347,8 @@ static void vlv_prepare_pll(struct intel_crtc *crtc,
> >         mutex_unlock(&dev_priv->sb_lock);
> >  }
> >
> > -static void chv_update_pll(struct intel_crtc *crtc,
> > -                          struct intel_crtc_state *pipe_config)
> > +static void chv_compute_dpll(struct intel_crtc *crtc,
> > +                            struct intel_crtc_state *pipe_config)
> >  {
> >         pipe_config->dpll_hw_state.dpll = DPLL_SSC_REF_CLOCK_CHV |
> >                 DPLL_REFA_CLK_ENABLE_VLV | DPLL_VGA_MODE_DIS |
> > @@ -7487,11 +7487,11 @@ void vlv_force_pll_on(struct drm_device *dev, enum pipe pipe,
> >         };
> >
> >         if (IS_CHERRYVIEW(dev)) {
> > -               chv_update_pll(crtc, &pipe_config);
> > +               chv_compute_dpll(crtc, &pipe_config);
> >                 chv_prepare_pll(crtc, &pipe_config);
> >                 chv_enable_pll(crtc, &pipe_config);
> >         } else {
> > -               vlv_update_pll(crtc, &pipe_config);
> > +               vlv_compute_dpll(crtc, &pipe_config);
> >                 vlv_prepare_pll(crtc, &pipe_config);
> >                 vlv_enable_pll(crtc, &pipe_config);
> >         }
> > @@ -7513,10 +7513,10 @@ void vlv_force_pll_off(struct drm_device *dev, enum pipe pipe)
> >                 vlv_disable_pll(to_i915(dev), pipe);
> >  }
> >
> > -static void i9xx_update_pll(struct intel_crtc *crtc,
> > -                           struct intel_crtc_state *crtc_state,
> > -                           intel_clock_t *reduced_clock,
> > -                           int num_connectors)
> > +static void i9xx_compute_dpll(struct intel_crtc *crtc,
> > +                             struct intel_crtc_state *crtc_state,
> > +                             intel_clock_t *reduced_clock,
> > +                             int num_connectors)
> >  {
> >         struct drm_device *dev = crtc->base.dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -7590,10 +7590,10 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
> >         }
> >  }
> >
> > -static void i8xx_update_pll(struct intel_crtc *crtc,
> > -                           struct intel_crtc_state *crtc_state,
> > -                           intel_clock_t *reduced_clock,
> > -                           int num_connectors)
> > +static void i8xx_compute_dpll(struct intel_crtc *crtc,
> > +                             struct intel_crtc_state *crtc_state,
> > +                             intel_clock_t *reduced_clock,
> > +                             int num_connectors)
> >  {
> >         struct drm_device *dev = crtc->base.dev;
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -7887,15 +7887,15 @@ static int i9xx_crtc_compute_clock(struct intel_crtc *crtc,
> >         }
> >
> >         if (IS_GEN2(dev)) {
> > -               i8xx_update_pll(crtc, crtc_state, NULL,
> > -                               num_connectors);
> > +               i8xx_compute_dpll(crtc, crtc_state, NULL,
> > +                                 num_connectors);
> >         } else if (IS_CHERRYVIEW(dev)) {
> > -               chv_update_pll(crtc, crtc_state);
> > +               chv_compute_dpll(crtc, crtc_state);
> >         } else if (IS_VALLEYVIEW(dev)) {
> > -               vlv_update_pll(crtc, crtc_state);
> > +               vlv_compute_dpll(crtc, crtc_state);
> >         } else {
> > -               i9xx_update_pll(crtc, crtc_state, NULL,
> > -                               num_connectors);
> > +               i9xx_compute_dpll(crtc, crtc_state, NULL,
> > +                                 num_connectors);
> >         }
> >
> >         return 0;
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/9] drm/i915/drrs: Restrict buffer tracking to the DRRS pipe

[Daniel Vetter]

The current code tracks business across all pipes, but we're only
really interested in the one pipe DRRS is enabled on. Fairly tiny
optimization, but something I noticed while reading the code. But it
might matter a bit when e.g. showing a video or something only on the
external screen, while the panel is kept static.

Also regroup the code slightly: First compute new bitmasks, then take
appropriate actions.

Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f52eef138247..3e8c88d119bb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5485,16 +5485,15 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
+	dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
+
 	/* invalidate means busy screen hence upclock */
-	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
+	if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
 		intel_dp_set_drrs_state(dev_priv->dev,
 				dev_priv->drrs.dp->attached_connector->panel.
 				fixed_mode->vrefresh);
-	}
-
-	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 
-	dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
@@ -5530,10 +5529,12 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
+
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
 	/* flush means busy screen hence upclock */
-	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
+	if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
 		intel_dp_set_drrs_state(dev_priv->dev,
 				dev_priv->drrs.dp->attached_connector->panel.
 				fixed_mode->vrefresh);
-- 
2.1.0

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

Re: [PATCH 6/9] drm/i915/drrs: Restrict buffer tracking to the DRRS pipe

[Paulo Zanoni]

2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The current code tracks business across all pipes, but we're only
> really interested in the one pipe DRRS is enabled on. Fairly tiny
> optimization, but something I noticed while reading the code. But it
> might matter a bit when e.g. showing a video or something only on the
> external screen, while the panel is kept static.
>
> Also regroup the code slightly: First compute new bitmasks, then take
> appropriate actions.
>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

PS: with this, everything except P4 has a R-B tag (but P1 has a change
request). I'll let you and Chris decide what to do on P4.

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f52eef138247..3e8c88d119bb 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5485,16 +5485,15 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>         crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
>
> +       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> +       dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
> +
>         /* invalidate means busy screen hence upclock */
> -       if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
> +       if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
>                 intel_dp_set_drrs_state(dev_priv->dev,
>                                 dev_priv->drrs.dp->attached_connector->panel.
>                                 fixed_mode->vrefresh);
> -       }
> -
> -       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>
> -       dev_priv->drrs.busy_frontbuffer_bits |= frontbuffer_bits;
>         mutex_unlock(&dev_priv->drrs.mutex);
>  }
>
> @@ -5530,10 +5529,12 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>
>         crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
> +
> +       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>         dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
>
>         /* flush means busy screen hence upclock */
> -       if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
> +       if (frontbuffer_bits && dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
>                 intel_dp_set_drrs_state(dev_priv->dev,
>                                 dev_priv->drrs.dp->attached_connector->panel.
>                                 fixed_mode->vrefresh);
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

[PATCH 7/9] drm/i915/psr: Restrict buffer tracking to the PSR pipe

[Daniel Vetter]

The current code tracks business across all pipes, but we're only
really interested in the one pipe DRRS is enabled on. Fairly tiny
optimization, but something I noticed while reading the code. But it
might matter a bit when e.g. showing a video or something only on the
external screen, while the panel is kept static.

Also regroup the code slightly: First compute new bitmasks, then take
appropriate actions.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5ee0fa57ed19..e354ceacb628 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -663,11 +663,12 @@ void intel_psr_invalidate(struct drm_device *dev,
 	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
-	intel_psr_exit(dev);
-
 	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
-
 	dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
+
+	if (frontbuffer_bits)
+		intel_psr_exit(dev);
+
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -698,6 +699,8 @@ void intel_psr_flush(struct drm_device *dev,
 
 	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
+
+	frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
 	dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
 
 	/*
@@ -716,7 +719,7 @@ void intel_psr_flush(struct drm_device *dev,
 	 * invalidating. Which means we need to manually fake this in
 	 * software for all flushes, not just when we've seen a preceding
 	 * invalidation through frontbuffer rendering. */
-	if (!HAS_DDI(dev))
+	if (frontbuffer_bits && !HAS_DDI(dev))
 		intel_psr_exit(dev);
 
 	if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
-- 
2.1.0

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

Re: [PATCH 7/9] drm/i915/psr: Restrict buffer tracking to the PSR pipe

[Paulo Zanoni]

2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The current code tracks business across all pipes, but we're only
> really interested in the one pipe DRRS is enabled on. Fairly tiny

s/DRRS/PSR/

> optimization, but something I noticed while reading the code. But it
> might matter a bit when e.g. showing a video or something only on the
> external screen, while the panel is kept static.
>
> Also regroup the code slightly: First compute new bitmasks, then take
> appropriate actions.

One of the ideas I had last year was to implement a way to tell user
space (possibly through debugs) when FBC/PSR gets enabled/disabled. My
initial idea was to do this through timestamps. Our IGT suite would
then verify those timestamps: if we have 2 pipes enabled, then we
write on the non-FBC buffer, the FBC timestamps can't change. This
type of test would catch exactly the bug you're solving here.

I even implemented this, and added support for it on
kms_frontbuffer_tracking, but I ended never sending the Kernel patch
to the list due to the possible slowness created by the constant
timestamp generation. Maybe I should resurrect this and hide it under
some debug kconfig option. I also considered maybe moving the
implementation from timestamps debugfs file events, or maybe tracing
code, or something else. Ideas and bikesheds are welcome here.

So I added some debug messages to the PSR code, then I ran:
sudo ./kms_frontbuffer_tracking --run-subtest
psr-2p-scndscrn-pri-sfb-draw-mmap-cpu --step --step

And I kept watching dmesg as I stepped. Without the patch, I could see
psr being disabled/enabled as we were writing on the secondary screen
frontbuffer. With the patch, we stop calling intel_psr_exit()! So:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5ee0fa57ed19..e354ceacb628 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -663,11 +663,12 @@ void intel_psr_invalidate(struct drm_device *dev,
>         crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
>
> -       intel_psr_exit(dev);
> -
>         frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
> -
>         dev_priv->psr.busy_frontbuffer_bits |= frontbuffer_bits;
> +
> +       if (frontbuffer_bits)
> +               intel_psr_exit(dev);
> +
>         mutex_unlock(&dev_priv->psr.lock);
>  }
>
> @@ -698,6 +699,8 @@ void intel_psr_flush(struct drm_device *dev,
>
>         crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
> +
> +       frontbuffer_bits &= INTEL_FRONTBUFFER_ALL_MASK(pipe);
>         dev_priv->psr.busy_frontbuffer_bits &= ~frontbuffer_bits;
>
>         /*
> @@ -716,7 +719,7 @@ void intel_psr_flush(struct drm_device *dev,
>          * invalidating. Which means we need to manually fake this in
>          * software for all flushes, not just when we've seen a preceding
>          * invalidation through frontbuffer rendering. */
> -       if (!HAS_DDI(dev))
> +       if (frontbuffer_bits && !HAS_DDI(dev))
>                 intel_psr_exit(dev);
>
>         if (!dev_priv->psr.active && !dev_priv->psr.busy_frontbuffer_bits)
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 7/9] drm/i915/psr: Restrict buffer tracking to the PSR pipe

[Daniel Vetter]

On Tue, Jun 23, 2015 at 04:57:26PM -0300, Paulo Zanoni wrote:
> 2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > The current code tracks business across all pipes, but we're only
> > really interested in the one pipe DRRS is enabled on. Fairly tiny
> 
> s/DRRS/PSR/
> 
> > optimization, but something I noticed while reading the code. But it
> > might matter a bit when e.g. showing a video or something only on the
> > external screen, while the panel is kept static.
> >
> > Also regroup the code slightly: First compute new bitmasks, then take
> > appropriate actions.
> 
> One of the ideas I had last year was to implement a way to tell user
> space (possibly through debugs) when FBC/PSR gets enabled/disabled. My
> initial idea was to do this through timestamps. Our IGT suite would
> then verify those timestamps: if we have 2 pipes enabled, then we
> write on the non-FBC buffer, the FBC timestamps can't change. This
> type of test would catch exactly the bug you're solving here.
> 
> I even implemented this, and added support for it on
> kms_frontbuffer_tracking, but I ended never sending the Kernel patch
> to the list due to the possible slowness created by the constant
> timestamp generation. Maybe I should resurrect this and hide it under
> some debug kconfig option. I also considered maybe moving the
> implementation from timestamps debugfs file events, or maybe tracing
> code, or something else. Ideas and bikesheds are welcome here.
> 
> So I added some debug messages to the PSR code, then I ran:
> sudo ./kms_frontbuffer_tracking --run-subtest
> psr-2p-scndscrn-pri-sfb-draw-mmap-cpu --step --step
> 
> And I kept watching dmesg as I stepped. Without the patch, I could see
> psr being disabled/enabled as we were writing on the secondary screen
> frontbuffer. With the patch, we stop calling intel_psr_exit()! So:

I think there's two cases really: Adding tracepoints to watch the psr or
frontbuffer code could certainly be useful.

For testcases otoh I'm not in favour of coupling them tightly with the
current kernel implementation. If we only check behavior (like residency
or captured CRCs) we can change the kernel implementation (like e.g.
switch away from hw tracking or the other way round) without any changes
to testcases. If we couple tests tightly with such tracepoints/timestamps
then we might accidentally break the testcase while changing it.

And since upstream is really big about never breaking ABI (which means
observable behaviour, not never changing how it's implemented) I think
doing blackbox testcase is a big plus.

But if it's really not possibel to validate a feature without
introspection then I'm ok with adding tracepoints/ts or similar things.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/9] drm/i915/psr: Restrict single-shot updates to the PSR pipe

[Daniel Vetter]

The frontbuffer code gives us accurate information about activity,
let's use it. Again this should avoid unecessary updates when multiple
screens are on.

Also realing function paramaters, I couldn't resist that bit of OCD.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h         |  7 ++++---
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_psr.c         | 22 +++++++++++++---------
 3 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b7c69460fb20..0ba9065dec88 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1310,11 +1310,12 @@ void intel_backlight_unregister(struct drm_device *dev);
 void intel_psr_enable(struct intel_dp *intel_dp);
 void intel_psr_disable(struct intel_dp *intel_dp);
 void intel_psr_invalidate(struct drm_device *dev,
-			      unsigned frontbuffer_bits);
+			  unsigned frontbuffer_bits);
 void intel_psr_flush(struct drm_device *dev,
-			 unsigned frontbuffer_bits);
+		     unsigned frontbuffer_bits);
 void intel_psr_init(struct drm_device *dev);
-void intel_psr_single_frame_update(struct drm_device *dev);
+void intel_psr_single_frame_update(struct drm_device *dev,
+				   unsigned frontbuffer_bits);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 3c4862c8629d..ede6ccc45375 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -190,7 +190,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
-	intel_psr_single_frame_update(dev);
+	intel_psr_single_frame_update(dev, frontbuffer_bits);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index e354ceacb628..d79ba58637d7 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -596,13 +596,15 @@ static void intel_psr_exit(struct drm_device *dev)
 /**
  * intel_psr_single_frame_update - Single Frame Update
  * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
  *
  * Some platforms support a single frame update feature that is used to
  * send and update only one frame on Remote Frame Buffer.
  * So far it is only implemented for Valleyview and Cherryview because
  * hardware requires this to be done before a page flip.
  */
-void intel_psr_single_frame_update(struct drm_device *dev)
+void intel_psr_single_frame_update(struct drm_device *dev,
+				   unsigned frontbuffer_bits)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
@@ -624,14 +626,16 @@ void intel_psr_single_frame_update(struct drm_device *dev)
 
 	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
-	val = I915_READ(VLV_PSRCTL(pipe));
 
-	/*
-	 * We need to set this bit before writing registers for a flip.
-	 * This bit will be self-clear when it gets to the PSR active state.
-	 */
-	I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
+	if (frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)) {
+		val = I915_READ(VLV_PSRCTL(pipe));
 
+		/*
+		 * We need to set this bit before writing registers for a flip.
+		 * This bit will be self-clear when it gets to the PSR active state.
+		 */
+		I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
+	}
 	mutex_unlock(&dev_priv->psr.lock);
 }
 
@@ -648,7 +652,7 @@ void intel_psr_single_frame_update(struct drm_device *dev)
  * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits."
  */
 void intel_psr_invalidate(struct drm_device *dev,
-			      unsigned frontbuffer_bits)
+			  unsigned frontbuffer_bits)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
@@ -685,7 +689,7 @@ void intel_psr_invalidate(struct drm_device *dev,
  * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
  */
 void intel_psr_flush(struct drm_device *dev,
-			 unsigned frontbuffer_bits)
+		     unsigned frontbuffer_bits)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *crtc;
-- 
2.1.0

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

Re: [PATCH 8/9] drm/i915/psr: Restrict single-shot updates to the PSR pipe

[Chris Wilson]

On Thu, Jun 18, 2015 at 10:30:27AM +0200, Daniel Vetter wrote:
> The frontbuffer code gives us accurate information about activity,
> let's use it. Again this should avoid unecessary updates when multiple
> screens are on.
> 
> Also realing function paramaters, I couldn't resist that bit of OCD.

realign. I ocd'ed your ocd.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/9] drm/i915/psr: Restrict single-shot updates to the PSR pipe

[Paulo Zanoni]

2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The frontbuffer code gives us accurate information about activity,
> let's use it. Again this should avoid unecessary updates when multiple
> screens are on.
>
> Also realing function paramaters, I couldn't resist that bit of OCD.

Can't test this since -ENOHW. But it appears correct, so with and only
with the fix mentioned by Chris:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

This bug could also be verified by the idea I mentioned in my comment
to patch 7, if it was implemented.

Or we can also verify it by adding some debugfs messages at the
correct places of intel_psr_single_frame_update(), then run "sudo
./kms_frontbuffer_tracking --run-subtest psr-2p-scndscrn-sfb-flip-blt
--step --step" and watch the PSR single frame updates messages appear
on dmesg as we do flips on the non-PSR screen.

>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Durgadoss R <durgadoss.r@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h         |  7 ++++---
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c         | 22 +++++++++++++---------
>  3 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b7c69460fb20..0ba9065dec88 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1310,11 +1310,12 @@ void intel_backlight_unregister(struct drm_device *dev);
>  void intel_psr_enable(struct intel_dp *intel_dp);
>  void intel_psr_disable(struct intel_dp *intel_dp);
>  void intel_psr_invalidate(struct drm_device *dev,
> -                             unsigned frontbuffer_bits);
> +                         unsigned frontbuffer_bits);
>  void intel_psr_flush(struct drm_device *dev,
> -                        unsigned frontbuffer_bits);
> +                    unsigned frontbuffer_bits);
>  void intel_psr_init(struct drm_device *dev);
> -void intel_psr_single_frame_update(struct drm_device *dev);
> +void intel_psr_single_frame_update(struct drm_device *dev,
> +                                  unsigned frontbuffer_bits);
>
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 3c4862c8629d..ede6ccc45375 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -190,7 +190,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>         dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
>         mutex_unlock(&dev_priv->fb_tracking.lock);
>
> -       intel_psr_single_frame_update(dev);
> +       intel_psr_single_frame_update(dev, frontbuffer_bits);
>  }
>
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index e354ceacb628..d79ba58637d7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -596,13 +596,15 @@ static void intel_psr_exit(struct drm_device *dev)
>  /**
>   * intel_psr_single_frame_update - Single Frame Update
>   * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
>   *
>   * Some platforms support a single frame update feature that is used to
>   * send and update only one frame on Remote Frame Buffer.
>   * So far it is only implemented for Valleyview and Cherryview because
>   * hardware requires this to be done before a page flip.
>   */
> -void intel_psr_single_frame_update(struct drm_device *dev)
> +void intel_psr_single_frame_update(struct drm_device *dev,
> +                                  unsigned frontbuffer_bits)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct drm_crtc *crtc;
> @@ -624,14 +626,16 @@ void intel_psr_single_frame_update(struct drm_device *dev)
>
>         crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
>         pipe = to_intel_crtc(crtc)->pipe;
> -       val = I915_READ(VLV_PSRCTL(pipe));
>
> -       /*
> -        * We need to set this bit before writing registers for a flip.
> -        * This bit will be self-clear when it gets to the PSR active state.
> -        */
> -       I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
> +       if (frontbuffer_bits & INTEL_FRONTBUFFER_ALL_MASK(pipe)) {
> +               val = I915_READ(VLV_PSRCTL(pipe));
>
> +               /*
> +                * We need to set this bit before writing registers for a flip.
> +                * This bit will be self-clear when it gets to the PSR active state.
> +                */
> +               I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
> +       }
>         mutex_unlock(&dev_priv->psr.lock);
>  }
>
> @@ -648,7 +652,7 @@ void intel_psr_single_frame_update(struct drm_device *dev)
>   * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits."
>   */
>  void intel_psr_invalidate(struct drm_device *dev,
> -                             unsigned frontbuffer_bits)
> +                         unsigned frontbuffer_bits)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct drm_crtc *crtc;
> @@ -685,7 +689,7 @@ void intel_psr_invalidate(struct drm_device *dev,
>   * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits.
>   */
>  void intel_psr_flush(struct drm_device *dev,
> -                        unsigned frontbuffer_bits)
> +                    unsigned frontbuffer_bits)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct drm_crtc *crtc;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

[PATCH 9/9] drm/i915: Use to_i915 in intel_frontbuffer.c

[Daniel Vetter]

Must have missed the transition.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_frontbuffer.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index ede6ccc45375..0635ee004c29 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -82,7 +82,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 			     enum fb_op_origin origin)
 {
 	struct drm_device *dev = obj->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
@@ -117,7 +117,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 void intel_frontbuffer_flush(struct drm_device *dev,
 			     unsigned frontbuffer_bits)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	/* Delay flushing when rings are still busy.*/
 	mutex_lock(&dev_priv->fb_tracking.lock);
@@ -145,7 +145,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
 			bool retire)
 {
 	struct drm_device *dev = obj->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned frontbuffer_bits;
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
@@ -182,7 +182,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
 void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 				    unsigned frontbuffer_bits)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	mutex_lock(&dev_priv->fb_tracking.lock);
 	dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
@@ -206,7 +206,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	mutex_lock(&dev_priv->fb_tracking.lock);
 	/* Mask any cancelled flips. */
@@ -232,7 +232,7 @@ inline
 void intel_frontbuffer_flip(struct drm_device *dev,
 			    unsigned frontbuffer_bits)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	mutex_lock(&dev_priv->fb_tracking.lock);
 	dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
-- 
2.1.0

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

Re: [PATCH 9/9] drm/i915: Use to_i915 in intel_frontbuffer.c

[Paulo Zanoni]

2015-06-18 5:30 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Must have missed the transition.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index ede6ccc45375..0635ee004c29 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -82,7 +82,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>                              enum fb_op_origin origin)
>  {
>         struct drm_device *dev = obj->base.dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_i915_private *dev_priv = to_i915(dev);
>
>         WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> @@ -117,7 +117,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>  void intel_frontbuffer_flush(struct drm_device *dev,
>                              unsigned frontbuffer_bits)
>  {
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_i915_private *dev_priv = to_i915(dev);
>
>         /* Delay flushing when rings are still busy.*/
>         mutex_lock(&dev_priv->fb_tracking.lock);
> @@ -145,7 +145,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
>                         bool retire)
>  {
>         struct drm_device *dev = obj->base.dev;
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_i915_private *dev_priv = to_i915(dev);
>         unsigned frontbuffer_bits;
>
>         WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> @@ -182,7 +182,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
>  void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>                                     unsigned frontbuffer_bits)
>  {
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_i915_private *dev_priv = to_i915(dev);
>
>         mutex_lock(&dev_priv->fb_tracking.lock);
>         dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
> @@ -206,7 +206,7 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>  void intel_frontbuffer_flip_complete(struct drm_device *dev,
>                                      unsigned frontbuffer_bits)
>  {
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_i915_private *dev_priv = to_i915(dev);
>
>         mutex_lock(&dev_priv->fb_tracking.lock);
>         /* Mask any cancelled flips. */
> @@ -232,7 +232,7 @@ inline
>  void intel_frontbuffer_flip(struct drm_device *dev,
>                             unsigned frontbuffer_bits)
>  {
> -       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct drm_i915_private *dev_priv = to_i915(dev);
>
>         mutex_lock(&dev_priv->fb_tracking.lock);
>         dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 1/9] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[Ville Syrjälä]

On Thu, Jun 18, 2015 at 10:30:20AM +0200, Daniel Vetter wrote:
> The current/old frontbuffer might still have gpu frontbuffer rendering
> pending. But once flipped it won't have the corresponding frontbuffer
> bits any more and hence the request retire function won't ever clear
> the corresponding busy bits. The async flip tracking (with the
> flip_prepare and flip_complete functions) already does this, but
> somehow I've forgotten to do this for synchronous flips.
> 
> Note that we don't track outstanding rendering of the new framebuffer
> with busy_bits since all our plane update code waits for previous
> rendering to complete before displaying a new buffer. Hence a new
> buffer will never be busy.
> 
> Reported-by: Paulo Zanoni <przanoni@gmail.com>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
> busy buffer tests.
> ---
>  drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcafefcf048b..b7c69460fb20 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>  				     unsigned frontbuffer_bits);
>  void intel_frontbuffer_flush(struct drm_device *dev,
>  			     unsigned frontbuffer_bits);
> -/**
> - * intel_frontbuffer_flip - synchronous frontbuffer flip
> - * @dev: DRM device
> - * @frontbuffer_bits: frontbuffer plane tracking bits
> - *
> - * This function gets called after scheduling a flip on @obj. This is for
> - * synchronous plane updates which will happen on the next vblank and which will
> - * not get delayed by pending gpu rendering.
> - *
> - * Can be called without any locks held.
> - */
> -static inline
> +inline

inline?

>  void intel_frontbuffer_flip(struct drm_device *dev,
> -			    unsigned frontbuffer_bits)
> -{
> -	intel_frontbuffer_flush(dev, frontbuffer_bits);
> -}
> +			    unsigned frontbuffer_bits);
>  
>  unsigned int intel_fb_align_height(struct drm_device *dev,
>  				   unsigned int height,
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 57095f54c1f2..2a1611a7ce1d 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>  
>  	intel_frontbuffer_flush(dev, frontbuffer_bits);
>  }
> +
> +/**
> + * intel_frontbuffer_flip - synchronous frontbuffer flip
> + * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
> + *
> + * This function gets called after scheduling a flip on @obj. This is for
> + * synchronous plane updates which will happen on the next vblank and which will
> + * not get delayed by pending gpu rendering.
> + *
> + * Can be called without any locks held.
> + */
> +inline
> +void intel_frontbuffer_flip(struct drm_device *dev,
> +			    unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	mutex_lock(&dev_priv->fb_tracking.lock);
> +	dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
> +	/* Remove stale busy bits due to the old buffer. */
> +	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> +	mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +	intel_frontbuffer_flush(dev, frontbuffer_bits);
> +}
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/9] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[Chris Wilson]

On Thu, Jun 18, 2015 at 11:32:53AM +0300, Ville Syrjälä wrote:
> On Thu, Jun 18, 2015 at 10:30:20AM +0200, Daniel Vetter wrote:
> > The current/old frontbuffer might still have gpu frontbuffer rendering
> > pending. But once flipped it won't have the corresponding frontbuffer
> > bits any more and hence the request retire function won't ever clear
> > the corresponding busy bits. The async flip tracking (with the
> > flip_prepare and flip_complete functions) already does this, but
> > somehow I've forgotten to do this for synchronous flips.
> > 
> > Note that we don't track outstanding rendering of the new framebuffer
> > with busy_bits since all our plane update code waits for previous
> > rendering to complete before displaying a new buffer. Hence a new
> > buffer will never be busy.
> > 
> > Reported-by: Paulo Zanoni <przanoni@gmail.com>
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
> > busy buffer tests.
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
> >  drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bcafefcf048b..b7c69460fb20 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >  				     unsigned frontbuffer_bits);
> >  void intel_frontbuffer_flush(struct drm_device *dev,
> >  			     unsigned frontbuffer_bits);
> > -/**
> > - * intel_frontbuffer_flip - synchronous frontbuffer flip
> > - * @dev: DRM device
> > - * @frontbuffer_bits: frontbuffer plane tracking bits
> > - *
> > - * This function gets called after scheduling a flip on @obj. This is for
> > - * synchronous plane updates which will happen on the next vblank and which will
> > - * not get delayed by pending gpu rendering.
> > - *
> > - * Can be called without any locks held.
> > - */
> > -static inline
> > +inline
> 
> inline?

Daniel's probably looked at a perf profile and felt guilty. :)

Hijacking the thread purely to promote
http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=fe05042a9dda46f4bdd9e40612bf48befaee1ad3
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[Daniel Vetter]

The current/old frontbuffer might still have gpu frontbuffer rendering
pending. But once flipped it won't have the corresponding frontbuffer
bits any more and hence the request retire function won't ever clear
the corresponding busy bits. The async flip tracking (with the
flip_prepare and flip_complete functions) already does this, but
somehow I've forgotten to do this for synchronous flips.

Note that we don't track outstanding rendering of the new framebuffer
with busy_bits since all our plane update code waits for previous
rendering to complete before displaying a new buffer. Hence a new
buffer will never be busy.

v2: Drop the spurious inline Ville spotted.

Reported-by: Paulo Zanoni <przanoni@gmail.com>
Cc: Paulo Zanoni <przanoni@gmail.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
busy buffer tests.
---
 drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
 drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bcafefcf048b..b7c69460fb20 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 				     unsigned frontbuffer_bits);
 void intel_frontbuffer_flush(struct drm_device *dev,
 			     unsigned frontbuffer_bits);
-/**
- * intel_frontbuffer_flip - synchronous frontbuffer flip
- * @dev: DRM device
- * @frontbuffer_bits: frontbuffer plane tracking bits
- *
- * This function gets called after scheduling a flip on @obj. This is for
- * synchronous plane updates which will happen on the next vblank and which will
- * not get delayed by pending gpu rendering.
- *
- * Can be called without any locks held.
- */
-static inline
+inline
 void intel_frontbuffer_flip(struct drm_device *dev,
-			    unsigned frontbuffer_bits)
-{
-	intel_frontbuffer_flush(dev, frontbuffer_bits);
-}
+			    unsigned frontbuffer_bits);
 
 unsigned int intel_fb_align_height(struct drm_device *dev,
 				   unsigned int height,
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 57095f54c1f2..c3559fde25e5 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 
 	intel_frontbuffer_flush(dev, frontbuffer_bits);
 }
+
+/**
+ * intel_frontbuffer_flip - synchronous frontbuffer flip
+ * @dev: DRM device
+ * @frontbuffer_bits: frontbuffer plane tracking bits
+ *
+ * This function gets called after scheduling a flip on @obj. This is for
+ * synchronous plane updates which will happen on the next vblank and which will
+ * not get delayed by pending gpu rendering.
+ *
+ * Can be called without any locks held.
+ */
+
+void intel_frontbuffer_flip(struct drm_device *dev,
+			    unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
+	/* Remove stale busy bits due to the old buffer. */
+	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_frontbuffer_flush(dev, frontbuffer_bits);
+}
-- 
2.1.0

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

Re: [PATCH] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[Paulo Zanoni]

2015-06-18 6:23 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> The current/old frontbuffer might still have gpu frontbuffer rendering
> pending. But once flipped it won't have the corresponding frontbuffer
> bits any more and hence the request retire function won't ever clear
> the corresponding busy bits. The async flip tracking (with the
> flip_prepare and flip_complete functions) already does this, but
> somehow I've forgotten to do this for synchronous flips.
>
> Note that we don't track outstanding rendering of the new framebuffer
> with busy_bits since all our plane update code waits for previous
> rendering to complete before displaying a new buffer. Hence a new
> buffer will never be busy.

Steps to reproduce:
- Boot your machine to the display manager
- Kill it, returning to fbcon
- Try to run any subtest from igt/tests/kms_frontbuffer_tracking: they
all fail because FBC never gets enabled
- Do any blt/render operation to the frontbuffer
- Now the tests will succeed

I also submitted a new test for this, that doesn't require the steps
above, so we can also add:
Testcase: igt/kms_frontbuffer_tracking/fbc-modesetfrombusy

More below:

>
> v2: Drop the spurious inline Ville spotted.
>
> Reported-by: Paulo Zanoni <przanoni@gmail.com>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
> busy buffer tests.
> ---
>  drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcafefcf048b..b7c69460fb20 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>                                      unsigned frontbuffer_bits);
>  void intel_frontbuffer_flush(struct drm_device *dev,
>                              unsigned frontbuffer_bits);
> -/**
> - * intel_frontbuffer_flip - synchronous frontbuffer flip
> - * @dev: DRM device
> - * @frontbuffer_bits: frontbuffer plane tracking bits
> - *
> - * This function gets called after scheduling a flip on @obj. This is for
> - * synchronous plane updates which will happen on the next vblank and which will
> - * not get delayed by pending gpu rendering.
> - *
> - * Can be called without any locks held.
> - */
> -static inline
> +inline
>  void intel_frontbuffer_flip(struct drm_device *dev,
> -                           unsigned frontbuffer_bits)
> -{
> -       intel_frontbuffer_flush(dev, frontbuffer_bits);
> -}
> +                           unsigned frontbuffer_bits);
>
>  unsigned int intel_fb_align_height(struct drm_device *dev,
>                                    unsigned int height,
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 57095f54c1f2..c3559fde25e5 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>
>         intel_frontbuffer_flush(dev, frontbuffer_bits);
>  }
> +
> +/**
> + * intel_frontbuffer_flip - synchronous frontbuffer flip
> + * @dev: DRM device
> + * @frontbuffer_bits: frontbuffer plane tracking bits
> + *
> + * This function gets called after scheduling a flip on @obj. This is for
> + * synchronous plane updates which will happen on the next vblank and which will
> + * not get delayed by pending gpu rendering.
> + *
> + * Can be called without any locks held.
> + */
> +
> +void intel_frontbuffer_flip(struct drm_device *dev,
> +                           unsigned frontbuffer_bits)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +       mutex_lock(&dev_priv->fb_tracking.lock);
> +       dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;

I don't understand the purpose of the line above. Is this really
necessary? It seems that flip_bits is only necessary for the
asynchronous flip case, where someone schedules a flip then draws on
the buffer, so we don't flush the buffer at flip_complete. We do
unconditional flush at intel_frontbuffer_flip(), so it seems like we
shouldn't be touching flip_bits here.

I tested both this version and the version without the flip_bits
change, and both appear to solve the problem I can reproduce, so for
both:
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

And, with the flip_bits problem explained or fixed:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> +       /* Remove stale busy bits due to the old buffer. */
> +       dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> +       mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +       intel_frontbuffer_flush(dev, frontbuffer_bits);
> +}
> --
> 2.1.0
>



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

Re: [PATCH] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

[Daniel Vetter]

On Tue, Jun 23, 2015 at 03:59:24PM -0300, Paulo Zanoni wrote:
> 2015-06-18 6:23 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > The current/old frontbuffer might still have gpu frontbuffer rendering
> > pending. But once flipped it won't have the corresponding frontbuffer
> > bits any more and hence the request retire function won't ever clear
> > the corresponding busy bits. The async flip tracking (with the
> > flip_prepare and flip_complete functions) already does this, but
> > somehow I've forgotten to do this for synchronous flips.
> >
> > Note that we don't track outstanding rendering of the new framebuffer
> > with busy_bits since all our plane update code waits for previous
> > rendering to complete before displaying a new buffer. Hence a new
> > buffer will never be busy.
> 
> Steps to reproduce:
> - Boot your machine to the display manager
> - Kill it, returning to fbcon
> - Try to run any subtest from igt/tests/kms_frontbuffer_tracking: they
> all fail because FBC never gets enabled
> - Do any blt/render operation to the frontbuffer
> - Now the tests will succeed
> 
> I also submitted a new test for this, that doesn't require the steps
> above, so we can also add:
> Testcase: igt/kms_frontbuffer_tracking/fbc-modesetfrombusy
> 
> More below:
> 
> >
> > v2: Drop the spurious inline Ville spotted.
> >
> > Reported-by: Paulo Zanoni <przanoni@gmail.com>
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
> > busy buffer tests.
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
> >  drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bcafefcf048b..b7c69460fb20 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >                                      unsigned frontbuffer_bits);
> >  void intel_frontbuffer_flush(struct drm_device *dev,
> >                              unsigned frontbuffer_bits);
> > -/**
> > - * intel_frontbuffer_flip - synchronous frontbuffer flip
> > - * @dev: DRM device
> > - * @frontbuffer_bits: frontbuffer plane tracking bits
> > - *
> > - * This function gets called after scheduling a flip on @obj. This is for
> > - * synchronous plane updates which will happen on the next vblank and which will
> > - * not get delayed by pending gpu rendering.
> > - *
> > - * Can be called without any locks held.
> > - */
> > -static inline
> > +inline
> >  void intel_frontbuffer_flip(struct drm_device *dev,
> > -                           unsigned frontbuffer_bits)
> > -{
> > -       intel_frontbuffer_flush(dev, frontbuffer_bits);
> > -}
> > +                           unsigned frontbuffer_bits);
> >
> >  unsigned int intel_fb_align_height(struct drm_device *dev,
> >                                    unsigned int height,
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index 57095f54c1f2..c3559fde25e5 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >
> >         intel_frontbuffer_flush(dev, frontbuffer_bits);
> >  }
> > +
> > +/**
> > + * intel_frontbuffer_flip - synchronous frontbuffer flip
> > + * @dev: DRM device
> > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > + *
> > + * This function gets called after scheduling a flip on @obj. This is for
> > + * synchronous plane updates which will happen on the next vblank and which will
> > + * not get delayed by pending gpu rendering.
> > + *
> > + * Can be called without any locks held.
> > + */
> > +
> > +void intel_frontbuffer_flip(struct drm_device *dev,
> > +                           unsigned frontbuffer_bits)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       mutex_lock(&dev_priv->fb_tracking.lock);
> > +       dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
> 
> I don't understand the purpose of the line above. Is this really
> necessary? It seems that flip_bits is only necessary for the
> asynchronous flip case, where someone schedules a flip then draws on
> the buffer, so we don't flush the buffer at flip_complete. We do
> unconditional flush at intel_frontbuffer_flip(), so it seems like we
> shouldn't be touching flip_bits here.

Indeed this was bogus, but luckily it was harmless (keeping track of an
async flip that will never happen isn't a problem) so either version
worked. But I removed it when merging the patch, thanks for spotting this.
-Daniel

> 
> I tested both this version and the version without the flip_bits
> change, and both appear to solve the problem I can reproduce, so for
> both:
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> And, with the flip_bits problem explained or fixed:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > +       /* Remove stale busy bits due to the old buffer. */
> > +       dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > +       mutex_unlock(&dev_priv->fb_tracking.lock);
> > +
> > +       intel_frontbuffer_flush(dev, frontbuffer_bits);
> > +}
> > --
> > 2.1.0
> >
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH igt] tests/kms_frontbuffer_tracking: add modesetfrombusy test

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This test exercies the dev_priv->fb_tracking.busy_bits bug I recently
found and Daniel fixed.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 113 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 72455c3..70007eb 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -27,6 +27,7 @@
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
+#include <pthread.h>
 
 #include "drmtest.h"
 #include "igt_aux.h"
@@ -259,6 +260,19 @@ struct {
 	struct igt_fb big;
 } fbs;
 
+struct {
+	pthread_t thread;
+	bool stop;
+
+	uint32_t handle;
+	uint32_t size;
+	uint32_t stride;
+	int width;
+	int height;
+} busy_thread = {
+	.stop = true,
+};
+
 static drmModeModeInfoPtr get_connector_smallest_mode(drmModeConnectorPtr c)
 {
 	int i;
@@ -876,6 +890,41 @@ static void disable_features(void)
 	psr_disable();
 }
 
+static void *busy_thread_func(void *data)
+{
+	while (!busy_thread.stop)
+		igt_draw_rect(drm.fd, drm.bufmgr, NULL, busy_thread.handle,
+			      busy_thread.size, busy_thread.stride,
+			      IGT_DRAW_BLT, 0, 0, busy_thread.width,
+			      busy_thread.height, 0xFF);
+
+	pthread_exit(0);
+}
+
+static void start_busy_thread(struct igt_fb *fb)
+{
+	int rc;
+
+	igt_assert(busy_thread.stop == true);
+	busy_thread.stop = false;
+	busy_thread.handle = fb->gem_handle;
+	busy_thread.size = fb->size;
+	busy_thread.stride = fb->stride;
+	busy_thread.width = fb->width;
+	busy_thread.height = fb->height;
+
+	rc = pthread_create(&busy_thread.thread, NULL, busy_thread_func, NULL);
+	igt_assert(rc == 0);
+}
+
+static void stop_busy_thread(void)
+{
+	if (!busy_thread.stop) {
+		busy_thread.stop = true;
+		igt_assert(pthread_join(busy_thread.thread, NULL) == 0);
+	}
+}
+
 static void print_crc(const char *str, struct both_crcs *crc)
 {
 	int i;
@@ -1199,6 +1248,8 @@ static void setup_environment(void)
 
 static void teardown_environment(void)
 {
+	stop_busy_thread();
+
 	teardown_crcs();
 	teardown_psr();
 	teardown_fbc();
@@ -1481,6 +1532,8 @@ static void prepare_subtest(const struct test_mode *t,
 {
 	check_test_requirements(t);
 
+	stop_busy_thread();
+
 	disable_features();
 	set_crtc_fbs(t);
 
@@ -1964,6 +2017,50 @@ static void fullscreen_plane_subtest(const struct test_mode *t)
 	igt_remove_fb(drm.fd, &fullscreen_fb);
 }
 
+/**
+ * modesetfrombusy - modeset from a busy buffer to a non-busy buffer
+ *
+ * METHOD
+ *   Set a mode, make the frontbuffer busy using BLT writes, do a modeset to a
+ *   non-busy buffer, then check if the features are enabled. The goal of this
+ *   test is to exercise a bug we had on the frontbuffer tracking infrastructure
+ *   code.
+ *
+ * EXPECTED RESULTS
+ *   No assertions fail.
+ *
+ * FAILURES
+ *   If you're failing this test, then you probably need "drm/i915: Clear
+ *   fb_tracking.busy_bits also for synchronous flips" or any other patch that
+ *   properly updates dev_priv->fb_tracking.busy_bits when we're alternating
+ *   between buffers with different busyness.
+ */
+static void modesetfrombusy_subtest(const struct test_mode *t)
+{
+	struct draw_pattern_info *pattern = &pattern1;
+	struct modeset_params *params = pick_params(t);
+	struct igt_fb fb2;
+
+	prepare_subtest(t, pattern);
+
+	igt_create_fb(drm.fd, params->fb.fb->width, params->fb.fb->height,
+		      DRM_FORMAT_XRGB8888, LOCAL_I915_FORMAT_MOD_X_TILED, &fb2);
+	igt_draw_fill_fb(drm.fd, &fb2, 0xFF);
+
+	start_busy_thread(params->fb.fb);
+	usleep(10000);
+
+	unset_all_crtcs();
+	params->fb.fb = &fb2;
+	set_mode_for_params(params);
+
+	do_assertions(0);
+
+	stop_busy_thread();
+
+	igt_remove_fb(drm.fd, &fb2);
+}
+
 static int opt_handler(int option, int option_index, void *data)
 {
 	switch (option) {
@@ -2248,6 +2345,22 @@ int main(int argc, char *argv[])
 			multidraw_subtest(&t);
 	TEST_MODE_ITER_END
 
+	TEST_MODE_ITER_BEGIN(t)
+		if (t.pipes != PIPE_SINGLE)
+			continue;
+		if (t.screen != SCREEN_PRIM)
+			continue;
+		if (t.plane != PLANE_PRI)
+			continue;
+		if (t.fbs != FBS_SINGLE)
+			continue;
+		if (t.method != IGT_DRAW_MMAP_CPU)
+			continue;
+
+		igt_subtest_f("%s-modesetfrombusy", feature_str(t.feature))
+			modesetfrombusy_subtest(&t);
+	TEST_MODE_ITER_END
+
 	/*
 	 * TODO: ideas for subtests:
 	 * - Add a new enum to struct test_mode that allows us to specify the
-- 
2.1.4

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