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

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

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

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