| Message ID | 1346962544-7439-6-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>: > See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This > applies to all platforms where we currently support eDP on, i.e. ilk, > snb & ivb. > Ok, so I looked at BSpec and the conclusion is: shouldn't we do this for eDP _and_ DP instead of just eDP? The only things to disable before the crtc are audio, the panel backlight, and then the panel power. Your patch looks correct, but maybe it could be even more correct by including DP too? Yes, I can help testing. > Without this change we fail to light up the eDP port on previously > unused crtcs (likely because something is stuck on the old pipe), and > we also fail to properly disable the old pipe (i.e. bit 30 in the > PIPECONF register is stuck as set until the next reboot). > > v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2. > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001 > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_dp.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 07f9521..f28353d 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder) > ironlake_edp_backlight_off(intel_dp); > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > ironlake_edp_panel_off(intel_dp); > - intel_dp_link_down(intel_dp); > + > + /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */ > + if (!is_cpu_edp(intel_dp)) > + intel_dp_link_down(intel_dp); > } > > static void intel_post_disable_dp(struct intel_encoder *encoder) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > - if (is_cpu_edp(intel_dp)) > + if (is_cpu_edp(intel_dp)) { > + intel_dp_link_down(intel_dp); > ironlake_edp_pll_off(intel_dp); > + } > } > > static void intel_enable_dp(struct intel_encoder *encoder) > -- > 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 04:11:20PM -0300, Paulo Zanoni wrote: > Hi > > 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>: > > See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This > > applies to all platforms where we currently support eDP on, i.e. ilk, > > snb & ivb. > > > > Ok, so I looked at BSpec and the conclusion is: shouldn't we do this > for eDP _and_ DP instead of just eDP? The only things to disable > before the crtc are audio, the panel backlight, and then the panel > power. > > Your patch looks correct, but maybe it could be even more correct by > including DP too? Yes, I can help testing. The dp pll for cpu edp is special, since we set it in the DP_A register. The pll for pch dp ports is just the regular pch pll iirc, and that is handled by the common code. Also, dp pch seems to wfm, whereas cpu edp was definitely broken. In any case, frobbing the pch dp sequence would be a separate patch imo. So can I still have an r-b for this on here? -Daniel > > > Without this change we fail to light up the eDP port on previously > > unused crtcs (likely because something is stuck on the old pipe), and > > we also fail to properly disable the old pipe (i.e. bit 30 in the > > PIPECONF register is stuck as set until the next reboot). > > > > v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2. > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001 > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 07f9521..f28353d 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder) > > ironlake_edp_backlight_off(intel_dp); > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > ironlake_edp_panel_off(intel_dp); > > - intel_dp_link_down(intel_dp); > > + > > + /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */ > > + if (!is_cpu_edp(intel_dp)) > > + intel_dp_link_down(intel_dp); > > } > > > > static void intel_post_disable_dp(struct intel_encoder *encoder) > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > > > - if (is_cpu_edp(intel_dp)) > > + if (is_cpu_edp(intel_dp)) { > > + intel_dp_link_down(intel_dp); > > ironlake_edp_pll_off(intel_dp); > > + } > > } > > > > static void intel_enable_dp(struct intel_encoder *encoder) > > -- > > 1.7.11.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
2012/9/13 Daniel Vetter <daniel@ffwll.ch>: > On Thu, Sep 13, 2012 at 04:11:20PM -0300, Paulo Zanoni wrote: >> Hi >> >> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>: >> > See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This >> > applies to all platforms where we currently support eDP on, i.e. ilk, >> > snb & ivb. >> > >> >> Ok, so I looked at BSpec and the conclusion is: shouldn't we do this >> for eDP _and_ DP instead of just eDP? The only things to disable >> before the crtc are audio, the panel backlight, and then the panel >> power. >> >> Your patch looks correct, but maybe it could be even more correct by >> including DP too? Yes, I can help testing. > > The dp pll for cpu edp is special, since we set it in the DP_A register. > The pll for pch dp ports is just the regular pch pll iirc, and that is > handled by the common code. Also, dp pch seems to wfm, whereas cpu edp was > definitely broken. > > In any case, frobbing the pch dp sequence would be a separate patch imo. > So can I still have an r-b for this on here? Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > -Daniel > >> >> > Without this change we fail to light up the eDP port on previously >> > unused crtcs (likely because something is stuck on the old pipe), and >> > we also fail to properly disable the old pipe (i.e. bit 30 in the >> > PIPECONF register is stuck as set until the next reboot). >> > >> > v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2. >> > >> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001 >> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> > --- >> > drivers/gpu/drm/i915/intel_dp.c | 9 +++++++-- >> > 1 file changed, 7 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index 07f9521..f28353d 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder) >> > ironlake_edp_backlight_off(intel_dp); >> > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> > ironlake_edp_panel_off(intel_dp); >> > - intel_dp_link_down(intel_dp); >> > + >> > + /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */ >> > + if (!is_cpu_edp(intel_dp)) >> > + intel_dp_link_down(intel_dp); >> > } >> > >> > static void intel_post_disable_dp(struct intel_encoder *encoder) >> > { >> > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> > >> > - if (is_cpu_edp(intel_dp)) >> > + if (is_cpu_edp(intel_dp)) { >> > + intel_dp_link_down(intel_dp); >> > ironlake_edp_pll_off(intel_dp); >> > + } >> > } >> > >> > static void intel_enable_dp(struct intel_encoder *encoder) >> > -- >> > 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
On Thu, Sep 13, 2012 at 04:43:57PM -0300, Paulo Zanoni wrote: > 2012/9/13 Daniel Vetter <daniel@ffwll.ch>: > > On Thu, Sep 13, 2012 at 04:11:20PM -0300, Paulo Zanoni wrote: > >> Hi > >> > >> 2012/9/6 Daniel Vetter <daniel.vetter@ffwll.ch>: > >> > See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This > >> > applies to all platforms where we currently support eDP on, i.e. ilk, > >> > snb & ivb. > >> > > >> > >> Ok, so I looked at BSpec and the conclusion is: shouldn't we do this > >> for eDP _and_ DP instead of just eDP? The only things to disable > >> before the crtc are audio, the panel backlight, and then the panel > >> power. > >> > >> Your patch looks correct, but maybe it could be even more correct by > >> including DP too? Yes, I can help testing. > > > > The dp pll for cpu edp is special, since we set it in the DP_A register. > > The pll for pch dp ports is just the regular pch pll iirc, and that is > > handled by the common code. Also, dp pch seems to wfm, whereas cpu edp was > > definitely broken. > > > > In any case, frobbing the pch dp sequence would be a separate patch imo. > > So can I still have an r-b for this on here? > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> I've slurped in the other 3 patches, thanks for the review. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 07f9521..f28353d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1315,15 +1315,20 @@ static void intel_disable_dp(struct intel_encoder *encoder) ironlake_edp_backlight_off(intel_dp); intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); ironlake_edp_panel_off(intel_dp); - intel_dp_link_down(intel_dp); + + /* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */ + if (!is_cpu_edp(intel_dp)) + intel_dp_link_down(intel_dp); } static void intel_post_disable_dp(struct intel_encoder *encoder) { struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); - if (is_cpu_edp(intel_dp)) + if (is_cpu_edp(intel_dp)) { + intel_dp_link_down(intel_dp); ironlake_edp_pll_off(intel_dp); + } } static void intel_enable_dp(struct intel_encoder *encoder)
See bspec, Vol3 Part2, Section 1.1.3 "Display Mode Set Sequence". This applies to all platforms where we currently support eDP on, i.e. ilk, snb & ivb. Without this change we fail to light up the eDP port on previously unused crtcs (likely because something is stuck on the old pipe), and we also fail to properly disable the old pipe (i.e. bit 30 in the PIPECONF register is stuck as set until the next reboot). v2: Rebased on top of the edp panel off sequence changes in 3.6-rc2. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=44001 Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_dp.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)