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

Commit Message

[e-mail] Dec. 23, 2013, 7:52 a.m. UTC

From: Pradeep Bhat <[e-mail]>

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

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

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

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

Signed-off-by: Pradeep Bhat <[e-mail]>
Signed-off-by: Vandana Kannan <[e-mail]>
---
 drivers/gpu/drm/i915/intel_dp.c  |   45 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |   30 +++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

Comments

Jani Nikula Jan. 22, 2014, 1:33 p.m. UTC | #1

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

> +/**
> + * HIGH_RR is the highest eDP panel refresh rate read from EDID
> + * LOW_RR is the lowest eDP panel refresh rate found from EDID
> + * parsing for same resolution.
> + */
> +enum edp_drrs_refresh_rate_type {
> +	DRRS_HIGH_RR,
> +	DRRS_LOW_RR,
> +	DRRS_MAX_RR, /* RR count */
> +};
> +/**
> + * The drrs_info struct will represent the DRRS feature for eDP
> + * panel.
> + */

This comment does not add any value.

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

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

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

[e-mail] Jan. 30, 2014, 3:33 a.m. UTC | #2

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

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

[e-mail] Feb. 11, 2014, 6:32 a.m. UTC | #3

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

Patch

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