[PATCH 0/5] Enabling DRRS in the kernel

[PATCH 0/5] Enabling DRRS in the kernel

[Vandana Kannan]

Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which     
enabled swtiching between low and high refresh rates based on the usage         
scenario. This feature is applciable for internal eDP panel. Indication that    
the panel supports DRRS is given by the panel EDID, which would list multiple   
refresh rates for one resolution.                                               
The patch series supports idleness detection in display i915 driver and         
switch to low refresh rate.                                                     
                                                                                
Based on review comments, the functions for idleness detection have been        
restructured.

Pradeep Bhat (3):
  drm/i915: Adding VBT fields to support eDP DRRS feature
  drm/i915: Parse EDID probed modes for DRRS support
  drm/i915: Add support for DRRS to switch RR

Vandana Kannan (2):
  drm/i915: Idleness detection for DRRS
  drm/i915/bdw: Add support for DRRS to switch RR

 drivers/gpu/drm/i915/i915_drv.h      |   25 +++++
 drivers/gpu/drm/i915/i915_reg.h      |    1 +
 drivers/gpu/drm/i915/intel_bios.c    |   23 +++++
 drivers/gpu/drm/i915/intel_bios.h    |   29 ++++++
 drivers/gpu/drm/i915/intel_display.c |   14 +++
 drivers/gpu/drm/i915/intel_dp.c      |  179 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |   40 +++++++-
 drivers/gpu/drm/i915/intel_pm.c      |  122 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |    2 +
 9 files changed, 434 insertions(+), 1 deletion(-)

-- 
1.7.9.5

[PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature

[Vandana Kannan]

From: Pradeep Bhat <pradeep.bhat@intel.com>

This patch reads the DRRS support and Mode type from VBT fields.
The read information will be stored in VBT struct during BIOS
parsing. The above functionality is needed for decision making
whether DRRS feature is supported in i915 driver for eDP panels.
This information helps us decide if seamless DRRS can be done
at runtime to support certain power saving features. This patch
was tested by setting necessary bit in VBT struct and merging
the new VBT with system BIOS so that we can read the value.

v2: Incorporated review comments from Chris Wilson
Removed "intel_" as a prefix for DRRS specific declarations.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |    9 +++++++++
 drivers/gpu/drm/i915/intel_bios.c |   23 +++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ae2c80c..f8fd045 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1174,6 +1174,15 @@ struct intel_vbt_data {
 	int lvds_ssc_freq;
 	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
 
+	/**
+	 * DRRS mode type (Seamless OR Static DRRS)
+	 * drrs_mode Val 0x2 is Seamless DRRS and 0 is Static DRRS.
+	 * These values correspond to the VBT values for drrs mode.
+	 */
+	int drrs_mode;
+	/* DRRS enabled or disabled in VBT */
+	bool drrs_enabled;
+
 	/* eDP */
 	int edp_rate;
 	int edp_lanes;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index f88e507..5b04a69 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -195,6 +195,21 @@ get_lvds_fp_timing(const struct bdb_header *bdb,
 	return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs);
 }
 
+/**
+ * This function returns the 2 bit information pertaining to a panel type
+ * present in a 32 bit field in VBT blocks. There are 16 panel types in VBT
+ * each occupying 2 bits of information in some 32 bit fields of VBT blocks.
+ */
+static int
+get_mode_by_paneltype(unsigned int word)
+{
+	/**
+	 * The caller of this API should interpret the 2 bits
+	 * based on VBT description for that field.
+	 */
+	return (word >> ((panel_type - 1) * 2)) & MODE_MASK;
+}
+
 /* Try to find integrated panel data */
 static void
 parse_lfp_panel_data(struct drm_i915_private *dev_priv,
@@ -218,6 +233,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 
 	panel_type = lvds_options->panel_type;
 
+	dev_priv->vbt.drrs_mode =
+		get_mode_by_paneltype(lvds_options->dps_panel_type_bits);
+	DRM_DEBUG_KMS("DRRS supported mode is : %s\n",
+			(dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS");
+
 	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
 	if (!lvds_lfp_data)
 		return;
@@ -488,6 +508,9 @@ parse_driver_features(struct drm_i915_private *dev_priv,
 
 	if (driver->dual_frequency)
 		dev_priv->render_reclock_avail = true;
+
+	dev_priv->vbt.drrs_enabled = driver->drrs_state;
+	DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state);
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 81ed58c..0a3a751 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -281,6 +281,9 @@ struct bdb_general_definitions {
 	union child_device_config devices[0];
 } __packed;
 
+/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
+#define MODE_MASK		0x3
+
 struct bdb_lvds_options {
 	u8 panel_type;
 	u8 rsvd1;
@@ -293,6 +296,18 @@ struct bdb_lvds_options {
 	u8 lvds_edid:1;
 	u8 rsvd2:1;
 	u8 rsvd4;
+	/* LVDS Panel channel bits stored here */
+	u32 lvds_panel_channel_bits;
+	/* LVDS SSC (Spread Spectrum Clock) bits stored here. */
+	u16 ssc_bits;
+	u16 ssc_freq;
+	u16 ssc_ddt;
+	/* Panel color depth defined here */
+	u16 panel_color_depth;
+	/* LVDS panel type bits stored here */
+	u32 dps_panel_type_bits;
+	/* LVDS backlight control type bits stored here */
+	u32 blt_control_type_bits;
 } __packed;
 
 /* LFP pointer table contains entries to the struct below */
@@ -462,6 +477,20 @@ struct bdb_driver_features {
 
 	u8 hdmi_termination;
 	u8 custom_vbt_version;
+	/* Driver features data block */
+	u16 rmpm_state:1;
+	u16 s2ddt_state:1;
+	u16 dpst_state:1;
+	u16 bltclt_state:1;
+	u16 adb_state:1;
+	u16 drrs_state:1;
+	u16 grs_state:1;
+	u16 gpmt_state:1;
+	u16 tbt_state:1;
+	u16 psr_state:1;
+	u16 ips_state:1;
+	u16 reserved3:4;
+	u16 pc_feature_validity:1;
 } __packed;
 
 #define EDP_18BPP	0
-- 
1.7.9.5

[PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support

[Vandana Kannan]

From: Pradeep Bhat <pradeep.bhat@intel.com>

This patch and finds out the lowest refresh rate supported for the resolution
same as the fixed_mode, based on the implementaion find_panel_downclock.
It also checks the VBT fields to see if panel supports seamless DRRS or not.
Based on above data it marks whether eDP panel supports seamless DRRS or not.
This information is needed for supporting seamless DRRS switch for certain
power saving usecases. This patch is tested by enabling the DRM logs and
user should see whether Seamless DRRS is supported or not.

v2: Daniel's review comments
Modified downclock deduction based on intel_find_panel_downclock

v3: Chris's review comments
Moved edp_downclock_avail and edp_downclock to intel_panel

v4: Jani's review comments.
Changed name of the enum edp_panel_type to drrs_support type.
Change is_drrs_supported to drrs_support of type enum drrs_support_type.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |   45 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |   30 +++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8f17f8f..079b53f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3522,6 +3522,46 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		      I915_READ(pp_div_reg));
 }
 
+static void
+intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
+			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_i915_private *dev_priv = dev->dev_private;
+
+	/**
+	 * Check if PSR is supported by panel and enabled
+	 * if so then DRRS is reported as not supported for Haswell.
+	 */
+	if (INTEL_INFO(dev)->gen < 8 &&	intel_edp_is_psr_enabled(dev)) {
+		DRM_INFO("eDP panel has PSR enabled. Cannot support DRRS\n");
+		return;
+	}
+
+	/* First check if DRRS is enabled from VBT struct */
+	if (!dev_priv->vbt.drrs_enabled) {
+		DRM_INFO("VBT doesn't support DRRS\n");
+		return;
+	}
+
+	intel_connector->panel.downclock_mode =	intel_find_panel_downclock(dev,
+					fixed_mode, connector);
+
+	if (intel_connector->panel.downclock_mode != NULL &&
+		dev_priv->vbt.drrs_mode == SEAMLESS_DRRS_SUPPORT) {
+		intel_connector->panel.edp_downclock_avail = true;
+		intel_connector->panel.edp_downclock =
+			intel_connector->panel.downclock_mode->clock;
+
+		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
+
+		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
+		DRM_INFO("SEAMLESS DRRS supported for eDP panel.\n");
+	}
+}
+
 static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 				     struct intel_connector *intel_connector)
 {
@@ -3535,6 +3575,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	struct drm_display_mode *scan;
 	struct edid *edid;
 
+	intel_dp->drrs_state.drrs_support = DRRS_NOT_SUPPORTED;
+
 	if (!is_edp(intel_dp))
 		return true;
 
@@ -3579,6 +3621,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	list_for_each_entry(scan, &connector->probed_modes, head) {
 		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
 			fixed_mode = drm_mode_duplicate(dev, scan);
+			if (INTEL_INFO(dev)->gen >= 5)
+				intel_dp_drrs_initialize(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 e903432..d208bf5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -168,6 +168,9 @@ struct intel_panel {
 		bool active_low_pwm;
 		struct backlight_device *device;
 	} backlight;
+
+	bool edp_downclock_avail;
+	int edp_downclock;
 };
 
 struct intel_connector {
@@ -462,6 +465,32 @@ struct intel_hdmi {
 
 #define DP_MAX_DOWNSTREAM_PORTS		0x10
 
+/**
+ * This enum is used to indicate the DRRS support type.
+ */
+enum drrs_support_type {
+	DRRS_NOT_SUPPORTED = -1,
+	STATIC_DRRS_SUPPORT = 0, /* 1:1 mapping with VBT */
+	SEAMLESS_DRRS_SUPPORT = 2 /* 1:1 mapping with VBT */ };
+/**
+ * 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 */
+};
+/**
+ * The drrs_info struct will represent the DRRS feature for eDP
+ * panel.
+ */
+struct drrs_info {
+	enum drrs_support_type drrs_support;
+	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
+};
+
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -487,6 +516,7 @@ struct intel_dp {
 	bool want_panel_vdd;
 	bool psr_setup_done;
 	struct intel_connector *attached_connector;
+	struct drrs_info drrs_state;
 };
 
 struct intel_digital_port {
-- 
1.7.9.5

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

[Vandana Kannan]

From: Pradeep Bhat <pradeep.bhat@intel.com>

This patch computes and stored 2nd M/N/TU for switching to different
refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
between alternate refresh rates programmed in 2nd M/N/TU registers.

v2: Daniel's review comments
Computing M2/N2 in compute_config and storing it in crtc_config

v3: Modified reference to edp_downclock and edp_downclock_avail based on the
changes made to move them from dev_private to intel_panel.

v4: Modified references to is_drrs_supported based on the changes made to
rename it to drrs_support.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_dp.c  |  106 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    6 ++-
 3 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f1eece4..57d2b64 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3206,6 +3206,7 @@
 #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
 #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
+#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 079b53f..d1e1d6e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -791,6 +791,21 @@ intel_dp_set_clock(struct intel_encoder *encoder,
 	}
 }
 
+static void
+intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder transcoder = crtc->config.cpu_transcoder;
+
+	I915_WRITE(PIPE_DATA_M2(transcoder),
+		TU_SIZE(m_n->tu) | m_n->gmch_m);
+	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
+	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
+	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
+	return;
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
@@ -894,6 +909,14 @@ found:
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (intel_connector->panel.edp_downclock_avail &&
+	intel_dp->drrs_state.drrs_support == SEAMLESS_DRRS_SUPPORT) {
+		intel_link_compute_m_n(bpp, lane_count,
+				intel_connector->panel.edp_downclock,
+				pipe_config->port_clock,
+				&pipe_config->dp_m2_n2);
+	}
+
 	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
 
 	return true;
@@ -3522,6 +3545,87 @@ 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) {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+	struct intel_crtc_config *config = NULL;
+	struct intel_crtc *intel_crtc = NULL;
+	struct intel_connector *intel_connector = NULL;
+	bool found_edp = false;
+	u32 reg, val;
+	int index = 0;
+
+	if (refresh_rate <= 0) {
+		DRM_INFO("Refresh rate should be positive non-zero.\n");
+		goto out;
+	}
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+			intel_dp = enc_to_intel_dp(&encoder->base);
+			intel_crtc = encoder->new_crtc;
+			if (!intel_crtc) {
+				DRM_INFO("DRRS: intel_crtc not initialized\n");
+				goto out;
+			}
+			config = &intel_crtc->config;
+			intel_connector = intel_dp->attached_connector;
+			found_edp = true;
+			break;
+		}
+	}
+
+	if (!found_edp) {
+		DRM_INFO("DRRS supported for eDP only.\n");
+		goto out;
+	}
+
+	if (intel_dp->drrs_state.drrs_support < SEAMLESS_DRRS_SUPPORT) {
+		DRM_INFO("Seamless DRRS not supported.\n");
+		goto out;
+	}
+
+	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
+		index = DRRS_HIGH_RR;
+	else
+		index = DRRS_LOW_RR;
+
+	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
+		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
+		goto out;
+	}
+
+	if (!intel_crtc->active) {
+		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
+		goto out;
+	}
+
+	mutex_lock(&intel_dp->drrs_state.mutex);
+
+	/* Haswell and below */
+	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
+		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
+		val = I915_READ(reg);
+		if (index > DRRS_HIGH_RR) {
+			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
+		} else
+			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+		I915_WRITE(reg, val);
+	}
+
+	intel_dp->drrs_state.drrs_refresh_rate_type = index;
+	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+
+	mutex_unlock(&intel_dp->drrs_state.mutex);
+
+out:
+	return;
+}
+
 static void
 intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3555,6 +3659,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			intel_connector->panel.downclock_mode->clock;
 
+		mutex_init(&intel_dp->drrs_state.mutex);
+
 		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
 
 		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d208bf5..d1c60fa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -291,6 +291,9 @@ struct intel_crtc_config {
 	int pipe_bpp;
 	struct intel_link_m_n dp_m_n;
 
+	/* m2_n2 for eDP downclock */
+	struct intel_link_m_n dp_m2_n2;
+
 	/*
 	 * Frequence the dpll for the port should run at. Differs from the
 	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
@@ -489,6 +492,7 @@ enum edp_drrs_refresh_rate_type {
 struct drrs_info {
 	enum drrs_support_type drrs_support;
 	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
+	struct mutex mutex;
 };
 
 struct intel_dp {
@@ -761,7 +765,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
-
+extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
 
 /* intel_dsi.c */
 bool intel_dsi_init(struct drm_device *dev);
-- 
1.7.9.5

[PATCH 4/5] drm/i915: Idleness detection for DRRS

[Vandana Kannan]

Adding support to detect display idleness by tracking page flip from
user space. Switch to low refresh rate is triggered after 2 seconds of
idleness. The delay is configurable. If there is a page flip or call to
update the plane, then high refresh rate is applied.
The feature is not used in dual-display mode.

v2: Chris Wilson's review comments incorporated.
Modify idleness detection implementation to make it similar to the
implementation of intel_update_fbc/intel_disable_fbc

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   16 +++++
 drivers/gpu/drm/i915/intel_display.c |   14 ++++
 drivers/gpu/drm/i915/intel_dp.c      |   10 +++
 drivers/gpu/drm/i915/intel_drv.h     |    4 ++
 drivers/gpu/drm/i915/intel_pm.c      |  122 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |    2 +
 6 files changed, 168 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f8fd045..d7308cc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -712,6 +712,21 @@ struct i915_fbc {
 	} no_fbc_reason;
 };
 
+/* configure the number of secs the system must be idle
+ * before DRRS is enabled
+*/
+#define DRRS_IDLENESS_TIME 2000 /* in millisecs */
+
+struct i915_drrs {
+	struct intel_connector *connector;
+	struct intel_dp *dp;
+	struct intel_drrs_work {
+		struct delayed_work work;
+		struct drm_crtc *crtc;
+		int interval;
+	} *drrs_work;
+};
+
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
@@ -1400,6 +1415,7 @@ typedef struct drm_i915_private {
 	int num_plane;
 
 	struct i915_fbc fbc;
+	struct i915_drrs drrs;
 	struct intel_opregion opregion;
 	struct intel_vbt_data vbt;
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a40651e..995d117 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2395,6 +2395,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	}
 
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 	intel_edp_psr_update(dev);
 	mutex_unlock(&dev->struct_mutex);
 
@@ -3559,6 +3560,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
@@ -3600,6 +3602,7 @@ static void haswell_crtc_enable_planes(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -3806,6 +3809,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -3853,6 +3857,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -8226,6 +8231,11 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	drm_gem_object_unreference(&work->old_fb_obj->base);
 
 	intel_update_fbc(dev);
+
+	/* disable current DRRS work scheduled and restart
+	 * to push work by another x seconds
+	 */
+	intel_update_drrs(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
@@ -8665,6 +8675,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 		goto cleanup_pending;
 
 	intel_disable_fbc(dev);
+	intel_disable_drrs(dev);
 	intel_mark_fb_busy(obj, NULL);
 	mutex_unlock(&dev->struct_mutex);
 
@@ -10880,6 +10891,7 @@ void intel_modeset_init(struct drm_device *dev)
 
 	/* Just in case the BIOS is doing something questionable. */
 	intel_disable_fbc(dev);
+	intel_disable_drrs(dev);
 }
 
 static void
@@ -11286,6 +11298,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_disable_fbc(dev);
 
+	intel_disable_drrs(dev);
+
 	intel_disable_gt_powersave(dev);
 
 	ironlake_teardown_rc6(dev);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d1e1d6e..7778808 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3289,11 +3289,18 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
 	struct intel_dp *intel_dp = &intel_dig_port->dp;
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	i2c_del_adapter(&intel_dp->adapter);
 	drm_encoder_cleanup(encoder);
 	if (is_edp(intel_dp)) {
 		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
+		/* DRRS cleanup */
+		if (intel_dp->drrs_state.drrs_support
+					== SEAMLESS_DRRS_SUPPORT) {
+			kfree(dev_priv->drrs.drrs_work);
+			dev_priv->drrs.drrs_work = NULL;
+		}
 		mutex_lock(&dev->mode_config.mutex);
 		ironlake_panel_vdd_off_sync(intel_dp);
 		mutex_unlock(&dev->mode_config.mutex);
@@ -3659,6 +3666,9 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			intel_connector->panel.downclock_mode->clock;
 
+		intel_init_drrs_idleness_detection(dev,
+			intel_connector, intel_dp);
+
 		mutex_init(&intel_dp->drrs_state.mutex);
 
 		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d1c60fa..84cadf8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -902,6 +902,10 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
 void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
 void ilk_wm_get_hw_state(struct drm_device *dev);
 
+void intel_init_drrs_idleness_detection(struct drm_device *dev,
+		struct intel_connector *connector, struct intel_dp *dp);
+void intel_update_drrs(struct drm_device *dev);
+void intel_disable_drrs(struct drm_device *dev);
 
 /* intel_sdvo.c */
 bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index b35f65e..69f31c5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -615,6 +615,128 @@ out_disable:
 	i915_gem_stolen_cleanup_compression(dev);
 }
 
+static void intel_drrs_work_fn(struct work_struct *__work)
+{
+	struct intel_drrs_work *work =
+		container_of(to_delayed_work(__work),
+			     struct intel_drrs_work, work);
+	struct drm_device *dev = work->crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	intel_dp_set_drrs_state(work->crtc->dev,
+		dev_priv->drrs.connector->panel.downclock_mode->vrefresh);
+}
+
+static void intel_cancel_drrs_work(struct drm_i915_private *dev_priv)
+{
+	if (dev_priv->drrs.drrs_work == NULL)
+		return;
+
+	DRM_DEBUG_KMS("cancelling pending DRRS enable\n");
+
+	cancel_delayed_work_sync(&dev_priv->drrs.drrs_work->work);
+}
+
+static void intel_enable_drrs(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	intel_cancel_drrs_work(dev_priv);
+
+	if (dev_priv->drrs.dp->drrs_state.drrs_refresh_rate_type
+							!= DRRS_LOW_RR) {
+		dev_priv->drrs.drrs_work->crtc = crtc;
+
+		/* Delay the actual enabling to let pageflipping cease and the
+		 * display to settle before starting DRRS
+		 */
+		schedule_delayed_work(&dev_priv->drrs.drrs_work->work,
+			msecs_to_jiffies(dev_priv->drrs.drrs_work->interval));
+	}
+}
+
+void intel_disable_drrs(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+
+	/* as part of disable DRRS, reset refresh rate to HIGH_RR */
+	if (dev_priv->drrs.dp->drrs_state.drrs_refresh_rate_type
+							== DRRS_LOW_RR) {
+		intel_cancel_drrs_work(dev_priv);
+		intel_dp_set_drrs_state(dev,
+			dev_priv->drrs.connector->panel.fixed_mode->vrefresh);
+	}
+}
+
+/**
+ * intel_update_drrs - enable/disable DRRS as needed
+ * @dev: the drm_device
+ * @update: if set to true, cancel current work and schedule new work.
+*	    if set to false, cancel current work and disable DRRS.
+*/
+void intel_update_drrs(struct drm_device *dev)
+{
+	struct drm_crtc *crtc = NULL, *tmp_crtc;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	/* if drrs.connector is NULL, then drrs_init did not get called.
+	 * which means DRRS is not supported.
+	 */
+	if (dev_priv->drrs.connector == NULL) {
+		DRM_INFO("DRRS is not supported.\n");
+		return;
+	}
+
+	list_for_each_entry(tmp_crtc, &dev->mode_config.crtc_list, head) {
+		if (tmp_crtc != NULL && intel_crtc_active(tmp_crtc) &&
+		    to_intel_crtc(tmp_crtc)->primary_enabled) {
+			if (crtc) {
+				DRM_DEBUG_KMS(
+				"more than one pipe active, disabling DRRS\n");
+				goto out_drrs_disable;
+			}
+			crtc = tmp_crtc;
+		}
+	}
+
+	if (crtc == NULL) {
+		DRM_INFO("DRRS: crtc not initialized\n");
+		return;
+	}
+
+	intel_disable_drrs(dev);
+
+	/* re-enable idleness detection */
+	intel_enable_drrs(crtc);
+
+out_drrs_disable:
+	intel_disable_drrs(dev);
+}
+
+void intel_init_drrs_idleness_detection(struct drm_device *dev,
+					struct intel_connector *connector,
+					struct intel_dp *dp)
+{
+	struct intel_drrs_work *work;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	work = kzalloc(sizeof(struct intel_drrs_work), GFP_KERNEL);
+	if (!work) {
+		DRM_ERROR("Failed to allocate DRRS work structure\n");
+		return;
+	}
+
+	dev_priv->drrs.connector = connector;
+	dev_priv->drrs.dp = dp;
+
+	work->interval = DRRS_IDLENESS_TIME;
+	INIT_DELAYED_WORK(&work->work, intel_drrs_work_fn);
+
+	dev_priv->drrs.drrs_work = work;
+}
+
 static void i915_pineview_get_mem_freq(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 90a3f6d..c3dcffd 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -560,6 +560,7 @@ intel_enable_primary(struct drm_crtc *crtc)
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 	mutex_unlock(&dev->struct_mutex);
 }
 
@@ -579,6 +580,7 @@ intel_disable_primary(struct drm_crtc *crtc)
 	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == intel_crtc->plane)
 		intel_disable_fbc(dev);
+	intel_disable_drrs(dev);
 	mutex_unlock(&dev->struct_mutex);
 
 	/*
-- 
1.7.9.5

[PATCH 5/5] 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.

v2: Incorporated Chris's review comments
Changed to check for gen >=8 or gen > 5 before setting M/N registers

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7778808..f18a585 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -798,11 +798,20 @@ intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum transcoder transcoder = crtc->config.cpu_transcoder;
 
-	I915_WRITE(PIPE_DATA_M2(transcoder),
-		TU_SIZE(m_n->tu) | m_n->gmch_m);
-	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
-	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
-	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
+	if (INTEL_INFO(dev)->gen >= 8) {
+		I915_WRITE(PIPE_DATA_M1(transcoder),
+			TU_SIZE(m_n->tu) | m_n->gmch_m);
+		I915_WRITE(PIPE_DATA_N1(transcoder), m_n->gmch_n);
+		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
+		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
+	} else if (INTEL_INFO(dev)->gen >= 5) {
+		I915_WRITE(PIPE_DATA_M2(transcoder),
+			TU_SIZE(m_n->tu) | m_n->gmch_m);
+		I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
+		I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
+		I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
+	}
+
 	return;
 }
 
@@ -3612,8 +3621,17 @@ intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate) {
 
 	mutex_lock(&intel_dp->drrs_state.mutex);
 
-	/* Haswell and below */
-	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
+	if (INTEL_INFO(dev)->gen >= 8) {
+		switch (index) {
+		case DRRS_HIGH_RR:
+			intel_dp_set_m2_n2(intel_crtc, &config->dp_m_n);
+			break;
+		case DRRS_LOW_RR:
+			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
+			break;
+		};
+	} else if (INTEL_INFO(dev)->gen >= 5) {
+		/* Haswell and below */
 		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
 		val = I915_READ(reg);
 		if (index > DRRS_HIGH_RR) {
-- 
1.7.9.5

Re: [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature

[Jani Nikula]

On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
> From: Pradeep Bhat <pradeep.bhat@intel.com>
>
> This patch reads the DRRS support and Mode type from VBT fields.
> The read information will be stored in VBT struct during BIOS
> parsing. The above functionality is needed for decision making
> whether DRRS feature is supported in i915 driver for eDP panels.
> This information helps us decide if seamless DRRS can be done
> at runtime to support certain power saving features. This patch
> was tested by setting necessary bit in VBT struct and merging
> the new VBT with system BIOS so that we can read the value.
>
> v2: Incorporated review comments from Chris Wilson
> Removed "intel_" as a prefix for DRRS specific declarations.
>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |    9 +++++++++
>  drivers/gpu/drm/i915/intel_bios.c |   23 +++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>  3 files changed, 61 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ae2c80c..f8fd045 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1174,6 +1174,15 @@ struct intel_vbt_data {
>  	int lvds_ssc_freq;
>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>  
> +	/**

This is not a kerneldoc comment, so drop the extra *.

> +	 * DRRS mode type (Seamless OR Static DRRS)
> +	 * drrs_mode Val 0x2 is Seamless DRRS and 0 is Static DRRS.
> +	 * These values correspond to the VBT values for drrs mode.
> +	 */
> +	int drrs_mode;
> +	/* DRRS enabled or disabled in VBT */
> +	bool drrs_enabled;

Both the drrs_mode and drrs_enabled should be replaced by one enum
drrs_support_type which you introduce later. It's all self-explanatory
that way, and you don't need to explain so much.

> +
>  	/* eDP */
>  	int edp_rate;
>  	int edp_lanes;
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index f88e507..5b04a69 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -195,6 +195,21 @@ get_lvds_fp_timing(const struct bdb_header *bdb,
>  	return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs);
>  }
>  
> +/**
> + * This function returns the 2 bit information pertaining to a panel type
> + * present in a 32 bit field in VBT blocks. There are 16 panel types in VBT
> + * each occupying 2 bits of information in some 32 bit fields of VBT blocks.
> + */
> +static int
> +get_mode_by_paneltype(unsigned int word)
> +{
> +	/**
> +	 * The caller of this API should interpret the 2 bits
> +	 * based on VBT description for that field.
> +	 */
> +	return (word >> ((panel_type - 1) * 2)) & MODE_MASK;

Everywhere else in intel_bios.c panel_type is used 0-based.

> +}

You use the above function only once. IMHO with all the explaining above
it's just too much of a burden to the reader. Just do this in the code.

> +
>  /* Try to find integrated panel data */
>  static void
>  parse_lfp_panel_data(struct drm_i915_private *dev_priv,
> @@ -218,6 +233,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  
>  	panel_type = lvds_options->panel_type;
>  
> +	dev_priv->vbt.drrs_mode =
> +		get_mode_by_paneltype(lvds_options->dps_panel_type_bits);
> +	DRM_DEBUG_KMS("DRRS supported mode is : %s\n",
                                             ^ drop the space here

> +			(dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS");

Why shouting?

> +
>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>  	if (!lvds_lfp_data)
>  		return;
> @@ -488,6 +508,9 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>  
>  	if (driver->dual_frequency)
>  		dev_priv->render_reclock_avail = true;
> +
> +	dev_priv->vbt.drrs_enabled = driver->drrs_state;
> +	DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state);
                                         ^ and here and everywhere

>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 81ed58c..0a3a751 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -281,6 +281,9 @@ struct bdb_general_definitions {
>  	union child_device_config devices[0];
>  } __packed;
>  
> +/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
> +#define MODE_MASK		0x3
> +
>  struct bdb_lvds_options {
>  	u8 panel_type;
>  	u8 rsvd1;
> @@ -293,6 +296,18 @@ struct bdb_lvds_options {
>  	u8 lvds_edid:1;
>  	u8 rsvd2:1;
>  	u8 rsvd4;
> +	/* LVDS Panel channel bits stored here */
> +	u32 lvds_panel_channel_bits;

Why does this have "lvds" but the rest not? Why do some fields end in
"bits" but some others not? Some consistency please.

> +	/* LVDS SSC (Spread Spectrum Clock) bits stored here. */
> +	u16 ssc_bits;
> +	u16 ssc_freq;
> +	u16 ssc_ddt;
> +	/* Panel color depth defined here */
> +	u16 panel_color_depth;

I really *really* wish I knew why some stuff is specified in the lvds
bios blob and some other in edp blob and some stuff specified here is
used in the edp side. Ugh. I wonder if we should check this
panel_color_depth for edp too.

> +	/* LVDS panel type bits stored here */
> +	u32 dps_panel_type_bits;
> +	/* LVDS backlight control type bits stored here */
> +	u32 blt_control_type_bits;
>  } __packed;
>  
>  /* LFP pointer table contains entries to the struct below */
> @@ -462,6 +477,20 @@ struct bdb_driver_features {
>  
>  	u8 hdmi_termination;
>  	u8 custom_vbt_version;
> +	/* Driver features data block */
> +	u16 rmpm_state:1;
> +	u16 s2ddt_state:1;
> +	u16 dpst_state:1;
> +	u16 bltclt_state:1;
> +	u16 adb_state:1;
> +	u16 drrs_state:1;
> +	u16 grs_state:1;
> +	u16 gpmt_state:1;
> +	u16 tbt_state:1;
> +	u16 psr_state:1;
> +	u16 ips_state:1;

All of the above should be foo_enabled to make them self-explanatory.

> +	u16 reserved3:4;
> +	u16 pc_feature_validity:1;

Similarly for this one, should be pc_feature_valid.

>  } __packed;
>  
>  #define EDP_18BPP	0
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support

[Jani Nikula]

On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
> From: Pradeep Bhat <pradeep.bhat@intel.com>
>
> This patch and finds out the lowest refresh rate supported for the resolution
> same as the fixed_mode, based on the implementaion find_panel_downclock.
> It also checks the VBT fields to see if panel supports seamless DRRS or not.
> Based on above data it marks whether eDP panel supports seamless DRRS or not.
> This information is needed for supporting seamless DRRS switch for certain
> power saving usecases. This patch is tested by enabling the DRM logs and
> user should see whether Seamless DRRS is supported or not.

This patch (and therefore the later patches) no longer apply to
drm-intel-nightly. It might affect my review a bit, but here goes
anyway.

>
> v2: Daniel's review comments
> Modified downclock deduction based on intel_find_panel_downclock
>
> v3: Chris's review comments
> Moved edp_downclock_avail and edp_downclock to intel_panel
>
> v4: Jani's review comments.
> Changed name of the enum edp_panel_type to drrs_support type.
> Change is_drrs_supported to drrs_support of type enum drrs_support_type.
>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |   45 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |   30 +++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8f17f8f..079b53f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3522,6 +3522,46 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		      I915_READ(pp_div_reg));
>  }
>  
> +static void
> +intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
> +			struct intel_connector *intel_connector,
> +			struct drm_display_mode *fixed_mode) {

I'll explain later why I think you should change the signature of the
function.

> +	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_i915_private *dev_priv = dev->dev_private;
> +
> +	/**
> +	 * Check if PSR is supported by panel and enabled
> +	 * if so then DRRS is reported as not supported for Haswell.
> +	 */
> +	if (INTEL_INFO(dev)->gen < 8 &&	intel_edp_is_psr_enabled(dev)) {
> +		DRM_INFO("eDP panel has PSR enabled. Cannot support DRRS\n");
> +		return;
> +	}
> +
> +	/* First check if DRRS is enabled from VBT struct */
> +	if (!dev_priv->vbt.drrs_enabled) {
> +		DRM_INFO("VBT doesn't support DRRS\n");
> +		return;
> +	}
> +
> +	intel_connector->panel.downclock_mode =	intel_find_panel_downclock(dev,
> +					fixed_mode, connector);
> +
> +	if (intel_connector->panel.downclock_mode != NULL &&
> +		dev_priv->vbt.drrs_mode == SEAMLESS_DRRS_SUPPORT) {
> +		intel_connector->panel.edp_downclock_avail = true;

If you rearranged the code a bit, you could make the
panel.downclock_mode != NULL mean the same as
edp_downclock_avail. I.e. if you have the downclock_mode there, it's
available.

> +		intel_connector->panel.edp_downclock =
> +			intel_connector->panel.downclock_mode->clock;

I don't understand why you need two copies of the clock.

In general, we should try and avoid adding extra state and copies of
information for stuff that we can readily derive from other information.

> +
> +		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;

Again. I can't see intel_dp->drrs_state.drrs_support ever needing to be
different from dev_priv->vbt.drrs_mode. So why the copy?

> +
> +		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
> +		DRM_INFO("SEAMLESS DRRS supported for eDP panel.\n");
> +	}
> +}
> +
>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  				     struct intel_connector *intel_connector)
>  {
> @@ -3535,6 +3575,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
>  
> +	intel_dp->drrs_state.drrs_support = DRRS_NOT_SUPPORTED;
> +
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> @@ -3579,6 +3621,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	list_for_each_entry(scan, &connector->probed_modes, head) {
>  		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
>  			fixed_mode = drm_mode_duplicate(dev, scan);
> +			if (INTEL_INFO(dev)->gen >= 5)
> +				intel_dp_drrs_initialize(intel_dig_port,
> +					intel_connector, fixed_mode);

Is there any reason not to do this at the top level after checking for
the VBT mode?

Also, we have a separate function for initializing the panel struct, so
I think you should make intel_dp_drrs_initialize() return the downclock
mode or NULL, and pass that to intel_panel_init() instead of
initializing the panel struct directly within the function.

>  			break;
>  		}
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e903432..d208bf5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -168,6 +168,9 @@ struct intel_panel {
>  		bool active_low_pwm;
>  		struct backlight_device *device;
>  	} backlight;
> +
> +	bool edp_downclock_avail;
> +	int edp_downclock;

As I said, I think you can get rid of both of these.

>  };
>  
>  struct intel_connector {
> @@ -462,6 +465,32 @@ struct intel_hdmi {
>  
>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>  
> +/**
> + * This enum is used to indicate the DRRS support type.
> + */
> +enum drrs_support_type {
> +	DRRS_NOT_SUPPORTED = -1,
> +	STATIC_DRRS_SUPPORT = 0, /* 1:1 mapping with VBT */
> +	SEAMLESS_DRRS_SUPPORT = 2 /* 1:1 mapping with VBT */ };

I don't see any value in having 1:1 mapping with VBT. Not even in having
1:1 mapping between struct intel_vbt_data and the actual VBT. It's
supposed to be parsed data.

Instead, I do see value in making DRRS_NOT_SUPPORTED == 0 as the logical
thing to do.

> +/**
> + * 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 */
> +};
> +/**
> + * The drrs_info struct will represent the DRRS feature for eDP
> + * panel.
> + */

This comment does not add any value.

> +struct drrs_info {
> +	enum drrs_support_type drrs_support;
> +	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;

Because this will be accessed through intel_dp->drrs_state, there's no
need to duplicate "drrs" in the field names here. It will be obvious
from the context.

> +};
> +
>  struct intel_dp {
>  	uint32_t output_reg;
>  	uint32_t aux_ch_ctl_reg;
> @@ -487,6 +516,7 @@ struct intel_dp {
>  	bool want_panel_vdd;
>  	bool psr_setup_done;
>  	struct intel_connector *attached_connector;
> +	struct drrs_info drrs_state;
>  };
>  
>  struct intel_digital_port {
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/5] drm/i915: Add support for DRRS to switch RR

[Jani Nikula]

On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
> From: Pradeep Bhat <pradeep.bhat@intel.com>
>
> This patch computes and stored 2nd M/N/TU for switching to different
> refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
> between alternate refresh rates programmed in 2nd M/N/TU registers.
>
> v2: Daniel's review comments
> Computing M2/N2 in compute_config and storing it in crtc_config
>
> v3: Modified reference to edp_downclock and edp_downclock_avail based on the
> changes made to move them from dev_private to intel_panel.
>
> v4: Modified references to is_drrs_supported based on the changes made to
> rename it to drrs_support.
>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>  drivers/gpu/drm/i915/intel_dp.c  |  106 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>  3 files changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f1eece4..57d2b64 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3206,6 +3206,7 @@
>  #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
>  #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
>  #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
> +#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
>  #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
>  #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
>  #define   PIPECONF_BPC_MASK	(0x7 << 5)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 079b53f..d1e1d6e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -791,6 +791,21 @@ intel_dp_set_clock(struct intel_encoder *encoder,
>  	}
>  }
>  
> +static void
> +intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum transcoder transcoder = crtc->config.cpu_transcoder;
> +
> +	I915_WRITE(PIPE_DATA_M2(transcoder),
> +		TU_SIZE(m_n->tu) | m_n->gmch_m);
> +	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
> +	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
> +	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
> +	return;

Superfluous return statement.

> +}
> +
>  bool
>  intel_dp_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_config *pipe_config)
> @@ -894,6 +909,14 @@ found:
>  			       pipe_config->port_clock,
>  			       &pipe_config->dp_m_n);
>  
> +	if (intel_connector->panel.edp_downclock_avail &&
> +	intel_dp->drrs_state.drrs_support == SEAMLESS_DRRS_SUPPORT) {
> +		intel_link_compute_m_n(bpp, lane_count,
> +				intel_connector->panel.edp_downclock,
> +				pipe_config->port_clock,
> +				&pipe_config->dp_m2_n2);
> +	}

Indentation in the above block.

> +
>  	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>  
>  	return true;
> @@ -3522,6 +3545,87 @@ 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) {

The opening brace belongs on a new line.

> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_mode_config *mode_config = &dev->mode_config;
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp = NULL;
> +	struct intel_crtc_config *config = NULL;
> +	struct intel_crtc *intel_crtc = NULL;
> +	struct intel_connector *intel_connector = NULL;
> +	bool found_edp = false;
> +	u32 reg, val;
> +	int index = 0;
> +
> +	if (refresh_rate <= 0) {
> +		DRM_INFO("Refresh rate should be positive non-zero.\n");
> +		goto out;
> +	}

Under what conditions do you expect this to happen?

> +
> +	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
> +		if (encoder->type == INTEL_OUTPUT_EDP) {
> +			intel_dp = enc_to_intel_dp(&encoder->base);
> +			intel_crtc = encoder->new_crtc;
> +			if (!intel_crtc) {
> +				DRM_INFO("DRRS: intel_crtc not initialized\n");

DRM_INFO is probably a bit verbose.

> +				goto out;
> +			}
> +			config = &intel_crtc->config;
> +			intel_connector = intel_dp->attached_connector;
> +			found_edp = true;
> +			break;
> +		}
> +	}

I'm confused. You go through all the trouble of saving the crtc and the
connector (although only in the next patch) but here you iterate all the
encoders to find the one. Why?

> +
> +	if (!found_edp) {

There's no need to add an extra variable for this. You already have
intel_dp, intel_crtc, config and intel_connector you can check!

> +		DRM_INFO("DRRS supported for eDP only.\n");
> +		goto out;
> +	}
> +
> +	if (intel_dp->drrs_state.drrs_support < SEAMLESS_DRRS_SUPPORT) {
> +		DRM_INFO("Seamless DRRS not supported.\n");
> +		goto out;
> +	}
> +
> +	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
> +		index = DRRS_HIGH_RR;
> +	else
> +		index = DRRS_LOW_RR;
> +
> +	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
> +		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
> +		goto out;
> +	}
> +
> +	if (!intel_crtc->active) {
> +		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
> +		goto out;
> +	}
> +
> +	mutex_lock(&intel_dp->drrs_state.mutex);
> +
> +	/* Haswell and below */
> +	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
> +		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
> +		val = I915_READ(reg);
> +		if (index > DRRS_HIGH_RR) {
> +			val |= PIPECONF_EDP_RR_MODE_SWITCH;

Please check bspec for workarounds you need to do wrt frame start delay
before enabling the pipe/transcoder.

> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
> +		} else
> +			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;

Per coding style, if one branch needs braces, then all do.

> +		I915_WRITE(reg, val);
> +	}
> +
> +	intel_dp->drrs_state.drrs_refresh_rate_type = index;
> +	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
> +
> +	mutex_unlock(&intel_dp->drrs_state.mutex);
> +
> +out:
> +	return;

Superfluous return statement.

> +}
> +
>  static void
>  intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>  			struct intel_connector *intel_connector,
> @@ -3555,6 +3659,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>  		intel_connector->panel.edp_downclock =
>  			intel_connector->panel.downclock_mode->clock;
>  
> +		mutex_init(&intel_dp->drrs_state.mutex);
> +
>  		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>  
>  		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d208bf5..d1c60fa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -291,6 +291,9 @@ struct intel_crtc_config {
>  	int pipe_bpp;
>  	struct intel_link_m_n dp_m_n;
>  
> +	/* m2_n2 for eDP downclock */
> +	struct intel_link_m_n dp_m2_n2;
> +
>  	/*
>  	 * Frequence the dpll for the port should run at. Differs from the
>  	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
> @@ -489,6 +492,7 @@ enum edp_drrs_refresh_rate_type {
>  struct drrs_info {
>  	enum drrs_support_type drrs_support;
>  	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
> +	struct mutex mutex;
>  };
>  
>  struct intel_dp {
> @@ -761,7 +765,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  void intel_edp_psr_update(struct drm_device *dev);
> -
> +extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);

No need for the "extern".

>  
>  /* intel_dsi.c */
>  bool intel_dsi_init(struct drm_device *dev);
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/5] drm/i915: Idleness detection for DRRS

[Jani Nikula]

On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
> Adding support to detect display idleness by tracking page flip from
> user space. Switch to low refresh rate is triggered after 2 seconds of
> idleness. The delay is configurable. If there is a page flip or call to
> update the plane, then high refresh rate is applied.
> The feature is not used in dual-display mode.
>
> v2: Chris Wilson's review comments incorporated.
> Modify idleness detection implementation to make it similar to the
> implementation of intel_update_fbc/intel_disable_fbc
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   16 +++++
>  drivers/gpu/drm/i915/intel_display.c |   14 ++++
>  drivers/gpu/drm/i915/intel_dp.c      |   10 +++
>  drivers/gpu/drm/i915/intel_drv.h     |    4 ++
>  drivers/gpu/drm/i915/intel_pm.c      |  122 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |    2 +
>  6 files changed, 168 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f8fd045..d7308cc 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -712,6 +712,21 @@ struct i915_fbc {
>  	} no_fbc_reason;
>  };
>  
> +/* configure the number of secs the system must be idle
> + * before DRRS is enabled
> +*/
> +#define DRRS_IDLENESS_TIME 2000 /* in millisecs */
> +
> +struct i915_drrs {
> +	struct intel_connector *connector;
> +	struct intel_dp *dp;
> +	struct intel_drrs_work {
> +		struct delayed_work work;
> +		struct drm_crtc *crtc;
> +		int interval;

I'll probably see this more useful as a module parameter than a field
here, with 0 meaning disable.

> +	} *drrs_work;
> +};
> +
>  struct i915_psr {
>  	bool sink_support;
>  	bool source_ok;
> @@ -1400,6 +1415,7 @@ typedef struct drm_i915_private {
>  	int num_plane;
>  
>  	struct i915_fbc fbc;
> +	struct i915_drrs drrs;
>  	struct intel_opregion opregion;
>  	struct intel_vbt_data vbt;
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a40651e..995d117 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2395,6 +2395,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  	}
>  
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  	intel_edp_psr_update(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -3559,6 +3560,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
> @@ -3600,6 +3602,7 @@ static void haswell_crtc_enable_planes(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> @@ -3806,6 +3809,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> @@ -3853,6 +3857,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> @@ -8226,6 +8231,11 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  	drm_gem_object_unreference(&work->old_fb_obj->base);
>  
>  	intel_update_fbc(dev);
> +
> +	/* disable current DRRS work scheduled and restart
> +	 * to push work by another x seconds
> +	 */
> +	intel_update_drrs(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
> @@ -8665,6 +8675,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  		goto cleanup_pending;
>  
>  	intel_disable_fbc(dev);
> +	intel_disable_drrs(dev);
>  	intel_mark_fb_busy(obj, NULL);
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -10880,6 +10891,7 @@ void intel_modeset_init(struct drm_device *dev)
>  
>  	/* Just in case the BIOS is doing something questionable. */
>  	intel_disable_fbc(dev);
> +	intel_disable_drrs(dev);
>  }
>  
>  static void
> @@ -11286,6 +11298,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	intel_disable_fbc(dev);
>  
> +	intel_disable_drrs(dev);
> +
>  	intel_disable_gt_powersave(dev);
>  
>  	ironlake_teardown_rc6(dev);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d1e1d6e..7778808 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3289,11 +3289,18 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	i2c_del_adapter(&intel_dp->adapter);
>  	drm_encoder_cleanup(encoder);
>  	if (is_edp(intel_dp)) {
>  		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> +		/* DRRS cleanup */
> +		if (intel_dp->drrs_state.drrs_support
> +					== SEAMLESS_DRRS_SUPPORT) {
> +			kfree(dev_priv->drrs.drrs_work);
> +			dev_priv->drrs.drrs_work = NULL;
> +		}
>  		mutex_lock(&dev->mode_config.mutex);
>  		ironlake_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.mutex);
> @@ -3659,6 +3666,9 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>  		intel_connector->panel.edp_downclock =
>  			intel_connector->panel.downclock_mode->clock;
>  
> +		intel_init_drrs_idleness_detection(dev,
> +			intel_connector, intel_dp);
> +
>  		mutex_init(&intel_dp->drrs_state.mutex);
>  
>  		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d1c60fa..84cadf8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -902,6 +902,10 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>  void ilk_wm_get_hw_state(struct drm_device *dev);
>  
> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
> +		struct intel_connector *connector, struct intel_dp *dp);
> +void intel_update_drrs(struct drm_device *dev);
> +void intel_disable_drrs(struct drm_device *dev);
>  
>  /* intel_sdvo.c */
>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index b35f65e..69f31c5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -615,6 +615,128 @@ out_disable:
>  	i915_gem_stolen_cleanup_compression(dev);
>  }
>  
> +static void intel_drrs_work_fn(struct work_struct *__work)
> +{
> +	struct intel_drrs_work *work =
> +		container_of(to_delayed_work(__work),
> +			     struct intel_drrs_work, work);
> +	struct drm_device *dev = work->crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	intel_dp_set_drrs_state(work->crtc->dev,
> +		dev_priv->drrs.connector->panel.downclock_mode->vrefresh);
> +}
> +
> +static void intel_cancel_drrs_work(struct drm_i915_private *dev_priv)
> +{
> +	if (dev_priv->drrs.drrs_work == NULL)
> +		return;
> +
> +	DRM_DEBUG_KMS("cancelling pending DRRS enable\n");
> +
> +	cancel_delayed_work_sync(&dev_priv->drrs.drrs_work->work);
> +}
> +
> +static void intel_enable_drrs(struct drm_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	intel_cancel_drrs_work(dev_priv);
> +
> +	if (dev_priv->drrs.dp->drrs_state.drrs_refresh_rate_type
> +							!= DRRS_LOW_RR) {
> +		dev_priv->drrs.drrs_work->crtc = crtc;
> +
> +		/* Delay the actual enabling to let pageflipping cease and the
> +		 * display to settle before starting DRRS
> +		 */
> +		schedule_delayed_work(&dev_priv->drrs.drrs_work->work,
> +			msecs_to_jiffies(dev_priv->drrs.drrs_work->interval));
> +	}
> +}
> +
> +void intel_disable_drrs(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +
> +	/* as part of disable DRRS, reset refresh rate to HIGH_RR */
> +	if (dev_priv->drrs.dp->drrs_state.drrs_refresh_rate_type
> +							== DRRS_LOW_RR) {
> +		intel_cancel_drrs_work(dev_priv);
> +		intel_dp_set_drrs_state(dev,
> +			dev_priv->drrs.connector->panel.fixed_mode->vrefresh);
> +	}
> +}
> +
> +/**
> + * intel_update_drrs - enable/disable DRRS as needed
> + * @dev: the drm_device
> + * @update: if set to true, cancel current work and schedule new work.
> +*	    if set to false, cancel current work and disable DRRS.

There's no update parameter.

> +*/
> +void intel_update_drrs(struct drm_device *dev)
> +{
> +	struct drm_crtc *crtc = NULL, *tmp_crtc;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	/* if drrs.connector is NULL, then drrs_init did not get called.
> +	 * which means DRRS is not supported.
> +	 */
> +	if (dev_priv->drrs.connector == NULL) {
> +		DRM_INFO("DRRS is not supported.\n");
> +		return;
> +	}
> +
> +	list_for_each_entry(tmp_crtc, &dev->mode_config.crtc_list, head) {
> +		if (tmp_crtc != NULL && intel_crtc_active(tmp_crtc) &&
> +		    to_intel_crtc(tmp_crtc)->primary_enabled) {
> +			if (crtc) {
> +				DRM_DEBUG_KMS(
> +				"more than one pipe active, disabling DRRS\n");
> +				goto out_drrs_disable;
> +			}
> +			crtc = tmp_crtc;
> +		}
> +	}
> +
> +	if (crtc == NULL) {
> +		DRM_INFO("DRRS: crtc not initialized\n");
> +		return;
> +	}
> +
> +	intel_disable_drrs(dev);
> +
> +	/* re-enable idleness detection */
> +	intel_enable_drrs(crtc);
> +
> +out_drrs_disable:
> +	intel_disable_drrs(dev);

The happy day disable, enable, disable sequence seems like a bug.

> +}
> +
> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
> +					struct intel_connector *connector,
> +					struct intel_dp *dp)
> +{
> +	struct intel_drrs_work *work;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	work = kzalloc(sizeof(struct intel_drrs_work), GFP_KERNEL);
> +	if (!work) {
> +		DRM_ERROR("Failed to allocate DRRS work structure\n");
> +		return;
> +	}
> +
> +	dev_priv->drrs.connector = connector;
> +	dev_priv->drrs.dp = dp;
> +
> +	work->interval = DRRS_IDLENESS_TIME;
> +	INIT_DELAYED_WORK(&work->work, intel_drrs_work_fn);
> +
> +	dev_priv->drrs.drrs_work = work;
> +}
> +
>  static void i915_pineview_get_mem_freq(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 90a3f6d..c3dcffd 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -560,6 +560,7 @@ intel_enable_primary(struct drm_crtc *crtc)
>  
>  	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> @@ -579,6 +580,7 @@ intel_disable_primary(struct drm_crtc *crtc)
>  	mutex_lock(&dev->struct_mutex);
>  	if (dev_priv->fbc.plane == intel_crtc->plane)
>  		intel_disable_fbc(dev);
> +	intel_disable_drrs(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	/*
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Jani Nikula]

On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
> 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.
>
> v2: Incorporated Chris's review comments
> Changed to check for gen >=8 or gen > 5 before setting M/N registers
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7778808..f18a585 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -798,11 +798,20 @@ intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum transcoder transcoder = crtc->config.cpu_transcoder;
>  
> -	I915_WRITE(PIPE_DATA_M2(transcoder),
> -		TU_SIZE(m_n->tu) | m_n->gmch_m);
> -	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
> -	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
> -	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
> +	if (INTEL_INFO(dev)->gen >= 8) {
> +		I915_WRITE(PIPE_DATA_M1(transcoder),
> +			TU_SIZE(m_n->tu) | m_n->gmch_m);
> +		I915_WRITE(PIPE_DATA_N1(transcoder), m_n->gmch_n);
> +		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
> +		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);

There's already a function for this part, called
intel_cpu_transcoder_set_m_n. Reuse it.

> +	} else if (INTEL_INFO(dev)->gen >= 5) {
> +		I915_WRITE(PIPE_DATA_M2(transcoder),
> +			TU_SIZE(m_n->tu) | m_n->gmch_m);
> +		I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
> +		I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
> +		I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
> +	}
> +
>  	return;
>  }
>  
> @@ -3612,8 +3621,17 @@ intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate) {
>  
>  	mutex_lock(&intel_dp->drrs_state.mutex);
>  
> -	/* Haswell and below */

Forgot to mention in the earlier patch that comments like this are
redundant with the code. And in this case, it already *contradicts* the
code.

> -	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
> +	if (INTEL_INFO(dev)->gen >= 8) {
> +		switch (index) {
> +		case DRRS_HIGH_RR:
> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m_n);
> +			break;
> +		case DRRS_LOW_RR:
> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
> +			break;
> +		};
> +	} else if (INTEL_INFO(dev)->gen >= 5) {
> +		/* Haswell and below */
>  		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
>  		val = I915_READ(reg);
>  		if (index > DRRS_HIGH_RR) {
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature

[Vandana Kannan]

On Jan-22-2014 6:39 PM, Jani Nikula wrote:
> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>
>> This patch reads the DRRS support and Mode type from VBT fields.
>> The read information will be stored in VBT struct during BIOS
>> parsing. The above functionality is needed for decision making
>> whether DRRS feature is supported in i915 driver for eDP panels.
>> This information helps us decide if seamless DRRS can be done
>> at runtime to support certain power saving features. This patch
>> was tested by setting necessary bit in VBT struct and merging
>> the new VBT with system BIOS so that we can read the value.
>>
>> v2: Incorporated review comments from Chris Wilson
>> Removed "intel_" as a prefix for DRRS specific declarations.
>>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   |    9 +++++++++
>>  drivers/gpu/drm/i915/intel_bios.c |   23 +++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>>  3 files changed, 61 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index ae2c80c..f8fd045 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1174,6 +1174,15 @@ struct intel_vbt_data {
>>  	int lvds_ssc_freq;
>>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>>  
>> +	/**
> 
> This is not a kerneldoc comment, so drop the extra *.
> 
Ok.
>> +	 * DRRS mode type (Seamless OR Static DRRS)
>> +	 * drrs_mode Val 0x2 is Seamless DRRS and 0 is Static DRRS.
>> +	 * These values correspond to the VBT values for drrs mode.
>> +	 */
>> +	int drrs_mode;
>> +	/* DRRS enabled or disabled in VBT */
>> +	bool drrs_enabled;
> 
> Both the drrs_mode and drrs_enabled should be replaced by one enum
> drrs_support_type which you introduce later. It's all self-explanatory
> that way, and you don't need to explain so much.
> 
There are 2 options in the VBT. drrs_enabled to check if DRRS is
supported, drrs_mode to show which type. It has been added here as it is
for readability.
>> +
>>  	/* eDP */
>>  	int edp_rate;
>>  	int edp_lanes;
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index f88e507..5b04a69 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -195,6 +195,21 @@ get_lvds_fp_timing(const struct bdb_header *bdb,
>>  	return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs);
>>  }
>>  
>> +/**
>> + * This function returns the 2 bit information pertaining to a panel type
>> + * present in a 32 bit field in VBT blocks. There are 16 panel types in VBT
>> + * each occupying 2 bits of information in some 32 bit fields of VBT blocks.
>> + */
>> +static int
>> +get_mode_by_paneltype(unsigned int word)
>> +{
>> +	/**
>> +	 * The caller of this API should interpret the 2 bits
>> +	 * based on VBT description for that field.
>> +	 */
>> +	return (word >> ((panel_type - 1) * 2)) & MODE_MASK;
> 
> Everywhere else in intel_bios.c panel_type is used 0-based.
> 
VBT indexes panel type as 1,2,3. Therefore, we have to make the change
to match kernel's 0-based usage.
>> +}
> 
> You use the above function only once. IMHO with all the explaining above
> it's just too much of a burden to the reader. Just do this in the code.
> 
Ok.
>> +
>>  /* Try to find integrated panel data */
>>  static void
>>  parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>> @@ -218,6 +233,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>  
>>  	panel_type = lvds_options->panel_type;
>>  
>> +	dev_priv->vbt.drrs_mode =
>> +		get_mode_by_paneltype(lvds_options->dps_panel_type_bits);
>> +	DRM_DEBUG_KMS("DRRS supported mode is : %s\n",
>                                              ^ drop the space here
> 
Ok
>> +			(dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS");
> 
> Why shouting?
> 
>> +
>>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>>  	if (!lvds_lfp_data)
>>  		return;
>> @@ -488,6 +508,9 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>>  
>>  	if (driver->dual_frequency)
>>  		dev_priv->render_reclock_avail = true;
>> +
>> +	dev_priv->vbt.drrs_enabled = driver->drrs_state;
>> +	DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state);
>                                          ^ and here and everywhere
> 
Ok
>>  }
>>  
>>  static void
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 81ed58c..0a3a751 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -281,6 +281,9 @@ struct bdb_general_definitions {
>>  	union child_device_config devices[0];
>>  } __packed;
>>  
>> +/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
>> +#define MODE_MASK		0x3
>> +
>>  struct bdb_lvds_options {
>>  	u8 panel_type;
>>  	u8 rsvd1;
>> @@ -293,6 +296,18 @@ struct bdb_lvds_options {
>>  	u8 lvds_edid:1;
>>  	u8 rsvd2:1;
>>  	u8 rsvd4;
>> +	/* LVDS Panel channel bits stored here */
>> +	u32 lvds_panel_channel_bits;
> 
> Why does this have "lvds" but the rest not? Why do some fields end in
> "bits" but some others not? Some consistency please.
> 
This is how the vbt block definition doc mentions each of these fields.
This is the reason why it has been added in this manner.

>> +	/* LVDS SSC (Spread Spectrum Clock) bits stored here. */
>> +	u16 ssc_bits;
>> +	u16 ssc_freq;
>> +	u16 ssc_ddt;
>> +	/* Panel color depth defined here */
>> +	u16 panel_color_depth;
> 
> I really *really* wish I knew why some stuff is specified in the lvds
> bios blob and some other in edp blob and some stuff specified here is
> used in the edp side. Ugh. I wonder if we should check this
> panel_color_depth for edp too.
>
This is how the vbt block definition doc mentions each of these fields.
This is the reason why it has been added in this manner.

>> +	/* LVDS panel type bits stored here */
>> +	u32 dps_panel_type_bits;
>> +	/* LVDS backlight control type bits stored here */
>> +	u32 blt_control_type_bits;
>>  } __packed;
>>  
>>  /* LFP pointer table contains entries to the struct below */
>> @@ -462,6 +477,20 @@ struct bdb_driver_features {
>>  
>>  	u8 hdmi_termination;
>>  	u8 custom_vbt_version;
>> +	/* Driver features data block */
>> +	u16 rmpm_state:1;
>> +	u16 s2ddt_state:1;
>> +	u16 dpst_state:1;
>> +	u16 bltclt_state:1;
>> +	u16 adb_state:1;
>> +	u16 drrs_state:1;
>> +	u16 grs_state:1;
>> +	u16 gpmt_state:1;
>> +	u16 tbt_state:1;
>> +	u16 psr_state:1;
>> +	u16 ips_state:1;
> 
> All of the above should be foo_enabled to make them self-explanatory.
> 
>> +	u16 reserved3:4;
>> +	u16 pc_feature_validity:1;
> 
> Similarly for this one, should be pc_feature_valid.
> 
This is how the vbt block definition doc mentions this field.
This is the reason why it has been added in this manner.
>>  } __packed;
>>  
>>  #define EDP_18BPP	0
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support

[Vandana Kannan]

On Jan-22-2014 7:03 PM, Jani Nikula wrote:
> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>
>> This patch and finds out the lowest refresh rate supported for the resolution
>> same as the fixed_mode, based on the implementaion find_panel_downclock.
>> It also checks the VBT fields to see if panel supports seamless DRRS or not.
>> Based on above data it marks whether eDP panel supports seamless DRRS or not.
>> This information is needed for supporting seamless DRRS switch for certain
>> power saving usecases. This patch is tested by enabling the DRM logs and
>> user should see whether Seamless DRRS is supported or not.
> 
> This patch (and therefore the later patches) no longer apply to
> drm-intel-nightly. It might affect my review a bit, but here goes
> anyway.
> 
I will rebase and resend the patch.
>>
>> v2: Daniel's review comments
>> Modified downclock deduction based on intel_find_panel_downclock
>>
>> v3: Chris's review comments
>> Moved edp_downclock_avail and edp_downclock to intel_panel
>>
>> v4: Jani's review comments.
>> Changed name of the enum edp_panel_type to drrs_support type.
>> Change is_drrs_supported to drrs_support of type enum drrs_support_type.
>>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c  |   45 ++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |   30 +++++++++++++++++++++++++
>>  2 files changed, 75 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 8f17f8f..079b53f 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3522,6 +3522,46 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>>  		      I915_READ(pp_div_reg));
>>  }
>>  
>> +static void
>> +intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>> +			struct intel_connector *intel_connector,
>> +			struct drm_display_mode *fixed_mode) {
> 
> I'll explain later why I think you should change the signature of the
> function.
> 
>> +	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_i915_private *dev_priv = dev->dev_private;
>> +
>> +	/**
>> +	 * Check if PSR is supported by panel and enabled
>> +	 * if so then DRRS is reported as not supported for Haswell.
>> +	 */
>> +	if (INTEL_INFO(dev)->gen < 8 &&	intel_edp_is_psr_enabled(dev)) {
>> +		DRM_INFO("eDP panel has PSR enabled. Cannot support DRRS\n");
>> +		return;
>> +	}
>> +
>> +	/* First check if DRRS is enabled from VBT struct */
>> +	if (!dev_priv->vbt.drrs_enabled) {
>> +		DRM_INFO("VBT doesn't support DRRS\n");
>> +		return;
>> +	}
>> +
>> +	intel_connector->panel.downclock_mode =	intel_find_panel_downclock(dev,
>> +					fixed_mode, connector);
>> +
>> +	if (intel_connector->panel.downclock_mode != NULL &&
>> +		dev_priv->vbt.drrs_mode == SEAMLESS_DRRS_SUPPORT) {
>> +		intel_connector->panel.edp_downclock_avail = true;
> 
> If you rearranged the code a bit, you could make the
> panel.downclock_mode != NULL mean the same as
> edp_downclock_avail. I.e. if you have the downclock_mode there, it's
> available.
> 
This was done to be in sync with lvds_downclock implementation based on
previous review comments.
>> +		intel_connector->panel.edp_downclock =
>> +			intel_connector->panel.downclock_mode->clock;
> 
> I don't understand why you need two copies of the clock.
> 
> In general, we should try and avoid adding extra state and copies of
> information for stuff that we can readily derive from other information.
> 
>> +
>> +		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
> 
> Again. I can't see intel_dp->drrs_state.drrs_support ever needing to be
> different from dev_priv->vbt.drrs_mode. So why the copy?
> 
This was done to make things more readable.
>> +
>> +		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
>> +		DRM_INFO("SEAMLESS DRRS supported for eDP panel.\n");
>> +	}
>> +}
>> +
>>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  				     struct intel_connector *intel_connector)
>>  {
>> @@ -3535,6 +3575,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  	struct drm_display_mode *scan;
>>  	struct edid *edid;
>>  
>> +	intel_dp->drrs_state.drrs_support = DRRS_NOT_SUPPORTED;
>> +
>>  	if (!is_edp(intel_dp))
>>  		return true;
>>  
>> @@ -3579,6 +3621,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  	list_for_each_entry(scan, &connector->probed_modes, head) {
>>  		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
>>  			fixed_mode = drm_mode_duplicate(dev, scan);
>> +			if (INTEL_INFO(dev)->gen >= 5)
>> +				intel_dp_drrs_initialize(intel_dig_port,
>> +					intel_connector, fixed_mode);
> 
> Is there any reason not to do this at the top level after checking for
> the VBT mode?
> 
This was done as fixed_mode was required.

> Also, we have a separate function for initializing the panel struct, so
> I think you should make intel_dp_drrs_initialize() return the downclock
> mode or NULL, and pass that to intel_panel_init() instead of
> initializing the panel struct directly within the function.
> 
I will make this change.
>>  			break;
>>  		}
>>  	}
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index e903432..d208bf5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -168,6 +168,9 @@ struct intel_panel {
>>  		bool active_low_pwm;
>>  		struct backlight_device *device;
>>  	} backlight;
>> +
>> +	bool edp_downclock_avail;
>> +	int edp_downclock;
> 
> As I said, I think you can get rid of both of these.
> 
As mentioned above, this was done to be in sync with lvds_downclock
implementation based on previous review comments.
>>  };
>>  
>>  struct intel_connector {
>> @@ -462,6 +465,32 @@ struct intel_hdmi {
>>  
>>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>>  
>> +/**
>> + * This enum is used to indicate the DRRS support type.
>> + */
>> +enum drrs_support_type {
>> +	DRRS_NOT_SUPPORTED = -1,
>> +	STATIC_DRRS_SUPPORT = 0, /* 1:1 mapping with VBT */
>> +	SEAMLESS_DRRS_SUPPORT = 2 /* 1:1 mapping with VBT */ };
> 
> I don't see any value in having 1:1 mapping with VBT. Not even in having
> 1:1 mapping between struct intel_vbt_data and the actual VBT. It's
> supposed to be parsed data.
> 
> Instead, I do see value in making DRRS_NOT_SUPPORTED == 0 as the logical
> thing to do.
> 
Ok. I will make necessary changes..
>> +/**
>> + * 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 */
>> +};
>> +/**
>> + * The drrs_info struct will represent the DRRS feature for eDP
>> + * panel.
>> + */
> 
> This comment does not add any value.
> 
Ok.
>> +struct drrs_info {
>> +	enum drrs_support_type drrs_support;
>> +	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
> 
> Because this will be accessed through intel_dp->drrs_state, there's no
> need to duplicate "drrs" in the field names here. It will be obvious
> from the context.
> 
Ok.
>> +};
>> +
>>  struct intel_dp {
>>  	uint32_t output_reg;
>>  	uint32_t aux_ch_ctl_reg;
>> @@ -487,6 +516,7 @@ struct intel_dp {
>>  	bool want_panel_vdd;
>>  	bool psr_setup_done;
>>  	struct intel_connector *attached_connector;
>> +	struct drrs_info drrs_state;
>>  };
>>  
>>  struct intel_digital_port {
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 3/5] drm/i915: Add support for DRRS to switch RR

[Vandana Kannan]

On Jan-22-2014 7:44 PM, Jani Nikula wrote:
> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>
>> This patch computes and stored 2nd M/N/TU for switching to different
>> refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
>> between alternate refresh rates programmed in 2nd M/N/TU registers.
>>
>> v2: Daniel's review comments
>> Computing M2/N2 in compute_config and storing it in crtc_config
>>
>> v3: Modified reference to edp_downclock and edp_downclock_avail based on the
>> changes made to move them from dev_private to intel_panel.
>>
>> v4: Modified references to is_drrs_supported based on the changes made to
>> rename it to drrs_support.
>>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>>  drivers/gpu/drm/i915/intel_dp.c  |  106 ++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>>  3 files changed, 112 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index f1eece4..57d2b64 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3206,6 +3206,7 @@
>>  #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
>>  #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
>>  #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
>> +#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
>>  #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
>>  #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
>>  #define   PIPECONF_BPC_MASK	(0x7 << 5)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 079b53f..d1e1d6e 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -791,6 +791,21 @@ intel_dp_set_clock(struct intel_encoder *encoder,
>>  	}
>>  }
>>  
>> +static void
>> +intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
>> +{
>> +	struct drm_device *dev = crtc->base.dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	enum transcoder transcoder = crtc->config.cpu_transcoder;
>> +
>> +	I915_WRITE(PIPE_DATA_M2(transcoder),
>> +		TU_SIZE(m_n->tu) | m_n->gmch_m);
>> +	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
>> +	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
>> +	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
>> +	return;
> 
> Superfluous return statement.
> 
Ok.
>> +}
>> +
>>  bool
>>  intel_dp_compute_config(struct intel_encoder *encoder,
>>  			struct intel_crtc_config *pipe_config)
>> @@ -894,6 +909,14 @@ found:
>>  			       pipe_config->port_clock,
>>  			       &pipe_config->dp_m_n);
>>  
>> +	if (intel_connector->panel.edp_downclock_avail &&
>> +	intel_dp->drrs_state.drrs_support == SEAMLESS_DRRS_SUPPORT) {
>> +		intel_link_compute_m_n(bpp, lane_count,
>> +				intel_connector->panel.edp_downclock,
>> +				pipe_config->port_clock,
>> +				&pipe_config->dp_m2_n2);
>> +	}
> 
> Indentation in the above block.
> 
Ok.
>> +
>>  	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>>  
>>  	return true;
>> @@ -3522,6 +3545,87 @@ 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) {
> 
> The opening brace belongs on a new line.
> 
Ok.
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct drm_mode_config *mode_config = &dev->mode_config;
>> +	struct intel_encoder *encoder;
>> +	struct intel_dp *intel_dp = NULL;
>> +	struct intel_crtc_config *config = NULL;
>> +	struct intel_crtc *intel_crtc = NULL;
>> +	struct intel_connector *intel_connector = NULL;
>> +	bool found_edp = false;
>> +	u32 reg, val;
>> +	int index = 0;
>> +
>> +	if (refresh_rate <= 0) {
>> +		DRM_INFO("Refresh rate should be positive non-zero.\n");
>> +		goto out;
>> +	}
> 
> Under what conditions do you expect this to happen?
> 
In future, when refresh rate switching based on user space requests will
be implemented, intel_dp_set_drrs_state() will be used. This check here
is to safeguard calls from user space.
>> +
>> +	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
>> +		if (encoder->type == INTEL_OUTPUT_EDP) {
>> +			intel_dp = enc_to_intel_dp(&encoder->base);
>> +			intel_crtc = encoder->new_crtc;
>> +			if (!intel_crtc) {
>> +				DRM_INFO("DRRS: intel_crtc not initialized\n");
> 
> DRM_INFO is probably a bit verbose.
> 
>> +				goto out;
>> +			}
>> +			config = &intel_crtc->config;
>> +			intel_connector = intel_dp->attached_connector;
>> +			found_edp = true;
>> +			break;
>> +		}
>> +	}
> 
> I'm confused. You go through all the trouble of saving the crtc and the
> connector (although only in the next patch) but here you iterate all the
> encoders to find the one. Why?
> 
>> +
>> +	if (!found_edp) {
> 
> There's no need to add an extra variable for this. You already have
> intel_dp, intel_crtc, config and intel_connector you can check!
> 
Ok. I will make necessary changes.
>> +		DRM_INFO("DRRS supported for eDP only.\n");
>> +		goto out;
>> +	}
>> +
>> +	if (intel_dp->drrs_state.drrs_support < SEAMLESS_DRRS_SUPPORT) {
>> +		DRM_INFO("Seamless DRRS not supported.\n");
>> +		goto out;
>> +	}
>> +
>> +	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
>> +		index = DRRS_HIGH_RR;
>> +	else
>> +		index = DRRS_LOW_RR;
>> +
>> +	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
>> +		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
>> +		goto out;
>> +	}
>> +
>> +	if (!intel_crtc->active) {
>> +		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
>> +		goto out;
>> +	}
>> +
>> +	mutex_lock(&intel_dp->drrs_state.mutex);
>> +
>> +	/* Haswell and below */
>> +	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
>> +		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
>> +		val = I915_READ(reg);
>> +		if (index > DRRS_HIGH_RR) {
>> +			val |= PIPECONF_EDP_RR_MODE_SWITCH;
> 
> Please check bspec for workarounds you need to do wrt frame start delay
> before enabling the pipe/transcoder.
> 
We checked the BSpec and there was no workaround mentioned for this part.
>> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
>> +		} else
>> +			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
> 
> Per coding style, if one branch needs braces, then all do.
> 
Ok.
>> +		I915_WRITE(reg, val);
>> +	}
>> +
>> +	intel_dp->drrs_state.drrs_refresh_rate_type = index;
>> +	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
>> +
>> +	mutex_unlock(&intel_dp->drrs_state.mutex);
>> +
>> +out:
>> +	return;
> 
> Superfluous return statement.
> 
Ok.
>> +}
>> +
>>  static void
>>  intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>  			struct intel_connector *intel_connector,
>> @@ -3555,6 +3659,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>  		intel_connector->panel.edp_downclock =
>>  			intel_connector->panel.downclock_mode->clock;
>>  
>> +		mutex_init(&intel_dp->drrs_state.mutex);
>> +
>>  		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>>  
>>  		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d208bf5..d1c60fa 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -291,6 +291,9 @@ struct intel_crtc_config {
>>  	int pipe_bpp;
>>  	struct intel_link_m_n dp_m_n;
>>  
>> +	/* m2_n2 for eDP downclock */
>> +	struct intel_link_m_n dp_m2_n2;
>> +
>>  	/*
>>  	 * Frequence the dpll for the port should run at. Differs from the
>>  	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
>> @@ -489,6 +492,7 @@ enum edp_drrs_refresh_rate_type {
>>  struct drrs_info {
>>  	enum drrs_support_type drrs_support;
>>  	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
>> +	struct mutex mutex;
>>  };
>>  
>>  struct intel_dp {
>> @@ -761,7 +765,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
>>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>>  void intel_edp_psr_update(struct drm_device *dev);
>> -
>> +extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
> 
> No need for the "extern".
> 
Ok.
>>  
>>  /* intel_dsi.c */
>>  bool intel_dsi_init(struct drm_device *dev);
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 4/5] drm/i915: Idleness detection for DRRS

[Vandana Kannan]

On Jan-22-2014 7:56 PM, Jani Nikula wrote:
> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> Adding support to detect display idleness by tracking page flip from
>> user space. Switch to low refresh rate is triggered after 2 seconds of
>> idleness. The delay is configurable. If there is a page flip or call to
>> update the plane, then high refresh rate is applied.
>> The feature is not used in dual-display mode.
>>
>> v2: Chris Wilson's review comments incorporated.
>> Modify idleness detection implementation to make it similar to the
>> implementation of intel_update_fbc/intel_disable_fbc
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |   16 +++++
>>  drivers/gpu/drm/i915/intel_display.c |   14 ++++
>>  drivers/gpu/drm/i915/intel_dp.c      |   10 +++
>>  drivers/gpu/drm/i915/intel_drv.h     |    4 ++
>>  drivers/gpu/drm/i915/intel_pm.c      |  122 ++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_sprite.c  |    2 +
>>  6 files changed, 168 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index f8fd045..d7308cc 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -712,6 +712,21 @@ struct i915_fbc {
>>  	} no_fbc_reason;
>>  };
>>  
>> +/* configure the number of secs the system must be idle
>> + * before DRRS is enabled
>> +*/
>> +#define DRRS_IDLENESS_TIME 2000 /* in millisecs */
>> +
>> +struct i915_drrs {
>> +	struct intel_connector *connector;
>> +	struct intel_dp *dp;
>> +	struct intel_drrs_work {
>> +		struct delayed_work work;
>> +		struct drm_crtc *crtc;
>> +		int interval;
> 
> I'll probably see this more useful as a module parameter than a field
> here, with 0 meaning disable.
> 
Ok. I will look into this.
>> +	} *drrs_work;
>> +};
>> +
>>  struct i915_psr {
>>  	bool sink_support;
>>  	bool source_ok;
>> @@ -1400,6 +1415,7 @@ typedef struct drm_i915_private {
>>  	int num_plane;
>>  
>>  	struct i915_fbc fbc;
>> +	struct i915_drrs drrs;
>>  	struct intel_opregion opregion;
>>  	struct intel_vbt_data vbt;
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a40651e..995d117 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2395,6 +2395,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>>  	}
>>  
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  	intel_edp_psr_update(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>> @@ -3559,6 +3560,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>>  
>>  	mutex_lock(&dev->struct_mutex);
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>> @@ -3600,6 +3602,7 @@ static void haswell_crtc_enable_planes(struct drm_crtc *crtc)
>>  
>>  	mutex_lock(&dev->struct_mutex);
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  }
>>  
>> @@ -3806,6 +3809,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>>  
>>  	mutex_lock(&dev->struct_mutex);
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  }
>>  
>> @@ -3853,6 +3857,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>>  
>>  	mutex_lock(&dev->struct_mutex);
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  }
>>  
>> @@ -8226,6 +8231,11 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>>  	drm_gem_object_unreference(&work->old_fb_obj->base);
>>  
>>  	intel_update_fbc(dev);
>> +
>> +	/* disable current DRRS work scheduled and restart
>> +	 * to push work by another x seconds
>> +	 */
>> +	intel_update_drrs(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>>  	BUG_ON(atomic_read(&to_intel_crtc(work->crtc)->unpin_work_count) == 0);
>> @@ -8665,6 +8675,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>  		goto cleanup_pending;
>>  
>>  	intel_disable_fbc(dev);
>> +	intel_disable_drrs(dev);
>>  	intel_mark_fb_busy(obj, NULL);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>> @@ -10880,6 +10891,7 @@ void intel_modeset_init(struct drm_device *dev)
>>  
>>  	/* Just in case the BIOS is doing something questionable. */
>>  	intel_disable_fbc(dev);
>> +	intel_disable_drrs(dev);
>>  }
>>  
>>  static void
>> @@ -11286,6 +11298,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
>>  
>>  	intel_disable_fbc(dev);
>>  
>> +	intel_disable_drrs(dev);
>> +
>>  	intel_disable_gt_powersave(dev);
>>  
>>  	ironlake_teardown_rc6(dev);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index d1e1d6e..7778808 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3289,11 +3289,18 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>>  	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
>>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>>  	i2c_del_adapter(&intel_dp->adapter);
>>  	drm_encoder_cleanup(encoder);
>>  	if (is_edp(intel_dp)) {
>>  		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
>> +		/* DRRS cleanup */
>> +		if (intel_dp->drrs_state.drrs_support
>> +					== SEAMLESS_DRRS_SUPPORT) {
>> +			kfree(dev_priv->drrs.drrs_work);
>> +			dev_priv->drrs.drrs_work = NULL;
>> +		}
>>  		mutex_lock(&dev->mode_config.mutex);
>>  		ironlake_panel_vdd_off_sync(intel_dp);
>>  		mutex_unlock(&dev->mode_config.mutex);
>> @@ -3659,6 +3666,9 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>  		intel_connector->panel.edp_downclock =
>>  			intel_connector->panel.downclock_mode->clock;
>>  
>> +		intel_init_drrs_idleness_detection(dev,
>> +			intel_connector, intel_dp);
>> +
>>  		mutex_init(&intel_dp->drrs_state.mutex);
>>  
>>  		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d1c60fa..84cadf8 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -902,6 +902,10 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
>>  void ilk_wm_get_hw_state(struct drm_device *dev);
>>  
>> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
>> +		struct intel_connector *connector, struct intel_dp *dp);
>> +void intel_update_drrs(struct drm_device *dev);
>> +void intel_disable_drrs(struct drm_device *dev);
>>  
>>  /* intel_sdvo.c */
>>  bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index b35f65e..69f31c5 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -615,6 +615,128 @@ out_disable:
>>  	i915_gem_stolen_cleanup_compression(dev);
>>  }
>>  
>> +static void intel_drrs_work_fn(struct work_struct *__work)
>> +{
>> +	struct intel_drrs_work *work =
>> +		container_of(to_delayed_work(__work),
>> +			     struct intel_drrs_work, work);
>> +	struct drm_device *dev = work->crtc->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	intel_dp_set_drrs_state(work->crtc->dev,
>> +		dev_priv->drrs.connector->panel.downclock_mode->vrefresh);
>> +}
>> +
>> +static void intel_cancel_drrs_work(struct drm_i915_private *dev_priv)
>> +{
>> +	if (dev_priv->drrs.drrs_work == NULL)
>> +		return;
>> +
>> +	DRM_DEBUG_KMS("cancelling pending DRRS enable\n");
>> +
>> +	cancel_delayed_work_sync(&dev_priv->drrs.drrs_work->work);
>> +}
>> +
>> +static void intel_enable_drrs(struct drm_crtc *crtc)
>> +{
>> +	struct drm_device *dev = crtc->dev;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	intel_cancel_drrs_work(dev_priv);
>> +
>> +	if (dev_priv->drrs.dp->drrs_state.drrs_refresh_rate_type
>> +							!= DRRS_LOW_RR) {
>> +		dev_priv->drrs.drrs_work->crtc = crtc;
>> +
>> +		/* Delay the actual enabling to let pageflipping cease and the
>> +		 * display to settle before starting DRRS
>> +		 */
>> +		schedule_delayed_work(&dev_priv->drrs.drrs_work->work,
>> +			msecs_to_jiffies(dev_priv->drrs.drrs_work->interval));
>> +	}
>> +}
>> +
>> +void intel_disable_drrs(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +
>> +	/* as part of disable DRRS, reset refresh rate to HIGH_RR */
>> +	if (dev_priv->drrs.dp->drrs_state.drrs_refresh_rate_type
>> +							== DRRS_LOW_RR) {
>> +		intel_cancel_drrs_work(dev_priv);
>> +		intel_dp_set_drrs_state(dev,
>> +			dev_priv->drrs.connector->panel.fixed_mode->vrefresh);
>> +	}
>> +}
>> +
>> +/**
>> + * intel_update_drrs - enable/disable DRRS as needed
>> + * @dev: the drm_device
>> + * @update: if set to true, cancel current work and schedule new work.
>> +*	    if set to false, cancel current work and disable DRRS.
> 
> There's no update parameter.
> 
My miss. I will remove this comment.
>> +*/
>> +void intel_update_drrs(struct drm_device *dev)
>> +{
>> +	struct drm_crtc *crtc = NULL, *tmp_crtc;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	/* if drrs.connector is NULL, then drrs_init did not get called.
>> +	 * which means DRRS is not supported.
>> +	 */
>> +	if (dev_priv->drrs.connector == NULL) {
>> +		DRM_INFO("DRRS is not supported.\n");
>> +		return;
>> +	}
>> +
>> +	list_for_each_entry(tmp_crtc, &dev->mode_config.crtc_list, head) {
>> +		if (tmp_crtc != NULL && intel_crtc_active(tmp_crtc) &&
>> +		    to_intel_crtc(tmp_crtc)->primary_enabled) {
>> +			if (crtc) {
>> +				DRM_DEBUG_KMS(
>> +				"more than one pipe active, disabling DRRS\n");
>> +				goto out_drrs_disable;
>> +			}
>> +			crtc = tmp_crtc;
>> +		}
>> +	}
>> +
>> +	if (crtc == NULL) {
>> +		DRM_INFO("DRRS: crtc not initialized\n");
>> +		return;
>> +	}
>> +
>> +	intel_disable_drrs(dev);
>> +
>> +	/* re-enable idleness detection */
>> +	intel_enable_drrs(crtc);
>> +
>> +out_drrs_disable:
>> +	intel_disable_drrs(dev);
> 
> The happy day disable, enable, disable sequence seems like a bug.
> 
I will make necessary changes..
>> +}
>> +
>> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
>> +					struct intel_connector *connector,
>> +					struct intel_dp *dp)
>> +{
>> +	struct intel_drrs_work *work;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	work = kzalloc(sizeof(struct intel_drrs_work), GFP_KERNEL);
>> +	if (!work) {
>> +		DRM_ERROR("Failed to allocate DRRS work structure\n");
>> +		return;
>> +	}
>> +
>> +	dev_priv->drrs.connector = connector;
>> +	dev_priv->drrs.dp = dp;
>> +
>> +	work->interval = DRRS_IDLENESS_TIME;
>> +	INIT_DELAYED_WORK(&work->work, intel_drrs_work_fn);
>> +
>> +	dev_priv->drrs.drrs_work = work;
>> +}
>> +
>>  static void i915_pineview_get_mem_freq(struct drm_device *dev)
>>  {
>>  	drm_i915_private_t *dev_priv = dev->dev_private;
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 90a3f6d..c3dcffd 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -560,6 +560,7 @@ intel_enable_primary(struct drm_crtc *crtc)
>>  
>>  	mutex_lock(&dev->struct_mutex);
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  }
>>  
>> @@ -579,6 +580,7 @@ intel_disable_primary(struct drm_crtc *crtc)
>>  	mutex_lock(&dev->struct_mutex);
>>  	if (dev_priv->fbc.plane == intel_crtc->plane)
>>  		intel_disable_fbc(dev);
>> +	intel_disable_drrs(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>>  	/*
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

[Vandana Kannan]

On Jan-22-2014 8:04 PM, Jani Nikula wrote:
> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> 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.
>>
>> v2: Incorporated Chris's review comments
>> Changed to check for gen >=8 or gen > 5 before setting M/N registers
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c |   32 +++++++++++++++++++++++++-------
>>  1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 7778808..f18a585 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -798,11 +798,20 @@ intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	enum transcoder transcoder = crtc->config.cpu_transcoder;
>>  
>> -	I915_WRITE(PIPE_DATA_M2(transcoder),
>> -		TU_SIZE(m_n->tu) | m_n->gmch_m);
>> -	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
>> -	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
>> -	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
>> +	if (INTEL_INFO(dev)->gen >= 8) {
>> +		I915_WRITE(PIPE_DATA_M1(transcoder),
>> +			TU_SIZE(m_n->tu) | m_n->gmch_m);
>> +		I915_WRITE(PIPE_DATA_N1(transcoder), m_n->gmch_n);
>> +		I915_WRITE(PIPE_LINK_M1(transcoder), m_n->link_m);
>> +		I915_WRITE(PIPE_LINK_N1(transcoder), m_n->link_n);
> 
> There's already a function for this part, called
> intel_cpu_transcoder_set_m_n. Reuse it.
> 
Ok. I will make necessary changes
>> +	} else if (INTEL_INFO(dev)->gen >= 5) {
>> +		I915_WRITE(PIPE_DATA_M2(transcoder),
>> +			TU_SIZE(m_n->tu) | m_n->gmch_m);
>> +		I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
>> +		I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
>> +		I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
>> +	}
>> +
>>  	return;
>>  }
>>  
>> @@ -3612,8 +3621,17 @@ intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate) {
>>  
>>  	mutex_lock(&intel_dp->drrs_state.mutex);
>>  
>> -	/* Haswell and below */
> 
> Forgot to mention in the earlier patch that comments like this are
> redundant with the code. And in this case, it already *contradicts* the
> code.
> 
>> -	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
>> +	if (INTEL_INFO(dev)->gen >= 8) {
>> +		switch (index) {
>> +		case DRRS_HIGH_RR:
>> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m_n);
>> +			break;
>> +		case DRRS_LOW_RR:
>> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
>> +			break;
>> +		};
>> +	} else if (INTEL_INFO(dev)->gen >= 5) {
>> +		/* Haswell and below */
This comment can be removed.
>>  		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
>>  		val = I915_READ(reg);
>>  		if (index > DRRS_HIGH_RR) {
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature

[Jani Nikula]

On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan@intel.com> wrote:
> On Jan-22-2014 6:39 PM, Jani Nikula wrote:
>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>>
>>> This patch reads the DRRS support and Mode type from VBT fields.
>>> The read information will be stored in VBT struct during BIOS
>>> parsing. The above functionality is needed for decision making
>>> whether DRRS feature is supported in i915 driver for eDP panels.
>>> This information helps us decide if seamless DRRS can be done
>>> at runtime to support certain power saving features. This patch
>>> was tested by setting necessary bit in VBT struct and merging
>>> the new VBT with system BIOS so that we can read the value.
>>>
>>> v2: Incorporated review comments from Chris Wilson
>>> Removed "intel_" as a prefix for DRRS specific declarations.
>>>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h   |    9 +++++++++
>>>  drivers/gpu/drm/i915/intel_bios.c |   23 +++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>>>  3 files changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index ae2c80c..f8fd045 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1174,6 +1174,15 @@ struct intel_vbt_data {
>>>  	int lvds_ssc_freq;
>>>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>>>  
>>> +	/**
>> 
>> This is not a kerneldoc comment, so drop the extra *.
>> 
> Ok.
>>> +	 * DRRS mode type (Seamless OR Static DRRS)
>>> +	 * drrs_mode Val 0x2 is Seamless DRRS and 0 is Static DRRS.
>>> +	 * These values correspond to the VBT values for drrs mode.
>>> +	 */
>>> +	int drrs_mode;
>>> +	/* DRRS enabled or disabled in VBT */
>>> +	bool drrs_enabled;
>> 
>> Both the drrs_mode and drrs_enabled should be replaced by one enum
>> drrs_support_type which you introduce later. It's all self-explanatory
>> that way, and you don't need to explain so much.
>> 
> There are 2 options in the VBT. drrs_enabled to check if DRRS is
> supported, drrs_mode to show which type. It has been added here as it is
> for readability.

I can understand the argument for anything defined in intel_bios.[ch],
but for the rest, including struct intel_vbt_data, readability comes
from other reasons than being equivalent with the VBT specs.

>>> +
>>>  	/* eDP */
>>>  	int edp_rate;
>>>  	int edp_lanes;
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>> index f88e507..5b04a69 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -195,6 +195,21 @@ get_lvds_fp_timing(const struct bdb_header *bdb,
>>>  	return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs);
>>>  }
>>>  
>>> +/**
>>> + * This function returns the 2 bit information pertaining to a panel type
>>> + * present in a 32 bit field in VBT blocks. There are 16 panel types in VBT
>>> + * each occupying 2 bits of information in some 32 bit fields of VBT blocks.
>>> + */
>>> +static int
>>> +get_mode_by_paneltype(unsigned int word)
>>> +{
>>> +	/**
>>> +	 * The caller of this API should interpret the 2 bits
>>> +	 * based on VBT description for that field.
>>> +	 */
>>> +	return (word >> ((panel_type - 1) * 2)) & MODE_MASK;
>> 
>> Everywhere else in intel_bios.c panel_type is used 0-based.
>> 
> VBT indexes panel type as 1,2,3. Therefore, we have to make the change
> to match kernel's 0-based usage.

Again, everywhere else in intel_bios.c we use panel_type, directly as it
is in VBT, 0-based. Are you saying it's all wrong? panel_type can't be
1-based in this one instance, and 0-based in all other instances, right?

This is actually the first priority to check.

>>> +}
>> 
>> You use the above function only once. IMHO with all the explaining above
>> it's just too much of a burden to the reader. Just do this in the code.
>> 
> Ok.
>>> +
>>>  /* Try to find integrated panel data */
>>>  static void
>>>  parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>> @@ -218,6 +233,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>>  
>>>  	panel_type = lvds_options->panel_type;
>>>  
>>> +	dev_priv->vbt.drrs_mode =
>>> +		get_mode_by_paneltype(lvds_options->dps_panel_type_bits);
>>> +	DRM_DEBUG_KMS("DRRS supported mode is : %s\n",
>>                                              ^ drop the space here
>> 
> Ok
>>> +			(dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS");
>> 
>> Why shouting?
>> 
>>> +
>>>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>>>  	if (!lvds_lfp_data)
>>>  		return;
>>> @@ -488,6 +508,9 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>>>  
>>>  	if (driver->dual_frequency)
>>>  		dev_priv->render_reclock_avail = true;
>>> +
>>> +	dev_priv->vbt.drrs_enabled = driver->drrs_state;
>>> +	DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state);
>>                                          ^ and here and everywhere
>> 
> Ok
>>>  }
>>>  
>>>  static void
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>>> index 81ed58c..0a3a751 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.h
>>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>>> @@ -281,6 +281,9 @@ struct bdb_general_definitions {
>>>  	union child_device_config devices[0];
>>>  } __packed;
>>>  
>>> +/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
>>> +#define MODE_MASK		0x3
>>> +
>>>  struct bdb_lvds_options {
>>>  	u8 panel_type;
>>>  	u8 rsvd1;
>>> @@ -293,6 +296,18 @@ struct bdb_lvds_options {
>>>  	u8 lvds_edid:1;
>>>  	u8 rsvd2:1;
>>>  	u8 rsvd4;
>>> +	/* LVDS Panel channel bits stored here */
>>> +	u32 lvds_panel_channel_bits;
>> 
>> Why does this have "lvds" but the rest not? Why do some fields end in
>> "bits" but some others not? Some consistency please.
>> 
> This is how the vbt block definition doc mentions each of these fields.
> This is the reason why it has been added in this manner.

I don't have my vbt spec handy right now, but when I was last checking
these it didn't look consistent with the spec.

Same for the ones below.

>
>>> +	/* LVDS SSC (Spread Spectrum Clock) bits stored here. */
>>> +	u16 ssc_bits;
>>> +	u16 ssc_freq;
>>> +	u16 ssc_ddt;
>>> +	/* Panel color depth defined here */
>>> +	u16 panel_color_depth;
>> 
>> I really *really* wish I knew why some stuff is specified in the lvds
>> bios blob and some other in edp blob and some stuff specified here is
>> used in the edp side. Ugh. I wonder if we should check this
>> panel_color_depth for edp too.
>>
> This is how the vbt block definition doc mentions each of these fields.
> This is the reason why it has been added in this manner.
>
>>> +	/* LVDS panel type bits stored here */
>>> +	u32 dps_panel_type_bits;
>>> +	/* LVDS backlight control type bits stored here */
>>> +	u32 blt_control_type_bits;
>>>  } __packed;
>>>  
>>>  /* LFP pointer table contains entries to the struct below */
>>> @@ -462,6 +477,20 @@ struct bdb_driver_features {
>>>  
>>>  	u8 hdmi_termination;
>>>  	u8 custom_vbt_version;
>>> +	/* Driver features data block */
>>> +	u16 rmpm_state:1;
>>> +	u16 s2ddt_state:1;
>>> +	u16 dpst_state:1;
>>> +	u16 bltclt_state:1;
>>> +	u16 adb_state:1;
>>> +	u16 drrs_state:1;
>>> +	u16 grs_state:1;
>>> +	u16 gpmt_state:1;
>>> +	u16 tbt_state:1;
>>> +	u16 psr_state:1;
>>> +	u16 ips_state:1;
>> 
>> All of the above should be foo_enabled to make them self-explanatory.
>> 
>>> +	u16 reserved3:4;
>>> +	u16 pc_feature_validity:1;
>> 
>> Similarly for this one, should be pc_feature_valid.
>> 
> This is how the vbt block definition doc mentions this field.
> This is the reason why it has been added in this manner.
>>>  } __packed;
>>>  
>>>  #define EDP_18BPP	0
>>> -- 
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 3/5] drm/i915: Add support for DRRS to switch RR

[Jani Nikula]

On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan@intel.com> wrote:
> On Jan-22-2014 7:44 PM, Jani Nikula wrote:
>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>>
>>> This patch computes and stored 2nd M/N/TU for switching to different
>>> refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
>>> between alternate refresh rates programmed in 2nd M/N/TU registers.
>>>
>>> v2: Daniel's review comments
>>> Computing M2/N2 in compute_config and storing it in crtc_config
>>>
>>> v3: Modified reference to edp_downclock and edp_downclock_avail based on the
>>> changes made to move them from dev_private to intel_panel.
>>>
>>> v4: Modified references to is_drrs_supported based on the changes made to
>>> rename it to drrs_support.
>>>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>>>  drivers/gpu/drm/i915/intel_dp.c  |  106 ++++++++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>>>  3 files changed, 112 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index f1eece4..57d2b64 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -3206,6 +3206,7 @@
>>>  #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
>>>  #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
>>>  #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
>>> +#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
>>>  #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
>>>  #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
>>>  #define   PIPECONF_BPC_MASK	(0x7 << 5)
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 079b53f..d1e1d6e 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -791,6 +791,21 @@ intel_dp_set_clock(struct intel_encoder *encoder,
>>>  	}
>>>  }
>>>  
>>> +static void
>>> +intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
>>> +{
>>> +	struct drm_device *dev = crtc->base.dev;
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	enum transcoder transcoder = crtc->config.cpu_transcoder;
>>> +
>>> +	I915_WRITE(PIPE_DATA_M2(transcoder),
>>> +		TU_SIZE(m_n->tu) | m_n->gmch_m);
>>> +	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
>>> +	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
>>> +	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
>>> +	return;
>> 
>> Superfluous return statement.
>> 
> Ok.
>>> +}
>>> +
>>>  bool
>>>  intel_dp_compute_config(struct intel_encoder *encoder,
>>>  			struct intel_crtc_config *pipe_config)
>>> @@ -894,6 +909,14 @@ found:
>>>  			       pipe_config->port_clock,
>>>  			       &pipe_config->dp_m_n);
>>>  
>>> +	if (intel_connector->panel.edp_downclock_avail &&
>>> +	intel_dp->drrs_state.drrs_support == SEAMLESS_DRRS_SUPPORT) {
>>> +		intel_link_compute_m_n(bpp, lane_count,
>>> +				intel_connector->panel.edp_downclock,
>>> +				pipe_config->port_clock,
>>> +				&pipe_config->dp_m2_n2);
>>> +	}
>> 
>> Indentation in the above block.
>> 
> Ok.
>>> +
>>>  	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>>>  
>>>  	return true;
>>> @@ -3522,6 +3545,87 @@ 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) {
>> 
>> The opening brace belongs on a new line.
>> 
> Ok.
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>> +	struct drm_mode_config *mode_config = &dev->mode_config;
>>> +	struct intel_encoder *encoder;
>>> +	struct intel_dp *intel_dp = NULL;
>>> +	struct intel_crtc_config *config = NULL;
>>> +	struct intel_crtc *intel_crtc = NULL;
>>> +	struct intel_connector *intel_connector = NULL;
>>> +	bool found_edp = false;
>>> +	u32 reg, val;
>>> +	int index = 0;
>>> +
>>> +	if (refresh_rate <= 0) {
>>> +		DRM_INFO("Refresh rate should be positive non-zero.\n");
>>> +		goto out;
>>> +	}
>> 
>> Under what conditions do you expect this to happen?
>> 
> In future, when refresh rate switching based on user space requests will
> be implemented, intel_dp_set_drrs_state() will be used. This check here
> is to safeguard calls from user space.
>>> +
>>> +	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
>>> +		if (encoder->type == INTEL_OUTPUT_EDP) {
>>> +			intel_dp = enc_to_intel_dp(&encoder->base);
>>> +			intel_crtc = encoder->new_crtc;
>>> +			if (!intel_crtc) {
>>> +				DRM_INFO("DRRS: intel_crtc not initialized\n");
>> 
>> DRM_INFO is probably a bit verbose.
>> 
>>> +				goto out;
>>> +			}
>>> +			config = &intel_crtc->config;
>>> +			intel_connector = intel_dp->attached_connector;
>>> +			found_edp = true;
>>> +			break;
>>> +		}
>>> +	}
>> 
>> I'm confused. You go through all the trouble of saving the crtc and the
>> connector (although only in the next patch) but here you iterate all the
>> encoders to find the one. Why?
>> 
>>> +
>>> +	if (!found_edp) {
>> 
>> There's no need to add an extra variable for this. You already have
>> intel_dp, intel_crtc, config and intel_connector you can check!
>> 
> Ok. I will make necessary changes.
>>> +		DRM_INFO("DRRS supported for eDP only.\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (intel_dp->drrs_state.drrs_support < SEAMLESS_DRRS_SUPPORT) {
>>> +		DRM_INFO("Seamless DRRS not supported.\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
>>> +		index = DRRS_HIGH_RR;
>>> +	else
>>> +		index = DRRS_LOW_RR;
>>> +
>>> +	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
>>> +		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	if (!intel_crtc->active) {
>>> +		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
>>> +		goto out;
>>> +	}
>>> +
>>> +	mutex_lock(&intel_dp->drrs_state.mutex);
>>> +
>>> +	/* Haswell and below */
>>> +	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
>>> +		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
>>> +		val = I915_READ(reg);
>>> +		if (index > DRRS_HIGH_RR) {
>>> +			val |= PIPECONF_EDP_RR_MODE_SWITCH;
>> 
>> Please check bspec for workarounds you need to do wrt frame start delay
>> before enabling the pipe/transcoder.
>> 
> We checked the BSpec and there was no workaround mentioned for this part.

Oh, I'm sorry, I should have been more specific. This was in the SNB
specs: "Workaround : If this pipe is connected to a port on the PCH and
this power savings mode will be used, then before the pipe and
transcoder are enabled, the frame start delay in the pipe and transcoder
must be set to 11b. When these conditions are no longer true, the frame
start delay must be returned to the previous value after the pipe and
transcoder are disabled."

Going forward, I think it's okay to enable drrs on ivb+ at first, and
enable snb with the workarounds in a follow-up patch. Others may
disagree...

>>> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
>>> +		} else
>>> +			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
>> 
>> Per coding style, if one branch needs braces, then all do.
>> 
> Ok.
>>> +		I915_WRITE(reg, val);
>>> +	}
>>> +
>>> +	intel_dp->drrs_state.drrs_refresh_rate_type = index;
>>> +	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
>>> +
>>> +	mutex_unlock(&intel_dp->drrs_state.mutex);
>>> +
>>> +out:
>>> +	return;
>> 
>> Superfluous return statement.
>> 
> Ok.
>>> +}
>>> +
>>>  static void
>>>  intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>>  			struct intel_connector *intel_connector,
>>> @@ -3555,6 +3659,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>>  		intel_connector->panel.edp_downclock =
>>>  			intel_connector->panel.downclock_mode->clock;
>>>  
>>> +		mutex_init(&intel_dp->drrs_state.mutex);
>>> +
>>>  		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>>>  
>>>  		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index d208bf5..d1c60fa 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -291,6 +291,9 @@ struct intel_crtc_config {
>>>  	int pipe_bpp;
>>>  	struct intel_link_m_n dp_m_n;
>>>  
>>> +	/* m2_n2 for eDP downclock */
>>> +	struct intel_link_m_n dp_m2_n2;
>>> +
>>>  	/*
>>>  	 * Frequence the dpll for the port should run at. Differs from the
>>>  	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
>>> @@ -489,6 +492,7 @@ enum edp_drrs_refresh_rate_type {
>>>  struct drrs_info {
>>>  	enum drrs_support_type drrs_support;
>>>  	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
>>> +	struct mutex mutex;
>>>  };
>>>  
>>>  struct intel_dp {
>>> @@ -761,7 +765,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>>>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
>>>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>>>  void intel_edp_psr_update(struct drm_device *dev);
>>> -
>>> +extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
>> 
>> No need for the "extern".
>> 
> Ok.
>>>  
>>>  /* intel_dsi.c */
>>>  bool intel_dsi_init(struct drm_device *dev);
>>> -- 
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> 
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature

[Vandana Kannan]

On Jan-30-2014 11:50 AM, Jani Nikula wrote:
> On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> On Jan-22-2014 6:39 PM, Jani Nikula wrote:
>>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>>>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>>>
>>>> This patch reads the DRRS support and Mode type from VBT fields.
>>>> The read information will be stored in VBT struct during BIOS
>>>> parsing. The above functionality is needed for decision making
>>>> whether DRRS feature is supported in i915 driver for eDP panels.
>>>> This information helps us decide if seamless DRRS can be done
>>>> at runtime to support certain power saving features. This patch
>>>> was tested by setting necessary bit in VBT struct and merging
>>>> the new VBT with system BIOS so that we can read the value.
>>>>
>>>> v2: Incorporated review comments from Chris Wilson
>>>> Removed "intel_" as a prefix for DRRS specific declarations.
>>>>
>>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_drv.h   |    9 +++++++++
>>>>  drivers/gpu/drm/i915/intel_bios.c |   23 +++++++++++++++++++++++
>>>>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>>>>  3 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index ae2c80c..f8fd045 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1174,6 +1174,15 @@ struct intel_vbt_data {
>>>>  	int lvds_ssc_freq;
>>>>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>>>>  
>>>> +	/**
>>>
>>> This is not a kerneldoc comment, so drop the extra *.
>>>
>> Ok.
>>>> +	 * DRRS mode type (Seamless OR Static DRRS)
>>>> +	 * drrs_mode Val 0x2 is Seamless DRRS and 0 is Static DRRS.
>>>> +	 * These values correspond to the VBT values for drrs mode.
>>>> +	 */
>>>> +	int drrs_mode;
>>>> +	/* DRRS enabled or disabled in VBT */
>>>> +	bool drrs_enabled;
>>>
>>> Both the drrs_mode and drrs_enabled should be replaced by one enum
>>> drrs_support_type which you introduce later. It's all self-explanatory
>>> that way, and you don't need to explain so much.
>>>
>> There are 2 options in the VBT. drrs_enabled to check if DRRS is
>> supported, drrs_mode to show which type. It has been added here as it is
>> for readability.
> 
> I can understand the argument for anything defined in intel_bios.[ch],
> but for the rest, including struct intel_vbt_data, readability comes
> from other reasons than being equivalent with the VBT specs.
> 
>>>> +
>>>>  	/* eDP */
>>>>  	int edp_rate;
>>>>  	int edp_lanes;
>>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>>> index f88e507..5b04a69 100644
>>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>>> @@ -195,6 +195,21 @@ get_lvds_fp_timing(const struct bdb_header *bdb,
>>>>  	return (const struct lvds_fp_timing *)((const u8 *)bdb + ofs);
>>>>  }
>>>>  
>>>> +/**
>>>> + * This function returns the 2 bit information pertaining to a panel type
>>>> + * present in a 32 bit field in VBT blocks. There are 16 panel types in VBT
>>>> + * each occupying 2 bits of information in some 32 bit fields of VBT blocks.
>>>> + */
>>>> +static int
>>>> +get_mode_by_paneltype(unsigned int word)
>>>> +{
>>>> +	/**
>>>> +	 * The caller of this API should interpret the 2 bits
>>>> +	 * based on VBT description for that field.
>>>> +	 */
>>>> +	return (word >> ((panel_type - 1) * 2)) & MODE_MASK;
>>>
>>> Everywhere else in intel_bios.c panel_type is used 0-based.
>>>
>> VBT indexes panel type as 1,2,3. Therefore, we have to make the change
>> to match kernel's 0-based usage.
> 
> Again, everywhere else in intel_bios.c we use panel_type, directly as it
> is in VBT, 0-based. Are you saying it's all wrong? panel_type can't be
> 1-based in this one instance, and 0-based in all other instances, right?
> 
> This is actually the first priority to check.
> 
I have discussed with the author of the patch and i will modify this to
make panel_type 0-based.
>>>> +}
>>>
>>> You use the above function only once. IMHO with all the explaining above
>>> it's just too much of a burden to the reader. Just do this in the code.
>>>
>> Ok.
>>>> +
>>>>  /* Try to find integrated panel data */
>>>>  static void
>>>>  parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>>> @@ -218,6 +233,11 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>>>  
>>>>  	panel_type = lvds_options->panel_type;
>>>>  
>>>> +	dev_priv->vbt.drrs_mode =
>>>> +		get_mode_by_paneltype(lvds_options->dps_panel_type_bits);
>>>> +	DRM_DEBUG_KMS("DRRS supported mode is : %s\n",
>>>                                              ^ drop the space here
>>>
>> Ok
>>>> +			(dev_priv->vbt.drrs_mode == 0) ? "STATIC" : "SEAMLESS");
>>>
>>> Why shouting?
>>>
>>>> +
>>>>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>>>>  	if (!lvds_lfp_data)
>>>>  		return;
>>>> @@ -488,6 +508,9 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>>>>  
>>>>  	if (driver->dual_frequency)
>>>>  		dev_priv->render_reclock_avail = true;
>>>> +
>>>> +	dev_priv->vbt.drrs_enabled = driver->drrs_state;
>>>> +	DRM_DEBUG_KMS("DRRS State Enabled : %d\n", driver->drrs_state);
>>>                                          ^ and here and everywhere
>>>
>> Ok
>>>>  }
>>>>  
>>>>  static void
>>>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>>>> index 81ed58c..0a3a751 100644
>>>> --- a/drivers/gpu/drm/i915/intel_bios.h
>>>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>>>> @@ -281,6 +281,9 @@ struct bdb_general_definitions {
>>>>  	union child_device_config devices[0];
>>>>  } __packed;
>>>>  
>>>> +/* Mask for DRRS / Panel Channel / SSC / BLT control bits extraction */
>>>> +#define MODE_MASK		0x3
>>>> +
>>>>  struct bdb_lvds_options {
>>>>  	u8 panel_type;
>>>>  	u8 rsvd1;
>>>> @@ -293,6 +296,18 @@ struct bdb_lvds_options {
>>>>  	u8 lvds_edid:1;
>>>>  	u8 rsvd2:1;
>>>>  	u8 rsvd4;
>>>> +	/* LVDS Panel channel bits stored here */
>>>> +	u32 lvds_panel_channel_bits;
>>>
>>> Why does this have "lvds" but the rest not? Why do some fields end in
>>> "bits" but some others not? Some consistency please.
>>>
>> This is how the vbt block definition doc mentions each of these fields.
>> This is the reason why it has been added in this manner.
> 
> I don't have my vbt spec handy right now, but when I was last checking
> these it didn't look consistent with the spec.
> 
> Same for the ones below.
> 
VBT-driver interface document has been referred for this.
>>
>>>> +	/* LVDS SSC (Spread Spectrum Clock) bits stored here. */
>>>> +	u16 ssc_bits;
>>>> +	u16 ssc_freq;
>>>> +	u16 ssc_ddt;
>>>> +	/* Panel color depth defined here */
>>>> +	u16 panel_color_depth;
>>>
>>> I really *really* wish I knew why some stuff is specified in the lvds
>>> bios blob and some other in edp blob and some stuff specified here is
>>> used in the edp side. Ugh. I wonder if we should check this
>>> panel_color_depth for edp too.
>>>
>> This is how the vbt block definition doc mentions each of these fields.
>> This is the reason why it has been added in this manner.
>>
>>>> +	/* LVDS panel type bits stored here */
>>>> +	u32 dps_panel_type_bits;
>>>> +	/* LVDS backlight control type bits stored here */
>>>> +	u32 blt_control_type_bits;
>>>>  } __packed;
>>>>  
>>>>  /* LFP pointer table contains entries to the struct below */
>>>> @@ -462,6 +477,20 @@ struct bdb_driver_features {
>>>>  
>>>>  	u8 hdmi_termination;
>>>>  	u8 custom_vbt_version;
>>>> +	/* Driver features data block */
>>>> +	u16 rmpm_state:1;
>>>> +	u16 s2ddt_state:1;
>>>> +	u16 dpst_state:1;
>>>> +	u16 bltclt_state:1;
>>>> +	u16 adb_state:1;
>>>> +	u16 drrs_state:1;
>>>> +	u16 grs_state:1;
>>>> +	u16 gpmt_state:1;
>>>> +	u16 tbt_state:1;
>>>> +	u16 psr_state:1;
>>>> +	u16 ips_state:1;
>>>
>>> All of the above should be foo_enabled to make them self-explanatory.
>>>
>>>> +	u16 reserved3:4;
>>>> +	u16 pc_feature_validity:1;
>>>
>>> Similarly for this one, should be pc_feature_valid.
>>>
>> This is how the vbt block definition doc mentions this field.
>> This is the reason why it has been added in this manner.
>>>>  } __packed;
>>>>  
>>>>  #define EDP_18BPP	0
>>>> -- 
>>>> 1.7.9.5
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>
> 

Re: [PATCH 3/5] drm/i915: Add support for DRRS to switch RR

[Vandana Kannan]

On Jan-30-2014 12:22 PM, Jani Nikula wrote:
> On Thu, 30 Jan 2014, Vandana Kannan <vandana.kannan@intel.com> wrote:
>> On Jan-22-2014 7:44 PM, Jani Nikula wrote:
>>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>>>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>>>
>>>> This patch computes and stored 2nd M/N/TU for switching to different
>>>> refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
>>>> between alternate refresh rates programmed in 2nd M/N/TU registers.
>>>>
>>>> v2: Daniel's review comments
>>>> Computing M2/N2 in compute_config and storing it in crtc_config
>>>>
>>>> v3: Modified reference to edp_downclock and edp_downclock_avail based on the
>>>> changes made to move them from dev_private to intel_panel.
>>>>
>>>> v4: Modified references to is_drrs_supported based on the changes made to
>>>> rename it to drrs_support.
>>>>
>>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>>>>  drivers/gpu/drm/i915/intel_dp.c  |  106 ++++++++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>>>>  3 files changed, 112 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index f1eece4..57d2b64 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -3206,6 +3206,7 @@
>>>>  #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
>>>>  #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
>>>>  #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
>>>> +#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
>>>>  #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
>>>>  #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
>>>>  #define   PIPECONF_BPC_MASK	(0x7 << 5)
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 079b53f..d1e1d6e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -791,6 +791,21 @@ intel_dp_set_clock(struct intel_encoder *encoder,
>>>>  	}
>>>>  }
>>>>  
>>>> +static void
>>>> +intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
>>>> +{
>>>> +	struct drm_device *dev = crtc->base.dev;
>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> +	enum transcoder transcoder = crtc->config.cpu_transcoder;
>>>> +
>>>> +	I915_WRITE(PIPE_DATA_M2(transcoder),
>>>> +		TU_SIZE(m_n->tu) | m_n->gmch_m);
>>>> +	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
>>>> +	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
>>>> +	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
>>>> +	return;
>>>
>>> Superfluous return statement.
>>>
>> Ok.
>>>> +}
>>>> +
>>>>  bool
>>>>  intel_dp_compute_config(struct intel_encoder *encoder,
>>>>  			struct intel_crtc_config *pipe_config)
>>>> @@ -894,6 +909,14 @@ found:
>>>>  			       pipe_config->port_clock,
>>>>  			       &pipe_config->dp_m_n);
>>>>  
>>>> +	if (intel_connector->panel.edp_downclock_avail &&
>>>> +	intel_dp->drrs_state.drrs_support == SEAMLESS_DRRS_SUPPORT) {
>>>> +		intel_link_compute_m_n(bpp, lane_count,
>>>> +				intel_connector->panel.edp_downclock,
>>>> +				pipe_config->port_clock,
>>>> +				&pipe_config->dp_m2_n2);
>>>> +	}
>>>
>>> Indentation in the above block.
>>>
>> Ok.
>>>> +
>>>>  	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>>>>  
>>>>  	return true;
>>>> @@ -3522,6 +3545,87 @@ 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) {
>>>
>>> The opening brace belongs on a new line.
>>>
>> Ok.
>>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>> +	struct drm_mode_config *mode_config = &dev->mode_config;
>>>> +	struct intel_encoder *encoder;
>>>> +	struct intel_dp *intel_dp = NULL;
>>>> +	struct intel_crtc_config *config = NULL;
>>>> +	struct intel_crtc *intel_crtc = NULL;
>>>> +	struct intel_connector *intel_connector = NULL;
>>>> +	bool found_edp = false;
>>>> +	u32 reg, val;
>>>> +	int index = 0;
>>>> +
>>>> +	if (refresh_rate <= 0) {
>>>> +		DRM_INFO("Refresh rate should be positive non-zero.\n");
>>>> +		goto out;
>>>> +	}
>>>
>>> Under what conditions do you expect this to happen?
>>>
>> In future, when refresh rate switching based on user space requests will
>> be implemented, intel_dp_set_drrs_state() will be used. This check here
>> is to safeguard calls from user space.
>>>> +
>>>> +	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
>>>> +		if (encoder->type == INTEL_OUTPUT_EDP) {
>>>> +			intel_dp = enc_to_intel_dp(&encoder->base);
>>>> +			intel_crtc = encoder->new_crtc;
>>>> +			if (!intel_crtc) {
>>>> +				DRM_INFO("DRRS: intel_crtc not initialized\n");
>>>
>>> DRM_INFO is probably a bit verbose.
>>>
>>>> +				goto out;
>>>> +			}
>>>> +			config = &intel_crtc->config;
>>>> +			intel_connector = intel_dp->attached_connector;
>>>> +			found_edp = true;
>>>> +			break;
>>>> +		}
>>>> +	}
>>>
>>> I'm confused. You go through all the trouble of saving the crtc and the
>>> connector (although only in the next patch) but here you iterate all the
>>> encoders to find the one. Why?
>>>
>>>> +
>>>> +	if (!found_edp) {
>>>
>>> There's no need to add an extra variable for this. You already have
>>> intel_dp, intel_crtc, config and intel_connector you can check!
>>>
>> Ok. I will make necessary changes.
>>>> +		DRM_INFO("DRRS supported for eDP only.\n");
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	if (intel_dp->drrs_state.drrs_support < SEAMLESS_DRRS_SUPPORT) {
>>>> +		DRM_INFO("Seamless DRRS not supported.\n");
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
>>>> +		index = DRRS_HIGH_RR;
>>>> +	else
>>>> +		index = DRRS_LOW_RR;
>>>> +
>>>> +	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
>>>> +		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	if (!intel_crtc->active) {
>>>> +		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	mutex_lock(&intel_dp->drrs_state.mutex);
>>>> +
>>>> +	/* Haswell and below */
>>>> +	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
>>>> +		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
>>>> +		val = I915_READ(reg);
>>>> +		if (index > DRRS_HIGH_RR) {
>>>> +			val |= PIPECONF_EDP_RR_MODE_SWITCH;
>>>
>>> Please check bspec for workarounds you need to do wrt frame start delay
>>> before enabling the pipe/transcoder.
>>>
>> We checked the BSpec and there was no workaround mentioned for this part.
> 
> Oh, I'm sorry, I should have been more specific. This was in the SNB
> specs: "Workaround : If this pipe is connected to a port on the PCH and
> this power savings mode will be used, then before the pipe and
> transcoder are enabled, the frame start delay in the pipe and transcoder
> must be set to 11b. When these conditions are no longer true, the frame
> start delay must be returned to the previous value after the pipe and
> transcoder are disabled."
> 
> Going forward, I think it's okay to enable drrs on ivb+ at first, and
> enable snb with the workarounds in a follow-up patch. Others may
> disagree...
> 
SNB support will be added as a separate patch. I will modify the checks
wherever applicable to include Gen6+
>>>> +			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
>>>> +		} else
>>>> +			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
>>>
>>> Per coding style, if one branch needs braces, then all do.
>>>
>> Ok.
>>>> +		I915_WRITE(reg, val);
>>>> +	}
>>>> +
>>>> +	intel_dp->drrs_state.drrs_refresh_rate_type = index;
>>>> +	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
>>>> +
>>>> +	mutex_unlock(&intel_dp->drrs_state.mutex);
>>>> +
>>>> +out:
>>>> +	return;
>>>
>>> Superfluous return statement.
>>>
>> Ok.
>>>> +}
>>>> +
>>>>  static void
>>>>  intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>>>  			struct intel_connector *intel_connector,
>>>> @@ -3555,6 +3659,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>>>  		intel_connector->panel.edp_downclock =
>>>>  			intel_connector->panel.downclock_mode->clock;
>>>>  
>>>> +		mutex_init(&intel_dp->drrs_state.mutex);
>>>> +
>>>>  		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>>>>  
>>>>  		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index d208bf5..d1c60fa 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -291,6 +291,9 @@ struct intel_crtc_config {
>>>>  	int pipe_bpp;
>>>>  	struct intel_link_m_n dp_m_n;
>>>>  
>>>> +	/* m2_n2 for eDP downclock */
>>>> +	struct intel_link_m_n dp_m2_n2;
>>>> +
>>>>  	/*
>>>>  	 * Frequence the dpll for the port should run at. Differs from the
>>>>  	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
>>>> @@ -489,6 +492,7 @@ enum edp_drrs_refresh_rate_type {
>>>>  struct drrs_info {
>>>>  	enum drrs_support_type drrs_support;
>>>>  	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
>>>> +	struct mutex mutex;
>>>>  };
>>>>  
>>>>  struct intel_dp {
>>>> @@ -761,7 +765,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
>>>>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
>>>>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>>>>  void intel_edp_psr_update(struct drm_device *dev);
>>>> -
>>>> +extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
>>>
>>> No need for the "extern".
>>>
>> Ok.
>>>>  
>>>>  /* intel_dsi.c */
>>>>  bool intel_dsi_init(struct drm_device *dev);
>>>> -- 
>>>> 1.7.9.5
>>>>
>>>> _______________________________________________
>>>> Intel-gfx mailing list
>>>> Intel-gfx@lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>
> 

Re: [PATCH 1/5] drm/i915: Adding VBT fields to support eDP DRRS feature

[Daniel Vetter]

On Mon, Feb 03, 2014 at 09:13:17AM +0530, Vandana Kannan wrote:
> > Again, everywhere else in intel_bios.c we use panel_type, directly as it
> > is in VBT, 0-based. Are you saying it's all wrong? panel_type can't be
> > 1-based in this one instance, and 0-based in all other instances, right?
> > 
> > This is actually the first priority to check.
> > 
> I have discussed with the author of the patch and i will modify this to
> make panel_type 0-based.

Can we drag the author of the patch itself into this discussion, here on
intel-gfx? Generally round-trip is much faster if we cut out as many
middlemen as possible ...

This is just a general upstreaming bkm.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915: Parse EDID probed modes for DRRS support

[Vandana Kannan]

On Jan-30-2014 9:03 AM, Vandana Kannan wrote:
> On Jan-22-2014 7:03 PM, Jani Nikula wrote:
>> On Mon, 23 Dec 2013, Vandana Kannan <vandana.kannan@intel.com> wrote:
>>> From: Pradeep Bhat <pradeep.bhat@intel.com>
>>>
>>> This patch and finds out the lowest refresh rate supported for the resolution
>>> same as the fixed_mode, based on the implementaion find_panel_downclock.
>>> It also checks the VBT fields to see if panel supports seamless DRRS or not.
>>> Based on above data it marks whether eDP panel supports seamless DRRS or not.
>>> This information is needed for supporting seamless DRRS switch for certain
>>> power saving usecases. This patch is tested by enabling the DRM logs and
>>> user should see whether Seamless DRRS is supported or not.
>>
>> This patch (and therefore the later patches) no longer apply to
>> drm-intel-nightly. It might affect my review a bit, but here goes
>> anyway.
>>
> I will rebase and resend the patch.
>>>
>>> v2: Daniel's review comments
>>> Modified downclock deduction based on intel_find_panel_downclock
>>>
>>> v3: Chris's review comments
>>> Moved edp_downclock_avail and edp_downclock to intel_panel
>>>
>>> v4: Jani's review comments.
>>> Changed name of the enum edp_panel_type to drrs_support type.
>>> Change is_drrs_supported to drrs_support of type enum drrs_support_type.
>>>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/intel_dp.c  |   45 ++++++++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_drv.h |   30 +++++++++++++++++++++++++
>>>  2 files changed, 75 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 8f17f8f..079b53f 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3522,6 +3522,46 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>>>  		      I915_READ(pp_div_reg));
>>>  }
>>>  
>>> +static void
>>> +intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>>> +			struct intel_connector *intel_connector,
>>> +			struct drm_display_mode *fixed_mode) {
>>
>> I'll explain later why I think you should change the signature of the
>> function.
>>
>>> +	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_i915_private *dev_priv = dev->dev_private;
>>> +
>>> +	/**
>>> +	 * Check if PSR is supported by panel and enabled
>>> +	 * if so then DRRS is reported as not supported for Haswell.
>>> +	 */
>>> +	if (INTEL_INFO(dev)->gen < 8 &&	intel_edp_is_psr_enabled(dev)) {
>>> +		DRM_INFO("eDP panel has PSR enabled. Cannot support DRRS\n");
>>> +		return;
>>> +	}
>>> +
>>> +	/* First check if DRRS is enabled from VBT struct */
>>> +	if (!dev_priv->vbt.drrs_enabled) {
>>> +		DRM_INFO("VBT doesn't support DRRS\n");
>>> +		return;
>>> +	}
>>> +
>>> +	intel_connector->panel.downclock_mode =	intel_find_panel_downclock(dev,
>>> +					fixed_mode, connector);
>>> +
>>> +	if (intel_connector->panel.downclock_mode != NULL &&
>>> +		dev_priv->vbt.drrs_mode == SEAMLESS_DRRS_SUPPORT) {
>>> +		intel_connector->panel.edp_downclock_avail = true;
>>
>> If you rearranged the code a bit, you could make the
>> panel.downclock_mode != NULL mean the same as
>> edp_downclock_avail. I.e. if you have the downclock_mode there, it's
>> available.
>>
> This was done to be in sync with lvds_downclock implementation based on
> previous review comments.
>>> +		intel_connector->panel.edp_downclock =
>>> +			intel_connector->panel.downclock_mode->clock;
>>
>> I don't understand why you need two copies of the clock.
>>
>> In general, we should try and avoid adding extra state and copies of
>> information for stuff that we can readily derive from other information.
>>
>>> +
>>> +		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
>>
>> Again. I can't see intel_dp->drrs_state.drrs_support ever needing to be
>> different from dev_priv->vbt.drrs_mode. So why the copy?
>>
> This was done to make things more readable.
>>> +
>>> +		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
>>> +		DRM_INFO("SEAMLESS DRRS supported for eDP panel.\n");
>>> +	}
>>> +}
>>> +
>>>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>>  				     struct intel_connector *intel_connector)
>>>  {
>>> @@ -3535,6 +3575,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>>  	struct drm_display_mode *scan;
>>>  	struct edid *edid;
>>>  
>>> +	intel_dp->drrs_state.drrs_support = DRRS_NOT_SUPPORTED;
>>> +
>>>  	if (!is_edp(intel_dp))
>>>  		return true;
>>>  
>>> @@ -3579,6 +3621,9 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>>  	list_for_each_entry(scan, &connector->probed_modes, head) {
>>>  		if ((scan->type & DRM_MODE_TYPE_PREFERRED)) {
>>>  			fixed_mode = drm_mode_duplicate(dev, scan);
>>> +			if (INTEL_INFO(dev)->gen >= 5)
>>> +				intel_dp_drrs_initialize(intel_dig_port,
>>> +					intel_connector, fixed_mode);
>>
>> Is there any reason not to do this at the top level after checking for
>> the VBT mode?
>>
> This was done as fixed_mode was required.
> 
>> Also, we have a separate function for initializing the panel struct, so
>> I think you should make intel_dp_drrs_initialize() return the downclock
>> mode or NULL, and pass that to intel_panel_init() instead of
>> initializing the panel struct directly within the function.
>>
> I will make this change.
I have submitted a patch "[Intel-gfx] drm/i915: Initialize downclock
mode in panel init" to modify intel_panel_init() and all its callers.
>>>  			break;
>>>  		}
>>>  	}
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index e903432..d208bf5 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -168,6 +168,9 @@ struct intel_panel {
>>>  		bool active_low_pwm;
>>>  		struct backlight_device *device;
>>>  	} backlight;
>>> +
>>> +	bool edp_downclock_avail;
>>> +	int edp_downclock;
>>
>> As I said, I think you can get rid of both of these.
>>
> As mentioned above, this was done to be in sync with lvds_downclock
> implementation based on previous review comments.
>>>  };
>>>  
>>>  struct intel_connector {
>>> @@ -462,6 +465,32 @@ struct intel_hdmi {
>>>  
>>>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>>>  
>>> +/**
>>> + * This enum is used to indicate the DRRS support type.
>>> + */
>>> +enum drrs_support_type {
>>> +	DRRS_NOT_SUPPORTED = -1,
>>> +	STATIC_DRRS_SUPPORT = 0, /* 1:1 mapping with VBT */
>>> +	SEAMLESS_DRRS_SUPPORT = 2 /* 1:1 mapping with VBT */ };
>>
>> I don't see any value in having 1:1 mapping with VBT. Not even in having
>> 1:1 mapping between struct intel_vbt_data and the actual VBT. It's
>> supposed to be parsed data.
>>
>> Instead, I do see value in making DRRS_NOT_SUPPORTED == 0 as the logical
>> thing to do.
>>
> Ok. I will make necessary changes..
>>> +/**
>>> + * 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 */
>>> +};
>>> +/**
>>> + * The drrs_info struct will represent the DRRS feature for eDP
>>> + * panel.
>>> + */
>>
>> This comment does not add any value.
>>
> Ok.
>>> +struct drrs_info {
>>> +	enum drrs_support_type drrs_support;
>>> +	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
>>
>> Because this will be accessed through intel_dp->drrs_state, there's no
>> need to duplicate "drrs" in the field names here. It will be obvious
>> from the context.
>>
> Ok.
>>> +};
>>> +
>>>  struct intel_dp {
>>>  	uint32_t output_reg;
>>>  	uint32_t aux_ch_ctl_reg;
>>> @@ -487,6 +516,7 @@ struct intel_dp {
>>>  	bool want_panel_vdd;
>>>  	bool psr_setup_done;
>>>  	struct intel_connector *attached_connector;
>>> +	struct drrs_info drrs_state;
>>>  };
>>>  
>>>  struct intel_digital_port {
>>> -- 
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
> 

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

[Vandana Kannan]

From: Pradeep Bhat <pradeep.bhat@intel.com>

This patch computes and stored 2nd M/N/TU for switching to different
refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
between alternate refresh rates programmed in 2nd M/N/TU registers.

v2: Daniel's review comments
Computing M2/N2 in compute_config and storing it in crtc_config

v3: Modified reference to edp_downclock and edp_downclock_avail based on the
changes made to move them from dev_private to intel_panel.

v4: Modified references to is_drrs_supported based on the changes made to
rename it to drrs_support.

v5: Jani's review comments
Removed superfluous return statements. Changed support for Gen 7 and above.
Corrected indentation. Re-structured the code which finds crtc and connector
from encoder. Changed some logs to be less verbose.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |    6 +++
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_dp.c  |  101 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    6 ++-
 4 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6b4d0b20..3dd1d7e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -774,6 +774,11 @@ struct i915_fbc {
 	} no_fbc_reason;
 };
 
+struct i915_drrs {
+	struct intel_connector *connector;
+	struct intel_dp *dp;
+};
+
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
@@ -1466,6 +1471,7 @@ typedef struct drm_i915_private {
 	int num_plane;
 
 	struct i915_fbc fbc;
+	struct i915_drrs drrs;
 	struct intel_opregion opregion;
 	struct intel_vbt_data vbt;
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f73a49d..bfd7703 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3225,6 +3225,7 @@
 #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
 #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
+#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f7dba83..1933675 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -832,6 +832,20 @@ intel_dp_set_clock(struct intel_encoder *encoder,
 	}
 }
 
+static void
+intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder transcoder = crtc->config.cpu_transcoder;
+
+	I915_WRITE(PIPE_DATA_M2(transcoder),
+		TU_SIZE(m_n->tu) | m_n->gmch_m);
+	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
+	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
+	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
@@ -936,6 +950,15 @@ found:
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (intel_connector->panel.edp_downclock_avail &&
+		intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
+			intel_link_compute_m_n(bpp, lane_count,
+				intel_connector->panel.edp_downclock,
+				pipe_config->port_clock,
+				&pipe_config->dp_m2_n2);
+	}
+
+
 	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
 
 	return true;
@@ -3666,6 +3689,79 @@ 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)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *encoder;
+	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;
+	int index = 0;
+
+	if (refresh_rate <= 0) {
+		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
+		return;
+	}
+
+	if (intel_dp == NULL) {
+		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
+		return;
+	}
+
+	encoder = intel_attached_encoder(&intel_connector->base);
+	intel_crtc = encoder->new_crtc;
+
+	if (!intel_crtc) {
+		DRM_DEBUG_KMS("DRRS: intel_crtc not initialized\n");
+		return;
+	}
+
+	config = &intel_crtc->config;
+
+	if (intel_dp->drrs_state.type < SEAMLESS_DRRS_SUPPORT) {
+		DRM_DEBUG_KMS("Seamless DRRS not supported.\n");
+		return;
+	}
+
+	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
+		index = DRRS_HIGH_RR;
+	else
+		index = DRRS_LOW_RR;
+
+	if (index == intel_dp->drrs_state.refresh_rate_type) {
+		DRM_DEBUG_KMS(
+			"DRRS requested for previously set RR...ignoring\n");
+		return;
+	}
+
+	if (!intel_crtc->active) {
+		DRM_DEBUG_KMS("eDP encoder has been disabled. CRTC not Active\n");
+		return;
+	}
+
+	mutex_lock(&intel_dp->drrs_state.mutex);
+
+	if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
+		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
+		val = I915_READ(reg);
+		if (index > DRRS_HIGH_RR) {
+			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
+		} else {
+			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+		}
+		I915_WRITE(reg, val);
+	}
+
+	intel_dp->drrs_state.refresh_rate_type = index;
+	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+
+	mutex_unlock(&intel_dp->drrs_state.mutex);
+
+}
+
 static struct drm_display_mode *
 intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3700,6 +3796,11 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			downclock_mode->clock;
 
+		dev_priv->drrs.connector = intel_connector;
+		dev_priv->drrs.dp = intel_dp;
+
+		mutex_init(&intel_dp->drrs_state.mutex);
+
 		intel_dp->drrs_state.type = dev_priv->vbt.drrs_type;
 
 		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c41c735..4f7c816 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -291,6 +291,9 @@ struct intel_crtc_config {
 	int pipe_bpp;
 	struct intel_link_m_n dp_m_n;
 
+	/* m2_n2 for eDP downclock */
+	struct intel_link_m_n dp_m2_n2;
+
 	/*
 	 * Frequence the dpll for the port should run at. Differs from the
 	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
@@ -481,6 +484,7 @@ enum edp_drrs_refresh_rate_type {
 struct drrs_info {
 	enum drrs_support_type type;
 	enum edp_drrs_refresh_rate_type refresh_rate_type;
+	struct mutex mutex;
 };
 
 struct intel_dp {
@@ -769,7 +773,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
-
+void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
 
 /* intel_dsi.c */
 bool intel_dsi_init(struct drm_device *dev);
-- 
1.7.9.5

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

[Vandana Kannan]

From: Pradeep Bhat <pradeep.bhat@intel.com>

This patch computes and stored 2nd M/N/TU for switching to different
refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
between alternate refresh rates programmed in 2nd M/N/TU registers.

v2: Daniel's review comments
Computing M2/N2 in compute_config and storing it in crtc_config

v3: Modified reference to edp_downclock and edp_downclock_avail based on the
changes made to move them from dev_private to intel_panel.

v4: Modified references to is_drrs_supported based on the changes made to
rename it to drrs_support.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_dp.c  |  106 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    6 ++-
 3 files changed, 112 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f1eece4..57d2b64 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3206,6 +3206,7 @@
 #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
 #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
+#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 079b53f..c473d02 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -791,6 +791,21 @@ intel_dp_set_clock(struct intel_encoder *encoder,
 	}
 }
 
+static void
+intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n) 
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder transcoder = crtc->config.cpu_transcoder;
+
+	I915_WRITE(PIPE_DATA_M2(transcoder),
+		TU_SIZE(m_n->tu) | m_n->gmch_m);
+	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
+	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
+	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
+	return;
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
@@ -894,6 +909,14 @@ found:
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (intel_connector->panel.edp_downclock_avail &&
+	intel_dp->drrs_state.drrs_support == SEAMLESS_DRRS_SUPPORT) {
+		intel_link_compute_m_n(bpp, lane_count,
+				intel_connector->panel.edp_downclock,
+				pipe_config->port_clock,
+				&pipe_config->dp_m2_n2);
+	}
+
 	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
 
 	return true;
@@ -3522,6 +3545,87 @@ 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) {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+	struct intel_crtc_config *config = NULL;
+	struct intel_crtc *intel_crtc = NULL;
+	struct intel_connector *intel_connector = NULL;
+	bool found_edp = false;
+	u32 reg, val;
+	int index = 0;
+
+	if (refresh_rate <= 0) {
+		DRM_INFO("Refresh rate should be positive non-zero.\n");
+		goto out;
+	}
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+			intel_dp = enc_to_intel_dp(&encoder->base);
+			intel_crtc = encoder->new_crtc;
+			if (!intel_crtc) {
+				DRM_INFO("DRRS: intel_crtc not initialized\n");
+				goto out;
+			}
+			config = &intel_crtc->config;
+			intel_connector = intel_dp->attached_connector;
+			found_edp = true;
+			break;
+		}
+	}
+
+	if (!found_edp) {
+		DRM_INFO("DRRS supported for eDP only.\n");
+		goto out;
+	}
+
+	if (intel_dp->drrs_state.drrs_support < SEAMLESS_DRRS_SUPPORT) {
+		DRM_INFO("Seamless DRRS not supported.\n");
+		goto out;
+	}
+
+	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
+		index = DRRS_HIGH_RR;
+	else
+		index = DRRS_LOW_RR;
+
+	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
+		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
+		goto out;
+	}
+
+	if (!intel_crtc->active) {
+		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
+		goto out;
+	}
+
+	mutex_lock(&intel_dp->drrs_state.mutex);
+
+	/* Haswell and below */
+	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
+		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
+		val = I915_READ(reg);
+		if (index > DRRS_HIGH_RR) {
+			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
+		} else
+			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+		I915_WRITE(reg, val);
+	}
+
+	intel_dp->drrs_state.drrs_refresh_rate_type = index;
+	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+
+	mutex_unlock(&intel_dp->drrs_state.mutex);
+
+out:
+	return;
+}
+
 static void
 intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3555,6 +3659,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			intel_connector->panel.downclock_mode->clock;
 
