Hi
2012/9/6 Daniel Vetter <daniel.vetter at ffwll.ch>:
> With the previous patch to clean up where exactly these two functions
> are getting called, this patch can tackle the enable/disable code
> itself:
This is the kind of patch that's hard to proof-read. Basically, IMHO,
the intel_dp->DP variable makes things harder to understand and
review. Sometimes we use intel_dp->DP directly, sometimes we do "int
DP = intel_dp->DP" and then just change the temporary DP (sometimes
not updating the original intel_dp->DP after changing the temporary
DP), sometimes we read/write directly from intel_dp->output_reg,
sometimes we pass "int DP" as a function argument instead of storing
the value inside intel_dp->DP. I have to admit that even after working
some time with this code I'm not exactly sure why intel_dp->DP is
really necessary. One of my plans was to try to write a patch that
removes the variable so I could at least maybe understand why it's
necessary (the plan was to read/write from/to intel_dp->output_reg
directly). You can do that if you want :)
Complaints aside, here are my comments:
>
> - WARN if the port enable bit is in the wrong state or if the edp pll
> bit is in the wrong state, just for paranoia's sake.
Looks good.
> - Don't disable the edp pll harder in the modeset functions just for
> fun.
Which part of the patch is this message about? Did you mean
intel_dp_link_down instead of modeset? I could not find anything on
our documentation saying that the PLL must be disabled when retraining
the link, so your patch looks correct, but this is the kind of thing
that we should really try to test somehow :)
> - Don't set the edp pll enable flag in intel_dp->DP in modeset, do
> that while changing the actual hw state. We do the same with the
> actual port enable bit, so this is a bit more consistent.
Looks good.
> - Track the current DP register value when setting things up and add
> some comments how intel_dp->DP is used in the disable code.
>
> v2: Be more careful with resetting intel_dp->DP - otherwise dpms
> off->on will fail spectacularly, becuase we enable the eDP port when
> we should only enable the eDP pll.
>
These 2 chunks are the ones my comment above is about. I'll just trust
our beloved maintainer on these chunks :)
Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++++++-----------
> 1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c72d4f3..7c746d1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -887,7 +887,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> intel_dp->DP |= intel_crtc->pipe << 29;
>
> /* don't miss out required setting for eDP */
> - intel_dp->DP |= DP_PLL_ENABLE;
> if (adjusted_mode->clock < 200000)
> intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> else
> @@ -909,7 +908,6 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>
> if (is_cpu_edp(intel_dp)) {
> /* don't miss out required setting for eDP */
> - intel_dp->DP |= DP_PLL_ENABLE;
> if (adjusted_mode->clock < 200000)
> intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> else
> @@ -1192,8 +1190,15 @@ static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
>
> DRM_DEBUG_KMS("\n");
> dpa_ctl = I915_READ(DP_A);
> - dpa_ctl |= DP_PLL_ENABLE;
> - I915_WRITE(DP_A, dpa_ctl);
> + WARN(dpa_ctl & DP_PLL_ENABLE, "dp pll on, should be off\n");
> + WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> +
> + /* We don't adjust intel_dp->DP while tearing down the link, to
> + * facilitate link retraining (e.g. after hotplug). Hence clear all
> + * enable bits here to ensure that we don't enable too much. */
> + intel_dp->DP &= ~(DP_PORT_EN | DP_AUDIO_OUTPUT_ENABLE);
> + intel_dp->DP |= DP_PLL_ENABLE;
> + I915_WRITE(DP_A, intel_dp->DP);
> POSTING_READ(DP_A);
> udelay(200);
> }
> @@ -1209,6 +1214,13 @@ static void ironlake_edp_pll_off(struct intel_dp *intel_dp)
> to_intel_crtc(crtc)->pipe);
>
> dpa_ctl = I915_READ(DP_A);
> + WARN((dpa_ctl & DP_PLL_ENABLE) == 0,
> + "dp pll off, should be on\n");
> + WARN(dpa_ctl & DP_PORT_EN, "dp port still on, should be off\n");
> +
> + /* We can't rely on the value tracked for the DP register in
> + * intel_dp->DP because link_down must not change that (otherwise link
> + * re-training will fail. */
> dpa_ctl &= ~DP_PLL_ENABLE;
> I915_WRITE(DP_A, dpa_ctl);
> POSTING_READ(DP_A);
> @@ -1906,13 +1918,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>
> DRM_DEBUG_KMS("\n");
>
> - if (is_edp(intel_dp)) {
> - DP &= ~DP_PLL_ENABLE;
> - I915_WRITE(intel_dp->output_reg, DP);
> - POSTING_READ(intel_dp->output_reg);
> - udelay(100);
> - }
> -
> if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || !is_cpu_edp(intel_dp))) {
> DP &= ~DP_LINK_TRAIN_MASK_CPT;
> I915_WRITE(intel_dp->output_reg, DP | DP_LINK_TRAIN_PAT_IDLE_CPT);
> @@ -2456,6 +2461,8 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
>
> intel_dp->output_reg = output_reg;
> intel_dp->port = port;
> + /* Preserve the current hw state. */
> + intel_dp->DP = I915_READ(intel_dp->output_reg);
>
> intel_connector = kzalloc(sizeof(struct intel_connector), GFP_KERNEL);
> if (!intel_connector) {
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
