[PATCH 0/8] eDP DRRS based on frontbuffer tracking

[PATCH 0/8] eDP DRRS based on frontbuffer tracking

[Vandana Kannan]

This patch series inserts DRRS into frontbuffer tracking mechanism.

1. Previous submission for this feature was designed considering only eDP
DRRS. In this series, apart from following fb tracking, changes have been made
to make structures generic so that it can be of use to any other code
addition to support DRRS with other display types.
2. DRRS support is checked based on VBT setting and panel's capability (if
more than one RR is supported).
3. Based on DRRS support availability, related structures are initialized or
cleaned up through calls from enable/disable DDI respectively.
4. Since flip() indicates busyness, changes have been made to invalidate
DRRS during flip. This changes RR back to preferred mode RR. New work to set
low RR is scheduled after a delay of x ms.
5. This series includes patches to support RR switching on all platforms.
6. A module param has been added, carrying forward the input from the
previous submission of the feature. This param indicates the delay in ms after
which a switch to low RR can be made. By default, this is set to 0
indicating that the feature is disabled.

Durgadoss R (1):
  drm/i915: Enable eDP DRRS for CHV

Vandana Kannan (7):
  drm/i915: Modifying structures related to DRRS
  drm/i915: Initialize DRRS delayed work
  drm/i915: Enable/disable DRRS
  drm/i915: DRRS calls based on frontbuffer
  drm/i915/bdw: Add support for DRRS to switch RR
  drm/i915: Support for RR switching on VLV
  drm/i915: Add drrs_interval module parameter

 drivers/gpu/drm/i915/i915_drv.h          |  33 +++--
 drivers/gpu/drm/i915/i915_params.c       |   8 ++
 drivers/gpu/drm/i915/i915_reg.h          |   1 +
 drivers/gpu/drm/i915/intel_ddi.c         |   2 +
 drivers/gpu/drm/i915/intel_display.c     |  21 ++-
 drivers/gpu/drm/i915/intel_dp.c          | 217 ++++++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h         |  26 ++--
 drivers/gpu/drm/i915/intel_frontbuffer.c |   2 +
 8 files changed, 239 insertions(+), 71 deletions(-)

-- 
2.0.1

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

[PATCH 1/8] drm/i915: Modifying structures related to DRRS

[Vandana Kannan]

Earlier, DRRS structures were specific to eDP (used only in intel_dp).
Since DRRS can be extended to other internal display types
(if the panel supports multiple RR), modifying structures
to be part of drm_i915_private and have a provision to add display related
structs like intel_dp.
Also, aligning with frontbuffer tracking mechanism, the new structure
contains data for busy frontbuffer bits.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 32 ++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_dp.c  | 50 ++++++++++++++++++----------------------
 drivers/gpu/drm/i915/intel_drv.h | 18 ---------------
 3 files changed, 47 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11e85cb..370cbaa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -730,11 +730,33 @@ struct i915_fbc {
 	} no_fbc_reason;
 };
 
