On Thu, Sep 13, 2012 at 02:46:19PM -0300, Paulo Zanoni wrote:
> 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 :)
Oh, missed this one here a bit. Will blow up - the reason is that we need
to support re-training when the user unplugs, then replugs a dp screen.
Since that works perfectly fine with hdmi, dvi, vga it should work with
dp, too.
But on dp we need to retrain the link, which means disabling, then
re-enabling. Since that potentially clobbers the register, we keep the
value we've computed at ->mode_set time around so that we can reuse it for
the re-training. See my other mail for the exact semantics.
Imo once you've worked with the code long enough, and now that it'll lose
a lot of the ugly hacks and special cases, intel_dp->DP make sense.
Cheers, Daniel
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
