Re: [PATCH 19/19] drm/i915: init the DP panel power seq regs earlier

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 21 Nov 2013, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> When we call intel_dp_i2c_init we already get some I2C calls, which
> will trigger a VDD enable, which will make use of the panel power
> sequencing registers, so we need to have them ready by this time.

But this patch won't make the registers ready either! The only thing
this one does is initialize the delays in intel_dp. (And unlock the
registers, but that's done in the vdd enable function too.)

And you actually can't trivially initialize the registers either,
because we will only later figure out whether this is a ghost eDP, and
such initialization might botch up LVDS.

See
commit f30d26e468322b50d5e376bec40658683aff8ece
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Wed Jan 16 10:53:40 2013 +0200

    drm/i915/eDP: do not write power sequence registers for ghost eDP

I'm sorry I don't readily have any suggestions.

If you are only interested in fixing the delays for msleep, then please
fix the commit message!

BR,
Jani.



>
> 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
>
> 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 3a1ca80..23927a0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3565,14 +3565,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;
> @@ -3580,8 +3580,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);
> @@ -3599,8 +3597,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) {
> @@ -3649,6 +3646,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;
>  
> @@ -3748,13 +3746,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);
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center