Re: [PATCH 5/5] drm/i915: init the DP panel power seq variables earlier

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri,  6 Dec 2013 17:32:44 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Our driver has two different ways of waiting for panel power
> sequencing delays. One of these ways is through
> ironlake_wait_panel_status, which implicitly uses the values written
> to our registers. The other way is through the functions that call
> intel_wait_until_after, and on this case we do direct msleep() calls
> on the intel_dp->xxx_delay variables.
> 
> Function intel_dp_init_panel_power_sequencer is responsible for
> initializing the _delay variables and deciding which values we need to
> write to the registers, but it does not write these values to the
> registers. Only at intel_dp_init_panel_power_sequencer_registers we
> actually do this write.
> 
> Then problem is that when we call intel_dp_i2c_init, we will get some
> I2C calls, which will trigger a VDD enable, which will make use of the
> panel power sequencing registers and the _delay variables, so we need
> to have both ready by this time. Today, when this happens, the _delay
> variables are zero (because they were not computed) and the panel
> power sequence registers contain whatever values were written by the
> BIOS (which are usually correct).
> 
> What this patch does is to make sure that function
> intel_dp_init_panel_power_sequencer is called earlier, so by the time
> we call intel_dp_i2c_init, the _delay variables will already be
> initialized. The actual registers won't contain their final values,
> but at least they will contain the values set by the BIOS.
> 
> The good side is that we were reading the values, but were not using
> them for anything (because we were just skipping the msleep(0) calls),
> so this "fix" shouldn't fix any real existing bugs. I was only able to
> identify the problem because I added some debug code to check how much
> time time we were saving with my previous patch.
> 
> Regression introduced by:
>     commit ed92f0b239ac971edc509169ae3d6955fbe0a188
>     Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
>     Date:   Wed Jun 12 17:27:24 2013 -0300
>         drm/i915: extract intel_edp_init_connector
> 
> v2: - Rewrite commit message.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 79f7ec2..9cc5819 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3540,14 +3540,14 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev,
>  }
>  
>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> -				     struct intel_connector *intel_connector)
> +				     struct intel_connector *intel_connector,
> +				     struct edp_power_seq *power_seq)
>  {
>  	struct drm_connector *connector = &intel_connector->base;
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(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 edp_power_seq power_seq = { 0 };
>  	bool has_dpcd;
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
> @@ -3555,8 +3555,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> -	intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> -
>  	/* Cache DPCD and EDID for edp. */
>  	ironlake_edp_panel_vdd_on(intel_dp);
>  	has_dpcd = intel_dp_get_dpcd(intel_dp);
> @@ -3574,8 +3572,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	}
>  
>  	/* We now know it's not a ghost, init power sequence regs. */
> -	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp,
> -						      &power_seq);
> +	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq);
>  
>  	edid = drm_get_edid(connector, &intel_dp->adapter);
>  	if (edid) {
> @@ -3624,6 +3621,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = intel_dig_port->port;
> +	struct edp_power_seq power_seq = { 0 };
>  	const char *name = NULL;
>  	int type, error;
>  
> @@ -3707,13 +3705,16 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		BUG();
>  	}
>  
> +	if (is_edp(intel_dp))
> +		intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> +
>  	error = intel_dp_i2c_init(intel_dp, intel_connector, name);
>  	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>  	     error, port_name(port));
>  
>  	intel_dp->psr_setup_done = false;
>  
> -	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
> +	if (!intel_edp_init_connector(intel_dp, intel_connector, &power_seq)) {
>  		i2c_del_adapter(&intel_dp->adapter);
>  		if (is_edp(intel_dp)) {
>  			cancel_delayed_work_sync(&intel_dp->panel_vdd_work);

Yeah, seems reasonable.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center