On Tue, 2014-08-19 at 20:47 +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx>
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
