Re: [PATCH 1/2] drm/i915: factor out intel_edp_panel_vdd_sanitize

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Jun 27, 2014 at 09:35:13PM +0300, Imre Deak wrote:
> This will be needed by an upcoming patch too that needs to sanitize the
> VDD state during resume. The additional async disabling is only needed
> for the resume path, here it doesn't make a difference since we enable
> VDD right after the sanitize call.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 59 ++++++++++++++++++++++++++++------------
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  2 files changed, 43 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b5ec489..a40d914 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1234,6 +1234,19 @@ static void edp_panel_vdd_work(struct work_struct *__work)
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
> +static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp)
> +{
> +	unsigned long delay;
> +
> +	/*
> +	 * Queue the timer to fire a long time from now (relative to the power
> +	 * down delay) to keep the panel power up across a sequence of
> +	 * operations.
> +	 */
> +	delay = msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5);
> +	schedule_delayed_work(&intel_dp->panel_vdd_work, delay);
> +}
> +
>  static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  {
>  	if (!is_edp(intel_dp))
> @@ -1243,17 +1256,10 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  
>  	intel_dp->want_panel_vdd = false;
>  
> -	if (sync) {
> +	if (sync)
>  		edp_panel_vdd_off_sync(intel_dp);
> -	} else {
> -		/*
> -		 * Queue the timer to fire a long
> -		 * time from now (relative to the power down delay)
> -		 * to keep the panel power up across a sequence of operations
> -		 */
> -		schedule_delayed_work(&intel_dp->panel_vdd_work,
> -				      msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5));
> -	}
> +	else
> +		edp_panel_vdd_schedule_off(intel_dp);
>  }
>  
>  void intel_edp_panel_on(struct intel_dp *intel_dp)
> @@ -4179,6 +4185,31 @@ intel_dp_drrs_init(struct intel_digital_port *intel_dig_port,
>  	return downclock_mode;
>  }
>  
> +void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +	struct drm_device *dev = intel_encoder->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum intel_display_power_domain power_domain;
> +
> +	if (intel_encoder->type != INTEL_OUTPUT_EDP)
> +		return;

I'd move the intel_dp assignment after the encoder type check to avoid
anyone thinking that they're allowed to dereference it before we're
sure about the type.

Apart from that both patches are:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	if (!edp_have_panel_vdd(intel_dp))
> +		return;
> +	/*
> +	 * The VDD bit needs a power domain reference, so if the bit is
> +	 * already enabled when we boot or resume, grab this reference and
> +	 * schedule a vdd off, so we don't hold on to the reference
> +	 * indefinitely.
> +	 */
> +	DRM_DEBUG_KMS("VDD left on by BIOS, adjusting state tracking\n");
> +	power_domain = intel_display_port_power_domain(intel_encoder);
> +	intel_display_power_get(dev_priv, power_domain);
> +
> +	edp_panel_vdd_schedule_off(intel_dp);
> +}
> +
>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  				     struct intel_connector *intel_connector,
>  				     struct edp_power_seq *power_seq)
> @@ -4199,13 +4230,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	if (!is_edp(intel_dp))
>  		return true;
>  
> -	/* The VDD bit needs a power domain reference, so if the bit is already
> -	 * enabled when we boot, grab this reference. */
> -	if (edp_have_panel_vdd(intel_dp)) {
> -		enum intel_display_power_domain power_domain;
> -		power_domain = intel_display_port_power_domain(intel_encoder);
> -		intel_display_power_get(dev_priv, power_domain);
> -	}
> +	intel_edp_panel_vdd_sanitize(intel_encoder);
>  
>  	/* Cache DPCD and EDID for edp. */
>  	intel_edp_panel_vdd_on(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5f7c7bd..150c09b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -855,6 +855,7 @@ bool intel_dp_is_edp(struct drm_device *dev, enum port port);
>  void intel_edp_backlight_on(struct intel_dp *intel_dp);
>  void intel_edp_backlight_off(struct intel_dp *intel_dp);
>  void intel_edp_panel_vdd_on(struct intel_dp *intel_dp);
> +void intel_edp_panel_vdd_sanitize(struct intel_encoder *intel_encoder);
>  void intel_edp_panel_on(struct intel_dp *intel_dp);
>  void intel_edp_panel_off(struct intel_dp *intel_dp);
>  void intel_edp_psr_enable(struct intel_dp *intel_dp);
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC