From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: rip out edp special case from dp_link_down Date: Fri, 14 Sep 2012 18:33:50 +0200 Message-ID: <20120914163350.GD5799@phenom.ffwll.local> References: <1347213536-20340-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f171.google.com (mail-wi0-f171.google.com [209.85.212.171]) by gabe.freedesktop.org (Postfix) with ESMTP id D670EA10B1 for ; Fri, 14 Sep 2012 09:33:15 -0700 (PDT) Received: by wibhq4 with SMTP id hq4so25850wib.12 for ; Fri, 14 Sep 2012 09:33:15 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, Sep 13, 2012 at 07:17:55PM -0300, Paulo Zanoni wrote: > Hi > > 2012/9/9 Daniel Vetter : > > 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 > > 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 > > 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 > > 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 > > 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 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