| Message ID | 20180202122141.22746-1-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 02 Feb 2018, Mahesh Kumar <mahesh1.kumar@intel.com> wrote: > Platforms before Gen11 were sharing lanes between port-A & port-E. > This limitation is no more there. > > Changes since V1: > - optimize the code (Shashank/Jani) > - create helper function to get max lanes (ville) > Changes since V2: > - Include BIOS fail fix-up in same helper function (ville) > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 71 +++++++++++++++++++--------------------- > 1 file changed, 33 insertions(+), 38 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index cfcd9cb37d5d..ee9ba78d19c8 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2842,39 +2842,44 @@ static bool intel_ddi_a_force_4_lanes(struct intel_digital_port *dport) > return false; > } > > +static int > +intel_ddi_max_lanes(struct drm_i915_private *dev_priv, > + struct intel_digital_port *intel_dig_port) > +{ Please ditch the dev_priv parameter and add: struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev); > + enum port port = intel_dig_port->base.port; > + int max_lanes = 4; Unnecessary initialization. > + > + if (INTEL_GEN(dev_priv) >= 11) { > + return 4; Please either set max_lanes = 4 here, or remove the else. On early returns, you don't need the else. Having both is confusing. > + } else if (port == PORT_A || port == PORT_E) { > + if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) > + max_lanes = port == PORT_A ? 4 : 0; > + else > + /* Both A and E share 2 lanes */ > + max_lanes = 2; > + } > + > + /* > + * Some BIOS might fail to set this bit on port A if eDP > + * wasn't lit up at boot. Force this bit set when needed > + * so we use the proper lane count for our calculations. > + */ > + if (intel_ddi_a_force_4_lanes(intel_dig_port)) { > + DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); > + intel_dig_port->saved_port_bits |= DDI_A_4_LANES; > + max_lanes = 4; > + } > + > + return max_lanes; > +} > + > void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > { > struct intel_digital_port *intel_dig_port; > struct intel_encoder *intel_encoder; > struct drm_encoder *encoder; > bool init_hdmi, init_dp, init_lspcon = false; > - int max_lanes; > > - if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) { > - switch (port) { > - case PORT_A: > - max_lanes = 4; > - break; > - case PORT_E: > - max_lanes = 0; > - break; > - default: > - max_lanes = 4; > - break; > - } > - } else { > - switch (port) { > - case PORT_A: > - max_lanes = 2; > - break; > - case PORT_E: > - max_lanes = 2; > - break; > - default: > - max_lanes = 4; > - break; > - } > - } > > init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || > dev_priv->vbt.ddi_port_info[port].supports_hdmi); > @@ -2954,19 +2959,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > MISSING_CASE(port); > } > > - /* > - * Some BIOS might fail to set this bit on port A if eDP > - * wasn't lit up at boot. Force this bit set when needed > - * so we use the proper lane count for our calculations. > - */ > - if (intel_ddi_a_force_4_lanes(intel_dig_port)) { > - DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); > - intel_dig_port->saved_port_bits |= DDI_A_4_LANES; > - max_lanes = 4; > - } > - > intel_dig_port->dp.output_reg = INVALID_MMIO_REG; > - intel_dig_port->max_lanes = max_lanes; > > intel_encoder->type = INTEL_OUTPUT_DDI; > intel_encoder->power_domain = intel_port_to_power_domain(port); > @@ -2974,6 +2967,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) > intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); > intel_encoder->cloneable = 0; > > + intel_dig_port->max_lanes = intel_ddi_max_lanes(dev_priv, > + intel_dig_port); Please keep this at the original location above. BR, Jani. > intel_infoframe_init(intel_dig_port); > > if (init_dp) {
Hi, On 2/2/2018 6:26 PM, Jani Nikula wrote: > On Fri, 02 Feb 2018, Mahesh Kumar <mahesh1.kumar@intel.com> wrote: >> Platforms before Gen11 were sharing lanes between port-A & port-E. >> This limitation is no more there. >> >> Changes since V1: >> - optimize the code (Shashank/Jani) >> - create helper function to get max lanes (ville) >> Changes since V2: >> - Include BIOS fail fix-up in same helper function (ville) >> >> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 71 +++++++++++++++++++--------------------- >> 1 file changed, 33 insertions(+), 38 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index cfcd9cb37d5d..ee9ba78d19c8 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -2842,39 +2842,44 @@ static bool intel_ddi_a_force_4_lanes(struct intel_digital_port *dport) >> return false; >> } >> >> +static int >> +intel_ddi_max_lanes(struct drm_i915_private *dev_priv, >> + struct intel_digital_port *intel_dig_port) >> +{ > Please ditch the dev_priv parameter and add: > > struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev); ok, sure. > >> + enum port port = intel_dig_port->base.port; >> + int max_lanes = 4; > Unnecessary initialization. for ports other than PORT_A/E will have max_lanes=4 that's the reason initializing it here, will fix the usages. > >> + >> + if (INTEL_GEN(dev_priv) >= 11) { >> + return 4; > Please either set max_lanes = 4 here, or remove the else. On early > returns, you don't need the else. Having both is confusing. yes, agree, will remove the else > >> + } else if (port == PORT_A || port == PORT_E) { >> + if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) >> + max_lanes = port == PORT_A ? 4 : 0; >> + else >> + /* Both A and E share 2 lanes */ >> + max_lanes = 2; >> + } >> + >> + /* >> + * Some BIOS might fail to set this bit on port A if eDP >> + * wasn't lit up at boot. Force this bit set when needed >> + * so we use the proper lane count for our calculations. >> + */ >> + if (intel_ddi_a_force_4_lanes(intel_dig_port)) { >> + DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); >> + intel_dig_port->saved_port_bits |= DDI_A_4_LANES; >> + max_lanes = 4; >> + } >> + >> + return max_lanes; >> +} >> + >> void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) >> { >> struct intel_digital_port *intel_dig_port; >> struct intel_encoder *intel_encoder; >> struct drm_encoder *encoder; >> bool init_hdmi, init_dp, init_lspcon = false; >> - int max_lanes; >> >> - if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) { >> - switch (port) { >> - case PORT_A: >> - max_lanes = 4; >> - break; >> - case PORT_E: >> - max_lanes = 0; >> - break; >> - default: >> - max_lanes = 4; >> - break; >> - } >> - } else { >> - switch (port) { >> - case PORT_A: >> - max_lanes = 2; >> - break; >> - case PORT_E: >> - max_lanes = 2; >> - break; >> - default: >> - max_lanes = 4; >> - break; >> - } >> - } >> >> init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || >> dev_priv->vbt.ddi_port_info[port].supports_hdmi); >> @@ -2954,19 +2959,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) >> MISSING_CASE(port); >> } >> >> - /* >> - * Some BIOS might fail to set this bit on port A if eDP >> - * wasn't lit up at boot. Force this bit set when needed >> - * so we use the proper lane count for our calculations. >> - */ >> - if (intel_ddi_a_force_4_lanes(intel_dig_port)) { >> - DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); >> - intel_dig_port->saved_port_bits |= DDI_A_4_LANES; >> - max_lanes = 4; >> - } >> - >> intel_dig_port->dp.output_reg = INVALID_MMIO_REG; >> - intel_dig_port->max_lanes = max_lanes; >> >> intel_encoder->type = INTEL_OUTPUT_DDI; >> intel_encoder->power_domain = intel_port_to_power_domain(port); >> @@ -2974,6 +2967,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) >> intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); >> intel_encoder->cloneable = 0; >> >> + intel_dig_port->max_lanes = intel_ddi_max_lanes(dev_priv, >> + intel_dig_port); > Please keep this at the original location above. I moved this here because intel_encoder->port was not initialized in original location, will pass port along with intel_dig_port & move it to original location. Thanks for review. -Mahesh > > BR, > Jani. > >> intel_infoframe_init(intel_dig_port); >> >> if (init_dp) {
On Mon, 05 Feb 2018, "Kumar, Mahesh" <mahesh1.kumar@intel.com> wrote: > Hi, > > > On 2/2/2018 6:26 PM, Jani Nikula wrote: >> On Fri, 02 Feb 2018, Mahesh Kumar <mahesh1.kumar@intel.com> wrote: >>> Platforms before Gen11 were sharing lanes between port-A & port-E. >>> This limitation is no more there. >>> >>> Changes since V1: >>> - optimize the code (Shashank/Jani) >>> - create helper function to get max lanes (ville) >>> Changes since V2: >>> - Include BIOS fail fix-up in same helper function (ville) >>> >>> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> >>> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_ddi.c | 71 +++++++++++++++++++--------------------- >>> 1 file changed, 33 insertions(+), 38 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>> index cfcd9cb37d5d..ee9ba78d19c8 100644 >>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>> @@ -2842,39 +2842,44 @@ static bool intel_ddi_a_force_4_lanes(struct intel_digital_port *dport) >>> return false; >>> } >>> >>> +static int >>> +intel_ddi_max_lanes(struct drm_i915_private *dev_priv, >>> + struct intel_digital_port *intel_dig_port) >>> +{ >> Please ditch the dev_priv parameter and add: >> >> struct drm_i915_private *dev_priv = to_i915(dport->base.base.dev); > ok, sure. >> >>> + enum port port = intel_dig_port->base.port; >>> + int max_lanes = 4; >> Unnecessary initialization. > for ports other than PORT_A/E will have max_lanes=4 that's the reason > initializing it here, will fix the usages. >> >>> + >>> + if (INTEL_GEN(dev_priv) >= 11) { >>> + return 4; >> Please either set max_lanes = 4 here, or remove the else. On early >> returns, you don't need the else. Having both is confusing. > yes, agree, will remove the else >> >>> + } else if (port == PORT_A || port == PORT_E) { >>> + if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) >>> + max_lanes = port == PORT_A ? 4 : 0; >>> + else >>> + /* Both A and E share 2 lanes */ >>> + max_lanes = 2; >>> + } >>> + >>> + /* >>> + * Some BIOS might fail to set this bit on port A if eDP >>> + * wasn't lit up at boot. Force this bit set when needed >>> + * so we use the proper lane count for our calculations. >>> + */ >>> + if (intel_ddi_a_force_4_lanes(intel_dig_port)) { >>> + DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); >>> + intel_dig_port->saved_port_bits |= DDI_A_4_LANES; >>> + max_lanes = 4; >>> + } >>> + >>> + return max_lanes; >>> +} >>> + >>> void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) >>> { >>> struct intel_digital_port *intel_dig_port; >>> struct intel_encoder *intel_encoder; >>> struct drm_encoder *encoder; >>> bool init_hdmi, init_dp, init_lspcon = false; >>> - int max_lanes; >>> >>> - if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) { >>> - switch (port) { >>> - case PORT_A: >>> - max_lanes = 4; >>> - break; >>> - case PORT_E: >>> - max_lanes = 0; >>> - break; >>> - default: >>> - max_lanes = 4; >>> - break; >>> - } >>> - } else { >>> - switch (port) { >>> - case PORT_A: >>> - max_lanes = 2; >>> - break; >>> - case PORT_E: >>> - max_lanes = 2; >>> - break; >>> - default: >>> - max_lanes = 4; >>> - break; >>> - } >>> - } >>> >>> init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || >>> dev_priv->vbt.ddi_port_info[port].supports_hdmi); >>> @@ -2954,19 +2959,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) >>> MISSING_CASE(port); >>> } >>> >>> - /* >>> - * Some BIOS might fail to set this bit on port A if eDP >>> - * wasn't lit up at boot. Force this bit set when needed >>> - * so we use the proper lane count for our calculations. >>> - */ >>> - if (intel_ddi_a_force_4_lanes(intel_dig_port)) { >>> - DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); >>> - intel_dig_port->saved_port_bits |= DDI_A_4_LANES; >>> - max_lanes = 4; >>> - } >>> - >>> intel_dig_port->dp.output_reg = INVALID_MMIO_REG; >>> - intel_dig_port->max_lanes = max_lanes; >>> >>> intel_encoder->type = INTEL_OUTPUT_DDI; >>> intel_encoder->power_domain = intel_port_to_power_domain(port); >>> @@ -2974,6 +2967,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) >>> intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); >>> intel_encoder->cloneable = 0; >>> >>> + intel_dig_port->max_lanes = intel_ddi_max_lanes(dev_priv, >>> + intel_dig_port); >> Please keep this at the original location above. > I moved this here because intel_encoder->port was not initialized in > original location, will pass port along with intel_dig_port & move it to > original location. Right, so alternatively you could move all the intel_dig_port init to the same place. BR, Jani. > Thanks for review. > > -Mahesh >> >> BR, >> Jani. >> >>> intel_infoframe_init(intel_dig_port); >>> >>> if (init_dp) { >
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index cfcd9cb37d5d..ee9ba78d19c8 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2842,39 +2842,44 @@ static bool intel_ddi_a_force_4_lanes(struct intel_digital_port *dport) return false; } +static int +intel_ddi_max_lanes(struct drm_i915_private *dev_priv, + struct intel_digital_port *intel_dig_port) +{ + enum port port = intel_dig_port->base.port; + int max_lanes = 4; + + if (INTEL_GEN(dev_priv) >= 11) { + return 4; + } else if (port == PORT_A || port == PORT_E) { + if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) + max_lanes = port == PORT_A ? 4 : 0; + else + /* Both A and E share 2 lanes */ + max_lanes = 2; + } + + /* + * Some BIOS might fail to set this bit on port A if eDP + * wasn't lit up at boot. Force this bit set when needed + * so we use the proper lane count for our calculations. + */ + if (intel_ddi_a_force_4_lanes(intel_dig_port)) { + DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); + intel_dig_port->saved_port_bits |= DDI_A_4_LANES; + max_lanes = 4; + } + + return max_lanes; +} + void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) { struct intel_digital_port *intel_dig_port; struct intel_encoder *intel_encoder; struct drm_encoder *encoder; bool init_hdmi, init_dp, init_lspcon = false; - int max_lanes; - if (I915_READ(DDI_BUF_CTL(PORT_A)) & DDI_A_4_LANES) { - switch (port) { - case PORT_A: - max_lanes = 4; - break; - case PORT_E: - max_lanes = 0; - break; - default: - max_lanes = 4; - break; - } - } else { - switch (port) { - case PORT_A: - max_lanes = 2; - break; - case PORT_E: - max_lanes = 2; - break; - default: - max_lanes = 4; - break; - } - } init_hdmi = (dev_priv->vbt.ddi_port_info[port].supports_dvi || dev_priv->vbt.ddi_port_info[port].supports_hdmi); @@ -2954,19 +2959,7 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) MISSING_CASE(port); } - /* - * Some BIOS might fail to set this bit on port A if eDP - * wasn't lit up at boot. Force this bit set when needed - * so we use the proper lane count for our calculations. - */ - if (intel_ddi_a_force_4_lanes(intel_dig_port)) { - DRM_DEBUG_KMS("Forcing DDI_A_4_LANES for port A\n"); - intel_dig_port->saved_port_bits |= DDI_A_4_LANES; - max_lanes = 4; - } - intel_dig_port->dp.output_reg = INVALID_MMIO_REG; - intel_dig_port->max_lanes = max_lanes; intel_encoder->type = INTEL_OUTPUT_DDI; intel_encoder->power_domain = intel_port_to_power_domain(port); @@ -2974,6 +2967,8 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port) intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2); intel_encoder->cloneable = 0; + intel_dig_port->max_lanes = intel_ddi_max_lanes(dev_priv, + intel_dig_port); intel_infoframe_init(intel_dig_port); if (init_dp) {