| Message ID | 1346962544-7439-4-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 :) 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
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 :) > > 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 :) See below. > > > - 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 :) Yeah, intel_dp->DP is hard to follow, and I have some more patches. After all this has settled the rule is: - intel_dp->DP is computed freshly in intel_dp_mode_set - intel_dp->DP is update with the link training values in start_link_train so that complete_link_train writes the right registers. Same applies to edp_pll_on for similar reasons - All the disable functions don't frob intel_dp->DP so that we can re-enable after dpms off without going through ->mode_set So yeah, a bit a mess, but should work. -Daniel > 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); > > - } > > - This junk here is the "disable cpu edp pll harder" part. > > 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
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
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) {
With the previous patch to clean up where exactly these two functions are getting called, this patch can tackle the enable/disable code itself: - 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. - Don't disable the edp pll harder in the modeset functions just for fun. - 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. - 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. Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-)