-struct i915_drrs {
-	struct intel_connector *connector;
+/**
+ * HIGH_RR is the highest eDP panel refresh rate read from EDID
+ * LOW_RR is the lowest eDP panel refresh rate found from EDID
+ * parsing for same resolution.
+ */
+enum drrs_refresh_rate_type {
+	DRRS_HIGH_RR,
+	DRRS_LOW_RR,
+	DRRS_MAX_RR, /* RR count */
+};
+
+enum drrs_support_type {
+	DRRS_NOT_SUPPORTED = 0,
+	STATIC_DRRS_SUPPORT = 1,
+	SEAMLESS_DRRS_SUPPORT = 2
 };
 
 struct intel_dp;
+struct i915_drrs {
+	struct mutex mutex;
+	struct delayed_work work;
+	struct intel_dp *dp;
+	unsigned busy_frontbuffer_bits;
+	enum drrs_refresh_rate_type refresh_rate_type;
+	enum drrs_support_type type;
+};
+
 struct i915_psr {
 	struct mutex lock;
 	bool sink_support;
@@ -1300,12 +1322,6 @@ struct ddi_vbt_port_info {
 	uint8_t supports_dp:1;
 };
 
-enum drrs_support_type {
-	DRRS_NOT_SUPPORTED = 0,
-	STATIC_DRRS_SUPPORT = 1,
-	SEAMLESS_DRRS_SUPPORT = 2
-};
-
 enum psr_lines_to_wait {
 	PSR_0_LINES_TO_WAIT = 0,
 	PSR_1_LINE_TO_WAIT,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3fc3296..96d8e17 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1269,7 +1269,7 @@ found:
 			       &pipe_config->dp_m_n);
 
 	if (intel_connector->panel.downclock_mode != NULL &&
-		intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
+		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
 			pipe_config->has_drrs = true;
 			intel_link_compute_m_n(bpp, lane_count,
 				intel_connector->panel.downclock_mode->clock,
@@ -4745,24 +4745,24 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		      I915_READ(pp_div_reg));
 }
 
-void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
+static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *encoder;
-	struct intel_dp *intel_dp = NULL;
+	struct intel_digital_port *dig_port = NULL;
+	struct intel_dp *intel_dp = dev_priv->drrs.dp;
 	struct intel_crtc_config *config = NULL;
 	struct intel_crtc *intel_crtc = NULL;
-	struct intel_connector *intel_connector = dev_priv->drrs.connector;
 	u32 reg, val;
-	enum edp_drrs_refresh_rate_type index = DRRS_HIGH_RR;
+	enum drrs_refresh_rate_type index = DRRS_HIGH_RR;
 
 	if (refresh_rate <= 0) {
 		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
 		return;
 	}
 
-	if (intel_connector == NULL) {
-		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
+	if (intel_dp == NULL) {
+		DRM_DEBUG_KMS("DRRS not supported.\n");
 		return;
 	}
 
@@ -4771,8 +4771,8 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 	 * platforms that cannot have PSR and DRRS enabled at the same time.
 	 */
 
-	encoder = intel_attached_encoder(&intel_connector->base);
-	intel_dp = enc_to_intel_dp(&encoder->base);
+	dig_port = dp_to_dig_port(intel_dp);
+	encoder = &dig_port->base;
 	intel_crtc = encoder->new_crtc;
 
 	if (!intel_crtc) {
@@ -4782,15 +4782,16 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 
 	config = &intel_crtc->config;
 
-	if (intel_dp->drrs_state.type < SEAMLESS_DRRS_SUPPORT) {
+	if (dev_priv->drrs.type < SEAMLESS_DRRS_SUPPORT) {
 		DRM_DEBUG_KMS("Only Seamless DRRS supported.\n");
 		return;
 	}
 
-	if (intel_connector->panel.downclock_mode->vrefresh == refresh_rate)
+	if (intel_dp->attached_connector->panel.downclock_mode->vrefresh ==
+			refresh_rate)
 		index = DRRS_LOW_RR;
 
-	if (index == intel_dp->drrs_state.refresh_rate_type) {
+	if (index == dev_priv->drrs.refresh_rate_type) {
 		DRM_DEBUG_KMS(
 			"DRRS requested for previously set RR...ignoring\n");
 		return;
@@ -4820,23 +4821,21 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 	 * possible calls from user space to set differnt RR are made.
 	 */
 
-	mutex_lock(&intel_dp->drrs_state.mutex);
+	mutex_lock(&dev_priv->drrs.mutex);
 
-	intel_dp->drrs_state.refresh_rate_type = index;
+	dev_priv->drrs.refresh_rate_type = index;
 
-	mutex_unlock(&intel_dp->drrs_state.mutex);
+	mutex_unlock(&dev_priv->drrs.mutex);
 
 	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
 }
 
 static struct drm_display_mode *
-intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
-			struct intel_connector *intel_connector,
-			struct drm_display_mode *fixed_mode)
+intel_dp_drrs_init(struct intel_connector *intel_connector,
+		struct drm_display_mode *fixed_mode)
 {
 	struct drm_connector *connector = &intel_connector->base;
-	struct intel_dp *intel_dp = &intel_dig_port->dp;
-	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *downclock_mode = NULL;
 
@@ -4858,13 +4857,11 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 		return NULL;
 	}
 
-	dev_priv->drrs.connector = intel_connector;
-
-	mutex_init(&intel_dp->drrs_state.mutex);
+	mutex_init(&dev_priv->drrs.mutex);
 
-	intel_dp->drrs_state.type = dev_priv->vbt.drrs_type;
+	dev_priv->drrs.type = dev_priv->vbt.drrs_type;
 
-	intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
+	dev_priv->drrs.refresh_rate_type = DRRS_HIGH_RR;
 	DRM_DEBUG_KMS("seamless DRRS supported for eDP panel.\n");
 	return downclock_mode;
 }
@@ -4884,7 +4881,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	struct edid *edid;
 	enum pipe pipe = INVALID_PIPE;
 
-	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
+	dev_priv->drrs.type = DRRS_NOT_SUPPORTED;
 
 	if (!is_edp(intel_dp))
 		return true;
@@ -4933,7 +4930,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
 			fixed_mode = drm_mode_duplicate(dev, scan);
 			downclock_mode = intel_dp_drrs_init(
-						intel_dig_port,
 						intel_connector, fixed_mode);
 			break;
 		}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61a88fa..066db58 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -566,17 +566,6 @@ struct intel_hdmi {
 struct intel_dp_mst_encoder;
 #define DP_MAX_DOWNSTREAM_PORTS		0x10
 
-/**
- * HIGH_RR is the highest eDP panel refresh rate read from EDID
- * LOW_RR is the lowest eDP panel refresh rate found from EDID
- * parsing for same resolution.
- */
-enum edp_drrs_refresh_rate_type {
-	DRRS_HIGH_RR,
-	DRRS_LOW_RR,
-	DRRS_MAX_RR, /* RR count */
-};
-
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -632,12 +621,6 @@ struct intel_dp {
 				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
-	struct {
-		enum drrs_support_type type;
-		enum edp_drrs_refresh_rate_type refresh_rate_type;
-		struct mutex mutex;
-	} drrs_state;
-
 };
 
 struct intel_digital_port {
@@ -1005,7 +988,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp);
 void intel_edp_panel_vdd_on(struct intel_dp *intel_dp);
 void intel_edp_panel_on(struct intel_dp *intel_dp);
 void intel_edp_panel_off(struct intel_dp *intel_dp);
-void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
 void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector);
 void intel_dp_mst_suspend(struct drm_device *dev);
 void intel_dp_mst_resume(struct drm_device *dev);
-- 
2.0.1

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

[PATCH 2/8] drm/i915: Initialize DRRS delayed work

[Vandana Kannan]

Add DRRS work function to trigger a switch to low refresh rate when activity
is detected on screen.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 96d8e17..ef8b55d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4814,20 +4814,38 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		I915_WRITE(reg, val);
 	}
 
+	dev_priv->drrs.refresh_rate_type = index;
+
+	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+}
+
+static void intel_edp_drrs_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), drrs.work.work);
+	struct intel_dp *intel_dp = dev_priv->drrs.dp;
+
+	mutex_lock(&dev_priv->drrs.mutex);
+
+	if (!intel_dp)
+		goto unlock;
+
 	/*
-	 * mutex taken to ensure that there is no race between differnt
-	 * drrs calls trying to update refresh rate. This scenario may occur
-	 * in future when idleness detection based DRRS in kernel and
-	 * possible calls from user space to set differnt RR are made.
+	 * The delayed work can race with an invalidate hence we need to
+	 * recheck.
 	 */
 
-	mutex_lock(&dev_priv->drrs.mutex);
+	if (dev_priv->drrs.busy_frontbuffer_bits)
+		goto unlock;
 
-	dev_priv->drrs.refresh_rate_type = index;
+	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR)
+		intel_dp_set_drrs_state(dev_priv->dev,
+			intel_dp->attached_connector->panel.
+			downclock_mode->vrefresh);
 
-	mutex_unlock(&dev_priv->drrs.mutex);
+unlock:
 
-	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
 static struct drm_display_mode *
@@ -4857,6 +4875,8 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
 		return NULL;
 	}
 
+	INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_work);
+
 	mutex_init(&dev_priv->drrs.mutex);
 
 	dev_priv->drrs.type = dev_priv->vbt.drrs_type;
-- 
2.0.1

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

[PATCH 3/8] drm/i915: Enable/disable DRRS

[Vandana Kannan]

Calling enable/disable DRRS when enable/disable DDI are called.
These functions are responsible for setup of drrs data (in enable) and
reset of drrs (in disable).
has_drrs is true when downclock_mode is found and SEAMLESS_DRRS is set in
the VBT. A check has been added for has_drrs in these functions, to make
sure the functions go through only if DRRS will work on the platform with
the attached panel.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |  2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 54 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 3 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 4e2e860..9a3ba72 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1600,6 +1600,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 
 		intel_edp_backlight_on(intel_dp);
 		intel_psr_enable(intel_dp);
+		intel_edp_drrs_enable(intel_dp);
 	}
 
 	if (intel_crtc->config.has_audio) {
@@ -1625,6 +1626,7 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
+		intel_edp_drrs_disable(intel_dp);
 		intel_psr_disable(intel_dp);
 		intel_edp_backlight_off(intel_dp);
 	}
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ef8b55d..7098470 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4819,6 +4819,60 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
 }
 
