Re: [PATCH 3/5] drm/i915: robustify edp_pll_on/off

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 13, 2012 at 02:46:19PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/9/6 Daniel Vetter <daniel.vetter@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@intel.com>
> 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@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@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