On Thu, Sep 13, 2012 at 07:17:55PM -0300, Paulo Zanoni wrote: > Hi > > 2012/9/9 Daniel Vetter <daniel.vetter at 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 <zhenyuw at 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 <ajax at 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 <keithp at 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.vetter at 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.zanoni at 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
