Re: [PATCH] drm/i915: rip out edp special case from dp_link_down

[Daniel Vetter <daniel@ffwll.ch>]

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