+void intel_edp_drrs_enable(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_crtc *crtc = dig_port->base.base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (!intel_crtc->config.has_drrs) {
+		DRM_DEBUG_KMS("Panel doesn't support DRRS\n");
+		return;
+	}
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	if (dev_priv->drrs.dp) {
+		DRM_DEBUG_KMS("DRRS already enabled\n");
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
+	dev_priv->drrs.busy_frontbuffer_bits = 0;
+
+	dev_priv->drrs.dp = intel_dp;
+	mutex_unlock(&dev_priv->drrs.mutex);
+}
+
+void intel_edp_drrs_disable(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_crtc *crtc = dig_port->base.base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+	if (!intel_crtc->config.has_drrs)
+		return;
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
+	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR)
+		intel_dp_set_drrs_state(dev_priv->dev,
+			intel_dp->attached_connector->panel.
+			fixed_mode->vrefresh);
+
+	dev_priv->drrs.dp = NULL;
+	mutex_unlock(&dev_priv->drrs.mutex);
+
+	cancel_delayed_work_sync(&dev_priv->drrs.work);
+}
+
 static void intel_edp_drrs_work(struct work_struct *work)
 {
 	struct drm_i915_private *dev_priv =
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 066db58..1c3c25a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1002,6 +1002,8 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		       uint32_t src_x, uint32_t src_y,
 		       uint32_t src_w, uint32_t src_h);
 int intel_disable_plane(struct drm_plane *plane);
+void intel_edp_drrs_enable(struct intel_dp *intel_dp);
+void intel_edp_drrs_disable(struct intel_dp *intel_dp);
 
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
-- 
2.0.1

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

[PATCH 4/8] drm/i915: DRRS calls based on frontbuffer

[Vandana Kannan]

Calls have been added to invalidate/flush DRRS whenever invalidate/flush is
called as part of frontbuffer tracking.
Apart from calls as a result of GEM tracking to fb invalidate/flush, a
call has been added to invalidate fb obj from crtc_page_flip as well. This
is to track busyness through flip calls.
The call to fb_obj_invalidate (in flip) is placed before queuing flip for this
obj.

drrs_invalidate() and drrs_flush() check for drrs.dp which would be NULL if
it was setup in drrs_enable(). This covers for the condition when DRRS is
not supported.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c     |  8 +++--
 drivers/gpu/drm/i915/intel_dp.c          | 51 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h         |  3 ++
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
 4 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a9bdc12..a4a565c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9717,6 +9717,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (ret)
 		goto cleanup_pending;
 
+	i915_gem_track_fb(work->old_fb_obj, obj,
+			  INTEL_FRONTBUFFER_PRIMARY(pipe));
+
+	intel_fb_obj_invalidate(obj, ring);
+
 	work->gtt_offset =
 		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
 
@@ -9741,9 +9746,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
 	work->enable_stall_check = true;
 
-	i915_gem_track_fb(work->old_fb_obj, obj,
-			  INTEL_FRONTBUFFER_PRIMARY(pipe));
-
 	intel_disable_fbc(dev);
 	intel_frontbuffer_flip_prepare(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7098470..bc17900 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4902,6 +4902,57 @@ unlock:
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
+void intel_edp_drrs_invalidate(struct drm_device *dev,
+		unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+
+	if (!dev_priv->drrs.dp)
+		return;
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+
+	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
+		cancel_delayed_work_sync(&dev_priv->drrs.work);
+		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);
+}
+
+void intel_edp_drrs_flush(struct drm_device *dev,
+		unsigned frontbuffer_bits)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+
+	if (!dev_priv->drrs.dp)
+		return;
+
+	mutex_lock(&dev_priv->drrs.mutex);
+	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
+
+	cancel_delayed_work_sync(&dev_priv->drrs.work);
+
+	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
+			!dev_priv->drrs.busy_frontbuffer_bits)
+		schedule_delayed_work(&dev_priv->drrs.work,
+				msecs_to_jiffies(1000));
+	mutex_unlock(&dev_priv->drrs.mutex);
+}
+
 static struct drm_display_mode *
 intel_dp_drrs_init(struct intel_connector *intel_connector,
 		struct drm_display_mode *fixed_mode)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1c3c25a..763c097 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1004,6 +1004,9 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 int intel_disable_plane(struct drm_plane *plane);
 void intel_edp_drrs_enable(struct intel_dp *intel_dp);
 void intel_edp_drrs_disable(struct intel_dp *intel_dp);
+void intel_edp_drrs_invalidate(struct drm_device *dev,
+		unsigned frontbuffer_bits);
+void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
 
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 79f6d72..73cb6e0 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -157,6 +157,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
 
 	intel_psr_invalidate(dev, obj->frontbuffer_bits);
+	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
 }
 
 /**
@@ -182,6 +183,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 
 	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
 
+	intel_edp_drrs_flush(dev, frontbuffer_bits);
 	intel_psr_flush(dev, frontbuffer_bits);
 
 	/*
-- 
2.0.1

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

[PATCH 5/8] drm/i915/bdw: Add support for DRRS to switch RR

[Vandana Kannan]

For Broadwell, there is one instance of Transcoder MN values per transcoder.
For dynamic switching between multiple refreshr rates, M/N values may be
reprogrammed on the fly. Link N programming triggers update of all data and
link M & N registers and the new M/N values will be used in the next frame
that is output.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  9 +++------
 drivers/gpu/drm/i915/intel_dp.c      | 15 ++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |  3 +++
 3 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a4a565c..411d34e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -88,9 +88,6 @@ static int intel_framebuffer_init(struct drm_device *dev,
 				  struct drm_i915_gem_object *obj);
 static void i9xx_set_pipeconf(struct intel_crtc *intel_crtc);
 static void intel_set_pipe_timings(struct intel_crtc *intel_crtc);
-static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
-					 struct intel_link_m_n *m_n,
-					 struct intel_link_m_n *m2_n2);
 static void ironlake_set_pipeconf(struct drm_crtc *crtc);
 static void haswell_set_pipeconf(struct drm_crtc *crtc);
 static void intel_set_pipe_csc(struct drm_crtc *crtc);
@@ -5795,9 +5792,9 @@ static void intel_pch_transcoder_set_m_n(struct intel_crtc *crtc,
 	I915_WRITE(PCH_TRANS_LINK_N1(pipe), m_n->link_n);
 }
 
-static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
-					 struct intel_link_m_n *m_n,
-					 struct intel_link_m_n *m2_n2)
+void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
+				 struct intel_link_m_n *m_n,
+				 struct intel_link_m_n *m2_n2)
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bc17900..38d61f2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4802,7 +4802,20 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
-	if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
+	if (INTEL_INFO(dev)->gen >= 8) {
+		switch(index) {
+		case DRRS_HIGH_RR:
+			intel_dp_set_m_n(intel_crtc);
+			break;
+		case DRRS_LOW_RR:
+			intel_cpu_transcoder_set_m_n(intel_crtc,
+					&intel_crtc->config.dp_m2_n2, NULL);
+			break;
+		case DRRS_MAX_RR:
+		default:
+			break;
+		}
+	} else if (INTEL_INFO(dev)->gen > 6) {
 		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
 		val = I915_READ(reg);
 		if (index > DRRS_HIGH_RR) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 763c097..bdfdea8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -952,6 +952,9 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv);
 void intel_dp_get_m_n(struct intel_crtc *crtc,
 		      struct intel_crtc_config *pipe_config);
 void intel_dp_set_m_n(struct intel_crtc *crtc);
+void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
+				 struct intel_link_m_n *m_n,
+				 struct intel_link_m_n *m2_n2);
 int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
 void
 ironlake_check_encoder_dotclock(const struct intel_crtc_config *pipe_config,
-- 
2.0.1

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

[PATCH 6/8] drm/i915: Support for RR switching on VLV

[Vandana Kannan]

Definition of VLV RR switch bit and corresponding toggling in
set_drrs function.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |  1 +
 drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 78d89a7..16b426d 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3821,6 +3821,7 @@ enum punit_power_well {
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
 #define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
+#define   PIPECONF_EDP_RR_MODE_SWITCH_VLV	(1 << 14)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
 #define   PIPECONF_8BPC		(0<<5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 38d61f2..ef8fa94 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4819,10 +4819,16 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
 		val = I915_READ(reg);
 		if (index > DRRS_HIGH_RR) {
-			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			if (IS_VALLEYVIEW(dev))
+				val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val |= PIPECONF_EDP_RR_MODE_SWITCH;
 			intel_dp_set_m_n(intel_crtc);
 		} else {
-			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+			if (IS_VALLEYVIEW(dev))
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
 		}
 		I915_WRITE(reg, val);
 	}
-- 
2.0.1

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

[PATCH 7/8] drm/i915: Enable eDP DRRS for CHV

[Vandana Kannan]

From: Durgadoss R <durgadoss.r@intel.com>

This patch enables eDP DRRS for CHV by adding the
required IS_CHERRYVIEW() checks.
CHV uses the same register bit as VLV.

[Vandana]: Since CHV has 2 sets of M_N registers, it will follow the same code
path as gen < 8. Added CHV check in dp_set_m_n()

Signed-off-by: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++--
 drivers/gpu/drm/i915/intel_dp.c      | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 411d34e..07a6abf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5810,8 +5810,8 @@ void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
 		 * for gen < 8) and if DRRS is supported (to make sure the
 		 * registers are not unnecessarily accessed).
 		 */
