Re: [PATCH 2/2] drm/i915: Parse max HDMI TMDS clock from VBT

[Jani Nikula <jani.nikula@linux.intel.com>]

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;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx