| Message ID | 20171026151405.30710-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ville Syrjala (2017-10-26 16:14:05) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Starting from version 204 VBT can specify the max TMDS clock we are > allowed to use with HDMI ports. Parse that information and take it > into account when filtering modes and computing a crtc state. > > + if (bdb_version >= 204) { > + int max_tmds_clock; > + > + switch (child->hdmi_max_data_rate) { > + case 1: > + max_tmds_clock = 297000; > + break; > + case 2: > + max_tmds_clock = 165000; > + break; > + default: > + max_tmds_clock = 0; Is zero a valid value, as this will prevent us from using the output at all? -Chris
On Thu, Oct 26, 2017 at 04:44:39PM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2017-10-26 16:14:05) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Starting from version 204 VBT can specify the max TMDS clock we are > > allowed to use with HDMI ports. Parse that information and take it > > into account when filtering modes and computing a crtc state. > > > > + if (bdb_version >= 204) { > > + int max_tmds_clock; > > + > > + switch (child->hdmi_max_data_rate) { > > + case 1: > > + max_tmds_clock = 297000; > > + break; > > + case 2: > > + max_tmds_clock = 165000; > > + break; > > + default: > > + max_tmds_clock = 0; > > Is zero a valid value, as this will prevent us from using the output > at all? Zero means "unlimited" or "use the platform default maximum". The code using this will check to make sure it's non-zero before doing the min(). We use similar logic in hdmi_port_clock_limit() to handle the sink and dongle limits.
Quoting Ville Syrjälä (2017-10-26 16:51:23) > On Thu, Oct 26, 2017 at 04:44:39PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjala (2017-10-26 16:14:05) > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Starting from version 204 VBT can specify the max TMDS clock we are > > > allowed to use with HDMI ports. Parse that information and take it > > > into account when filtering modes and computing a crtc state. > > > > > > + if (bdb_version >= 204) { > > > + int max_tmds_clock; > > > + > > > + switch (child->hdmi_max_data_rate) { > > > + case 1: > > > + max_tmds_clock = 297000; > > > + break; > > > + case 2: > > > + max_tmds_clock = 165000; > > > + break; > > > + default: > > > + max_tmds_clock = 0; > > > > Is zero a valid value, as this will prevent us from using the output > > at all? > > Zero means "unlimited" or "use the platform default maximum". The code > using this will check to make sure it's non-zero before doing the min(). Indeed, I didn't look at the guard closely enough, just expected it to be a if (use_vbt_flag) and not if (max_tmds_clock). -Chris
On Thu, 26 Oct 2017, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Starting from version 204 VBT can specify the max TMDS clock we are > allowed to use with HDMI ports. Parse that information and take it > into account when filtering modes and computing a crtc state. > > Also take the opportunity to sort the platform check if ladder > from new to old. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_bios.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/i915/intel_hdmi.c | 30 ++++++++++++++++++++---------- > 3 files changed, 42 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 366ba74b0ad2..45d32a95ce4a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1698,6 +1698,8 @@ enum modeset_restore { > #define DDC_PIN_D 0x06 > > struct ddi_vbt_port_info { > + int max_tmds_clock; > + > /* > * This is an index in the HDMI/DVI DDI buffer translation table. > * The special value HDMI_LEVEL_SHIFT_UNKNOWN means the VBT didn't > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index fd23023df7c1..a0df8e3fefbe 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -1234,6 +1234,26 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port, > info->hdmi_level_shift = hdmi_level_shift; > } > > + if (bdb_version >= 204) { > + int max_tmds_clock; > + > + switch (child->hdmi_max_data_rate) { > + case 1: > + max_tmds_clock = 297000; > + break; > + case 2: > + max_tmds_clock = 165000; > + break; > + default: > + max_tmds_clock = 0; Please add the case values to intel_vbt_defs.h for documentation and reuse in intel_vbt_decode. Please add debug message about values other than 0, 1, or 2, i.e. a separate default case with fallback to 0. With that fixed this is Reviewed-by: Jani Nikula <jani.nikula@intel.com> but read on for some comments. > + break; > + } > + > + DRM_DEBUG_KMS("VBT HDMI max TMDS clock for port %c: %d kHz\n", > + port_name(port), max_tmds_clock); 0 will be most common leading to a funny message... > + info->max_tmds_clock = max_tmds_clock; > + } > + > /* Parse the I_boost config for SKL and above */ > if (bdb_version >= 196 && child->iboost) { > info->dp_boost_level = translate_iboost(child->dp_iboost_level); > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index aa486b8925cf..38fe24565b4d 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1224,24 +1224,34 @@ static void pch_post_disable_hdmi(struct intel_encoder *encoder, > intel_disable_hdmi(encoder, old_crtc_state, old_conn_state); > } > > -static int intel_hdmi_source_max_tmds_clock(struct drm_i915_private *dev_priv) > +static int intel_hdmi_source_max_tmds_clock(struct intel_encoder *encoder) > { > - if (IS_G4X(dev_priv)) > - return 165000; > - else if (IS_GEMINILAKE(dev_priv)) > - return 594000; > - else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8) > - return 300000; > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + const struct ddi_vbt_port_info *info = > + &dev_priv->vbt.ddi_port_info[encoder->port]; > + int max_tmds_clock; > + > + if (IS_GEMINILAKE(dev_priv)) > + max_tmds_clock = 594000; Hmm, are we missing Cannonlake here? If we are, that's a separate patch anyway. > + else if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) > + max_tmds_clock = 300000; > + else if (INTEL_GEN(dev_priv) >= 5) > + max_tmds_clock = 225000; > else > - return 225000; > + max_tmds_clock = 165000; > + > + if (info->max_tmds_clock) > + max_tmds_clock = min(max_tmds_clock, info->max_tmds_clock); > + > + return max_tmds_clock; > } > > static int hdmi_port_clock_limit(struct intel_hdmi *hdmi, > bool respect_downstream_limits, > bool force_dvi) > { > - struct drm_device *dev = intel_hdmi_to_dev(hdmi); > - int max_tmds_clock = intel_hdmi_source_max_tmds_clock(to_i915(dev)); > + struct intel_encoder *encoder = &hdmi_to_dig_port(hdmi)->base; > + int max_tmds_clock = intel_hdmi_source_max_tmds_clock(encoder); > > if (respect_downstream_limits) { > struct intel_connector *connector = hdmi->attached_connector;
On Fri, 27 Oct 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 26 Oct 2017, Ville Syrjala <ville.syrjala@linux.intel.com> wrote: >> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Starting from version 204 VBT can specify the max TMDS clock we are >> allowed to use with HDMI ports. Parse that information and take it >> into account when filtering modes and computing a crtc state. >> >> Also take the opportunity to sort the platform check if ladder >> from new to old. >> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/intel_bios.c | 20 ++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_hdmi.c | 30 ++++++++++++++++++++---------- >> 3 files changed, 42 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 366ba74b0ad2..45d32a95ce4a 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1698,6 +1698,8 @@ enum modeset_restore { >> #define DDC_PIN_D 0x06 >> >> struct ddi_vbt_port_info { >> + int max_tmds_clock; >> + >> /* >> * This is an index in the HDMI/DVI DDI buffer translation table. >> * The special value HDMI_LEVEL_SHIFT_UNKNOWN means the VBT didn't >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c >> index fd23023df7c1..a0df8e3fefbe 100644 >> --- a/drivers/gpu/drm/i915/intel_bios.c >> +++ b/drivers/gpu/drm/i915/intel_bios.c >> @@ -1234,6 +1234,26 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port, >> info->hdmi_level_shift = hdmi_level_shift; >> } >> >> + if (bdb_version >= 204) { >> + int max_tmds_clock; >> + >> + switch (child->hdmi_max_data_rate) { >> + case 1: >> + max_tmds_clock = 297000; >> + break; >> + case 2: >> + max_tmds_clock = 165000; >> + break; >> + default: >> + max_tmds_clock = 0; > > Please add the case values to intel_vbt_defs.h for documentation and > reuse in intel_vbt_decode. Please add debug message about values other > than 0, 1, or 2, i.e. a separate default case with fallback to 0. > > With that fixed this is > > Reviewed-by: Jani Nikula <jani.nikula@intel.com> Hold your horses with that! We have bogus field widths for level shift and max data rate in the child device config! Data rate is supposed to be 3 bits 7:5 and level shift 5 bits 4:0. Please fix that first! I don't think this should have an impact, because the defined values should fit in 4 bits. But this would have a worse and more surprising impact on the max data rate. The wrong mask was originally introduced in 6acab15a7b0d ("drm/i915: use the HDMI DDI buffer translations from VBT") and we started using the info in 96fb9f9b154a ("drm/i915/bxt: VSwing programming sequence"). I'm a bit suprised I missed this when I reworked the structures. BR, Jani. > > but read on for some comments. > >> + break; >> + } >> + >> + DRM_DEBUG_KMS("VBT HDMI max TMDS clock for port %c: %d kHz\n", >> + port_name(port), max_tmds_clock); > > 0 will be most common leading to a funny message... > >> + info->max_tmds_clock = max_tmds_clock; >> + } >> + >> /* Parse the I_boost config for SKL and above */ >> if (bdb_version >= 196 && child->iboost) { >> info->dp_boost_level = translate_iboost(child->dp_iboost_level); >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index aa486b8925cf..38fe24565b4d 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1224,24 +1224,34 @@ static void pch_post_disable_hdmi(struct intel_encoder *encoder, >> intel_disable_hdmi(encoder, old_crtc_state, old_conn_state); >> } >> >> -static int intel_hdmi_source_max_tmds_clock(struct drm_i915_private *dev_priv) >> +static int intel_hdmi_source_max_tmds_clock(struct intel_encoder *encoder) >> { >> - if (IS_G4X(dev_priv)) >> - return 165000; >> - else if (IS_GEMINILAKE(dev_priv)) >> - return 594000; >> - else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8) >> - return 300000; >> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> + const struct ddi_vbt_port_info *info = >> + &dev_priv->vbt.ddi_port_info[encoder->port]; >> + int max_tmds_clock; >> + >> + if (IS_GEMINILAKE(dev_priv)) >> + max_tmds_clock = 594000; > > Hmm, are we missing Cannonlake here? If we are, that's a separate patch > anyway. > >> + else if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) >> + max_tmds_clock = 300000; >> + else if (INTEL_GEN(dev_priv) >= 5) >> + max_tmds_clock = 225000; >> else >> - return 225000; >> + max_tmds_clock = 165000; >> + >> + if (info->max_tmds_clock) >> + max_tmds_clock = min(max_tmds_clock, info->max_tmds_clock); >> + >> + return max_tmds_clock; >> } >> >> static int hdmi_port_clock_limit(struct intel_hdmi *hdmi, >> bool respect_downstream_limits, >> bool force_dvi) >> { >> - struct drm_device *dev = intel_hdmi_to_dev(hdmi); >> - int max_tmds_clock = intel_hdmi_source_max_tmds_clock(to_i915(dev)); >> + struct intel_encoder *encoder = &hdmi_to_dig_port(hdmi)->base; >> + int max_tmds_clock = intel_hdmi_source_max_tmds_clock(encoder); >> >> if (respect_downstream_limits) { >> struct intel_connector *connector = hdmi->attached_connector;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 366ba74b0ad2..45d32a95ce4a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1698,6 +1698,8 @@ enum modeset_restore { #define DDC_PIN_D 0x06 struct ddi_vbt_port_info { + int max_tmds_clock; + /* * This is an index in the HDMI/DVI DDI buffer translation table. * The special value HDMI_LEVEL_SHIFT_UNKNOWN means the VBT didn't diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index fd23023df7c1..a0df8e3fefbe 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1234,6 +1234,26 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port, info->hdmi_level_shift = hdmi_level_shift; } + if (bdb_version >= 204) { + int max_tmds_clock; + + switch (child->hdmi_max_data_rate) { + case 1: + max_tmds_clock = 297000; + break; + case 2: + max_tmds_clock = 165000; + break; + default: + max_tmds_clock = 0; + break; + } + + DRM_DEBUG_KMS("VBT HDMI max TMDS clock for port %c: %d kHz\n", + port_name(port), max_tmds_clock); + info->max_tmds_clock = max_tmds_clock; + } + /* Parse the I_boost config for SKL and above */ if (bdb_version >= 196 && child->iboost) { info->dp_boost_level = translate_iboost(child->dp_iboost_level); diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index aa486b8925cf..38fe24565b4d 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1224,24 +1224,34 @@ static void pch_post_disable_hdmi(struct intel_encoder *encoder, intel_disable_hdmi(encoder, old_crtc_state, old_conn_state); } -static int intel_hdmi_source_max_tmds_clock(struct drm_i915_private *dev_priv) +static int intel_hdmi_source_max_tmds_clock(struct intel_encoder *encoder) { - if (IS_G4X(dev_priv)) - return 165000; - else if (IS_GEMINILAKE(dev_priv)) - return 594000; - else if (IS_HASWELL(dev_priv) || INTEL_INFO(dev_priv)->gen >= 8) - return 300000; + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + const struct ddi_vbt_port_info *info = + &dev_priv->vbt.ddi_port_info[encoder->port]; + int max_tmds_clock; + + if (IS_GEMINILAKE(dev_priv)) + max_tmds_clock = 594000; + else if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) + max_tmds_clock = 300000; + else if (INTEL_GEN(dev_priv) >= 5) + max_tmds_clock = 225000; else - return 225000; + max_tmds_clock = 165000; + + if (info->max_tmds_clock) + max_tmds_clock = min(max_tmds_clock, info->max_tmds_clock); + + return max_tmds_clock; } static int hdmi_port_clock_limit(struct intel_hdmi *hdmi, bool respect_downstream_limits, bool force_dvi) { - struct drm_device *dev = intel_hdmi_to_dev(hdmi); - int max_tmds_clock = intel_hdmi_source_max_tmds_clock(to_i915(dev)); + struct intel_encoder *encoder = &hdmi_to_dig_port(hdmi)->base; + int max_tmds_clock = intel_hdmi_source_max_tmds_clock(encoder); if (respect_downstream_limits) { struct intel_connector *connector = hdmi->attached_connector;