-		if (m2_n2 && INTEL_INFO(dev)->gen < 8 &&
-			crtc->config.has_drrs) {
+		if (m2_n2 && (IS_CHERRYVIEW(dev) || INTEL_INFO(dev)->gen < 8)
+			&& crtc->config.has_drrs) {
 			I915_WRITE(PIPE_DATA_M2(transcoder),
 					TU_SIZE(m2_n2->tu) | m2_n2->gmch_m);
 			I915_WRITE(PIPE_DATA_N2(transcoder), m2_n2->gmch_n);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ef8fa94..092ef91 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4802,7 +4802,7 @@ static void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
-	if (INTEL_INFO(dev)->gen >= 8) {
+	if (INTEL_INFO(dev)->gen >= 8 && !IS_CHERRYVIEW(dev)) {
 		switch(index) {
 		case DRRS_HIGH_RR:
 			intel_dp_set_m_n(intel_crtc);
-- 
2.0.1

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

[PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Vandana Kannan]

Adding i915 module parameter for setting drrs_interval. If this param is
set to 0, then drrs is disabled. If changed in runtime, then the new interval
value will be considered for scheduling the next drrs work.
drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
 drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++++++-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 370cbaa..dba5844 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2381,6 +2381,7 @@ struct i915_params {
 	int enable_ips;
 	int invert_brightness;
 	int enable_cmd_parser;
+	int drrs_interval;
 	/* leave bools at the end to not create holes */
 	bool enable_hangcheck;
 	bool fastboot;
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index c91cb20..80492e8 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = {
 	.disable_vtd_wa = 0,
 	.use_mmio_flip = 0,
 	.mmio_debug = 0,
+	.drrs_interval = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -173,3 +174,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
 MODULE_PARM_DESC(mmio_debug,
 	"Enable the MMIO debug code (default: false). This may negatively "
 	"affect performance.");
+
+module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
+MODULE_PARM_DESC(drrs_interval,
+	"DRRS idleness detection interval  (default: 0 ms). "
+	"If this field is set to 0, then seamless DRRS feature "
+	"based on idleness detection is disabled. "
+	"The interval is to be set in milliseconds.");
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 092ef91..88f46906 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4931,6 +4931,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	if (!dev_priv->drrs.dp)
 		return;
 
+	if (i915.drrs_interval == 0)
+		return;
+
 	mutex_lock(&dev_priv->drrs.mutex);
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
@@ -4958,6 +4961,9 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	if (!dev_priv->drrs.dp)
 		return;
 
+	if (i915.drrs_interval == 0)
+		return;
+
 	mutex_lock(&dev_priv->drrs.mutex);
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
@@ -4968,7 +4974,7 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
 			!dev_priv->drrs.busy_frontbuffer_bits)
 		schedule_delayed_work(&dev_priv->drrs.work,
-				msecs_to_jiffies(1000));
+				msecs_to_jiffies(i915.drrs_interval));
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
@@ -4991,6 +4997,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
 		return NULL;
 	}
 
+	if (i915.drrs_interval == 0)
+		DRM_DEBUG_KMS("DRRS disable by flag\n");
+
 	downclock_mode = intel_find_panel_downclock
 					(dev, fixed_mode, connector);
 
-- 
2.0.1

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

Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1-3              364/366              362/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt_kms_render_direct-render      PASS(7, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_blocking-absolute-wf_vblank-interruptible      DMESG_WARN(1, M26)PASS(7, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_vblank-vs-hang      DMESG_WARN(2, M26)PASS(2, M26)      DMESG_WARN(1, M26)
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(6, M26)PASS(20, M26M37)      PASS(1, M26)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Daniel Vetter]

On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote:
> Adding i915 module parameter for setting drrs_interval. If this param is
> set to 0, then drrs is disabled. If changed in runtime, then the new interval
> value will be considered for scheduling the next drrs work.
> drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.

Nope, please don't hide power saving features behind module options by
default. New stuff must be enabled by default, otherwise it'll bitrot and
merging to upstream is fairly useless since we still have the rebase pain
(just spread out over more people) with none of the testing.

Also such debug options need to be marked using the _debug variants to
make sure that people report issues and don't keep using them.

Now talking about validation gets me to the next point: Do we have a basic
igt to make sure DRRS doesn't break right after merging? I don't think we
need the full-blown test setup like for psr/fbc, but a basic test to make
sure it enters/exit RR mode should be there.
-Daniel

> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
>  drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++++++-
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 370cbaa..dba5844 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2381,6 +2381,7 @@ struct i915_params {
>  	int enable_ips;
>  	int invert_brightness;
>  	int enable_cmd_parser;
> +	int drrs_interval;
>  	/* leave bools at the end to not create holes */
>  	bool enable_hangcheck;
>  	bool fastboot;
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index c91cb20..80492e8 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = {
>  	.disable_vtd_wa = 0,
>  	.use_mmio_flip = 0,
>  	.mmio_debug = 0,
> +	.drrs_interval = 0,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -173,3 +174,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
>  MODULE_PARM_DESC(mmio_debug,
>  	"Enable the MMIO debug code (default: false). This may negatively "
>  	"affect performance.");
> +
> +module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
> +MODULE_PARM_DESC(drrs_interval,
> +	"DRRS idleness detection interval  (default: 0 ms). "
> +	"If this field is set to 0, then seamless DRRS feature "
> +	"based on idleness detection is disabled. "
> +	"The interval is to be set in milliseconds.");
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 092ef91..88f46906 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4931,6 +4931,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>  	if (!dev_priv->drrs.dp)
>  		return;
>  
> +	if (i915.drrs_interval == 0)
> +		return;
> +
>  	mutex_lock(&dev_priv->drrs.mutex);
>  	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>  	pipe = to_intel_crtc(crtc)->pipe;
> @@ -4958,6 +4961,9 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>  	if (!dev_priv->drrs.dp)
>  		return;
>  
> +	if (i915.drrs_interval == 0)
> +		return;
> +
>  	mutex_lock(&dev_priv->drrs.mutex);
>  	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>  	pipe = to_intel_crtc(crtc)->pipe;
> @@ -4968,7 +4974,7 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>  	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
>  			!dev_priv->drrs.busy_frontbuffer_bits)
>  		schedule_delayed_work(&dev_priv->drrs.work,
> -				msecs_to_jiffies(1000));
> +				msecs_to_jiffies(i915.drrs_interval));
>  	mutex_unlock(&dev_priv->drrs.mutex);
>  }
>  
> @@ -4991,6 +4997,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
>  		return NULL;
>  	}
>  
> +	if (i915.drrs_interval == 0)
> +		DRM_DEBUG_KMS("DRRS disable by flag\n");
> +
>  	downclock_mode = intel_find_panel_downclock
>  					(dev, fixed_mode, connector);
>  
> -- 
> 2.0.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/8] drm/i915: DRRS calls based on frontbuffer

[Daniel Vetter]

On Thu, Dec 11, 2014 at 02:22:53AM +0530, Vandana Kannan wrote:
> Calls have been added to invalidate/flush DRRS whenever invalidate/flush is
> called as part of frontbuffer tracking.
> Apart from calls as a result of GEM tracking to fb invalidate/flush, a
> call has been added to invalidate fb obj from crtc_page_flip as well. This
> is to track busyness through flip calls.

That shouldn't be required really. Or maybe I misunderstand what you mean
here ...

> The call to fb_obj_invalidate (in flip) is placed before queuing flip for this
> obj.
> 
> drrs_invalidate() and drrs_flush() check for drrs.dp which would be NULL if
> it was setup in drrs_enable(). This covers for the condition when DRRS is
> not supported.
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c     |  8 +++--
>  drivers/gpu/drm/i915/intel_dp.c          | 51 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h         |  3 ++
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
>  4 files changed, 61 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a9bdc12..a4a565c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9717,6 +9717,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (ret)
>  		goto cleanup_pending;
>  
> +	i915_gem_track_fb(work->old_fb_obj, obj,
> +			  INTEL_FRONTBUFFER_PRIMARY(pipe));
> +
> +	intel_fb_obj_invalidate(obj, ring);
> +
>  	work->gtt_offset =
>  		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
>  
> @@ -9741,9 +9746,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
>  	work->enable_stall_check = true;
>  
> -	i915_gem_track_fb(work->old_fb_obj, obj,
> -			  INTEL_FRONTBUFFER_PRIMARY(pipe));
> -
>  	intel_disable_fbc(dev);
>  	intel_frontbuffer_flip_prepare(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
>  	mutex_unlock(&dev->struct_mutex);

This hunk here looks spurious. Or is it relate to what you mention in the
commit message? Please explain more in-depth.

> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7098470..bc17900 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4902,6 +4902,57 @@ unlock:
>  	mutex_unlock(&dev_priv->drrs.mutex);
>  }
>  
> +void intel_edp_drrs_invalidate(struct drm_device *dev,
> +		unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +
> +	if (!dev_priv->drrs.dp)
> +		return;
> +
> +	mutex_lock(&dev_priv->drrs.mutex);
> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +
> +	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
> +		cancel_delayed_work_sync(&dev_priv->drrs.work);
> +		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);
> +}
> +
> +void intel_edp_drrs_flush(struct drm_device *dev,
> +		unsigned frontbuffer_bits)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +
> +	if (!dev_priv->drrs.dp)
> +		return;
> +
> +	mutex_lock(&dev_priv->drrs.mutex);
> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
> +
> +	cancel_delayed_work_sync(&dev_priv->drrs.work);
> +
> +	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
> +			!dev_priv->drrs.busy_frontbuffer_bits)
> +		schedule_delayed_work(&dev_priv->drrs.work,
> +				msecs_to_jiffies(1000));

With mod_delayed_work you can avoid the cancel_work. Also sync will
probably deadlock.
-Daniel

> +	mutex_unlock(&dev_priv->drrs.mutex);
> +}
> +
>  static struct drm_display_mode *
>  intel_dp_drrs_init(struct intel_connector *intel_connector,
>  		struct drm_display_mode *fixed_mode)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1c3c25a..763c097 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1004,6 +1004,9 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  int intel_disable_plane(struct drm_plane *plane);
>  void intel_edp_drrs_enable(struct intel_dp *intel_dp);
>  void intel_edp_drrs_disable(struct intel_dp *intel_dp);
> +void intel_edp_drrs_invalidate(struct drm_device *dev,
> +		unsigned frontbuffer_bits);
> +void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
>  
>  /* intel_dp_mst.c */
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 79f6d72..73cb6e0 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -157,6 +157,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>  	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
>  
>  	intel_psr_invalidate(dev, obj->frontbuffer_bits);
> +	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
>  }
>  
>  /**
> @@ -182,6 +183,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>  
>  	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
>  
> +	intel_edp_drrs_flush(dev, frontbuffer_bits);
>  	intel_psr_flush(dev, frontbuffer_bits);
>  
>  	/*
> -- 
> 2.0.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/8] eDP DRRS based on frontbuffer tracking

[Daniel Vetter]

On Thu, Dec 11, 2014 at 02:22:49AM +0530, Vandana Kannan wrote:
> This patch series inserts DRRS into frontbuffer tracking mechanism.
> 
> 1. Previous submission for this feature was designed considering only eDP
> DRRS. In this series, apart from following fb tracking, changes have been made
> to make structures generic so that it can be of use to any other code
> addition to support DRRS with other display types.
> 2. DRRS support is checked based on VBT setting and panel's capability (if
> more than one RR is supported).
> 3. Based on DRRS support availability, related structures are initialized or
> cleaned up through calls from enable/disable DDI respectively.
> 4. Since flip() indicates busyness, changes have been made to invalidate
> DRRS during flip. This changes RR back to preferred mode RR. New work to set
> low RR is scheduled after a delay of x ms.
> 5. This series includes patches to support RR switching on all platforms.
> 6. A module param has been added, carrying forward the input from the
> previous submission of the feature. This param indicates the delay in ms after
> which a switch to low RR can be made. By default, this is set to 0
> indicating that the feature is disabled.

Looks good overall on a very cursory read. Some smaller comments in reply
to individual patches. A few high-level things here:
- I haven't fully checked the locking, so that still needs to be done in
  the in-depth review. I'll also double-check after having applied
  everything.
- I think intel_dp_set_drrs_state doesn't need to be exported any with the
  frontbuffer tracking implementation? Please add a patch to mark it
  static and remove the header declaration.
- Please add one patch with kerneldoc for the non-static DRRS functions
  (_enable/disable and _flush/invalidate) and a short overview DOC:
  section plus integration into drm.tmpl. See Paulo's in-flight fbc
  patches and the work from Rodrigo with PSR for examples.

I think for detailed review someone who knows the frontbuffer tracking
stuff well would be good. So please either pick Rodrigo or Paulo.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Kannan, Vandana]

On 15-Dec-14 3:17 PM, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote:
>> Adding i915 module parameter for setting drrs_interval. If this param is
>> set to 0, then drrs is disabled. If changed in runtime, then the new interval
>> value will be considered for scheduling the next drrs work.
>> drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.
>
> Nope, please don't hide power saving features behind module options by
> default. New stuff must be enabled by default, otherwise it'll bitrot and
> merging to upstream is fairly useless since we still have the rebase pain
> (just spread out over more people) with none of the testing.
ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second
or let user set this delay at runtime through the same module param 
(excluding the disable feature if interval is 0 part) ?
>
> Also such debug options need to be marked using the _debug variants to
> make sure that people report issues and don't keep using them.
>
> Now talking about validation gets me to the next point: Do we have a basic
> igt to make sure DRRS doesn't break right after merging? I don't think we
> need the full-blown test setup like for psr/fbc, but a basic test to make
> sure it enters/exit RR mode should be there.
libdrm has vbltest which prints refresh rate..
Apart from this, I'm adding downclock mode (if supported) in kms_flip.
Shall I add a test which involves low RR trigger after idle time and 
then forces some activity on screen and switches back to high RR ?
- Vandana

> -Daniel
>
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h    |  1 +
>>   drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
>>   drivers/gpu/drm/i915/intel_dp.c    | 11 ++++++++++-
>>   3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 370cbaa..dba5844 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2381,6 +2381,7 @@ struct i915_params {
>>   	int enable_ips;
>>   	int invert_brightness;
>>   	int enable_cmd_parser;
>> +	int drrs_interval;
>>   	/* leave bools at the end to not create holes */
>>   	bool enable_hangcheck;
>>   	bool fastboot;
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index c91cb20..80492e8 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = {
>>   	.disable_vtd_wa = 0,
>>   	.use_mmio_flip = 0,
>>   	.mmio_debug = 0,
>> +	.drrs_interval = 0,
>>   };
>>
>>   module_param_named(modeset, i915.modeset, int, 0400);
>> @@ -173,3 +174,10 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
>>   MODULE_PARM_DESC(mmio_debug,
>>   	"Enable the MMIO debug code (default: false). This may negatively "
>>   	"affect performance.");
>> +
>> +module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
>> +MODULE_PARM_DESC(drrs_interval,
>> +	"DRRS idleness detection interval  (default: 0 ms). "
>> +	"If this field is set to 0, then seamless DRRS feature "
>> +	"based on idleness detection is disabled. "
>> +	"The interval is to be set in milliseconds.");
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 092ef91..88f46906 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4931,6 +4931,9 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>>   	if (!dev_priv->drrs.dp)
>>   		return;
>>
>> +	if (i915.drrs_interval == 0)
>> +		return;
>> +
>>   	mutex_lock(&dev_priv->drrs.mutex);
>>   	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>>   	pipe = to_intel_crtc(crtc)->pipe;
>> @@ -4958,6 +4961,9 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>>   	if (!dev_priv->drrs.dp)
>>   		return;
>>
>> +	if (i915.drrs_interval == 0)
>> +		return;
>> +
>>   	mutex_lock(&dev_priv->drrs.mutex);
>>   	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>>   	pipe = to_intel_crtc(crtc)->pipe;
>> @@ -4968,7 +4974,7 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>>   	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
>>   			!dev_priv->drrs.busy_frontbuffer_bits)
>>   		schedule_delayed_work(&dev_priv->drrs.work,
>> -				msecs_to_jiffies(1000));
>> +				msecs_to_jiffies(i915.drrs_interval));
>>   	mutex_unlock(&dev_priv->drrs.mutex);
>>   }
>>
>> @@ -4991,6 +4997,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
>>   		return NULL;
>>   	}
>>
>> +	if (i915.drrs_interval == 0)
>> +		DRM_DEBUG_KMS("DRRS disable by flag\n");
>> +
>>   	downclock_mode = intel_find_panel_downclock
>>   					(dev, fixed_mode, connector);
>>
>> --
>> 2.0.1
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/8] eDP DRRS based on frontbuffer tracking

[Kannan, Vandana]

On 15-Dec-14 3:30 PM, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 02:22:49AM +0530, Vandana Kannan wrote:
>> This patch series inserts DRRS into frontbuffer tracking mechanism.
>>
>> 1. Previous submission for this feature was designed considering only eDP
>> DRRS. In this series, apart from following fb tracking, changes have been made
>> to make structures generic so that it can be of use to any other code
>> addition to support DRRS with other display types.
>> 2. DRRS support is checked based on VBT setting and panel's capability (if
>> more than one RR is supported).
>> 3. Based on DRRS support availability, related structures are initialized or
>> cleaned up through calls from enable/disable DDI respectively.
>> 4. Since flip() indicates busyness, changes have been made to invalidate
>> DRRS during flip. This changes RR back to preferred mode RR. New work to set
>> low RR is scheduled after a delay of x ms.
>> 5. This series includes patches to support RR switching on all platforms.
>> 6. A module param has been added, carrying forward the input from the
>> previous submission of the feature. This param indicates the delay in ms after
>> which a switch to low RR can be made. By default, this is set to 0
>> indicating that the feature is disabled.
>
> Looks good overall on a very cursory read. Some smaller comments in reply
> to individual patches. A few high-level things here:
> - I haven't fully checked the locking, so that still needs to be done in
>    the in-depth review. I'll also double-check after having applied
>    everything.
> - I think intel_dp_set_drrs_state doesn't need to be exported any with the
>    frontbuffer tracking implementation? Please add a patch to mark it
>    static and remove the header declaration.
I have made this change as part of patch 1/8..
> - Please add one patch with kerneldoc for the non-static DRRS functions
>    (_enable/disable and _flush/invalidate) and a short overview DOC:
>    section plus integration into drm.tmpl. See Paulo's in-flight fbc
>    patches and the work from Rodrigo with PSR for examples.
Ok sure..

- Vandana
> I think for detailed review someone who knows the frontbuffer tracking
> stuff well would be good. So please either pick Rodrigo or Paulo.
>
> Thanks, Daniel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/8] drm/i915: DRRS calls based on frontbuffer

[Kannan, Vandana]

On 15-Dec-14 3:27 PM, Daniel Vetter wrote:
> On Thu, Dec 11, 2014 at 02:22:53AM +0530, Vandana Kannan wrote:
>> Calls have been added to invalidate/flush DRRS whenever invalidate/flush is
>> called as part of frontbuffer tracking.
>> Apart from calls as a result of GEM tracking to fb invalidate/flush, a
>> call has been added to invalidate fb obj from crtc_page_flip as well. This
>> is to track busyness through flip calls.
>
> That shouldn't be required really. Or maybe I misunderstand what you mean
> here ...
>
Last month when I was implementing this, DRRS worked as expected only 
when this call was made from crtc_page_flip. I checked the recent code 
today and as you mentioned above, this call isn't required.
I can remove this call from this patch..
Having said that, sometime back I observed that Android used 
crtc_page_flip and its possible that DRRS would break in that scenario 
without this call to invalidate..
- Vandana

>> The call to fb_obj_invalidate (in flip) is placed before queuing flip for this
>> obj.
>>
>> drrs_invalidate() and drrs_flush() check for drrs.dp which would be NULL if
>> it was setup in drrs_enable(). This covers for the condition when DRRS is
>> not supported.
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c     |  8 +++--
>>   drivers/gpu/drm/i915/intel_dp.c          | 51 ++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h         |  3 ++
>>   drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
>>   4 files changed, 61 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a9bdc12..a4a565c 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9717,6 +9717,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>   	if (ret)
>>   		goto cleanup_pending;
>>
>> +	i915_gem_track_fb(work->old_fb_obj, obj,
>> +			  INTEL_FRONTBUFFER_PRIMARY(pipe));
>> +
>> +	intel_fb_obj_invalidate(obj, ring);
>> +
>>   	work->gtt_offset =
>>   		i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
>>
>> @@ -9741,9 +9746,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>   	work->flip_queued_vblank = drm_vblank_count(dev, intel_crtc->pipe);
>>   	work->enable_stall_check = true;
>>
>> -	i915_gem_track_fb(work->old_fb_obj, obj,
>> -			  INTEL_FRONTBUFFER_PRIMARY(pipe));
>> -
>>   	intel_disable_fbc(dev);
>>   	intel_frontbuffer_flip_prepare(dev, INTEL_FRONTBUFFER_PRIMARY(pipe));
>>   	mutex_unlock(&dev->struct_mutex);
>
> This hunk here looks spurious. Or is it relate to what you mention in the
> commit message? Please explain more in-depth.
>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 7098470..bc17900 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4902,6 +4902,57 @@ unlock:
>>   	mutex_unlock(&dev_priv->drrs.mutex);
>>   }
>>
>> +void intel_edp_drrs_invalidate(struct drm_device *dev,
>> +		unsigned frontbuffer_bits)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_crtc *crtc;
>> +	enum pipe pipe;
>> +
>> +	if (!dev_priv->drrs.dp)
>> +		return;
>> +
>> +	mutex_lock(&dev_priv->drrs.mutex);
>> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>> +	pipe = to_intel_crtc(crtc)->pipe;
>> +
>> +	if (dev_priv->drrs.refresh_rate_type == DRRS_LOW_RR) {
>> +		cancel_delayed_work_sync(&dev_priv->drrs.work);
>> +		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);
>> +}
>> +
>> +void intel_edp_drrs_flush(struct drm_device *dev,
>> +		unsigned frontbuffer_bits)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_crtc *crtc;
>> +	enum pipe pipe;
>> +
>> +	if (!dev_priv->drrs.dp)
>> +		return;
>> +
>> +	mutex_lock(&dev_priv->drrs.mutex);
>> +	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>> +	pipe = to_intel_crtc(crtc)->pipe;
>> +	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
>> +
>> +	cancel_delayed_work_sync(&dev_priv->drrs.work);
>> +
>> +	if (dev_priv->drrs.refresh_rate_type != DRRS_LOW_RR &&
>> +			!dev_priv->drrs.busy_frontbuffer_bits)
>> +		schedule_delayed_work(&dev_priv->drrs.work,
>> +				msecs_to_jiffies(1000));
>
> With mod_delayed_work you can avoid the cancel_work. Also sync will
> probably deadlock.
> -Daniel
>
>> +	mutex_unlock(&dev_priv->drrs.mutex);
>> +}
>> +
>>   static struct drm_display_mode *
>>   intel_dp_drrs_init(struct intel_connector *intel_connector,
>>   		struct drm_display_mode *fixed_mode)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 1c3c25a..763c097 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1004,6 +1004,9 @@ int intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>   int intel_disable_plane(struct drm_plane *plane);
>>   void intel_edp_drrs_enable(struct intel_dp *intel_dp);
>>   void intel_edp_drrs_disable(struct intel_dp *intel_dp);
>> +void intel_edp_drrs_invalidate(struct drm_device *dev,
>> +		unsigned frontbuffer_bits);
>> +void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
>>
>>   /* intel_dp_mst.c */
>>   int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
>> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
>> index 79f6d72..73cb6e0 100644
>> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
>> @@ -157,6 +157,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>>   	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
>>
>>   	intel_psr_invalidate(dev, obj->frontbuffer_bits);
>> +	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
>>   }
>>
>>   /**
>> @@ -182,6 +183,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
>>
>>   	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
>>
>> +	intel_edp_drrs_flush(dev, frontbuffer_bits);
>>   	intel_psr_flush(dev, frontbuffer_bits);
>>
>>   	/*
>> --
>> 2.0.1
>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Daniel Vetter]

On Mon, Dec 15, 2014 at 04:25:32PM +0530, Kannan, Vandana wrote:
> 
> 
> On 15-Dec-14 3:17 PM, Daniel Vetter wrote:
> >On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote:
> >>Adding i915 module parameter for setting drrs_interval. If this param is
> >>set to 0, then drrs is disabled. If changed in runtime, then the new interval
> >>value will be considered for scheduling the next drrs work.
> >>drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.
> >
> >Nope, please don't hide power saving features behind module options by
> >default. New stuff must be enabled by default, otherwise it'll bitrot and
> >merging to upstream is fairly useless since we still have the rebase pain
> >(just spread out over more people) with none of the testing.
> ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second
> or let user set this delay at runtime through the same module param
> (excluding the disable feature if interval is 0 part) ?

Imo we should just set an optimal value (does vbt have any hints?).

> >Also such debug options need to be marked using the _debug variants to
> >make sure that people report issues and don't keep using them.
> >
> >Now talking about validation gets me to the next point: Do we have a basic
> >igt to make sure DRRS doesn't break right after merging? I don't think we
> >need the full-blown test setup like for psr/fbc, but a basic test to make
> >sure it enters/exit RR mode should be there.
> libdrm has vbltest which prints refresh rate..
> Apart from this, I'm adding downclock mode (if supported) in kms_flip.
> Shall I add a test which involves low RR trigger after idle time and then
> forces some activity on screen and switches back to high RR ?

kms_flip is already huge, probably better to start a new kms_drrs
testcase, similar to kms_fbc_crc (but without crc checks since that's not
ineteresting here). And yeah you should trigger RR entry and then check in
debugfs that it has indeed happened, then pageflip and check that we're
out of RR again. That should be enough to have a decent smoketest for
DRRS. We can add anything missing later on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/8] drm/i915: DRRS calls based on frontbuffer

[Daniel Vetter]

On Mon, Dec 15, 2014 at 07:45:13PM +0530, Kannan, Vandana wrote:
> 
> 
> On 15-Dec-14 3:27 PM, Daniel Vetter wrote:
> >On Thu, Dec 11, 2014 at 02:22:53AM +0530, Vandana Kannan wrote:
> >>Calls have been added to invalidate/flush DRRS whenever invalidate/flush is
> >>called as part of frontbuffer tracking.
> >>Apart from calls as a result of GEM tracking to fb invalidate/flush, a
> >>call has been added to invalidate fb obj from crtc_page_flip as well. This
> >>is to track busyness through flip calls.
> >
> >That shouldn't be required really. Or maybe I misunderstand what you mean
> >here ...
> >
> Last month when I was implementing this, DRRS worked as expected only when
> this call was made from crtc_page_flip. I checked the recent code today and
> as you mentioned above, this call isn't required.
> I can remove this call from this patch..
> Having said that, sometime back I observed that Android used crtc_page_flip
> and its possible that DRRS would break in that scenario without this call to
> invalidate..

We might have had a bug, but in your current patch you only move it
around. You no longer add a new call. So I think we can just remove this.

But please make sure it still works for Android, otherwise we have a big
problem. The intel_frontbuffer_flip_prepare should result in a _flush call
in the frontbuffer tracking code eventually, so it should work. But better
check.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Kannan, Vandana]

On 15-Dec-14 7:46 PM, Daniel Vetter wrote:
> On Mon, Dec 15, 2014 at 04:25:32PM +0530, Kannan, Vandana wrote:
>>
>>
>> On 15-Dec-14 3:17 PM, Daniel Vetter wrote:
>>> On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote:
>>>> Adding i915 module parameter for setting drrs_interval. If this param is
>>>> set to 0, then drrs is disabled. If changed in runtime, then the new interval
>>>> value will be considered for scheduling the next drrs work.
>>>> drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.
>>>
>>> Nope, please don't hide power saving features behind module options by
>>> default. New stuff must be enabled by default, otherwise it'll bitrot and
>>> merging to upstream is fairly useless since we still have the rebase pain
>>> (just spread out over more people) with none of the testing.
>> ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second
>> or let user set this delay at runtime through the same module param
>> (excluding the disable feature if interval is 0 part) ?
>
> Imo we should just set an optimal value (does vbt have any hints?).
>
VBT does not contain a delay value..
Based on data collected from testing so far, 1 second seems stable..
Maybe it can go down to 800ms or so - I can test it out..
Anything as low as 100ms just triggered too many RR switches back and forth.
- Vandana

>>> Also such debug options need to be marked using the _debug variants to
>>> make sure that people report issues and don't keep using them.
>>>
>>> Now talking about validation gets me to the next point: Do we have a basic
>>> igt to make sure DRRS doesn't break right after merging? I don't think we
>>> need the full-blown test setup like for psr/fbc, but a basic test to make
>>> sure it enters/exit RR mode should be there.
>> libdrm has vbltest which prints refresh rate..
>> Apart from this, I'm adding downclock mode (if supported) in kms_flip.
>> Shall I add a test which involves low RR trigger after idle time and then
>> forces some activity on screen and switches back to high RR ?
>
> kms_flip is already huge, probably better to start a new kms_drrs
> testcase, similar to kms_fbc_crc (but without crc checks since that's not
> ineteresting here). And yeah you should trigger RR entry and then check in
> debugfs that it has indeed happened, then pageflip and check that we're
> out of RR again. That should be enough to have a decent smoketest for
> DRRS. We can add anything missing later on.
> -Daniel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: Add drrs_interval module parameter

[Daniel Vetter]

On Mon, Dec 15, 2014 at 07:56:08PM +0530, Kannan, Vandana wrote:
> 
> 
> On 15-Dec-14 7:46 PM, Daniel Vetter wrote:
> >On Mon, Dec 15, 2014 at 04:25:32PM +0530, Kannan, Vandana wrote:
> >>
> >>
> >>On 15-Dec-14 3:17 PM, Daniel Vetter wrote:
> >>>On Thu, Dec 11, 2014 at 02:22:57AM +0530, Vandana Kannan wrote:
> >>>>Adding i915 module parameter for setting drrs_interval. If this param is
> >>>>set to 0, then drrs is disabled. If changed in runtime, then the new interval
> >>>>value will be considered for scheduling the next drrs work.
> >>>>drrs_interval is set to 0 by default, i.e. DRRS is disabled by default.
> >>>
> >>>Nope, please don't hide power saving features behind module options by
> >>>default. New stuff must be enabled by default, otherwise it'll bitrot and
> >>>merging to upstream is fairly useless since we still have the rebase pain
> >>>(just spread out over more people) with none of the testing.
> >>ok.. so, shall I just make the delay (drrs_interval) fixed at 1 second
> >>or let user set this delay at runtime through the same module param
> >>(excluding the disable feature if interval is 0 part) ?
> >
> >Imo we should just set an optimal value (does vbt have any hints?).
> >
> VBT does not contain a delay value..
> Based on data collected from testing so far, 1 second seems stable..
> Maybe it can go down to 800ms or so - I can test it out..
> Anything as low as 100ms just triggered too many RR switches back and forth.

There's not need imo to go to the lowest possible value, there's always a
tradeoff. 1s sounds good (psr has even worse timeout on some panels ...).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx