[PATCH 0/6] v6: Enabling DRRS in the kernel

[PATCH 0/6] v6: Enabling DRRS in the kernel

[Vandana Kannan]

Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which     
enables 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. DRRS idleness time has been modified to be a kernel module param.

Added support for VLV.

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

Vandana Kannan (3):
  [Intel-gfx] drm/i915: Idleness detection for DRRS
  [Intel-gfx] drm/i915/bdw: Add support for DRRS to switch RR
  [Intel-gfx] drm/i915: Support for RR switching on VLV

 drivers/gpu/drm/i915/i915_drv.h      |   25 +++++
 drivers/gpu/drm/i915/i915_params.c   |    8 ++
 drivers/gpu/drm/i915/i915_reg.h      |    2 +
 drivers/gpu/drm/i915/intel_bios.c    |   29 +++++
 drivers/gpu/drm/i915/intel_bios.h    |   29 +++++
 drivers/gpu/drm/i915/intel_display.c |   18 ++-
 drivers/gpu/drm/i915/intel_dp.c      |  203 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   33 +++++-
 drivers/gpu/drm/i915/intel_pm.c      |  138 +++++++++++++++++++++++
 9 files changed, 481 insertions(+), 4 deletions(-)

-- 
1.7.9.5

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

v3: Incorporated Jani's review comments
Removed function which deducts drrs mode from panel_type. Modified some
print statements. Made changes to use DRRS_NOT_SUPPORTED as 0 instead of -1.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_drv.h   |   13 +++++++++++++
 drivers/gpu/drm/i915/intel_bios.c |   29 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 728b9c3..6b4d0b20 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1218,6 +1218,12 @@ struct ddi_vbt_port_info {
 	uint8_t supports_dp:1;
 };
 
+enum drrs_support_type {
+	DRRS_NOT_SUPPORTED = 0,
+	STATIC_DRRS_SUPPORT = 1,
+	SEAMLESS_DRRS_SUPPORT = 2
+};
+
 struct intel_vbt_data {
 	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
 	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
@@ -1233,6 +1239,13 @@ struct intel_vbt_data {
 	int lvds_ssc_freq;
 	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
 
+	/*
+	 * DRRS support type (Seamless OR Static DRRS OR not supported)
+	 * drrs_support type Val 0x2 is Seamless DRRS and 0 is Static DRRS.
+	 * These values correspond to the VBT values for drrs mode.
+	 */
+	enum drrs_support_type drrs_type;
+
 	/* 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 86b95ca..2414eca 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -218,6 +218,25 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
 
 	panel_type = lvds_options->panel_type;
 
+	dev_priv->vbt.drrs_type = (lvds_options->dps_panel_type_bits
+					>> (panel_type * 2)) & MODE_MASK;
+	/*
+	 * VBT has static DRRS = 0 and seamless DRRS = 2.
+	 * The below piece of code is required to adjust vbt.drrs_type
+	 * to match the enum drrs_support_type.
+	 */
+	switch (dev_priv->vbt.drrs_type) {
+	case 0:
+		dev_priv->vbt.drrs_type = STATIC_DRRS_SUPPORT;
+		DRM_DEBUG_KMS("DRRS supported mode is static\n");
+		break;
+	case 2:
+		DRM_DEBUG_KMS("DRRS supported mode is seamless\n");
+		break;
+	default:
+		break;
+	}
+
 	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
 	if (!lvds_lfp_data)
 		return;
@@ -516,6 +535,16 @@ parse_driver_features(struct drm_i915_private *dev_priv,
 
 	if (driver->dual_frequency)
 		dev_priv->render_reclock_avail = true;
+
+	DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled);
+	/*
+	 * If DRRS is not supported, drrs_type has to be set to 0.
+	 * This is because, VBT is configured in such a way that
+	 * static DRRS is 0 and DRRS not supported is represented by
+	 * driver->drrs_enabled=false
+	 */
+	if (!driver->drrs_enabled)
+		dev_priv->vbt.drrs_type = driver->drrs_enabled;
 }
 
 static void
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 282de5e..5505d6c 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 */
@@ -478,6 +493,20 @@ struct bdb_driver_features {
 
 	u8 hdmi_termination;
 	u8 custom_vbt_version;
+	/* Driver features data block */
+	u16 rmpm_enabled:1;
+	u16 s2ddt_enabled:1;
+	u16 dpst_enabled:1;
+	u16 bltclt_enabled:1;
+	u16 adb_enabled:1;
+	u16 drrs_enabled:1;
+	u16 grs_enabled:1;
+	u16 gpmt_enabled:1;
+	u16 tbt_enabled:1;
+	u16 psr_enabled:1;
+	u16 ips_enabled:1;
+	u16 reserved3:4;
+	u16 pc_feature_valid:1;
 } __packed;
 
 #define EDP_18BPP	0
-- 
1.7.9.5

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

[Jani Nikula]

On Fri, 07 Mar 2014, 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.
>
> v3: Incorporated Jani's review comments
> Removed function which deducts drrs mode from panel_type. Modified some
> print statements. Made changes to use DRRS_NOT_SUPPORTED as 0 instead of -1.
>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |   13 +++++++++++++
>  drivers/gpu/drm/i915/intel_bios.c |   29 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 728b9c3..6b4d0b20 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1218,6 +1218,12 @@ struct ddi_vbt_port_info {
>  	uint8_t supports_dp:1;
>  };
>  
> +enum drrs_support_type {
> +	DRRS_NOT_SUPPORTED = 0,
> +	STATIC_DRRS_SUPPORT = 1,
> +	SEAMLESS_DRRS_SUPPORT = 2
> +};
> +
>  struct intel_vbt_data {
>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
> @@ -1233,6 +1239,13 @@ struct intel_vbt_data {
>  	int lvds_ssc_freq;
>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>  
> +	/*
> +	 * DRRS support type (Seamless OR Static DRRS OR not supported)
> +	 * drrs_support type Val 0x2 is Seamless DRRS and 0 is Static DRRS.
> +	 * These values correspond to the VBT values for drrs mode.
> +	 */

The comment is wrong.

> +	enum drrs_support_type drrs_type;
> +
>  	/* 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 86b95ca..2414eca 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -218,6 +218,25 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>  
>  	panel_type = lvds_options->panel_type;
>  
> +	dev_priv->vbt.drrs_type = (lvds_options->dps_panel_type_bits
> +					>> (panel_type * 2)) & MODE_MASK;
> +	/*
> +	 * VBT has static DRRS = 0 and seamless DRRS = 2.
> +	 * The below piece of code is required to adjust vbt.drrs_type
> +	 * to match the enum drrs_support_type.
> +	 */
> +	switch (dev_priv->vbt.drrs_type) {

Please don't mix the vbt and the enum at all like this, it's error
prone. Just make the switch on the value in vbt, and for each case
assign the appropriate enum to dev_priv->vbt.drrs_type.

> +	case 0:
> +		dev_priv->vbt.drrs_type = STATIC_DRRS_SUPPORT;
> +		DRM_DEBUG_KMS("DRRS supported mode is static\n");
> +		break;
> +	case 2:
> +		DRM_DEBUG_KMS("DRRS supported mode is seamless\n");
> +		break;
> +	default:
> +		break;

And fail on the default.

> +	}
> +
>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>  	if (!lvds_lfp_data)
>  		return;
> @@ -516,6 +535,16 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>  
>  	if (driver->dual_frequency)
>  		dev_priv->render_reclock_avail = true;
> +
> +	DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled);
> +	/*
> +	 * If DRRS is not supported, drrs_type has to be set to 0.
> +	 * This is because, VBT is configured in such a way that
> +	 * static DRRS is 0 and DRRS not supported is represented by
> +	 * driver->drrs_enabled=false
> +	 */
> +	if (!driver->drrs_enabled)
> +		dev_priv->vbt.drrs_type = driver->drrs_enabled;

Don't assign a boolean to an enum.

>  }
>  
>  static void
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 282de5e..5505d6c 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 */
> @@ -478,6 +493,20 @@ struct bdb_driver_features {
>  
>  	u8 hdmi_termination;
>  	u8 custom_vbt_version;
> +	/* Driver features data block */
> +	u16 rmpm_enabled:1;
> +	u16 s2ddt_enabled:1;
> +	u16 dpst_enabled:1;
> +	u16 bltclt_enabled:1;
> +	u16 adb_enabled:1;
> +	u16 drrs_enabled:1;
> +	u16 grs_enabled:1;
> +	u16 gpmt_enabled:1;
> +	u16 tbt_enabled:1;
> +	u16 psr_enabled:1;
> +	u16 ips_enabled:1;
> +	u16 reserved3:4;
> +	u16 pc_feature_valid:1;
>  } __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 1/6] drm/i915: Adding VBT fields to support eDP DRRS feature

[Vandana Kannan]

On Mar-26-2014 6:06 PM, Jani Nikula wrote:
> On Fri, 07 Mar 2014, 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.
>>
>> v3: Incorporated Jani's review comments
>> Removed function which deducts drrs mode from panel_type. Modified some
>> print statements. Made changes to use DRRS_NOT_SUPPORTED as 0 instead of -1.
>>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h   |   13 +++++++++++++
>>  drivers/gpu/drm/i915/intel_bios.c |   29 +++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>>  3 files changed, 71 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 728b9c3..6b4d0b20 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1218,6 +1218,12 @@ struct ddi_vbt_port_info {
>>  	uint8_t supports_dp:1;
>>  };
>>  
>> +enum drrs_support_type {
>> +	DRRS_NOT_SUPPORTED = 0,
>> +	STATIC_DRRS_SUPPORT = 1,
>> +	SEAMLESS_DRRS_SUPPORT = 2
>> +};
>> +
>>  struct intel_vbt_data {
>>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
>> @@ -1233,6 +1239,13 @@ struct intel_vbt_data {
>>  	int lvds_ssc_freq;
>>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>>  
>> +	/*
>> +	 * DRRS support type (Seamless OR Static DRRS OR not supported)
>> +	 * drrs_support type Val 0x2 is Seamless DRRS and 0 is Static DRRS.
>> +	 * These values correspond to the VBT values for drrs mode.
>> +	 */
> 
> The comment is wrong.
>
Could you elaborate on this?

>> +	enum drrs_support_type drrs_type;
>> +
>>  	/* 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 86b95ca..2414eca 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -218,6 +218,25 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>  
>>  	panel_type = lvds_options->panel_type;
>>  
>> +	dev_priv->vbt.drrs_type = (lvds_options->dps_panel_type_bits
>> +					>> (panel_type * 2)) & MODE_MASK;
>> +	/*
>> +	 * VBT has static DRRS = 0 and seamless DRRS = 2.
>> +	 * The below piece of code is required to adjust vbt.drrs_type
>> +	 * to match the enum drrs_support_type.
>> +	 */
>> +	switch (dev_priv->vbt.drrs_type) {
> 
> Please don't mix the vbt and the enum at all like this, it's error
> prone. Just make the switch on the value in vbt, and for each case
> assign the appropriate enum to dev_priv->vbt.drrs_type.
> 
Ok
>> +	case 0:
>> +		dev_priv->vbt.drrs_type = STATIC_DRRS_SUPPORT;
>> +		DRM_DEBUG_KMS("DRRS supported mode is static\n");
>> +		break;
>> +	case 2:
>> +		DRM_DEBUG_KMS("DRRS supported mode is seamless\n");
>> +		break;
>> +	default:
>> +		break;
> 
> And fail on the default.
> 
Ok
>> +	}
>> +
>>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>>  	if (!lvds_lfp_data)
>>  		return;
>> @@ -516,6 +535,16 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>>  
>>  	if (driver->dual_frequency)
>>  		dev_priv->render_reclock_avail = true;
>> +
>> +	DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled);
>> +	/*
>> +	 * If DRRS is not supported, drrs_type has to be set to 0.
>> +	 * This is because, VBT is configured in such a way that
>> +	 * static DRRS is 0 and DRRS not supported is represented by
>> +	 * driver->drrs_enabled=false
>> +	 */
>> +	if (!driver->drrs_enabled)
>> +		dev_priv->vbt.drrs_type = driver->drrs_enabled;
> 
> Don't assign a boolean to an enum.
> 
Ok
>>  }
>>  
>>  static void
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 282de5e..5505d6c 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 */
>> @@ -478,6 +493,20 @@ struct bdb_driver_features {
>>  
>>  	u8 hdmi_termination;
>>  	u8 custom_vbt_version;
>> +	/* Driver features data block */
>> +	u16 rmpm_enabled:1;
>> +	u16 s2ddt_enabled:1;
>> +	u16 dpst_enabled:1;
>> +	u16 bltclt_enabled:1;
>> +	u16 adb_enabled:1;
>> +	u16 drrs_enabled:1;
>> +	u16 grs_enabled:1;
>> +	u16 gpmt_enabled:1;
>> +	u16 tbt_enabled:1;
>> +	u16 psr_enabled:1;
>> +	u16 ips_enabled:1;
>> +	u16 reserved3:4;
>> +	u16 pc_feature_valid:1;
>>  } __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 1/6] drm/i915: Adding VBT fields to support eDP DRRS feature

[Jani Nikula]

On Thu, 27 Mar 2014, Vandana Kannan <vandana.kannan@intel.com> wrote:
> On Mar-26-2014 6:06 PM, Jani Nikula wrote:
>> On Fri, 07 Mar 2014, 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.
>>>
>>> v3: Incorporated Jani's review comments
>>> Removed function which deducts drrs mode from panel_type. Modified some
>>> print statements. Made changes to use DRRS_NOT_SUPPORTED as 0 instead of -1.
>>>
>>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>>> Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h   |   13 +++++++++++++
>>>  drivers/gpu/drm/i915/intel_bios.c |   29 +++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/i915/intel_bios.h |   29 +++++++++++++++++++++++++++++
>>>  3 files changed, 71 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 728b9c3..6b4d0b20 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1218,6 +1218,12 @@ struct ddi_vbt_port_info {
>>>  	uint8_t supports_dp:1;
>>>  };
>>>  
>>> +enum drrs_support_type {
>>> +	DRRS_NOT_SUPPORTED = 0,
>>> +	STATIC_DRRS_SUPPORT = 1,
>>> +	SEAMLESS_DRRS_SUPPORT = 2
>>> +};
>>> +
>>>  struct intel_vbt_data {
>>>  	struct drm_display_mode *lfp_lvds_vbt_mode; /* if any */
>>>  	struct drm_display_mode *sdvo_lvds_vbt_mode; /* if any */
>>> @@ -1233,6 +1239,13 @@ struct intel_vbt_data {
>>>  	int lvds_ssc_freq;
>>>  	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
>>>  
>>> +	/*
>>> +	 * DRRS support type (Seamless OR Static DRRS OR not supported)
>>> +	 * drrs_support type Val 0x2 is Seamless DRRS and 0 is Static DRRS.
>>> +	 * These values correspond to the VBT values for drrs mode.
>>> +	 */
>> 
>> The comment is wrong.
>>
> Could you elaborate on this?

The values are defined in enum drrs_support_type now and don't
correspond to VBT, e.g. 0 is not static drrs.

Please avoid having values like this in comments altogether, because
experience says they will not be updated if the enum is updated.

BR,
Jani.

>
>>> +	enum drrs_support_type drrs_type;
>>> +
>>>  	/* 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 86b95ca..2414eca 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -218,6 +218,25 @@ parse_lfp_panel_data(struct drm_i915_private *dev_priv,
>>>  
>>>  	panel_type = lvds_options->panel_type;
>>>  
>>> +	dev_priv->vbt.drrs_type = (lvds_options->dps_panel_type_bits
>>> +					>> (panel_type * 2)) & MODE_MASK;
>>> +	/*
>>> +	 * VBT has static DRRS = 0 and seamless DRRS = 2.
>>> +	 * The below piece of code is required to adjust vbt.drrs_type
>>> +	 * to match the enum drrs_support_type.
>>> +	 */
>>> +	switch (dev_priv->vbt.drrs_type) {
>> 
>> Please don't mix the vbt and the enum at all like this, it's error
>> prone. Just make the switch on the value in vbt, and for each case
>> assign the appropriate enum to dev_priv->vbt.drrs_type.
>> 
> Ok
>>> +	case 0:
>>> +		dev_priv->vbt.drrs_type = STATIC_DRRS_SUPPORT;
>>> +		DRM_DEBUG_KMS("DRRS supported mode is static\n");
>>> +		break;
>>> +	case 2:
>>> +		DRM_DEBUG_KMS("DRRS supported mode is seamless\n");
>>> +		break;
>>> +	default:
>>> +		break;
>> 
>> And fail on the default.
>> 
> Ok
>>> +	}
>>> +
>>>  	lvds_lfp_data = find_section(bdb, BDB_LVDS_LFP_DATA);
>>>  	if (!lvds_lfp_data)
>>>  		return;
>>> @@ -516,6 +535,16 @@ parse_driver_features(struct drm_i915_private *dev_priv,
>>>  
>>>  	if (driver->dual_frequency)
>>>  		dev_priv->render_reclock_avail = true;
>>> +
>>> +	DRM_DEBUG_KMS("DRRS State Enabled:%d\n", driver->drrs_enabled);
>>> +	/*
>>> +	 * If DRRS is not supported, drrs_type has to be set to 0.
>>> +	 * This is because, VBT is configured in such a way that
>>> +	 * static DRRS is 0 and DRRS not supported is represented by
>>> +	 * driver->drrs_enabled=false
>>> +	 */
>>> +	if (!driver->drrs_enabled)
>>> +		dev_priv->vbt.drrs_type = driver->drrs_enabled;
>> 
>> Don't assign a boolean to an enum.
>> 
> Ok
>>>  }
>>>  
>>>  static void
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>>> index 282de5e..5505d6c 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 */
>>> @@ -478,6 +493,20 @@ struct bdb_driver_features {
>>>  
>>>  	u8 hdmi_termination;
>>>  	u8 custom_vbt_version;
>>> +	/* Driver features data block */
>>> +	u16 rmpm_enabled:1;
>>> +	u16 s2ddt_enabled:1;
>>> +	u16 dpst_enabled:1;
>>> +	u16 bltclt_enabled:1;
>>> +	u16 adb_enabled:1;
>>> +	u16 drrs_enabled:1;
>>> +	u16 grs_enabled:1;
>>> +	u16 gpmt_enabled:1;
>>> +	u16 tbt_enabled:1;
>>> +	u16 psr_enabled:1;
>>> +	u16 ips_enabled:1;
>>> +	u16 reserved3:4;
>>> +	u16 pc_feature_valid:1;
>>>  } __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

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

v5: Incorporated Jani's review comments
Modify intel_dp_drrs_initialize to return downclock mode. Support for Gen7
and above.

v6: Incorporated Chris's review comments.
Changed initialize to init in intel_drrs_initialize

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  |   54 +++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h |   20 ++++++++++++++
 2 files changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88cc9d3..39365bf 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3666,6 +3666,50 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
 		      I915_READ(pp_div_reg));
 }
 
+static struct drm_display_mode *
+intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
+			struct intel_connector *intel_connector,
+			struct drm_display_mode *fixed_mode)
+{
+	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;
+	struct drm_display_mode *downclock_mode = NULL;
+
+	/**
+	 * 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 downclock_mode;
+	}
+
+	/* First check if DRRS is enabled from VBT struct */
+	if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) {
+		DRM_INFO("VBT doesn't support DRRS\n");
+		return downclock_mode;
+	}
+
+	downclock_mode = intel_find_panel_downclock
+					(dev, fixed_mode, connector);
+
+	if (downclock_mode != NULL &&
+		dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
+		intel_connector->panel.edp_downclock_avail = true;
+		intel_connector->panel.edp_downclock =
+			downclock_mode->clock;
+
+		intel_dp->drrs_state.type = dev_priv->vbt.drrs_type;
+
+		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
+		DRM_INFO("seamless DRRS supported for eDP panel.\n");
+	}
+
+	return downclock_mode;
+}
+
 static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 				     struct intel_connector *intel_connector,
 				     struct edp_power_seq *power_seq)
@@ -3675,10 +3719,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *fixed_mode = NULL;
+	struct drm_display_mode *downclock_mode = NULL;
 	bool has_dpcd;
 	struct drm_display_mode *scan;
 	struct edid *edid;
 
+	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
+
 	if (!is_edp(intel_dp))
 		return true;
 
@@ -3720,6 +3767,11 @@ 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 > 6)
+				downclock_mode =
+					intel_dp_drrs_init(
+						intel_dig_port,
+						intel_connector, fixed_mode);
 			break;
 		}
 	}
@@ -3732,7 +3784,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
 	}
 
