On Tue, 1 Nov 2011 23:20:28 -0700
Keith Packard <keithp@xxxxxxxxxx> wrote:
> Make sure the sequence of operations in all three functions makes
> sense:
>
> 1) The backlight must be off unless the screen is running
> 2) The link must be running to turn the eDP panel on/off
> 3) The CPU eDP PLL must be running until everything is off
A few comments on this one (also, is it strictly required to fix your
bug)?
> Signed-off-by: Keith Packard <keithp@xxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 22 +++++++++++++---------
> 1 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index d6c6608..6be6a04 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1234,17 +1234,18 @@ static void intel_dp_prepare(struct drm_encoder *encoder)
> {
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>
> + ironlake_edp_backlight_off(intel_dp);
> + ironlake_edp_panel_off(intel_dp);
> +
> /* Wake up the sink first */
> ironlake_edp_panel_vdd_on(intel_dp);
> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> + intel_dp_link_down(intel_dp);
> ironlake_edp_panel_vdd_off(intel_dp, false);
>
> /* Make sure the panel is off before trying to
> * change the mode
> */
> - ironlake_edp_backlight_off(intel_dp);
> - intel_dp_link_down(intel_dp);
> - ironlake_edp_panel_off(intel_dp);
> }
Ok so you're making sure the panel has power to down the link, I think
that's fine.
> static void intel_dp_commit(struct drm_encoder *encoder)
> @@ -1276,16 +1277,20 @@ intel_dp_dpms(struct drm_encoder *encoder, int mode)
> uint32_t dp_reg = I915_READ(intel_dp->output_reg);
>
> if (mode != DRM_MODE_DPMS_ON) {
> + ironlake_edp_backlight_off(intel_dp);
> + ironlake_edp_panel_off(intel_dp);
> +
> ironlake_edp_panel_vdd_on(intel_dp);
> - if (is_edp(intel_dp))
> - ironlake_edp_backlight_off(intel_dp);
> intel_dp_sink_dpms(intel_dp, mode);
> intel_dp_link_down(intel_dp);
> - ironlake_edp_panel_off(intel_dp);
> - if (is_edp(intel_dp) && !is_pch_edp(intel_dp))
> - ironlake_edp_pll_off(encoder);
> ironlake_edp_panel_vdd_off(intel_dp, false);
> +
> + if (is_cpu_edp(intel_dp))
> + ironlake_edp_pll_off(encoder);
But here it looks like you're shutting it off, then downing the link?
Should this be reordered?
--
Jesse Barnes, Intel Open Source Technology Center
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
