[Intel-gfx] [PATCH v3 3/3] drm/i915: Group all the PPS init steps to one place

[Ville Syrjälä]

On Tue, Jun 21, 2016 at 11:51:49AM +0300, Imre Deak wrote:
> Move the early PPS initialization calls next to the rest of PPS
> initialization steps. This allows us to forgo a duplicated call to
> intel_dp_init_panel_power_sequencer_registers() on VLV/CHV.
> 
> This will swap the order of DP AUX registration wrt. PPS initialization.
> There is an existing race here in case of a user space access via the
> DPAUX device node after DP AUX registration and before calling
> intel_dp_init_panel_power_sequencer_registers(), but this change won't
> make this worse. The fix for this is to separate DP AUX initialization
> and registration, that's a separate work already underway.
> 
> The order of MST wrt. PPS init as well as the order of
> intel_dp_init_panel_power_sequencer_registers() wrt.
> intel_edp_panel_vdd_sanitize() also swap, which is ok, there are no
> dependencies between these steps.
> 
> Suggested by Ville.
> 
> CC: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak at intel.com>

lgtm

Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 46 +++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 277b74a..d15604d2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5332,8 +5332,18 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	}
>  
>  	pps_lock(intel_dp);
> +
> +	intel_dp_init_panel_power_timestamps(intel_dp);
> +
> +	if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> +		vlv_initial_power_sequencer_setup(intel_dp);
> +	} else {
> +		intel_dp_init_panel_power_sequencer(dev, intel_dp);
> +		intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> +	}
> +
>  	intel_edp_panel_vdd_sanitize(intel_dp);
> -	intel_dp_init_panel_power_sequencer_registers(dev, intel_dp);
> +
>  	pps_unlock(intel_dp);
>  
>  	/* Cache DPCD and EDID for edp. */
> @@ -5347,7 +5357,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	} else {
>  		/* if this fails, presume the device is a ghost */
>  		DRM_INFO("failed to retrieve link info, disabling eDP\n");
> -		return false;
> +		goto out_vdd_off;
>  	}
>  
>  	mutex_lock(&dev->mode_config.mutex);
> @@ -5417,6 +5427,18 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	intel_panel_setup_backlight(connector, pipe);
>  
>  	return true;
> +
> +out_vdd_off:
> +	cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> +	/*
> +	 * vdd might still be enabled do to the delayed vdd off.
> +	 * Make sure vdd is actually turned off here.
> +	 */
> +	pps_lock(intel_dp);
> +	edp_panel_vdd_off_sync(intel_dp);
> +	pps_unlock(intel_dp);
> +
> +	return false;
>  }
>  
>  bool
> @@ -5522,16 +5544,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		BUG();
>  	}
>  
> -	if (is_edp(intel_dp)) {
> -		pps_lock(intel_dp);
> -		intel_dp_init_panel_power_timestamps(intel_dp);
> -		if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev))
> -			vlv_initial_power_sequencer_setup(intel_dp);
> -		else
> -			intel_dp_init_panel_power_sequencer(dev, intel_dp);
> -		pps_unlock(intel_dp);
> -	}
> -
>  	ret = intel_dp_aux_init(intel_dp, intel_connector);
>  	if (ret)
>  		goto fail;
> @@ -5564,16 +5576,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  	return true;
>  
>  fail:
> -	if (is_edp(intel_dp)) {
> -		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> -		/*
> -		 * vdd might still be enabled do to the delayed vdd off.
> -		 * Make sure vdd is actually turned off here.
> -		 */
> -		pps_lock(intel_dp);
> -		edp_panel_vdd_off_sync(intel_dp);
> -		pps_unlock(intel_dp);
> -	}
>  	drm_connector_unregister(connector);
>  	drm_connector_cleanup(connector);
>  
> -- 
> 2.5.0

-- 
Ville Syrjälä
Intel OTC