-	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
+	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
 	intel_panel_setup_backlight(connector);
 
 	return true;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 6aa549a..c41c735 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 {
@@ -464,6 +467,22 @@ struct intel_hdmi {
 
 #define DP_MAX_DOWNSTREAM_PORTS		0x10
 
+/**
+ * HIGH_RR is the highest eDP panel refresh rate read from EDID
+ * LOW_RR is the lowest eDP panel refresh rate found from EDID
+ * parsing for same resolution.
+ */
+enum edp_drrs_refresh_rate_type {
+	DRRS_HIGH_RR,
+	DRRS_LOW_RR,
+	DRRS_MAX_RR, /* RR count */
+};
+
+struct drrs_info {
+	enum drrs_support_type type;
+	enum edp_drrs_refresh_rate_type refresh_rate_type;
+};
+
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -503,6 +522,7 @@ struct intel_dp {
 				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
+	struct drrs_info drrs_state;
 };
 
 struct intel_digital_port {
-- 
1.7.9.5

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

[Jani Nikula]

This and the following patches need to be rebased on top of current
-nightly.

On Fri, 07 Mar 2014, 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.
> 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.
>
> v5: Incorporated Jani's review comments
> Modify intel_dp_drrs_initialize to return downclock mode. Support for Gen7
> and above.
>
> v6: Incorporated Chris's review comments.
> Changed initialize to init in intel_drrs_initialize
>
> 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  |   54 +++++++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h |   20 ++++++++++++++
>  2 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 88cc9d3..39365bf 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3666,6 +3666,50 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  		      I915_READ(pp_div_reg));
>  }
>  
> +static struct drm_display_mode *
> +intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
> +			struct intel_connector *intel_connector,
> +			struct drm_display_mode *fixed_mode)
> +{
> +	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;
> +	struct drm_display_mode *downclock_mode = NULL;
> +
> +	/**
> +	 * 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 downclock_mode;

Just return NULL explicitly if that's the intention.

> +	}
> +
> +	/* First check if DRRS is enabled from VBT struct */
> +	if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) {
> +		DRM_INFO("VBT doesn't support DRRS\n");
> +		return downclock_mode;

Same here.

> +	}
> +
> +	downclock_mode = intel_find_panel_downclock
> +					(dev, fixed_mode, connector);
> +
> +	if (downclock_mode != NULL &&
> +		dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
> +		intel_connector->panel.edp_downclock_avail = true;
> +		intel_connector->panel.edp_downclock =
> +			downclock_mode->clock;

Why do you need a copy of downclock_mode->clock in
intel_connector->panel.edp_downclock? You can always get that through
intel_connector->panel.downclock_mode->clock. Single point of truth.

> +
> +		intel_dp->drrs_state.type = dev_priv->vbt.drrs_type;
> +
> +		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
> +		DRM_INFO("seamless DRRS supported for eDP panel.\n");
> +	}
> +
> +	return downclock_mode;
> +}
> +
>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  				     struct intel_connector *intel_connector,
>  				     struct edp_power_seq *power_seq)
> @@ -3675,10 +3719,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_display_mode *fixed_mode = NULL;
> +	struct drm_display_mode *downclock_mode = NULL;
>  	bool has_dpcd;
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
>  
> +	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
> +
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> @@ -3720,6 +3767,11 @@ 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 > 6)

It seems to me this condition should be inside intel_dp_drrs_init().

> +				downclock_mode =
> +					intel_dp_drrs_init(
> +						intel_dig_port,
> +						intel_connector, fixed_mode);
>  			break;
>  		}
>  	}
> @@ -3732,7 +3784,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>  	}
>  
> -	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> +	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>  	intel_panel_setup_backlight(connector);
>  
>  	return true;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6aa549a..c41c735 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 {
> @@ -464,6 +467,22 @@ struct intel_hdmi {
>  
>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>  
> +/**
> + * HIGH_RR is the highest eDP panel refresh rate read from EDID
> + * LOW_RR is the lowest eDP panel refresh rate found from EDID
> + * parsing for same resolution.
> + */
> +enum edp_drrs_refresh_rate_type {
> +	DRRS_HIGH_RR,
> +	DRRS_LOW_RR,
> +	DRRS_MAX_RR, /* RR count */
> +};
> +
> +struct drrs_info {
> +	enum drrs_support_type type;
> +	enum edp_drrs_refresh_rate_type refresh_rate_type;
> +};
> +
>  struct intel_dp {
>  	uint32_t output_reg;
>  	uint32_t aux_ch_ctl_reg;
> @@ -503,6 +522,7 @@ struct intel_dp {
>  				     bool has_aux_irq,
>  				     int send_bytes,
>  				     uint32_t aux_clock_divider);
> +	struct drrs_info drrs_state;

Any reason this isn't an unnamed struct here? And if you need the name,
why is it different from the field name? drrs_info vs. 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 2/6] drm/i915: Parse EDID probed modes for DRRS support

[Jani Nikula]

On Wed, 26 Mar 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> This and the following patches need to be rebased on top of current
> -nightly.
>
> On Fri, 07 Mar 2014, 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.
>> 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.
>>
>> v5: Incorporated Jani's review comments
>> Modify intel_dp_drrs_initialize to return downclock mode. Support for Gen7
>> and above.
>>
>> v6: Incorporated Chris's review comments.
>> Changed initialize to init in intel_drrs_initialize
>>
>> 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  |   54 +++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/drm/i915/intel_drv.h |   20 ++++++++++++++
>>  2 files changed, 73 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 88cc9d3..39365bf 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3666,6 +3666,50 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>>  		      I915_READ(pp_div_reg));
>>  }
>>  
>> +static struct drm_display_mode *
>> +intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>> +			struct intel_connector *intel_connector,
>> +			struct drm_display_mode *fixed_mode)
>> +{
>> +	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;
>> +	struct drm_display_mode *downclock_mode = NULL;
>> +
>> +	/**
>> +	 * 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 downclock_mode;
>
> Just return NULL explicitly if that's the intention.
>
>> +	}
>> +
>> +	/* First check if DRRS is enabled from VBT struct */
>> +	if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) {
>> +		DRM_INFO("VBT doesn't support DRRS\n");
>> +		return downclock_mode;
>
> Same here.
>
>> +	}
>> +
>> +	downclock_mode = intel_find_panel_downclock
>> +					(dev, fixed_mode, connector);
>> +
>> +	if (downclock_mode != NULL &&
>> +		dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
>> +		intel_connector->panel.edp_downclock_avail = true;
>> +		intel_connector->panel.edp_downclock =
>> +			downclock_mode->clock;
>
> Why do you need a copy of downclock_mode->clock in
> intel_connector->panel.edp_downclock? You can always get that through
> intel_connector->panel.downclock_mode->clock. Single point of truth.

Also, what does intel_connector->panel.edp_downclock_avail indicate that
can't be derived from downclock_mode != NULL && dev_priv->vbt.drrs_type
== SEAMLESS_DRRS_SUPPORT?

>
>> +
>> +		intel_dp->drrs_state.type = dev_priv->vbt.drrs_type;
>> +
>> +		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
>> +		DRM_INFO("seamless DRRS supported for eDP panel.\n");
>> +	}
>> +
>> +	return downclock_mode;
>> +}
>> +
>>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  				     struct intel_connector *intel_connector,
>>  				     struct edp_power_seq *power_seq)
>> @@ -3675,10 +3719,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_display_mode *fixed_mode = NULL;
>> +	struct drm_display_mode *downclock_mode = NULL;
>>  	bool has_dpcd;
>>  	struct drm_display_mode *scan;
>>  	struct edid *edid;
>>  
>> +	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
>> +
>>  	if (!is_edp(intel_dp))
>>  		return true;
>>  
>> @@ -3720,6 +3767,11 @@ 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 > 6)
>
> It seems to me this condition should be inside intel_dp_drrs_init().
>
>> +				downclock_mode =
>> +					intel_dp_drrs_init(
>> +						intel_dig_port,
>> +						intel_connector, fixed_mode);
>>  			break;
>>  		}
>>  	}
>> @@ -3732,7 +3784,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>>  	}
>>  
>> -	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>> +	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>>  	intel_panel_setup_backlight(connector);
>>  
>>  	return true;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 6aa549a..c41c735 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 {
>> @@ -464,6 +467,22 @@ struct intel_hdmi {
>>  
>>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>>  
>> +/**
>> + * HIGH_RR is the highest eDP panel refresh rate read from EDID
>> + * LOW_RR is the lowest eDP panel refresh rate found from EDID
>> + * parsing for same resolution.
>> + */
>> +enum edp_drrs_refresh_rate_type {
>> +	DRRS_HIGH_RR,
>> +	DRRS_LOW_RR,
>> +	DRRS_MAX_RR, /* RR count */
>> +};
>> +
>> +struct drrs_info {
>> +	enum drrs_support_type type;
>> +	enum edp_drrs_refresh_rate_type refresh_rate_type;
>> +};
>> +
>>  struct intel_dp {
>>  	uint32_t output_reg;
>>  	uint32_t aux_ch_ctl_reg;
>> @@ -503,6 +522,7 @@ struct intel_dp {
>>  				     bool has_aux_irq,
>>  				     int send_bytes,
>>  				     uint32_t aux_clock_divider);
>> +	struct drrs_info drrs_state;
>
> Any reason this isn't an unnamed struct here? And if you need the name,
> why is it different from the field name? drrs_info vs. 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

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Vandana Kannan]

On Mar-26-2014 6:19 PM, Jani Nikula wrote:
> On Wed, 26 Mar 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> This and the following patches need to be rebased on top of current
>> -nightly.
>>
>> On Fri, 07 Mar 2014, 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.
>>> 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.
>>>
>>> v5: Incorporated Jani's review comments
>>> Modify intel_dp_drrs_initialize to return downclock mode. Support for Gen7
>>> and above.
>>>
>>> v6: Incorporated Chris's review comments.
>>> Changed initialize to init in intel_drrs_initialize
>>>
>>> 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  |   54 +++++++++++++++++++++++++++++++++++++-
>>>  drivers/gpu/drm/i915/intel_drv.h |   20 ++++++++++++++
>>>  2 files changed, 73 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 88cc9d3..39365bf 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3666,6 +3666,50 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>>>  		      I915_READ(pp_div_reg));
>>>  }
>>>  
>>> +static struct drm_display_mode *
>>> +intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>> +			struct intel_connector *intel_connector,
>>> +			struct drm_display_mode *fixed_mode)
>>> +{
>>> +	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;
>>> +	struct drm_display_mode *downclock_mode = NULL;
>>> +
>>> +	/**
>>> +	 * 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 downclock_mode;
>>
>> Just return NULL explicitly if that's the intention.
>>
Ok
>>> +	}
>>> +
>>> +	/* First check if DRRS is enabled from VBT struct */
>>> +	if (dev_priv->vbt.drrs_type == DRRS_NOT_SUPPORTED) {
>>> +		DRM_INFO("VBT doesn't support DRRS\n");
>>> +		return downclock_mode;
>>
>> Same here.
>>
Ok
>>> +	}
>>> +
>>> +	downclock_mode = intel_find_panel_downclock
>>> +					(dev, fixed_mode, connector);
>>> +
>>> +	if (downclock_mode != NULL &&
>>> +		dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
>>> +		intel_connector->panel.edp_downclock_avail = true;
>>> +		intel_connector->panel.edp_downclock =
>>> +			downclock_mode->clock;
>>
>> Why do you need a copy of downclock_mode->clock in
>> intel_connector->panel.edp_downclock? You can always get that through
>> intel_connector->panel.downclock_mode->clock. Single point of truth.
> 
> Also, what does intel_connector->panel.edp_downclock_avail indicate that
> can't be derived from downclock_mode != NULL && dev_priv->vbt.drrs_type
> == SEAMLESS_DRRS_SUPPORT?
> 
>>
edp_downclock_avail and edp_downclock were introduced based on early
review comments to keep edp downclock implementation in line with
lvds_downclock implementation. These 2 variables can be removed and
referenced as you mentioned above..
>>> +
>>> +		intel_dp->drrs_state.type = dev_priv->vbt.drrs_type;
>>> +
>>> +		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
>>> +		DRM_INFO("seamless DRRS supported for eDP panel.\n");
>>> +	}
>>> +
>>> +	return downclock_mode;
>>> +}
>>> +
>>>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>>  				     struct intel_connector *intel_connector,
>>>  				     struct edp_power_seq *power_seq)
>>> @@ -3675,10 +3719,13 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>  	struct drm_display_mode *fixed_mode = NULL;
>>> +	struct drm_display_mode *downclock_mode = NULL;
>>>  	bool has_dpcd;
>>>  	struct drm_display_mode *scan;
>>>  	struct edid *edid;
>>>  
>>> +	intel_dp->drrs_state.type = DRRS_NOT_SUPPORTED;
>>> +
>>>  	if (!is_edp(intel_dp))
>>>  		return true;
>>>  
>>> @@ -3720,6 +3767,11 @@ 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 > 6)
>>
>> It seems to me this condition should be inside intel_dp_drrs_init().
>>
Ok
>>> +				downclock_mode =
>>> +					intel_dp_drrs_init(
>>> +						intel_dig_port,
>>> +						intel_connector, fixed_mode);
>>>  			break;
>>>  		}
>>>  	}
>>> @@ -3732,7 +3784,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>>  			fixed_mode->type |= DRM_MODE_TYPE_PREFERRED;
>>>  	}
>>>  
>>> -	intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
>>> +	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>>>  	intel_panel_setup_backlight(connector);
>>>  
>>>  	return true;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 6aa549a..c41c735 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 {
>>> @@ -464,6 +467,22 @@ struct intel_hdmi {
>>>  
>>>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>>>  
>>> +/**
>>> + * HIGH_RR is the highest eDP panel refresh rate read from EDID
>>> + * LOW_RR is the lowest eDP panel refresh rate found from EDID
>>> + * parsing for same resolution.
>>> + */
>>> +enum edp_drrs_refresh_rate_type {
>>> +	DRRS_HIGH_RR,
>>> +	DRRS_LOW_RR,
>>> +	DRRS_MAX_RR, /* RR count */
>>> +};
>>> +
>>> +struct drrs_info {
>>> +	enum drrs_support_type type;
>>> +	enum edp_drrs_refresh_rate_type refresh_rate_type;
>>> +};
>>> +
>>>  struct intel_dp {
>>>  	uint32_t output_reg;
>>>  	uint32_t aux_ch_ctl_reg;
>>> @@ -503,6 +522,7 @@ struct intel_dp {
>>>  				     bool has_aux_irq,
>>>  				     int send_bytes,
>>>  				     uint32_t aux_clock_divider);
>>> +	struct drrs_info drrs_state;
>>
>> Any reason this isn't an unnamed struct here? And if you need the name,
>> why is it different from the field name? drrs_info vs. drrs_state.
>>
I can make this an unnamed struct..
>>>  };
>>>  
>>>  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
> 

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

v6: Modifying i915_drrs to include only intel connector as intel_dp can be
derived from intel connector when required.

v7: As per internal review comments, acquiring mutex just before accessing
drrs RR. As per Chris's review comments, added documentation about the use
of locking in the function.

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  |    5 ++
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_dp.c  |  108 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    6 ++-
 4 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6b4d0b20..43d3dfe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -774,6 +774,10 @@ struct i915_fbc {
 	} no_fbc_reason;
 };
 
+struct i915_drrs {
+	struct intel_connector *connector;
+};
+
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
@@ -1466,6 +1470,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 39365bf..54cde57 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,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 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 = 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_connector == NULL) {
+		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
+		return;
+	}
+
+	encoder = intel_attached_encoder(&intel_connector->base);
+	intel_dp = enc_to_intel_dp(&encoder->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 disabled. CRTC not Active\n");
+		return;
+	}
+
+	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);
+	}
+
+	/*
+	 * mutex taken to ensure that there is no race between differnt
+	 * drrs calls trying to update refresh rate. This scenario may occur
+	 * in future when idleness detection based DRRS in kernel and
+	 * possible calls from user space to set differnt RR are made.
+	 */
+
+	mutex_lock(&intel_dp->drrs_state.mutex);
+
+	intel_dp->drrs_state.refresh_rate_type = index;
+
+	mutex_unlock(&intel_dp->drrs_state.mutex);
+
+	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+}
+
 static struct drm_display_mode *
 intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3701,6 +3805,10 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			downclock_mode->clock;
 
+		dev_priv->drrs.connector = intel_connector;
+
+		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

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

[Jani Nikula]

On Fri, 07 Mar 2014, 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.
>
> 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.
>
> v6: Modifying i915_drrs to include only intel connector as intel_dp can be
> derived from intel connector when required.
>
> v7: As per internal review comments, acquiring mutex just before accessing
> drrs RR. As per Chris's review comments, added documentation about the use
> of locking in the function.
>
> 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  |    5 ++
>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>  drivers/gpu/drm/i915/intel_dp.c  |  108 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>  4 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6b4d0b20..43d3dfe 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -774,6 +774,10 @@ struct i915_fbc {
>  	} no_fbc_reason;
>  };
>  
> +struct i915_drrs {
> +	struct intel_connector *connector;
> +};
> +
>  struct i915_psr {
>  	bool sink_support;
>  	bool source_ok;
> @@ -1466,6 +1470,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 39365bf..54cde57 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) {

Again, seems like intel_connector->panel.edp_downclock_avail is useless
state. Why not just look at intel_connector->panel.downclock_mode !=
NULL?

> +			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,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 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 = dev_priv->drrs.connector;

This drrs.connector is also starting to look like extra state that could
be derived from somewhere else.

> +	u32 reg, val;
> +	int index = 0;
> +
> +	if (refresh_rate <= 0) {
> +		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
> +		return;
> +	}
> +
> +	if (intel_connector == NULL) {
> +		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
> +		return;
> +	}
> +
> +	encoder = intel_attached_encoder(&intel_connector->base);
> +	intel_dp = enc_to_intel_dp(&encoder->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 disabled. CRTC not Active\n");
> +		return;
> +	}
> +
> +	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);
> +	}
> +
> +	/*
> +	 * mutex taken to ensure that there is no race between differnt
> +	 * drrs calls trying to update refresh rate. This scenario may occur
> +	 * in future when idleness detection based DRRS in kernel and
> +	 * possible calls from user space to set differnt RR are made.
> +	 */
> +
> +	mutex_lock(&intel_dp->drrs_state.mutex);

Isn't this always done with dev->struct_mutex held? Why extra locking?

> +
> +	intel_dp->drrs_state.refresh_rate_type = index;
> +
> +	mutex_unlock(&intel_dp->drrs_state.mutex);
> +
> +	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
> +}
> +
>  static struct drm_display_mode *
>  intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  			struct intel_connector *intel_connector,
> @@ -3701,6 +3805,10 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  		intel_connector->panel.edp_downclock =
>  			downclock_mode->clock;
>  
> +		dev_priv->drrs.connector = intel_connector;
> +
> +		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
>
> _______________________________________________
> 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/6] drm/i915: Add support for DRRS to switch RR

[Vandana Kannan]

On Mar-26-2014 6:25 PM, Jani Nikula wrote:
> On Fri, 07 Mar 2014, 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.
>>
>> 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.
>>
>> v6: Modifying i915_drrs to include only intel connector as intel_dp can be
>> derived from intel connector when required.
>>
>> v7: As per internal review comments, acquiring mutex just before accessing
>> drrs RR. As per Chris's review comments, added documentation about the use
>> of locking in the function.
>>
>> 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  |    5 ++
>>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>>  drivers/gpu/drm/i915/intel_dp.c  |  108 ++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>>  4 files changed, 119 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 6b4d0b20..43d3dfe 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -774,6 +774,10 @@ struct i915_fbc {
>>  	} no_fbc_reason;
>>  };
>>  
>> +struct i915_drrs {
>> +	struct intel_connector *connector;
>> +};
>> +
>>  struct i915_psr {
>>  	bool sink_support;
>>  	bool source_ok;
>> @@ -1466,6 +1470,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 39365bf..54cde57 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) {
> 
> Again, seems like intel_connector->panel.edp_downclock_avail is useless
> state. Why not just look at intel_connector->panel.downclock_mode !=
> NULL?
> 
I will make this change..
>> +			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,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 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 = dev_priv->drrs.connector;
> 
> This drrs.connector is also starting to look like extra state that could
> be derived from somewhere else.
> 
Yes, it can be derived from dev. To avoid iterating through the
connector list each time, I have saved the state here..
>> +	u32 reg, val;
>> +	int index = 0;
>> +
>> +	if (refresh_rate <= 0) {
>> +		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
>> +		return;
>> +	}
>> +
>> +	if (intel_connector == NULL) {
>> +		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
>> +		return;
>> +	}
>> +
>> +	encoder = intel_attached_encoder(&intel_connector->base);
>> +	intel_dp = enc_to_intel_dp(&encoder->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 disabled. CRTC not Active\n");
>> +		return;
>> +	}
>> +
>> +	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);
>> +	}
>> +
>> +	/*
>> +	 * mutex taken to ensure that there is no race between differnt
>> +	 * drrs calls trying to update refresh rate. This scenario may occur
>> +	 * in future when idleness detection based DRRS in kernel and
>> +	 * possible calls from user space to set differnt RR are made.
>> +	 */
>> +
>> +	mutex_lock(&intel_dp->drrs_state.mutex);
> 
> Isn't this always done with dev->struct_mutex held? Why extra locking?
> 
This has been added to take care of scenarios that may occur in future
based on user space implementation.
>> +
>> +	intel_dp->drrs_state.refresh_rate_type = index;
>> +
>> +	mutex_unlock(&intel_dp->drrs_state.mutex);
>> +
>> +	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
>> +}
>> +
>>  static struct drm_display_mode *
>>  intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>  			struct intel_connector *intel_connector,
>> @@ -3701,6 +3805,10 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>  		intel_connector->panel.edp_downclock =
>>  			downclock_mode->clock;
>>  
>> +		dev_priv->drrs.connector = intel_connector;
>> +
>> +		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
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

[PATCH 4/6] 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

v3: Internal review comments incorporated
Add NULL pointer check in intel_disable_drrs.
Add drrs calls in i9xx_crtc_enable/disable and valleyview_crtc_enable.

v4: Jani's review comments incorporated.
Change in sequence in intel_update_drrs. Comment modified to remove details
of update param. Modified DRRS idleness interval to a module parameter.

v5: Chris's review comments incorporated.
Initialize connector in idleness detection init. Modifications made to
use only intel_connector in i915_drrs and derive intel_dp when required.
Added a function drrs_fini to cleanup DRRS work.

v6: Internal review comments. Removed check for primary enabled, which is
a redundant check, in the case of clone mode. Added a flag to track
dual-display configuration. Remove print statement for "cancel DRR work"
and print "DRRS not supported" only once.

v7: As per internal review comments, removing calls to update/disable drrs
from sprite update path. For sprite, all drrs related updates would be
taken care of with calls to crtc page flip itself. This will have to be
revisited later if flip infrastructure changes for sprite.

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      |    7 ++
 drivers/gpu/drm/i915/i915_params.c   |    8 ++
 drivers/gpu/drm/i915/intel_display.c |   16 ++++
 drivers/gpu/drm/i915/intel_dp.c      |   26 ++++++-
 drivers/gpu/drm/i915/intel_drv.h     |    5 +-
 drivers/gpu/drm/i915/intel_pm.c      |  138 ++++++++++++++++++++++++++++++++++
 6 files changed, 197 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 43d3dfe..87865e9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -776,6 +776,12 @@ struct i915_fbc {
 
 struct i915_drrs {
 	struct intel_connector *connector;
+	bool is_clone;
+	struct intel_drrs_work {
+		struct delayed_work work;
+		struct drm_crtc *crtc;
+		int interval;
+	} *drrs_work;
 };
 
 struct i915_psr {
@@ -1975,6 +1981,7 @@ struct i915_params {
 	bool prefault_disable;
 	bool reset;
 	int invert_brightness;
+	int drrs_interval;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index c743057..69f8b83 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -47,6 +47,7 @@ struct i915_params i915 __read_mostly = {
 	.prefault_disable = 0,
 	.reset = true,
 	.invert_brightness = 0,
+	.drrs_interval = 2000,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -153,3 +154,10 @@ MODULE_PARM_DESC(invert_brightness,
 	"report PCI device ID, subsystem vendor and subsystem device ID "
 	"to dri-devel@lists.freedesktop.org, if your machine needs it. "
 	"It will then be included in an upcoming module version.");
+
+module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
+MODULE_PARM_DESC(drrs_interval,
+	"DRRS idleness detection interval  (default: 2000 ms)."
+	"If this field is set to 0, then seamless DRRS feature "
+	"based on idleness detection is disabled."
+	"The interval is to be set in milliseconds.");
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d4a0d9..86cd603 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2410,6 +2410,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);
 
@@ -3598,6 +3599,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)
@@ -3639,6 +3641,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);
 }
 
@@ -3845,6 +3848,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);
 }
 
@@ -3892,6 +3896,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);
 }
 
@@ -4176,6 +4181,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_update_cursor(crtc, true);
 
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4221,6 +4227,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	intel_crtc_dpms_overlay(intel_crtc, true);
 
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4286,6 +4293,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_update_watermarks(crtc);
 
 	intel_update_fbc(dev);
+	intel_update_drrs(dev);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)
@@ -8281,6 +8289,10 @@ static void intel_unpin_work_fn(struct work_struct *__work)
 	drm_gem_object_unreference(&work->pending_flip_obj->base);
 	drm_gem_object_unreference(&work->old_fb_obj->base);
 
+	/* disable current DRRS work scheduled and restart
+	 * to push work by another x seconds
+	 */
+	intel_update_drrs(dev);
 	intel_update_fbc(dev);
 	mutex_unlock(&dev->struct_mutex);
 
@@ -8722,6 +8734,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);
 
@@ -11055,6 +11068,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
@@ -11461,6 +11475,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 54cde57..6a91856 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3406,16 +3406,37 @@ intel_dp_connector_destroy(struct drm_connector *connector)
 	kfree(connector);
 }
 
+static void
+intel_dp_drrs_fini(struct drm_i915_private *dev_priv,
+			struct intel_dp *intel_dp)
+{
+	if (intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
+		if (cancel_delayed_work_sync
+			(&dev_priv->drrs.drrs_work->work)) {
+			kfree(dev_priv->drrs.drrs_work);
+			dev_priv->drrs.drrs_work = NULL;
+			dev_priv->drrs.connector = NULL;
+		}
+	}
+}
+
 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);
+
+		if (dev_priv->drrs.connector &&
+			intel_dp == enc_to_intel_dp(
+				&dev_priv->drrs.connector->encoder->base))
+			intel_dp_drrs_fini(dev_priv, intel_dp);
+
 		mutex_lock(&dev->mode_config.mutex);
 		edp_panel_vdd_off_sync(intel_dp);
 		mutex_unlock(&dev->mode_config.mutex);
@@ -3805,7 +3826,7 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			downclock_mode->clock;
 
-		dev_priv->drrs.connector = intel_connector;
+		intel_init_drrs_idleness_detection(dev, intel_connector);
 
 		mutex_init(&intel_dp->drrs_state.mutex);
 
@@ -3813,7 +3834,8 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 
 		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
 		DRM_INFO("seamless DRRS supported for eDP panel.\n");
-	}
+	} else
+		DRM_INFO("DRRS not supported\n");
 
 	return downclock_mode;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4f7c816..7b6dcc0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -911,7 +911,10 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 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);
+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 9ab3883..e210cbc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -618,6 +618,144 @@ 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;
+
+	/* Double check if the dual-display mode is active. */
+	if (dev_priv->drrs.is_clone)
+		return;
+
+	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;
+
+	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;
+	struct intel_dp *intel_dp = NULL;
+
+	intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
+
+	if (intel_dp == NULL)
+		return;
+
+	intel_cancel_drrs_work(dev_priv);
+
+	if (intel_dp->drrs_state.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;
+	struct intel_dp *intel_dp = NULL;
+
+	if (dev_priv->drrs.connector == NULL)
+		return;
+
+	intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
+
+	if (intel_dp == NULL)
+		return;
+
+	/* as part of disable DRRS, reset refresh rate to HIGH_RR */
+	if (intel_dp->drrs_state.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
+*/
+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)
+		return;
+
+	if (dev_priv->drrs.connector->panel.downclock_mode == NULL)
+		return;
+
+	list_for_each_entry(tmp_crtc, &dev->mode_config.crtc_list, head) {
+		if (tmp_crtc != NULL && intel_crtc_active(tmp_crtc)) {
+			if (crtc) {
+				DRM_DEBUG_KMS(
+				"more than one pipe active, disabling DRRS\n");
+				dev_priv->drrs.is_clone = true;
+				intel_disable_drrs(dev);
+				return;
+			}
+			crtc = tmp_crtc;
+		}
+	}
+
+	if (crtc == NULL) {
+		DRM_DEBUG_KMS("DRRS: crtc not initialized\n");
+		return;
+	}
+
+	dev_priv->drrs.is_clone = false;
+	intel_disable_drrs(dev);
+
+	/* re-enable idleness detection */
+	intel_enable_drrs(crtc);
+}
+
+void intel_init_drrs_idleness_detection(struct drm_device *dev,
+					struct intel_connector *connector)
+{
+	struct intel_drrs_work *work;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (i915.drrs_interval == 0) {
+		DRM_INFO("DRRS disable by flag\n");
+		return;
+	}
+
+	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.is_clone = false;
+
+	work->interval = i915.drrs_interval;
+	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;
-- 
1.7.9.5

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

[Jani Nikula]

On Fri, 07 Mar 2014, 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
>
> v3: Internal review comments incorporated
> Add NULL pointer check in intel_disable_drrs.
> Add drrs calls in i9xx_crtc_enable/disable and valleyview_crtc_enable.
>
> v4: Jani's review comments incorporated.
> Change in sequence in intel_update_drrs. Comment modified to remove details
> of update param. Modified DRRS idleness interval to a module parameter.
>
> v5: Chris's review comments incorporated.
> Initialize connector in idleness detection init. Modifications made to
> use only intel_connector in i915_drrs and derive intel_dp when required.
> Added a function drrs_fini to cleanup DRRS work.
>
> v6: Internal review comments. Removed check for primary enabled, which is
> a redundant check, in the case of clone mode. Added a flag to track
> dual-display configuration. Remove print statement for "cancel DRR work"
> and print "DRRS not supported" only once.
>
> v7: As per internal review comments, removing calls to update/disable drrs
> from sprite update path. For sprite, all drrs related updates would be
> taken care of with calls to crtc page flip itself. This will have to be
> revisited later if flip infrastructure changes for sprite.
>
> 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      |    7 ++
>  drivers/gpu/drm/i915/i915_params.c   |    8 ++
>  drivers/gpu/drm/i915/intel_display.c |   16 ++++
>  drivers/gpu/drm/i915/intel_dp.c      |   26 ++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |    5 +-
>  drivers/gpu/drm/i915/intel_pm.c      |  138 ++++++++++++++++++++++++++++++++++
>  6 files changed, 197 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 43d3dfe..87865e9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -776,6 +776,12 @@ struct i915_fbc {
>  
>  struct i915_drrs {
>  	struct intel_connector *connector;
> +	bool is_clone;
> +	struct intel_drrs_work {
> +		struct delayed_work work;
> +		struct drm_crtc *crtc;
> +		int interval;
> +	} *drrs_work;
>  };
>  
>  struct i915_psr {
> @@ -1975,6 +1981,7 @@ struct i915_params {
>  	bool prefault_disable;
>  	bool reset;
>  	int invert_brightness;
> +	int drrs_interval;
>  };
>  extern struct i915_params i915 __read_mostly;
>  
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index c743057..69f8b83 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -47,6 +47,7 @@ struct i915_params i915 __read_mostly = {
>  	.prefault_disable = 0,
>  	.reset = true,
>  	.invert_brightness = 0,
> +	.drrs_interval = 2000,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -153,3 +154,10 @@ MODULE_PARM_DESC(invert_brightness,
>  	"report PCI device ID, subsystem vendor and subsystem device ID "
>  	"to dri-devel@lists.freedesktop.org, if your machine needs it. "
>  	"It will then be included in an upcoming module version.");
> +
> +module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
> +MODULE_PARM_DESC(drrs_interval,
> +	"DRRS idleness detection interval  (default: 2000 ms)."
> +	"If this field is set to 0, then seamless DRRS feature "
> +	"based on idleness detection is disabled."
> +	"The interval is to be set in milliseconds.");

When the strings are concatenated there won't be a space after the
periods.

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d4a0d9..86cd603 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2410,6 +2410,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);
>  
> @@ -3598,6 +3599,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)
> @@ -3639,6 +3641,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);
>  }
>  
> @@ -3845,6 +3848,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);
>  }
>  
> @@ -3892,6 +3896,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);
>  }
>  
> @@ -4176,6 +4181,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_update_cursor(crtc, true);
>  
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4221,6 +4227,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	intel_crtc_dpms_overlay(intel_crtc, true);
>  
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4286,6 +4293,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	intel_update_watermarks(crtc);
>  
>  	intel_update_fbc(dev);
> +	intel_update_drrs(dev);
>  }
>  
>  static void i9xx_crtc_off(struct drm_crtc *crtc)
> @@ -8281,6 +8289,10 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>  	drm_gem_object_unreference(&work->pending_flip_obj->base);
>  	drm_gem_object_unreference(&work->old_fb_obj->base);
>  
> +	/* disable current DRRS work scheduled and restart
> +	 * to push work by another x seconds
> +	 */
> +	intel_update_drrs(dev);
>  	intel_update_fbc(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> @@ -8722,6 +8734,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);
>  
> @@ -11055,6 +11068,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
> @@ -11461,6 +11475,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 54cde57..6a91856 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3406,16 +3406,37 @@ intel_dp_connector_destroy(struct drm_connector *connector)
>  	kfree(connector);
>  }
>  
> +static void
> +intel_dp_drrs_fini(struct drm_i915_private *dev_priv,
> +			struct intel_dp *intel_dp)
> +{
> +	if (intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {

It's not obvious to me why this gets only done if
SEAMLESS_DRRS_SUPPORT. The scheduling of the work is not indirectly
dependent on this.

> +		if (cancel_delayed_work_sync
> +			(&dev_priv->drrs.drrs_work->work)) {
> +			kfree(dev_priv->drrs.drrs_work);
> +			dev_priv->drrs.drrs_work = NULL;
> +			dev_priv->drrs.connector = NULL;
> +		}
> +	}
> +}
> +
>  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);
> +
> +		if (dev_priv->drrs.connector &&
> +			intel_dp == enc_to_intel_dp(
> +				&dev_priv->drrs.connector->encoder->base))
> +			intel_dp_drrs_fini(dev_priv, intel_dp);
> +
>  		mutex_lock(&dev->mode_config.mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.mutex);
> @@ -3805,7 +3826,7 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  		intel_connector->panel.edp_downclock =
>  			downclock_mode->clock;
>  
> -		dev_priv->drrs.connector = intel_connector;
> +		intel_init_drrs_idleness_detection(dev, intel_connector);
>  
>  		mutex_init(&intel_dp->drrs_state.mutex);
>  
> @@ -3813,7 +3834,8 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  
>  		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
>  		DRM_INFO("seamless DRRS supported for eDP panel.\n");
> -	}
> +	} else
> +		DRM_INFO("DRRS not supported\n");
>  
>  	return downclock_mode;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4f7c816..7b6dcc0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -911,7 +911,10 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>  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);
> +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 9ab3883..e210cbc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -618,6 +618,144 @@ 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;
> +
> +	/* Double check if the dual-display mode is active. */
> +	if (dev_priv->drrs.is_clone)
> +		return;
> +
> +	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;
> +
> +	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;
> +	struct intel_dp *intel_dp = NULL;
> +
> +	intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
> +
> +	if (intel_dp == NULL)
> +		return;
> +
> +	intel_cancel_drrs_work(dev_priv);
> +
> +	if (intel_dp->drrs_state.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;
> +	struct intel_dp *intel_dp = NULL;
> +
> +	if (dev_priv->drrs.connector == NULL)
> +		return;
> +
> +	intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
> +
> +	if (intel_dp == NULL)
> +		return;
> +
> +	/* as part of disable DRRS, reset refresh rate to HIGH_RR */
> +	if (intel_dp->drrs_state.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
> +*/
> +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)
> +		return;
> +
> +	if (dev_priv->drrs.connector->panel.downclock_mode == NULL)
> +		return;
> +
> +	list_for_each_entry(tmp_crtc, &dev->mode_config.crtc_list, head) {
> +		if (tmp_crtc != NULL && intel_crtc_active(tmp_crtc)) {
> +			if (crtc) {
> +				DRM_DEBUG_KMS(
> +				"more than one pipe active, disabling DRRS\n");
> +				dev_priv->drrs.is_clone = true;
> +				intel_disable_drrs(dev);
> +				return;
> +			}
> +			crtc = tmp_crtc;
> +		}
> +	}
> +
> +	if (crtc == NULL) {
> +		DRM_DEBUG_KMS("DRRS: crtc not initialized\n");
> +		return;
> +	}
> +
> +	dev_priv->drrs.is_clone = false;
> +	intel_disable_drrs(dev);
> +
> +	/* re-enable idleness detection */
> +	intel_enable_drrs(crtc);
> +}
> +
> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
> +					struct intel_connector *connector)
> +{
> +	struct intel_drrs_work *work;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (i915.drrs_interval == 0) {
> +		DRM_INFO("DRRS disable by flag\n");
> +		return;
> +	}
> +
> +	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.is_clone = false;
> +
> +	work->interval = i915.drrs_interval;
> +	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;
> -- 
> 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/6] drm/i915: Idleness detection for DRRS

[Vandana Kannan]

On Mar-26-2014 6:35 PM, Jani Nikula wrote:
> On Fri, 07 Mar 2014, 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
>>
>> v3: Internal review comments incorporated
>> Add NULL pointer check in intel_disable_drrs.
>> Add drrs calls in i9xx_crtc_enable/disable and valleyview_crtc_enable.
>>
>> v4: Jani's review comments incorporated.
>> Change in sequence in intel_update_drrs. Comment modified to remove details
>> of update param. Modified DRRS idleness interval to a module parameter.
>>
>> v5: Chris's review comments incorporated.
>> Initialize connector in idleness detection init. Modifications made to
>> use only intel_connector in i915_drrs and derive intel_dp when required.
>> Added a function drrs_fini to cleanup DRRS work.
>>
>> v6: Internal review comments. Removed check for primary enabled, which is
>> a redundant check, in the case of clone mode. Added a flag to track
>> dual-display configuration. Remove print statement for "cancel DRR work"
>> and print "DRRS not supported" only once.
>>
>> v7: As per internal review comments, removing calls to update/disable drrs
>> from sprite update path. For sprite, all drrs related updates would be
>> taken care of with calls to crtc page flip itself. This will have to be
>> revisited later if flip infrastructure changes for sprite.
>>
>> 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      |    7 ++
>>  drivers/gpu/drm/i915/i915_params.c   |    8 ++
>>  drivers/gpu/drm/i915/intel_display.c |   16 ++++
>>  drivers/gpu/drm/i915/intel_dp.c      |   26 ++++++-
>>  drivers/gpu/drm/i915/intel_drv.h     |    5 +-
>>  drivers/gpu/drm/i915/intel_pm.c      |  138 ++++++++++++++++++++++++++++++++++
>>  6 files changed, 197 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 43d3dfe..87865e9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -776,6 +776,12 @@ struct i915_fbc {
>>  
>>  struct i915_drrs {
>>  	struct intel_connector *connector;
>> +	bool is_clone;
>> +	struct intel_drrs_work {
>> +		struct delayed_work work;
>> +		struct drm_crtc *crtc;
>> +		int interval;
>> +	} *drrs_work;
>>  };
>>  
>>  struct i915_psr {
>> @@ -1975,6 +1981,7 @@ struct i915_params {
>>  	bool prefault_disable;
>>  	bool reset;
>>  	int invert_brightness;
>> +	int drrs_interval;
>>  };
>>  extern struct i915_params i915 __read_mostly;
>>  
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index c743057..69f8b83 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -47,6 +47,7 @@ struct i915_params i915 __read_mostly = {
>>  	.prefault_disable = 0,
>>  	.reset = true,
>>  	.invert_brightness = 0,
>> +	.drrs_interval = 2000,
>>  };
>>  
>>  module_param_named(modeset, i915.modeset, int, 0400);
>> @@ -153,3 +154,10 @@ MODULE_PARM_DESC(invert_brightness,
>>  	"report PCI device ID, subsystem vendor and subsystem device ID "
>>  	"to dri-devel@lists.freedesktop.org, if your machine needs it. "
>>  	"It will then be included in an upcoming module version.");
>> +
>> +module_param_named(drrs_interval, i915.drrs_interval, int, 0600);
>> +MODULE_PARM_DESC(drrs_interval,
>> +	"DRRS idleness detection interval  (default: 2000 ms)."
>> +	"If this field is set to 0, then seamless DRRS feature "
>> +	"based on idleness detection is disabled."
>> +	"The interval is to be set in milliseconds.");
> 
> When the strings are concatenated there won't be a space after the
> periods.
> 
Ok.. Will make appropriate changes..
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 4d4a0d9..86cd603 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2410,6 +2410,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);
>>  
>> @@ -3598,6 +3599,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)
>> @@ -3639,6 +3641,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);
>>  }
>>  
>> @@ -3845,6 +3848,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);
>>  }
>>  
>> @@ -3892,6 +3896,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);
>>  }
>>  
>> @@ -4176,6 +4181,7 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>>  	intel_crtc_update_cursor(crtc, true);
>>  
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  
>>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>>  		encoder->enable(encoder);
>> @@ -4221,6 +4227,7 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>>  	intel_crtc_dpms_overlay(intel_crtc, true);
>>  
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  
>>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>>  		encoder->enable(encoder);
>> @@ -4286,6 +4293,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>>  	intel_update_watermarks(crtc);
>>  
>>  	intel_update_fbc(dev);
>> +	intel_update_drrs(dev);
>>  }
>>  
>>  static void i9xx_crtc_off(struct drm_crtc *crtc)
>> @@ -8281,6 +8289,10 @@ static void intel_unpin_work_fn(struct work_struct *__work)
>>  	drm_gem_object_unreference(&work->pending_flip_obj->base);
>>  	drm_gem_object_unreference(&work->old_fb_obj->base);
>>  
>> +	/* disable current DRRS work scheduled and restart
>> +	 * to push work by another x seconds
>> +	 */
>> +	intel_update_drrs(dev);
>>  	intel_update_fbc(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>> @@ -8722,6 +8734,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);
>>  
>> @@ -11055,6 +11068,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
>> @@ -11461,6 +11475,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 54cde57..6a91856 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3406,16 +3406,37 @@ intel_dp_connector_destroy(struct drm_connector *connector)
>>  	kfree(connector);
>>  }
>>  
>> +static void
>> +intel_dp_drrs_fini(struct drm_i915_private *dev_priv,
>> +			struct intel_dp *intel_dp)
>> +{
>> +	if (intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
> 
> It's not obvious to me why this gets only done if
> SEAMLESS_DRRS_SUPPORT. The scheduling of the work is not indirectly
> dependent on this.
> 
This check was added to confine all idleness detection related
infrastructure to seamless DRRS. This check may be redundant here - I
will make appropriate changes..
>> +		if (cancel_delayed_work_sync
>> +			(&dev_priv->drrs.drrs_work->work)) {
>> +			kfree(dev_priv->drrs.drrs_work);
>> +			dev_priv->drrs.drrs_work = NULL;
>> +			dev_priv->drrs.connector = NULL;
>> +		}
>> +	}
>> +}
>> +
>>  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);
>> +
>> +		if (dev_priv->drrs.connector &&
>> +			intel_dp == enc_to_intel_dp(
>> +				&dev_priv->drrs.connector->encoder->base))
>> +			intel_dp_drrs_fini(dev_priv, intel_dp);
>> +
>>  		mutex_lock(&dev->mode_config.mutex);
>>  		edp_panel_vdd_off_sync(intel_dp);
>>  		mutex_unlock(&dev->mode_config.mutex);
>> @@ -3805,7 +3826,7 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>  		intel_connector->panel.edp_downclock =
>>  			downclock_mode->clock;
>>  
>> -		dev_priv->drrs.connector = intel_connector;
>> +		intel_init_drrs_idleness_detection(dev, intel_connector);
>>  
>>  		mutex_init(&intel_dp->drrs_state.mutex);
>>  
>> @@ -3813,7 +3834,8 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>  
>>  		intel_dp->drrs_state.refresh_rate_type = DRRS_HIGH_RR;
>>  		DRM_INFO("seamless DRRS supported for eDP panel.\n");
>> -	}
>> +	} else
>> +		DRM_INFO("DRRS not supported\n");
>>  
>>  	return downclock_mode;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 4f7c816..7b6dcc0 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -911,7 +911,10 @@ void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>  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);
>> +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 9ab3883..e210cbc 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -618,6 +618,144 @@ 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;
>> +
>> +	/* Double check if the dual-display mode is active. */
>> +	if (dev_priv->drrs.is_clone)
>> +		return;
>> +
>> +	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;
>> +
>> +	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;
>> +	struct intel_dp *intel_dp = NULL;
>> +
>> +	intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
>> +
>> +	if (intel_dp == NULL)
>> +		return;
>> +
>> +	intel_cancel_drrs_work(dev_priv);
>> +
>> +	if (intel_dp->drrs_state.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;
>> +	struct intel_dp *intel_dp = NULL;
>> +
>> +	if (dev_priv->drrs.connector == NULL)
>> +		return;
>> +
>> +	intel_dp = enc_to_intel_dp(&dev_priv->drrs.connector->encoder->base);
>> +
>> +	if (intel_dp == NULL)
>> +		return;
>> +
>> +	/* as part of disable DRRS, reset refresh rate to HIGH_RR */
>> +	if (intel_dp->drrs_state.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
>> +*/
>> +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)
>> +		return;
>> +
>> +	if (dev_priv->drrs.connector->panel.downclock_mode == NULL)
>> +		return;
>> +
>> +	list_for_each_entry(tmp_crtc, &dev->mode_config.crtc_list, head) {
>> +		if (tmp_crtc != NULL && intel_crtc_active(tmp_crtc)) {
>> +			if (crtc) {
>> +				DRM_DEBUG_KMS(
>> +				"more than one pipe active, disabling DRRS\n");
>> +				dev_priv->drrs.is_clone = true;
>> +				intel_disable_drrs(dev);
>> +				return;
>> +			}
>> +			crtc = tmp_crtc;
>> +		}
>> +	}
>> +
>> +	if (crtc == NULL) {
>> +		DRM_DEBUG_KMS("DRRS: crtc not initialized\n");
>> +		return;
>> +	}
>> +
>> +	dev_priv->drrs.is_clone = false;
>> +	intel_disable_drrs(dev);
>> +
>> +	/* re-enable idleness detection */
>> +	intel_enable_drrs(crtc);
>> +}
>> +
>> +void intel_init_drrs_idleness_detection(struct drm_device *dev,
>> +					struct intel_connector *connector)
>> +{
>> +	struct intel_drrs_work *work;
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +	if (i915.drrs_interval == 0) {
>> +		DRM_INFO("DRRS disable by flag\n");
>> +		return;
>> +	}
>> +
>> +	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.is_clone = false;
>> +
>> +	work->interval = i915.drrs_interval;
>> +	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;
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

v3: Incorporated Jani's review comments
Re-use cpu_transcoder_set_m_n for BDW.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 86cd603..64ed4e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4901,7 +4901,7 @@ static void intel_pch_transcoder_set_m_n(struct intel_crtc *crtc,
 	I915_WRITE(PCH_TRANS_LINK_N1(pipe), m_n->link_n);
 }
 
-static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
+void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
 					 struct intel_link_m_n *m_n)
 {
 	struct drm_device *dev = crtc->base.dev;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6a91856..a76a58c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -839,11 +839,15 @@ 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) {
+		intel_cpu_transcoder_set_m_n(crtc, m_n);
+	} else if (INTEL_INFO(dev)->gen > 6) {
+		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
@@ -3763,7 +3767,16 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		return;
 	}
 
-	if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
+	if (INTEL_INFO(dev)->gen >= 8) {
+		switch (index) {
+		case DRRS_HIGH_RR:
+			intel_dp_set_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 > 6) {
 		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
 		val = I915_READ(reg);
 		if (index > DRRS_HIGH_RR) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b6dcc0..3d280b0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -751,6 +751,8 @@ void hsw_enable_ips(struct intel_crtc *crtc);
 void hsw_disable_ips(struct intel_crtc *crtc);
 void intel_display_set_init_power(struct drm_device *dev, bool enable);
 int valleyview_get_vco(struct drm_i915_private *dev_priv);
+void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
+				struct intel_link_m_n *m_n);
 
 /* intel_dp.c */
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
-- 
1.7.9.5

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

[Jani Nikula]

The last two patches seem okay, but let's get back to them once the
beginning of the series is rebased.

Thanks,
Jani.

On Fri, 07 Mar 2014, 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
>
> v3: Incorporated Jani's review comments
> Re-use cpu_transcoder_set_m_n for BDW.
>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  drivers/gpu/drm/i915/intel_dp.c      |   25 +++++++++++++++++++------
>  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
>  3 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 86cd603..64ed4e3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4901,7 +4901,7 @@ static void intel_pch_transcoder_set_m_n(struct intel_crtc *crtc,
>  	I915_WRITE(PCH_TRANS_LINK_N1(pipe), m_n->link_n);
>  }
>  
> -static void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
> +void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
>  					 struct intel_link_m_n *m_n)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6a91856..a76a58c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -839,11 +839,15 @@ 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) {
> +		intel_cpu_transcoder_set_m_n(crtc, m_n);
> +	} else if (INTEL_INFO(dev)->gen > 6) {
> +		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
> @@ -3763,7 +3767,16 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>  		return;
>  	}
>  
> -	if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
> +	if (INTEL_INFO(dev)->gen >= 8) {
> +		switch (index) {
> +		case DRRS_HIGH_RR:
> +			intel_dp_set_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 > 6) {
>  		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
>  		val = I915_READ(reg);
>  		if (index > DRRS_HIGH_RR) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7b6dcc0..3d280b0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -751,6 +751,8 @@ void hsw_enable_ips(struct intel_crtc *crtc);
>  void hsw_disable_ips(struct intel_crtc *crtc);
>  void intel_display_set_init_power(struct drm_device *dev, bool enable);
>  int valleyview_get_vco(struct drm_i915_private *dev_priv);
> +void intel_cpu_transcoder_set_m_n(struct intel_crtc *crtc,
> +				struct intel_link_m_n *m_n);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port 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

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

[Vandana Kannan]

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bfd7703..577654b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3227,6 +3227,7 @@
 #define   PIPECONF_INTERLACE_MODE_MASK		(7 << 21)
 #define   PIPECONF_EDP_RR_MODE_SWITCH		(1 << 20)
 #define   PIPECONF_CXSR_DOWNCLOCK	(1<<16)
+#define   PIPECONF_EDP_RR_MODE_SWITCH_VLV	(1 << 14)
 #define   PIPECONF_COLOR_RANGE_SELECT	(1 << 13)
 #define   PIPECONF_BPC_MASK	(0x7 << 5)
 #define   PIPECONF_8BPC		(0<<5)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a76a58c..3616d0e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3780,10 +3780,16 @@ void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
 		reg = PIPECONF(intel_crtc->config.cpu_transcoder);
 		val = I915_READ(reg);
 		if (index > DRRS_HIGH_RR) {
-			val |= PIPECONF_EDP_RR_MODE_SWITCH;
+			if (IS_VALLEYVIEW(dev))
+				val |= PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val |= PIPECONF_EDP_RR_MODE_SWITCH;
 			intel_dp_set_m2_n2(intel_crtc, &config->dp_m2_n2);
 		} else {
-			val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
+			if (IS_VALLEYVIEW(dev))
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH_VLV;
+			else
+				val &= ~PIPECONF_EDP_RR_MODE_SWITCH;
 		}
 		I915_WRITE(reg, val);
 	}
-- 
1.7.9.5

[PATCH 0/6] v7: Enabling DRRS in the kernel

[Vandana Kannan]

Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which     
enables 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. DRRS idleness time has been modified to be a kernel module param.

v6: Added support for VLV.

v7: Patches rebased, review comments incorporated

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 (3):
  drm/i915: Idleness detection for DRRS
  drm/i915/bdw: Add support for DRRS to switch RR
  drm/i915: Support for RR switching on VLV

 drivers/gpu/drm/i915/i915_drv.h      |   24 +++++
 drivers/gpu/drm/i915/i915_params.c   |    8 ++
 drivers/gpu/drm/i915/i915_reg.h      |    2 +
 drivers/gpu/drm/i915/intel_bios.c    |   34 +++++-
 drivers/gpu/drm/i915/intel_bios.h    |   29 +++++
 drivers/gpu/drm/i915/intel_display.c |   18 +++-
 drivers/gpu/drm/i915/intel_dp.c      |  198 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   29 ++++-
 drivers/gpu/drm/i915/intel_pm.c      |  138 ++++++++++++++++++++++++
 9 files changed, 475 insertions(+), 5 deletions(-)

-- 
1.7.9.5

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

v6: Modifying i915_drrs to include only intel connector as intel_dp can be
derived from intel connector when required.

v7: As per internal review comments, acquiring mutex just before accessing
drrs RR. As per Chris's review comments, added documentation about the use
of locking in the function.

v8: Incorporated Jani's review comments.
Removed reference to edp_downclock.

Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |    5 ++
 drivers/gpu/drm/i915/i915_reg.h  |    1 +
 drivers/gpu/drm/i915/intel_dp.c  |  108 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    6 ++-
 4 files changed, 119 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cd73a33..1c9d5cf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -794,6 +794,10 @@ struct i915_fbc {
 	} no_fbc_reason;
 };
 
+struct i915_drrs {
+	struct intel_connector *connector;
+};
+
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
@@ -1497,6 +1501,7 @@ typedef struct drm_i915_private {
 	struct timer_list hotplug_reenable_timer;
 
 	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 74f7d85..04fc64a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3255,6 +3255,7 @@ enum punit_power_well {
 #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 f2735de..9640df1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -736,6 +736,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)
@@ -840,6 +854,15 @@ found:
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n);
 
+	if (intel_connector->panel.downclock_mode != NULL &&
+		intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
+			intel_link_compute_m_n(bpp, lane_count,
+				intel_connector->panel.downclock_mode->clock,
+				pipe_config->port_clock,
+				&pipe_config->dp_m2_n2);
+	}
+
+
 	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
 
 	return true;
@@ -3611,6 +3634,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 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 = 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_connector == NULL) {
+		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
+		return;
+	}
+
+	encoder = intel_attached_encoder(&intel_connector->base);
+	intel_dp = enc_to_intel_dp(&encoder->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 disabled. CRTC not Active\n");
+		return;
+	}
+
+	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);
+	}
+
+	/*
+	 * mutex taken to ensure that there is no race between differnt
+	 * drrs calls trying to update refresh rate. This scenario may occur
+	 * in future when idleness detection based DRRS in kernel and
+	 * possible calls from user space to set differnt RR are made.
+	 */
+
+	mutex_lock(&intel_dp->drrs_state.mutex);
+
+	intel_dp->drrs_state.refresh_rate_type = index;
+
+	mutex_unlock(&intel_dp->drrs_state.mutex);
+
+	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
+}
+
 static struct drm_display_mode *
 intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3647,6 +3751,10 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 
 	if (downclock_mode != NULL &&
 		dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
+		dev_priv->drrs.connector = intel_connector;
+
+		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 d781165..46fc8b3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -305,6 +305,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
@@ -535,6 +538,7 @@ struct intel_dp {
 	struct {
 		enum drrs_support_type type;
 		enum edp_drrs_refresh_rate_type refresh_rate_type;
+		struct mutex mutex;
 	} drrs_state;
 
 };
@@ -789,7 +793,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

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

[Jani Nikula]

On Fri, 28 Mar 2014, 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.
>
> 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.
>
> v6: Modifying i915_drrs to include only intel connector as intel_dp can be
> derived from intel connector when required.
>
> v7: As per internal review comments, acquiring mutex just before accessing
> drrs RR. As per Chris's review comments, added documentation about the use
> of locking in the function.
>
> v8: Incorporated Jani's review comments.
> Removed reference to edp_downclock.
>
> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    5 ++
>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>  drivers/gpu/drm/i915/intel_dp.c  |  108 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>  4 files changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cd73a33..1c9d5cf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -794,6 +794,10 @@ struct i915_fbc {
>  	} no_fbc_reason;
>  };
>  
> +struct i915_drrs {
> +	struct intel_connector *connector;
> +};
> +
>  struct i915_psr {
>  	bool sink_support;
>  	bool source_ok;
> @@ -1497,6 +1501,7 @@ typedef struct drm_i915_private {
>  	struct timer_list hotplug_reenable_timer;
>  
>  	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 74f7d85..04fc64a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3255,6 +3255,7 @@ enum punit_power_well {
>  #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 f2735de..9640df1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -736,6 +736,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)
> @@ -840,6 +854,15 @@ found:
>  			       pipe_config->port_clock,
>  			       &pipe_config->dp_m_n);
>  
> +	if (intel_connector->panel.downclock_mode != NULL &&
> +		intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
> +			intel_link_compute_m_n(bpp, lane_count,
> +				intel_connector->panel.downclock_mode->clock,
> +				pipe_config->port_clock,
> +				&pipe_config->dp_m2_n2);
> +	}
> +
> +
>  	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>  
>  	return true;
> @@ -3611,6 +3634,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 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 = dev_priv->drrs.connector;
> +	u32 reg, val;
> +	int index = 0;

enum edp_drrs_refresh_rate.

> +
> +	if (refresh_rate <= 0) {
> +		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
> +		return;
> +	}
> +
> +	if (intel_connector == NULL) {
> +		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
> +		return;
> +	}
> +
> +	encoder = intel_attached_encoder(&intel_connector->base);
> +	intel_dp = enc_to_intel_dp(&encoder->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");

Isn't seamless the only thing that *is* supported?

> +		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 disabled. CRTC not Active\n");
> +		return;
> +	}
> +
> +	if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {

Can we end up here when drrs is not supported by the platform?

> +		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);
> +	}
> +
> +	/*
> +	 * mutex taken to ensure that there is no race between differnt
> +	 * drrs calls trying to update refresh rate. This scenario may occur
> +	 * in future when idleness detection based DRRS in kernel and
> +	 * possible calls from user space to set differnt RR are made.
> +	 */
> +
> +	mutex_lock(&intel_dp->drrs_state.mutex);
> +
> +	intel_dp->drrs_state.refresh_rate_type = index;
> +
> +	mutex_unlock(&intel_dp->drrs_state.mutex);
> +
> +	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
> +}
> +
>  static struct drm_display_mode *
>  intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  			struct intel_connector *intel_connector,
> @@ -3647,6 +3751,10 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  
>  	if (downclock_mode != NULL &&
>  		dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
> +		dev_priv->drrs.connector = intel_connector;
> +
> +		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 d781165..46fc8b3 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -305,6 +305,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
> @@ -535,6 +538,7 @@ struct intel_dp {
>  	struct {
>  		enum drrs_support_type type;
>  		enum edp_drrs_refresh_rate_type refresh_rate_type;
> +		struct mutex mutex;
>  	} drrs_state;
>  
>  };
> @@ -789,7 +793,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
>

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Vandana Kannan]

On Apr-01-2014 6:55 PM, Jani Nikula wrote:
> On Fri, 28 Mar 2014, 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.
>>
>> 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.
>>
>> v6: Modifying i915_drrs to include only intel connector as intel_dp can be
>> derived from intel connector when required.
>>
>> v7: As per internal review comments, acquiring mutex just before accessing
>> drrs RR. As per Chris's review comments, added documentation about the use
>> of locking in the function.
>>
>> v8: Incorporated Jani's review comments.
>> Removed reference to edp_downclock.
>>
>> Signed-off-by: Pradeep Bhat <pradeep.bhat@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |    5 ++
>>  drivers/gpu/drm/i915/i915_reg.h  |    1 +
>>  drivers/gpu/drm/i915/intel_dp.c  |  108 ++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |    6 ++-
>>  4 files changed, 119 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index cd73a33..1c9d5cf 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -794,6 +794,10 @@ struct i915_fbc {
>>  	} no_fbc_reason;
>>  };
>>  
>> +struct i915_drrs {
>> +	struct intel_connector *connector;
>> +};
>> +
>>  struct i915_psr {
>>  	bool sink_support;
>>  	bool source_ok;
>> @@ -1497,6 +1501,7 @@ typedef struct drm_i915_private {
>>  	struct timer_list hotplug_reenable_timer;
>>  
>>  	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 74f7d85..04fc64a 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3255,6 +3255,7 @@ enum punit_power_well {
>>  #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 f2735de..9640df1 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -736,6 +736,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)
>> @@ -840,6 +854,15 @@ found:
>>  			       pipe_config->port_clock,
>>  			       &pipe_config->dp_m_n);
>>  
>> +	if (intel_connector->panel.downclock_mode != NULL &&
>> +		intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
>> +			intel_link_compute_m_n(bpp, lane_count,
>> +				intel_connector->panel.downclock_mode->clock,
>> +				pipe_config->port_clock,
>> +				&pipe_config->dp_m2_n2);
>> +	}
>> +
>> +
>>  	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>>  
>>  	return true;
>> @@ -3611,6 +3634,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 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 = dev_priv->drrs.connector;
>> +	u32 reg, val;
>> +	int index = 0;
> 
> enum edp_drrs_refresh_rate.
> 
Will change this accordingly..
>> +
>> +	if (refresh_rate <= 0) {
>> +		DRM_DEBUG_KMS("Refresh rate should be positive non-zero.\n");
>> +		return;
>> +	}
>> +
>> +	if (intel_connector == NULL) {
>> +		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
>> +		return;
>> +	}
>> +
>> +	encoder = intel_attached_encoder(&intel_connector->base);
>> +	intel_dp = enc_to_intel_dp(&encoder->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");
> 
> Isn't seamless the only thing that *is* supported?
> 
Yes, will correct this..
>> +		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 disabled. CRTC not Active\n");
>> +		return;
>> +	}
>> +
>> +	if (INTEL_INFO(dev)->gen > 6 && INTEL_INFO(dev)->gen < 8) {
> 
> Can we end up here when drrs is not supported by the platform?
> 
When DRRS is not supported by the platform, drrs.connector would remain
NULL. So, the check for connector above should handle the case and avoid
coming up to this point.
For the PSR related changes (enable/disable DRRS according to status of
PSR) that would be made as part of Patch2, I think a check would be
required here to make sure any call to this function not coming through
update_drrs/disable_drrs returns without setting the registers.
>> +		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);
>> +	}
>> +
>> +	/*
>> +	 * mutex taken to ensure that there is no race between differnt
>> +	 * drrs calls trying to update refresh rate. This scenario may occur
>> +	 * in future when idleness detection based DRRS in kernel and
>> +	 * possible calls from user space to set differnt RR are made.
>> +	 */
>> +
>> +	mutex_lock(&intel_dp->drrs_state.mutex);
>> +
>> +	intel_dp->drrs_state.refresh_rate_type = index;
>> +
>> +	mutex_unlock(&intel_dp->drrs_state.mutex);
>> +
>> +	DRM_DEBUG_KMS("eDP Refresh Rate set to : %dHz\n", refresh_rate);
>> +}
>> +
>>  static struct drm_display_mode *
>>  intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>  			struct intel_connector *intel_connector,
>> @@ -3647,6 +3751,10 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>>  
>>  	if (downclock_mode != NULL &&
>>  		dev_priv->vbt.drrs_type == SEAMLESS_DRRS_SUPPORT) {
>> +		dev_priv->drrs.connector = intel_connector;
>> +
>> +		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 d781165..46fc8b3 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -305,6 +305,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
>> @@ -535,6 +538,7 @@ struct intel_dp {
>>  	struct {
>>  		enum drrs_support_type type;
>>  		enum edp_drrs_refresh_rate_type refresh_rate_type;
>> +		struct mutex mutex;
>>  	} drrs_state;
>>  
>>  };
>> @@ -789,7 +793,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 0/6] v6: Enabling DRRS in the kernel

[Vandana Kannan]

Dynamic Refresh Rate Switching (DRRS) is a power conservation feature which     
enables 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. DRRS idleness time has been modified to be a kernel module param.

Added support for VLV.

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

Vandana Kannan (3):
  [Intel-gfx] drm/i915: Idleness detection for DRRS
  [Intel-gfx] drm/i915/bdw: Add support for DRRS to switch RR
  [Intel-gfx] drm/i915: Support for RR switching on VLV

 drivers/gpu/drm/i915/i915_drv.h      |   25 +++++
 drivers/gpu/drm/i915/i915_params.c   |    8 ++
 drivers/gpu/drm/i915/i915_reg.h      |    2 +
 drivers/gpu/drm/i915/intel_bios.c    |   29 +++++
 drivers/gpu/drm/i915/intel_bios.h    |   29 +++++
 drivers/gpu/drm/i915/intel_display.c |   18 +++-
 drivers/gpu/drm/i915/intel_dp.c      |  196 +++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h     |   33 +++++-
 drivers/gpu/drm/i915/intel_pm.c      |  138 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sprite.c  |    2 +
 10 files changed, 476 insertions(+), 4 deletions(-)

-- 
1.7.9.5

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

v6: Modifying i915_drrs to include only intel connector as intel_dp can be
derived from intel connector when required.

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  |    5 ++
 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, 112 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6b4d0b20..43d3dfe 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -774,6 +774,10 @@ struct i915_fbc {
 	} no_fbc_reason;
 };
 
+struct i915_drrs {
+	struct intel_connector *connector;
+};
+
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
@@ -1466,6 +1470,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 39365bf..fd22914 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,80 @@ 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 = NULL;
+	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_connector == NULL) {
+		DRM_DEBUG_KMS("DRRS supported for eDP only.\n");
+		return;
+	}
+
+	encoder = intel_attached_encoder(&intel_connector->base);
+	intel_dp = enc_to_intel_dp(&encoder->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 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_init(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector,
@@ -3701,6 +3798,10 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
 		intel_connector->panel.edp_downclock =
 			downclock_mode->clock;
 
+		dev_priv->drrs.connector = intel_connector;
+
+		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

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

[Chris Wilson]

On Wed, Mar 05, 2014 at 03:13:06PM +0530, Vandana Kannan wrote:
> +void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp = NULL;
> +	struct intel_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;

This routine touches a register and reads state guarded by modeset locks,
I don't see any documentation to make sure the caller complies with
holding the right locks.

And since this function isn't used yet, I can't check that the callers
are correct at this point in time either. Please, please do document
locking requirements on new functions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Vandana Kannan]

On Mar-05-2014 3:28 PM, Chris Wilson wrote:
> On Wed, Mar 05, 2014 at 03:13:06PM +0530, Vandana Kannan wrote:
>> +void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	struct intel_encoder *encoder;
>> +	struct intel_dp *intel_dp = NULL;
>> +	struct intel_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;
> 
> This routine touches a register and reads state guarded by modeset locks,
> I don't see any documentation to make sure the caller complies with
> holding the right locks.
> 
> And since this function isn't used yet, I can't check that the callers
> are correct at this point in time either. Please, please do document
> locking requirements on new functions.
> -Chris
> 
Thanks for your inputs. I have added a short description explaining that
the locking may be required for future implementations/enhancements of DRRS.
I have pushed the revised set of patches.
-Vandana