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

[Vandana Kannan <vandana.kannan@intel.com>]

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
>