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

Commit Message

[e-mail] Dec. 17, 2013, 5:28 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.

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

Comments

Chris Wilson Dec. 17, 2013, 12:28 p.m. UTC | #1

On Tue, Dec 17, 2013 at 10:58:24AM +0530, Vandana Kannan 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.
> 
> Signed-off-by: Pradeep Bhat <[e-mail]>
> Signed-off-by: Vandana Kannan <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>  drivers/gpu/drm/i915/intel_dp.c  |   47 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |   29 +++++++++++++++++++++++
>  3 files changed, 78 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 02e11dc..c9bca16 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private {
>  	/* Reclocking support */
>  	bool render_reclock_avail;
>  	bool lvds_downclock_avail;
> +	bool edp_downclock_avail;
>  	/* indicates the reduced downclock for LVDS*/
>  	int lvds_downclock;
> +	int edp_downclock;
>  	u16 orig_clock;

Do any machines have both edp and lvds? Shouldn't this be a part of the
panel state?

>  
> +/**
> + * This enum is used to indicate the DRRS support type.
> + * The values of the enum map 1-to-1 with the values from VBT.
> + */
> +enum edp_panel_type {
> +	DRRS_NOT_SUPPORTED = -1,
> +	STATIC_DRRS_SUPPORT = 0,
> +	SEAMLESS_DRRS_SUPPORT = 2
> +};
> +/**
> + * 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 {
> +	int is_drrs_supported;
> +	int drrs_refresh_rate_type;

So what was the point of the enums again? Are you purposely trying to
disable gcc and sparse's type-safety?
-Chris

[e-mail] Dec. 18, 2013, 8:11 a.m. UTC | #2

On Dec-17-2013 5:58 PM, Chris Wilson wrote:
> On Tue, Dec 17, 2013 at 10:58:24AM +0530, Vandana Kannan 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.
>>
>> Signed-off-by: Pradeep Bhat <[e-mail]>
>> Signed-off-by: Vandana Kannan <[e-mail]>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>>  drivers/gpu/drm/i915/intel_dp.c  |   47 ++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |   29 +++++++++++++++++++++++
>>  3 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 02e11dc..c9bca16 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private {
>>  	/* Reclocking support */
>>  	bool render_reclock_avail;
>>  	bool lvds_downclock_avail;
>> +	bool edp_downclock_avail;
>>  	/* indicates the reduced downclock for LVDS*/
>>  	int lvds_downclock;
>> +	int edp_downclock;
>>  	u16 orig_clock;
> 
> Do any machines have both edp and lvds? Shouldn't this be a part of the
> panel state?
>
If there is a machine having both edp and lvds, then edp takes higher
priority. edp_downclock_avail and edp_downclock were added here
following the existing code having lvds_downclock_avail and
lvds_downclock here. If required, edp_downclock_avail and edp_downclock
can be moved to intel_panel structure. Kindly let us know.

>>  
>> +/**
>> + * This enum is used to indicate the DRRS support type.
>> + * The values of the enum map 1-to-1 with the values from VBT.
>> + */
>> +enum edp_panel_type {
>> +	DRRS_NOT_SUPPORTED = -1,
>> +	STATIC_DRRS_SUPPORT = 0,
>> +	SEAMLESS_DRRS_SUPPORT = 2
>> +};
>> +/**
>> + * 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 {
>> +	int is_drrs_supported;
>> +	int drrs_refresh_rate_type;
> 
> So what was the point of the enums again? Are you purposely trying to
> disable gcc and sparse's type-safety?
> -Chris
> 
The enum edp_panel_type is required to check DRRS capability of the
panel before performing any enabling. We will look into an
implementation which can do without edp_drrs_refresh_rate_type.

Chris Wilson Dec. 18, 2013, 9:06 a.m. UTC | #3

On Wed, Dec 18, 2013 at 01:41:21PM +0530, Vandana Kannan wrote:
> On Dec-17-2013 5:58 PM, Chris Wilson wrote:
> > On Tue, Dec 17, 2013 at 10:58:24AM +0530, Vandana Kannan 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.
> >>
> >> Signed-off-by: Pradeep Bhat <[e-mail]>
> >> Signed-off-by: Vandana Kannan <[e-mail]>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
> >>  drivers/gpu/drm/i915/intel_dp.c  |   47 ++++++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/intel_drv.h |   29 +++++++++++++++++++++++
> >>  3 files changed, 78 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 02e11dc..c9bca16 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private {
> >>  	/* Reclocking support */
> >>  	bool render_reclock_avail;
> >>  	bool lvds_downclock_avail;
> >> +	bool edp_downclock_avail;
> >>  	/* indicates the reduced downclock for LVDS*/
> >>  	int lvds_downclock;
> >> +	int edp_downclock;
> >>  	u16 orig_clock;
> > 
> > Do any machines have both edp and lvds? Shouldn't this be a part of the
> > panel state?
> >
> If there is a machine having both edp and lvds, then edp takes higher
> priority. edp_downclock_avail and edp_downclock were added here
> following the existing code having lvds_downclock_avail and
> lvds_downclock here. If required, edp_downclock_avail and edp_downclock
> can be moved to intel_panel structure. Kindly let us know.

And we can consolidate both into intel_panel.
 
> >>  
> >> +/**
> >> + * This enum is used to indicate the DRRS support type.
> >> + * The values of the enum map 1-to-1 with the values from VBT.
> >> + */
> >> +enum edp_panel_type {
> >> +	DRRS_NOT_SUPPORTED = -1,
> >> +	STATIC_DRRS_SUPPORT = 0,
> >> +	SEAMLESS_DRRS_SUPPORT = 2
> >> +};
> >> +/**
> >> + * 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 {
> >> +	int is_drrs_supported;
> >> +	int drrs_refresh_rate_type;
> > 
> > So what was the point of the enums again? Are you purposely trying to
> > disable gcc and sparse's type-safety?
> > 
> The enum edp_panel_type is required to check DRRS capability of the
> panel before performing any enabling. We will look into an
> implementation which can do without edp_drrs_refresh_rate_type.

All I am saying is have enum, use enum. It improves type safety.
-Chris

[e-mail] Dec. 18, 2013, 10:12 a.m. UTC | #4

On Dec-18-2013 2:36 PM, Chris Wilson wrote:
> On Wed, Dec 18, 2013 at 01:41:21PM +0530, Vandana Kannan wrote:
>> On Dec-17-2013 5:58 PM, Chris Wilson wrote:
>>> On Tue, Dec 17, 2013 at 10:58:24AM +0530, Vandana Kannan 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.
>>>>
>>>> Signed-off-by: Pradeep Bhat <[e-mail]>
>>>> Signed-off-by: Vandana Kannan <[e-mail]>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>>>>  drivers/gpu/drm/i915/intel_dp.c  |   47 ++++++++++++++++++++++++++++++++++++++
>>>>  drivers/gpu/drm/i915/intel_drv.h |   29 +++++++++++++++++++++++
>>>>  3 files changed, 78 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>> index 02e11dc..c9bca16 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private {
>>>>  	/* Reclocking support */
>>>>  	bool render_reclock_avail;
>>>>  	bool lvds_downclock_avail;
>>>> +	bool edp_downclock_avail;
>>>>  	/* indicates the reduced downclock for LVDS*/
>>>>  	int lvds_downclock;
>>>> +	int edp_downclock;
>>>>  	u16 orig_clock;
>>>
>>> Do any machines have both edp and lvds? Shouldn't this be a part of the
>>> panel state?
>>>
>> If there is a machine having both edp and lvds, then edp takes higher
>> priority. edp_downclock_avail and edp_downclock were added here
>> following the existing code having lvds_downclock_avail and
>> lvds_downclock here. If required, edp_downclock_avail and edp_downclock
>> can be moved to intel_panel structure. Kindly let us know.
> 
> And we can consolidate both into intel_panel.
>  
For this patch series, I will move edp_downclock_avail and edp_downclock
to intel_panel, and following that, work on a separate patch to move
lvds_downclock_avail and lvds_downclock to intel_panel.
>>>>  
>>>> +/**
>>>> + * This enum is used to indicate the DRRS support type.
>>>> + * The values of the enum map 1-to-1 with the values from VBT.
>>>> + */
>>>> +enum edp_panel_type {
>>>> +	DRRS_NOT_SUPPORTED = -1,
>>>> +	STATIC_DRRS_SUPPORT = 0,
>>>> +	SEAMLESS_DRRS_SUPPORT = 2
>>>> +};
>>>> +/**
>>>> + * 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 {
>>>> +	int is_drrs_supported;
>>>> +	int drrs_refresh_rate_type;
>>>
>>> So what was the point of the enums again? Are you purposely trying to
>>> disable gcc and sparse's type-safety?
>>>
>> The enum edp_panel_type is required to check DRRS capability of the
>> panel before performing any enabling. We will look into an
>> implementation which can do without edp_drrs_refresh_rate_type.
> 
> All I am saying is have enum, use enum. It improves type safety.
> -Chris
> 
Ok. I will not be making any change related to this.

Jani Nikula Dec. 19, 2013, 11:51 a.m. UTC | #5

On Tue, 17 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.
>
> Signed-off-by: Pradeep Bhat <[e-mail]>
> Signed-off-by: Vandana Kannan <[e-mail]>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>  drivers/gpu/drm/i915/intel_dp.c  |   47 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |   29 +++++++++++++++++++++++
>  3 files changed, 78 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 02e11dc..c9bca16 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private {
>  	/* Reclocking support */
>  	bool render_reclock_avail;
>  	bool lvds_downclock_avail;
> +	bool edp_downclock_avail;
>  	/* indicates the reduced downclock for LVDS*/
>  	int lvds_downclock;
> +	int edp_downclock;
>  	u16 orig_clock;
>  
>  	bool mchbar_need_disable;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 82de200..935b46e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3524,6 +3524,48 @@ 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.intel_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) {
> +		dev_priv->edp_downclock_avail = true;
> +		dev_priv->edp_downclock =
> +			intel_connector->panel.downclock_mode->clock;
> +
> +		intel_dp->drrs_state.is_drrs_supported
> +				= 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)
>  {
> @@ -3537,6 +3579,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
>  
> +	intel_dp->drrs_state.is_drrs_supported = DRRS_NOT_SUPPORTED;
> +
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> @@ -3581,6 +3625,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 9f8b465..6ec5f4e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -462,6 +462,34 @@ struct intel_hdmi {
>  
>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>  
> +/**
> + * This enum is used to indicate the DRRS support type.
> + * The values of the enum map 1-to-1 with the values from VBT.
> + */
> +enum edp_panel_type {

This is about DRRS, not some fundamental "eDP panel type". The comment
above explains what this is, but why not make the enum self explanatory,
for example panel_drrs_support or something.

As to the 1-to-1 map mention, the vbt data is unsigned, but not
supported is -1 below...

> +	DRRS_NOT_SUPPORTED = -1,
> +	STATIC_DRRS_SUPPORT = 0,
> +	SEAMLESS_DRRS_SUPPORT = 2
> +};
> +/**
> + * 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 {
> +	int is_drrs_supported;

IMHO anything that begins with "is" should be strictly boolean. When you
make this an enum per Chris' request, please change this as well. For
exampel drrs_support or something.

> +	int drrs_refresh_rate_type;
> +};
> +
>  struct intel_dp {
>  	uint32_t output_reg;
>  	uint32_t aux_ch_ctl_reg;
> @@ -487,6 +515,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] Dec. 20, 2013, 5:21 a.m. UTC | #6

On Dec-19-2013 5:21 PM, Jani Nikula wrote:
> On Tue, 17 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.
>>
>> Signed-off-by: Pradeep Bhat <[e-mail]>
>> Signed-off-by: Vandana Kannan <[e-mail]>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |    2 ++
>>  drivers/gpu/drm/i915/intel_dp.c  |   47 ++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_drv.h |   29 +++++++++++++++++++++++
>>  3 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 02e11dc..c9bca16 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1462,8 +1462,10 @@ typedef struct drm_i915_private {
>>  	/* Reclocking support */
>>  	bool render_reclock_avail;
>>  	bool lvds_downclock_avail;
>> +	bool edp_downclock_avail;
>>  	/* indicates the reduced downclock for LVDS*/
>>  	int lvds_downclock;
>> +	int edp_downclock;
>>  	u16 orig_clock;
>>  
>>  	bool mchbar_need_disable;
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 82de200..935b46e 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3524,6 +3524,48 @@ 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.intel_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) {
>> +		dev_priv->edp_downclock_avail = true;
>> +		dev_priv->edp_downclock =
>> +			intel_connector->panel.downclock_mode->clock;
>> +
>> +		intel_dp->drrs_state.is_drrs_supported
>> +				= 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)
>>  {
>> @@ -3537,6 +3579,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>>  	struct drm_display_mode *scan;
>>  	struct edid *edid;
>>  
>> +	intel_dp->drrs_state.is_drrs_supported = DRRS_NOT_SUPPORTED;
>> +
>>  	if (!is_edp(intel_dp))
>>  		return true;
>>  
>> @@ -3581,6 +3625,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 9f8b465..6ec5f4e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -462,6 +462,34 @@ struct intel_hdmi {
>>  
>>  #define DP_MAX_DOWNSTREAM_PORTS		0x10
>>  
>> +/**
>> + * This enum is used to indicate the DRRS support type.
>> + * The values of the enum map 1-to-1 with the values from VBT.
>> + */
>> +enum edp_panel_type {
> 
> This is about DRRS, not some fundamental "eDP panel type". The comment
> above explains what this is, but why not make the enum self explanatory,
> for example panel_drrs_support or something.
I will change this to drrs_support_type
> 
> As to the 1-to-1 map mention, the vbt data is unsigned, but not
> supported is -1 below...
> 
>> +	DRRS_NOT_SUPPORTED = -1,
>> +	STATIC_DRRS_SUPPORT = 0,
>> +	SEAMLESS_DRRS_SUPPORT = 2
>> +};
VBT would return 0 for static DRRS and 2 for seamless DRRS.
DRRS_NOT_SUPPORTED is for internal usage - not 1:1 mapped 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 {
>> +	int is_drrs_supported;
> 
> IMHO anything that begins with "is" should be strictly boolean. When you
> make this an enum per Chris' request, please change this as well. For
> exampel drrs_support or something.
> 
I will change is_drrs_supported to enum type and change the name.
>> +	int drrs_refresh_rate_type;
>> +};
>> +
>>  struct intel_dp {
>>  	uint32_t output_reg;
>>  	uint32_t aux_ch_ctl_reg;
>> @@ -487,6 +515,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/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 02e11dc..c9bca16 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1462,8 +1462,10 @@  typedef struct drm_i915_private {
 	/* Reclocking support */
 	bool render_reclock_avail;
 	bool lvds_downclock_avail;
+	bool edp_downclock_avail;
 	/* indicates the reduced downclock for LVDS*/
 	int lvds_downclock;
+	int edp_downclock;
 	u16 orig_clock;
 
 	bool mchbar_need_disable;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 82de200..935b46e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3524,6 +3524,48 @@  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.intel_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) {
+		dev_priv->edp_downclock_avail = true;
+		dev_priv->edp_downclock =
+			intel_connector->panel.downclock_mode->clock;
+
+		intel_dp->drrs_state.is_drrs_supported
+				= 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)
 {
@@ -3537,6 +3579,8 @@  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	struct drm_display_mode *scan;
 	struct edid *edid;
 
+	intel_dp->drrs_state.is_drrs_supported = DRRS_NOT_SUPPORTED;
+
 	if (!is_edp(intel_dp))
 		return true;
 
@@ -3581,6 +3625,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 9f8b465..6ec5f4e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -462,6 +462,34 @@  struct intel_hdmi {
 
 #define DP_MAX_DOWNSTREAM_PORTS		0x10
 
+/**
+ * This enum is used to indicate the DRRS support type.
+ * The values of the enum map 1-to-1 with the values from VBT.
+ */
+enum edp_panel_type {
+	DRRS_NOT_SUPPORTED = -1,
+	STATIC_DRRS_SUPPORT = 0,
+	SEAMLESS_DRRS_SUPPORT = 2
+};
+/**
+ * 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 {
+	int is_drrs_supported;
+	int drrs_refresh_rate_type;
+};
+
 struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
@@ -487,6 +515,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 {