[Intel-gfx] [PATCH] drm/i915/vbt: update DP max link rate table

[Lee, Shawn C]

On Fri, Feb 05, 2021, at 8:26 p.m, Ville Syrjälä wrote:
>On Mon, Feb 01, 2021 at 11:02:28PM +0800, Lee Shawn C wrote:
>> According to Bspec #20124, max link rate table for DP was updated at 
>> BDB version 230. Max link rate can support upto UHBR.
>> 
>> After migrate to BDB v230, the definition for LBR, HBR2 and HBR3 were 
>> changed. For backward compatibility. If BDB version was from 216 to 
>> 229. Driver have to follow original rule to configure DP max link rate 
>> value from VBT.
>> 
>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>> Cc: Imre Deak <imre.deak at intel.com>
>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>> Cc: Cooper Chiou <cooper.chiou at intel.com>
>> Cc: William Tseng <william.tseng at intel.com>
>> Signed-off-by: Lee Shawn C <shawn.c.lee at intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c     | 24 ++++++++++++++++++-
>>  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 14 +++++++----
>>  2 files changed, 32 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
>> b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 04337ac6f8c4..be1f732e6550 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1876,7 +1876,15 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv,
>>  	/* DP max link rate for CNL+ */
>>  	if (bdb_version >= 216) {
>>  		switch (child->dp_max_link_rate) {
>> -		default:
>> +		case VBT_DP_MAX_LINK_RATE_UHBR20:
>> +			info->dp_max_link_rate = 2000000;
>> +			break;
>> +		case VBT_DP_MAX_LINK_RATE_UHBR13P5:
>> +			info->dp_max_link_rate = 1350000;
>> +			break;
>> +		case VBT_DP_MAX_LINK_RATE_UHBR10:
>> +			info->dp_max_link_rate = 1000000;
>> +			break;
>>  		case VBT_DP_MAX_LINK_RATE_HBR3:
>>  			info->dp_max_link_rate = 810000;
>>  			break;
>> @@ -1889,7 +1897,21 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv,
>>  		case VBT_DP_MAX_LINK_RATE_LBR:
>>  			info->dp_max_link_rate = 162000;
>>  			break;
>> +		case VBT_DP_MAX_LINK_RATE_DEFAULT:
>> +		default:
>> +			info->dp_max_link_rate = 0;
>> +			break;
>> +		}
>> +
>> +		if (bdb_version < 230) {
>> +			if (child->dp_max_link_rate == VBT_DP_MAX_LINK_RATE_DEFAULT)
>> +				info->dp_max_link_rate = 810000;
>> +			else if (child->dp_max_link_rate == VBT_DP_MAX_LINK_RATE_LBR)
>> +				info->dp_max_link_rate = 540000;
>> +			else if (child->dp_max_link_rate == VBT_DP_MAX_LINK_RATE_HBR2)
>> +				info->dp_max_link_rate = 162000;
>>  		}
>
>I would split this into two separate functions, one does the new mapping, the other the old mapping. 
>

I will split this into two separate functions in patch v2.

>> +
>>  		drm_dbg_kms(&dev_priv->drm,
>>  			    "VBT DP max link rate for port %c: %d\n",
>>  			    port_name(port), info->dp_max_link_rate); diff --git 
>> a/drivers/gpu/drm/i915/display/intel_vbt_defs.h 
>> b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> index 6d10fa037751..578a54b33f32 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
>> @@ -343,10 +343,14 @@ enum vbt_gmbus_ddi {  #define DP_AUX_H 0x80  
>> #define DP_AUX_I 0x90
>>  
>> -#define VBT_DP_MAX_LINK_RATE_HBR3	0
>> -#define VBT_DP_MAX_LINK_RATE_HBR2	1
>> +#define VBT_DP_MAX_LINK_RATE_DEFAULT	0
>> +#define VBT_DP_MAX_LINK_RATE_LBR	1
>>  #define VBT_DP_MAX_LINK_RATE_HBR	2
>> -#define VBT_DP_MAX_LINK_RATE_LBR	3
>> +#define VBT_DP_MAX_LINK_RATE_HBR2	3
>> +#define VBT_DP_MAX_LINK_RATE_HBR3	4
>> +#define VBT_DP_MAX_LINK_RATE_UHBR10	5
>> +#define VBT_DP_MAX_LINK_RATE_UHBR13P5	6
>> +#define VBT_DP_MAX_LINK_RATE_UHBR20	7
>
>And we should keep both old and new names for these.
>
>Sadly I can't right now check the spec since it no longer has the
>old stuff apparently, and the VBT section got moved around so the
>history no longer shows anything either :( I'll have to pull the whole
>thing down I guess so I can double check against the old version.
>

Do you know any kernel doc or suggestion about the naming rule
(for new and old BDB version) to solve the problem like this?
Just want to know how i915 dirver manage the definition issue before.

Best regards,
Shawn

>>  
>>  /*
>>   * The child device config, aka the display device data structure, 
>> provides a @@ -445,8 +449,8 @@ struct child_device_config {
>>  	u16 dp_gpio_pin_num;					/* 195 */
>>  	u8 dp_iboost_level:4;					/* 196 */
>>  	u8 hdmi_iboost_level:4;					/* 196 */
>> -	u8 dp_max_link_rate:2;					/* 216 CNL+ */
>> -	u8 dp_max_link_rate_reserved:6;				/* 216 */
>> +	u8 dp_max_link_rate:3;					/* 230 */
>> +	u8 dp_max_link_rate_reserved:5;				/* 230 */
>>  } __packed;
>>  
>>  struct bdb_general_definitions {
>> --
>> 2.17.1
>
>--
>Ville Syrjälä
>Intel
>