| Message ID | 1501606569-11821-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
+ Art, couple questions below On Tue, 2017-08-01 at 09:56 -0700, clinton.a.taylor@intel.com wrote: > From: Clint Taylor <clinton.a.taylor@intel.com> > > DDIA Lane capability control 4 lane bit is not being set by firmware during > clone mode boot. This occurs when multiple monitors are connected during > boot. The driver will configure the port for 2 lane maximum width if this > bit is not set. > > Once DDIA/E lane split is supported in vbt and the i915 driver we will need > to revisit this code. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 494fbe0..e7644b4 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2713,9 +2713,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) we would need to fix more comment lines: /* * Bspec says that DDI_A_4_LANES is the only supported configuration * for Broxton. Yet some BIOS fail to set this bit on port A if eDP * wasn't lit up at boot. Force this bit on in our internal > * configuration so that we use the proper lane count for our > * calculations. > */ But I believe the approach we currently have with this might not be optimal. If BIOS is not bringing the port A up why should we expect it to set this bit ever? We probably need to be able to do it by ourselves, without expecting others to do... However, I've never seen a production BIOS that never brings port A when it is present. And it is also true that spec tells: "This field must be programmed at system boot based on board configuration and may not be changed afterwards." So I have kind of mixed feelings here on this bit. For BXT it is so clear: "Not supported on Broxton." Also few other future cases the 0 won't be valid, but not entirely sure if this is always true. Art? I just noticed on spec that DDI E for CNL-U and CNL-Y are marked as "not supported", what means that for that SKU we should be able to add this workaround here, but at same time means that we need to remove the power wells support for DDI-E for these SKUs probably. Art, could/should we set this bit blindly for all CNL? and if yes: Art, will it always be a decision based by SKUs now on? Or should we really rely on BIOS? Is there another way to detect the port E presence? I don't believe VBT has that info in a reliable way, does it? Otherwise we would be using it already for the missed straps... Thanks, Rodrigo. > - if (IS_GEN9_LP(dev_priv) && port == PORT_A) { > + if ((IS_GEN9_LP(dev_priv) || INTEL_GEN(dev_priv) >= 10) && > + port == PORT_A) { > if (!(intel_dig_port->saved_port_bits & DDI_A_4_LANES)) { > - DRM_DEBUG_KMS("BXT BIOS forgot to set DDI_A_4_LANES for port A; fixing\n"); > + DRM_DEBUG_KMS("BIOS forgot to set DDI_A_4_LANES for port A\n"); > intel_dig_port->saved_port_bits |= DDI_A_4_LANES; > max_lanes = 4; > }
we confirmed that with current supported CNL skus using IS_CANNONLAKE is enogh for now. So feel free to change that if and use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> on the v2.... On Tue, Aug 1, 2017 at 1:23 PM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote: > + Art, couple questions below > > On Tue, 2017-08-01 at 09:56 -0700, clinton.a.taylor@intel.com wrote: >> From: Clint Taylor <clinton.a.taylor@intel.com> >> >> DDIA Lane capability control 4 lane bit is not being set by firmware during >> clone mode boot. This occurs when multiple monitors are connected during >> boot. The driver will configure the port for 2 lane maximum width if this >> bit is not set. >> >> Once DDIA/E lane split is supported in vbt and the i915 driver we will need >> to revisit this code. >> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 494fbe0..e7644b4 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2713,9 +2713,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > > we would need to fix more comment lines: > > /* > * Bspec says that DDI_A_4_LANES is the only supported > configuration > * for Broxton. Yet some BIOS fail to set this bit on port A if > eDP > * wasn't lit up at boot. Force this bit on in our internal >> * configuration so that we use the proper lane count for our >> * calculations. >> */ > > But I believe the approach we currently have with this might not be > optimal. If BIOS is not bringing the port A up why should we expect it > to set this bit ever? We probably need to be able to do it by ourselves, > without expecting others to do... > > However, I've never seen a production BIOS that never brings port A when > it is present. And it is also true that spec tells: "This field must be > programmed at system boot based on board configuration and may not be > changed afterwards." > > So I have kind of mixed feelings here on this bit. > > For BXT it is so clear: "Not supported on Broxton." > > Also few other future cases the 0 won't be valid, but not entirely sure > if this is always true. Art? > > I just noticed on spec that DDI E for CNL-U and CNL-Y are marked as "not > supported", what means that for that SKU we should be able to add this > workaround here, but at same time means that we need to remove the power > wells support for DDI-E for these SKUs probably. > > Art, could/should we set this bit blindly for all CNL? > > and if yes: > > Art, will it always be a decision based by SKUs now on? Or should we > really rely on BIOS? > > Is there another way to detect the port E presence? I don't believe VBT > has that info in a reliable way, does it? Otherwise we would be using it > already for the missed straps... > > > Thanks, > Rodrigo. > >> - if (IS_GEN9_LP(dev_priv) && port == PORT_A) { >> + if ((IS_GEN9_LP(dev_priv) || INTEL_GEN(dev_priv) >= 10) && >> + port == PORT_A) { >> if (!(intel_dig_port->saved_port_bits & DDI_A_4_LANES)) { >> - DRM_DEBUG_KMS("BXT BIOS forgot to set DDI_A_4_LANES for port A; fixing\n"); >> + DRM_DEBUG_KMS("BIOS forgot to set DDI_A_4_LANES for port A\n"); >> intel_dig_port->saved_port_bits |= DDI_A_4_LANES; >> max_lanes = 4; >> } > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 494fbe0..e7644b4 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2713,9 +2713,10 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) * configuration so that we use the proper lane count for our * calculations. */ - if (IS_GEN9_LP(dev_priv) && port == PORT_A) { + if ((IS_GEN9_LP(dev_priv) || INTEL_GEN(dev_priv) >= 10) && + port == PORT_A) { if (!(intel_dig_port->saved_port_bits & DDI_A_4_LANES)) { - DRM_DEBUG_KMS("BXT BIOS forgot to set DDI_A_4_LANES for port A; fixing\n"); + DRM_DEBUG_KMS("BIOS forgot to set DDI_A_4_LANES for port A\n"); intel_dig_port->saved_port_bits |= DDI_A_4_LANES; max_lanes = 4; }