Re: [PATCH 15/14] drm/i915: Add comments explaining the vdd on/off functions

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3704 bytes --]

On Tue, 2014-08-19 at 20:47 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Jani wanted some comments to explain why we call certain vdd on/off
> functions in certain places.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1067082..1233a10 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1315,6 +1315,7 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
>  	return control;
>  }
>  
> +/* should be paired with edp_panel_vdd_off() */

It would be useful to clarify here the purpose of the edp_ vs.
intel_edp_* versions. Iiuc you need to hold pps_lock around the whole
edp_panel_vdd_on/off() sequence and these can be nested within an
intel_edp_panel_vdd_on/off() sequence. Otoh, you can't nest
intel_edp_panel_vdd_on() calls.

>  static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -1365,6 +1366,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
>  	return need_to_disable;
>  }
>  
> +/* should be paired with intel_edp_panel_vdd_off() */
>  void intel_edp_panel_vdd_on(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -1447,6 +1449,7 @@ static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp)
>  	schedule_delayed_work(&intel_dp->panel_vdd_work, delay);
>  }
>  
> +/* should be paired with edp_panel_vdd_on() */
>  static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -1467,6 +1470,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  		edp_panel_vdd_schedule_off(intel_dp);
>  }
>  
> +/* should be paired with intel_edp_panel_vdd_on() */
>  static void intel_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -4307,6 +4311,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	drm_encoder_cleanup(encoder);
>  	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.

s/do/due/

> +		 * Make sure vdd is actually turned off here.
> +		 */
>  		mutex_lock(&dev_priv->pps_mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev_priv->pps_mutex);
> @@ -4327,6 +4335,10 @@ static void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
>  	if (!is_edp(intel_dp))
>  		return;
>  
> +	/*
> +	 * vdd might still be enabled do to the delayed vdd off.
> +	 * Make sure vdd is actually turned off here.
> +	 */
>  	mutex_lock(&dev_priv->pps_mutex);
>  	edp_panel_vdd_off_sync(intel_dp);
>  	mutex_unlock(&dev_priv->pps_mutex);
> @@ -5025,6 +5037,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  		drm_dp_aux_unregister(&intel_dp->aux);
>  		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.
> +			 */
>  			mutex_lock(&dev_priv->pps_mutex);
>  			edp_panel_vdd_off_sync(intel_dp);
>  			mutex_unlock(&dev_priv->pps_mutex);

edp_panel_vdd_off_sync() could also use a clarification similar to the
the rest of the API.

With or without the above changes:
Reviewed-by: Imre Deak <imre.deak@intel.com>



[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx