Messages in this thread | | | Date | Thu, 3 Nov 2011 13:00:11 -0700 | From | Jesse Barnes <> | Subject | Re: [Intel-gfx] [PATCH 5/7] drm/i915: Make DP prepare/commit consistent with DP dpms |
| |
On Tue, 1 Nov 2011 23:20:28 -0700 Keith Packard <keithp@keithp.com> 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@keithp.com> > --- > 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 [unhandled content-type:application/pgp-signature] | |