From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: Thinkpad T420 and single/dual channel lvds Date: Sun, 18 Mar 2012 21:24:41 +0100 Message-ID: References: <201203141337.14798.helge.bahmann@secunet.com> <1331736306.30864.7.camel@atropine> <1331748559.30864.14.camel@atropine> <4F639AC8.907@redhat.com> <20120318175031.GD4286@phenom.ffwll.local> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by gabe.freedesktop.org (Postfix) with ESMTP id DFBD69E737 for ; Sun, 18 Mar 2012 13:24:42 -0700 (PDT) In-Reply-To: <20120318175031.GD4286@phenom.ffwll.local> 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: Daniel Vetter Cc: hcb@chaoticmind.net, intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org At Sun, 18 Mar 2012 18:50:31 +0100, Daniel Vetter wrote: > > On Fri, Mar 16, 2012 at 09:29:24PM +0100, Takashi Iwai wrote: > > At Fri, 16 Mar 2012 15:55:52 -0400, > > Adam Jackson wrote: > > > > > > On 3/15/12 10:42 AM, Takashi Iwai wrote: > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index f851db7..314af26 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -356,6 +356,23 @@ static const intel_limit_t intel_limits_ironlake_display_port = { > > > > .find_pll = intel_find_pll_ironlake_dp, > > > > }; > > > > > > > > +static bool is_dual_link_lvds(struct drm_i915_private *dev_priv, > > > > + unsigned int reg) > > > > +{ > > > > + /* BIOS should set the proper LVDS register value at boot, but > > > > + * in reality, it doesn't set the value when the lid is closed; > > > > + * thus when a machine is booted with the lid closed, the LVDS > > > > + * reg value can't be trusted. So we need to check "the value > > > > + * to be set" in VBT at first. > > > > + */ > > > > + if ((dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK) == > > > > + LVDS_CLKB_POWER_UP) > > > > + return true; > > > > > > Would slightly prefer if this was more like: > > > > > > if (dev_priv->bios_lvds_val) > > > return !!(dev_priv->bios_lvds_val & LVDS_CLKB_POWER_MASK); > > > > > > Since that way it eliminates some useless register reads in the normal > > > case even for single-link. Not going to insist on it though. > > > > Sounds reasonable, yes. > > > > Also, it'd be good to have a module option to override the lvds > > channel setup, e.g. lvds_channel=0 for probing BIOS like this, > > lvds_channel=1 and 2 are to set the single and dual-channel mode > > forcibly, respectively. If you guys think it's worth, I'll write an > > additional patch after fixing this as suggested. > > I think we can wait with adding debug options until this blows up. And if > there are indeed broken bios out there, we need a full quirk table anyway. The module option is often the easiest way to check, so I believe it's a good thing. It won't break the current behavior, at least. > I'll wait for the new patch with Adam's suggestion for merging into -next. I'll respin the patches tomorrow with people's suggestions. thanks, Takashi