Re: [Intel-gfx] [PATCH] drm/i915: rip out edp special case from dp_link_down
On Thu, Sep 13, 2012 at 07:17:55PM -0300, Paulo Zanoni wrote: Hi 2012/9/9 Daniel Vetter daniel.vet...@ffwll.ch: This has been tons of fun to figure out with git blame. The first notion of this code block goes back to the original cpu edp enabling for ilk in commit 32f9d658aee5be09ebdd28fc730630e61d0b46db Author: Zhenyu Wang zhen...@linux.intel.com Date: Fri Jul 24 01:00:32 2009 +0800 drm/i915: Add eDP support on IGDNG mobile chip Two things are notable in this commit wrt to the this edp special case: - The IS_eDP check _only_ fires for DP A, i.e. cpu edp ports. - The cpu edp port is disabled at the top of the dp_link_down function. My theory is that these hacks was added to work around the completely different modeset sequence for cpu edp ports compared to pch edp ports. With the cpu edp confusion on ilk (and snb/ivb) now fixed up, this shouldn't be required any more. The really interesting question is how this special cases survived this long in the code. The first step is declaring the pch port D as eDP if it's used for an internal panel: commit b329530ca7cdf6bf014f2124efd983e01265d623 Author: Adam Jackson a...@redhat.com Date: Fri Jul 16 14:46:28 2010 -0400 drm/i915/dp: Correctly report eDP in the core connector type This commit unfortunately failed to notice that not all edp ports are created equal. Then follow a flurry of refactorings, culminating in a patch from Keith Packard which resulted in the current logic (by making it correct for all platforms that have edp): commit 417e822deee1d2bcd8a8a60660c40a0903713f2b Author: Keith Packard kei...@keithp.com Date: Tue Nov 1 19:54:11 2011 -0700 drm/i915: Treat PCH eDP like DP in most places None of these cleanups or refactorings supply any reason why we need this code, they've simply carried it on as-is. Hence presume it might be harmful with the current code and rip it out. We do rewrite the link training bits completely anyway when re-training the link. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch After looking for a long time I could not find a reason why we should set the pattern to train off while disabling the eDP port. Notice that train off means send normal pixels (see BSpec). All the docs say is that when _enabling_ the port we need to set training pattern 1, which we should already be doing. After your patch, when we disable the port we will disable it with training pattern already set to 1 (instead of send normal pixels/ train off), but there's nothing saying we shouldn't do this. So yeah, your patch looks fine. If we ever revert this patch, let's make sure we add a big comment explaining it. Yeah, I if this blows up we can add a fun story to our code in a comment ;-) Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com Thanks for the review, patch is queued for -next. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: rip out edp special case from dp_link_down
Hi
2012/9/9 Daniel Vetter daniel.vet...@ffwll.ch:
This has been tons of fun to figure out with git blame. The first
notion of this code block goes back to the original cpu edp enabling
for ilk in
commit 32f9d658aee5be09ebdd28fc730630e61d0b46db
Author: Zhenyu Wang zhen...@linux.intel.com
Date: Fri Jul 24 01:00:32 2009 +0800
drm/i915: Add eDP support on IGDNG mobile chip
Two things are notable in this commit wrt to the this edp special
case:
- The IS_eDP check _only_ fires for DP A, i.e. cpu edp ports.
- The cpu edp port is disabled at the top of the dp_link_down function.
My theory is that these hacks was added to work around the completely
different modeset sequence for cpu edp ports compared to pch edp
ports. With the cpu edp confusion on ilk (and snb/ivb) now fixed up,
this shouldn't be required any more.
The really interesting question is how this special cases survived
this long in the code. The first step is declaring the pch port D as
eDP if it's used for an internal panel:
commit b329530ca7cdf6bf014f2124efd983e01265d623
Author: Adam Jackson a...@redhat.com
Date: Fri Jul 16 14:46:28 2010 -0400
drm/i915/dp: Correctly report eDP in the core connector type
This commit unfortunately failed to notice that not all edp ports are
created equal. Then follow a flurry of refactorings, culminating in a
patch from Keith Packard which resulted in the current logic (by
making it correct for all platforms that have edp):
commit 417e822deee1d2bcd8a8a60660c40a0903713f2b
Author: Keith Packard kei...@keithp.com
Date: Tue Nov 1 19:54:11 2011 -0700
drm/i915: Treat PCH eDP like DP in most places
None of these cleanups or refactorings supply any reason why we need
this code, they've simply carried it on as-is.
Hence presume it might be harmful with the current code and rip it
out. We do rewrite the link training bits completely anyway when
re-training the link.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
After looking for a long time I could not find a reason why we should
set the pattern to train off while disabling the eDP port. Notice
that train off means send normal pixels (see BSpec). All the docs
say is that when _enabling_ the port we need to set training pattern
1, which we should already be doing. After your patch, when we disable
the port we will disable it with training pattern already set to 1
(instead of send normal pixels/ train off), but there's nothing
saying we shouldn't do this. So yeah, your patch looks fine. If we
ever revert this patch, let's make sure we add a big comment
explaining it.
Reviewed-by: Paulo Zanoni paulo.r.zan...@intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f28353d..8adad5e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1934,13 +1934,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
msleep(17);
- if (is_edp(intel_dp)) {
- if (HAS_PCH_CPT(dev) (IS_GEN7(dev) ||
!is_cpu_edp(intel_dp)))
- DP |= DP_LINK_TRAIN_OFF_CPT;
- else
- DP |= DP_LINK_TRAIN_OFF;
- }
-
if (HAS_PCH_IBX(dev)
I915_READ(intel_dp-output_reg) DP_PIPEB_SELECT) {
struct drm_crtc *crtc = intel_dp-base.base.crtc;
--
1.7.11.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: rip out edp special case from dp_link_down
On 9/9/12 1:58 PM, Daniel Vetter wrote: The really interesting question is how this special cases survived this long in the code. The first step is declaring the pch port D as eDP if it's used for an internal panel: commit b329530ca7cdf6bf014f2124efd983e01265d623 Author: Adam Jackson a...@redhat.com Date: Fri Jul 16 14:46:28 2010 -0400 drm/i915/dp: Correctly report eDP in the core connector type This commit unfortunately failed to notice that not all edp ports are created equal. Then follow a flurry of refactorings, culminating in a patch from Keith Packard which resulted in the current logic (by making it correct for all platforms that have edp): The motivation for this patch was simply making eDP connectors be reported as eDP, since that has the same probably special properties as LVDS (correlation with lid state, probably the primary if no better guidance given, etc). - ajax ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: rip out edp special case from dp_link_down
This has been tons of fun to figure out with git blame. The first
notion of this code block goes back to the original cpu edp enabling
for ilk in
commit 32f9d658aee5be09ebdd28fc730630e61d0b46db
Author: Zhenyu Wang zhen...@linux.intel.com
Date: Fri Jul 24 01:00:32 2009 +0800
drm/i915: Add eDP support on IGDNG mobile chip
Two things are notable in this commit wrt to the this edp special
case:
- The IS_eDP check _only_ fires for DP A, i.e. cpu edp ports.
- The cpu edp port is disabled at the top of the dp_link_down function.
My theory is that these hacks was added to work around the completely
different modeset sequence for cpu edp ports compared to pch edp
ports. With the cpu edp confusion on ilk (and snb/ivb) now fixed up,
this shouldn't be required any more.
The really interesting question is how this special cases survived
this long in the code. The first step is declaring the pch port D as
eDP if it's used for an internal panel:
commit b329530ca7cdf6bf014f2124efd983e01265d623
Author: Adam Jackson a...@redhat.com
Date: Fri Jul 16 14:46:28 2010 -0400
drm/i915/dp: Correctly report eDP in the core connector type
This commit unfortunately failed to notice that not all edp ports are
created equal. Then follow a flurry of refactorings, culminating in a
patch from Keith Packard which resulted in the current logic (by
making it correct for all platforms that have edp):
commit 417e822deee1d2bcd8a8a60660c40a0903713f2b
Author: Keith Packard kei...@keithp.com
Date: Tue Nov 1 19:54:11 2011 -0700
drm/i915: Treat PCH eDP like DP in most places
None of these cleanups or refactorings supply any reason why we need
this code, they've simply carried it on as-is.
Hence presume it might be harmful with the current code and rip it
out. We do rewrite the link training bits completely anyway when
re-training the link.
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/intel_dp.c | 7 ---
1 file changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f28353d..8adad5e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1934,13 +1934,6 @@ intel_dp_link_down(struct intel_dp *intel_dp)
msleep(17);
- if (is_edp(intel_dp)) {
- if (HAS_PCH_CPT(dev) (IS_GEN7(dev) || !is_cpu_edp(intel_dp)))
- DP |= DP_LINK_TRAIN_OFF_CPT;
- else
- DP |= DP_LINK_TRAIN_OFF;
- }
-
if (HAS_PCH_IBX(dev)
I915_READ(intel_dp-output_reg) DP_PIPEB_SELECT) {
struct drm_crtc *crtc = intel_dp-base.base.crtc;
--
1.7.11.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