+		mutex_init(&intel_dp->drrs_state.mutex);
+
 		intel_dp->drrs_state.drrs_support = dev_priv->vbt.drrs_mode;
 
 		intel_dp->drrs_state.drrs_refresh_rate_type = DRRS_HIGH_RR;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d208bf5..d1c60fa 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -291,6 +291,9 @@ struct intel_crtc_config {
 	int pipe_bpp;
 	struct intel_link_m_n dp_m_n;
 
+	/* m2_n2 for eDP downclock */
+	struct intel_link_m_n dp_m2_n2;
+
 	/*
 	 * Frequence the dpll for the port should run at. Differs from the
 	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
@@ -489,6 +492,7 @@ enum edp_drrs_refresh_rate_type {
 struct drrs_info {
 	enum drrs_support_type drrs_support;
 	enum edp_drrs_refresh_rate_type drrs_refresh_rate_type;
+	struct mutex mutex;
 };
 
 struct intel_dp {
@@ -761,7 +765,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
-
+extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
 
 /* intel_dsi.c */
 bool intel_dsi_init(struct drm_device *dev);
-- 
1.7.9.5

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

[Vandana Kannan]

From: Pradeep Bhat <pradeep.bhat@intel.com>

This patch computes and stored 2nd M/N/TU for switching to different
refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
between alternate refresh rates programmed in 2nd M/N/TU registers.

v2: Daniel's review comments
Computing M2/N2 in compute_config and storing it in crtc_config

v3: Modified reference to edp_downclock and edp_downclock_avail based on the
changes made to move them from dev_private to intel_panel.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_dp.c  |  108 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    6 ++-
 3 files changed, 114 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3be449d..a52c663 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3203,6 +3203,7 @@
 #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
 #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
+#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 67674ee..e110f26 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -791,6 +791,22 @@ intel_dp_set_clock(struct intel_encoder *encoder,
 	}
 }
 
+static void
+intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder transcoder = crtc->config.cpu_transcoder;
+
+	I915_WRITE(PIPE_DATA_M2(transcoder),
+		   TU_SIZE(m_n->tu) | m_n->gmch_m);
+	I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
+	I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
+	I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
+
+	return;
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
@@ -894,6 +910,14 @@ found:
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (intel_connector->panel.edp_downclock_avail &&
+	intel_dp->drrs_state.is_drrs_supported == SEAMLESS_DRRS_SUPPORT) {
+		intel_link_compute_m_n(bpp, lane_count,
+				intel_connector->panel.edp_downclock,
+				pipe_config->port_clock,
+				&pipe_config->dp_m2_n2);
+	}
+
 	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
 
 	return true;
@@ -3524,6 +3548,88 @@ 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)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+	struct intel_crtc_config *config = NULL;
+	struct intel_crtc *intel_crtc = NULL;
+	struct intel_connector *intel_connector = NULL;
+	bool found_edp = false;
+	u32 reg, val;
+	int index = 0;
+
+	if (refresh_rate <= 0) {
+		DRM_INFO("Refresh rate should be positive non-zero.\n");
+		goto out;
+	}
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+			intel_dp = enc_to_intel_dp(&encoder->base);
+			intel_crtc = encoder->new_crtc;
+			if (!intel_crtc) {
+			DRM_INFO("DRRS: intel_crtc not initialized\n");
+				goto out;
+			}
+			config = &intel_crtc->config;
+			intel_connector = intel_dp->attached_connector;
+			found_edp = true;
+			break;
+		}
+	}
+
+	if (!found_edp) {
+		DRM_INFO("DRRS supported for eDP only.\n");
+		goto out;
+	}
+
+	if (intel_dp->drrs_state.is_drrs_supported < SEAMLESS_DRRS_SUPPORT) {
+		DRM_INFO("Seamless DRRS not supported.\n");
+		goto out;
+	}
+
+	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
+		index = DRRS_HIGH_RR;
+	else
+		index = DRRS_LOW_RR;
+
+	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
+		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
+		goto out;
+	}
+
+	if (!intel_crtc->active) {
+		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
+		goto out;
+	}
+
+	mutex_lock(&intel_dp->drrs_state.mutex);
+
+	/* Haswell and below */
+	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
+		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
+		val = I915_READ(reg);
+		if (index > DRRS_HIGH_RR) {
+			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
+		} else
+			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+		I915_WRITE(reg, val);
+	}
+
+	intel_dp->drrs_state.drrs_refresh_rate_type = index;
+	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+
+	mutex_unlock(&intel_dp->drrs_state.mutex);
+
+out:
+	return;
+}
+
 static void
 intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3558,6 +3664,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			intel_connector->panel.downclock_mode->clock;
 
+		mutex_init(&intel_dp->drrs_state.mutex);
+
 		intel_dp->drrs_state.is_drrs_supported
 				= dev_priv->vbt.drrs_mode;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index aa79adc..863fa60 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -291,6 +291,9 @@ struct intel_crtc_config {
 	int pipe_bpp;
 	struct intel_link_m_n dp_m_n;
 
+	/* m2_n2 for eDP downclock */
+	struct intel_link_m_n dp_m2_n2;
+
 	/*
 	 * Frequence the dpll for the port should run at. Differs from the
 	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
@@ -491,6 +494,7 @@ enum edp_drrs_refresh_rate_type {
 struct drrs_info {
 	int is_drrs_supported;
 	int drrs_refresh_rate_type;
+	struct mutex mutex;
 };
 
 struct intel_dp {
@@ -763,7 +767,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
-
+extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
 
 /* intel_dsi.c */
 bool intel_dsi_init(struct drm_device *dev);
-- 
1.7.9.5

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

[Vandana Kannan]

From: Pradeep Bhat <pradeep.bhat@intel.com>

This patch computes and stored 2nd M/N/TU for switching to different
refresh rate dynamically. PIPECONF_EDP_RR_MODE_SWITCH bit helps toggle
between alternate refresh rates programmed in 2nd M/N/TU registers.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_dp.c  |  109 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    6 ++-
 3 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3be449d..a52c663 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3203,6 +3203,7 @@
 #define   PIPECONF_INTERLACED_DBL_ILK		(4 << 21) /* ilk/snb only */
 #define   PIPECONF_PFIT_PF_INTERLACED_DBL_ILK	(5 << 21) /* ilk/snb only */
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
+#define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 935b46e..fbf71ed 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -791,6 +791,23 @@ intel_dp_set_clock(struct intel_encoder *encoder,
 	}
 }
 
+static void
+intel_dp_set_m2_n2(struct intel_crtc *crtc, struct intel_link_m_n *m_n)
+{
+	struct drm_device *dev = crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder transcoder = crtc->config.cpu_transcoder;
+
+	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
+		I915_WRITE(PIPE_DATA_M2(transcoder),
+			   TU_SIZE(m_n->tu) | m_n->gmch_m);
+		I915_WRITE(PIPE_DATA_N2(transcoder), m_n->gmch_n);
+		I915_WRITE(PIPE_LINK_M2(transcoder), m_n->link_m);
+		I915_WRITE(PIPE_LINK_N2(transcoder), m_n->link_n);
+	}
+	return;
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
@@ -894,6 +911,14 @@ found:
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (dev_priv->edp_downclock_avail &&
+	intel_dp->drrs_state.is_drrs_supported == SEAMLESS_DRRS_SUPPORT) {
+		intel_link_compute_m_n(bpp, lane_count,
+				dev_priv->edp_downclock,
+				pipe_config->port_clock,
+				&pipe_config->dp_m2_n2);
+	}
+
 	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
 
 	return true;
@@ -3524,6 +3549,88 @@ 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)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_mode_config *mode_config = &dev->mode_config;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+	struct intel_crtc_config *config = NULL;
+	struct intel_crtc *intel_crtc = NULL;
+	struct intel_connector *intel_connector = NULL;
+	bool found_edp = false;
+	u32 reg, val;
+	int index = 0;
+
+	if (refresh_rate <= 0) {
+		DRM_INFO("Refresh rate should be positive non-zero.\n");
+		goto out;
+	}
+
+	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+			intel_dp = enc_to_intel_dp(&encoder->base);
+			intel_crtc = encoder->new_crtc;
+			if (!intel_crtc) {
+			DRM_INFO("DRRS: intel_crtc not initialized\n");
+				goto out;
+			}
+			config = &intel_crtc->config;
+			intel_connector = intel_dp->attached_connector;
+			found_edp = true;
+			break;
+		}
+	}
+
+	if (!found_edp) {
+		DRM_INFO("DRRS supported for eDP only.\n");
+		goto out;
+	}
+
+	if (intel_dp->drrs_state.is_drrs_supported < SEAMLESS_DRRS_SUPPORT) {
+		DRM_INFO("Seamless DRRS not supported.\n");
+		goto out;
+	}
+
+	if (intel_connector->panel.fixed_mode->vrefresh == refresh_rate)
+		index = DRRS_HIGH_RR;
+	else
+		index = DRRS_LOW_RR;
+
+	if (index == intel_dp->drrs_state.drrs_refresh_rate_type) {
+		DRM_INFO("DRRS requested for previously set RR...ignoring\n");
+		goto out;
+	}
+
+	if (!intel_crtc->active) {
+		DRM_INFO("eDP encoder has been disabled. CRTC not Active\n");
+		goto out;
+	}
+
+	mutex_lock(&intel_dp->drrs_state.mutex);
+
+	/* Haswell and below */
+	if (INTEL_INFO(dev)->gen >= 5 && INTEL_INFO(dev)->gen < 8) {
+		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
+		val = I915_READ(reg);
+		if (index > DRRS_HIGH_RR) {
+			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
+		} else
+			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+		I915_WRITE(reg, val);
+	}
+
+	intel_dp->drrs_state.drrs_refresh_rate_type = index;
+	DRM_INFO("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+
+	mutex_unlock(&intel_dp->drrs_state.mutex);
+
+out:
+	return;
+}
+
 static void
 intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3558,6 +3665,8 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
 		dev_priv->edp_downclock =
 			intel_connector->panel.downclock_mode->clock;
 
+		mutex_init(&intel_dp->drrs_state.mutex);
+
 		intel_dp->drrs_state.is_drrs_supported
 				= dev_priv->vbt.drrs_mode;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6ec5f4e..3e4306b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -288,6 +288,9 @@ struct intel_crtc_config {
 	int pipe_bpp;
 	struct intel_link_m_n dp_m_n;
 
+	/* m2_n2 for eDP downclock */
+	struct intel_link_m_n dp_m2_n2;
+
 	/*
 	 * Frequence the dpll for the port should run at. Differs from the
 	 * adjusted dotclock e.g. for DP or 12bpc hdmi mode. This is also
@@ -488,6 +491,7 @@ enum edp_drrs_refresh_rate_type {
 struct drrs_info {
 	int is_drrs_supported;
 	int drrs_refresh_rate_type;
+	struct mutex mutex;
 };
 
 struct intel_dp {
@@ -760,7 +764,7 @@ void ironlake_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
-
+extern void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
 
 /* intel_dsi.c */
 bool intel_dsi_init(struct drm_device *dev);
-- 
1.7.9.5