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

[Lee, Shawn C]

On Wed, Feb 17, 2021 at 3:45 p.m., Ville Syrjälä wrote:
>On Wed, Feb 17, 2021 at 11:39:35PM +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.
>> 
>> v2: split the mapping table to two for old and new BDB definition.
>> v3: return link rate instead of assigning it.
>> 
>> Cc: Ville Syrjala 
>> Cc: Imre Deak 
>> Cc: Jani Nikula 
>> Cc: Cooper Chiou 
>> Cc: William Tseng 
>> Signed-off-by: Lee Shawn C 
>> ---
>>  drivers/gpu/drm/i915/display/intel_bios.c | 78 +++
>>  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 23 --
>>  2 files changed, 80 insertions(+), 21 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
>> b/drivers/gpu/drm/i915/display/intel_bios.c
>> index 7902d4c2673e..d8305c351b77 100644
>> --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> @@ -1759,6 +1759,64 @@ static enum port dvo_port_to_port(struct 
>> drm_i915_private *dev_priv,
>>dvo_port);
>>  }
>>  
>> +static int parse_bdb_230_dp_max_link_rate(const int 
>> +vbt_max_link_rate) {
>> +int link_rate;
>
>That variable is rather pointless...
>
>> +
>> +switch (vbt_max_link_rate) {
>> +case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR20:
>> +link_rate = 200;
>> +break;
>
>... when you can just 'return ' here directly.
>Would reduce the noise a bit as well since the break statements would vanish.
>

Update patch v4 and just return the value as you mentioned. 
Please help to review again.

BTW,  I'm sorry I press "test again" button two times and waste test machine 
resource.

Best regards,
Shawn

>> +case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR13P5:
>> +link_rate = 135;
>> +break;
>> +case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR10:
>> +link_rate = 100;
>> +break;
>> +case BDB_230_VBT_DP_MAX_LINK_RATE_HBR3:
>> +link_rate = 81;
>> +break;
>> +case BDB_230_VBT_DP_MAX_LINK_RATE_HBR2:
>> +link_rate = 54;
>> +break;
>> +case BDB_230_VBT_DP_MAX_LINK_RATE_HBR:
>> +link_rate = 27;
>> +break;
>> +case BDB_230_VBT_DP_MAX_LINK_RATE_LBR:
>> +link_rate = 162000;
>> +break;
>> +case BDB_230_VBT_DP_MAX_LINK_RATE_DEF:
>> +default:
>> +link_rate = 0;
>> +break;
>> +}
>> +
>> +return link_rate;
>> +}
>> +
>> +static int parse_bdb_216_dp_max_link_rate(const int 
>> +vbt_max_link_rate) {
>> +int link_rate;
>
>Same here.
>
>With that changed this is
>Reviewed-by: Ville Syrjälä 
>
>> +
>> +switch (vbt_max_link_rate) {
>> +default:
>> +case BDB_216_VBT_DP_MAX_LINK_RATE_HBR3:
>> +link_rate = 81;
>> +break;
>> +case BDB_216_VBT_DP_MAX_LINK_RATE_HBR2:
>> +link_rate = 54;
>> +break;
>> +case BDB_216_VBT_DP_MAX_LINK_RATE_HBR:
>> +link_rate = 27;
>> +break;
>> +case BDB_216_VBT_DP_MAX_LINK_RATE_LBR:
>> +link_rate = 162000;
>> +break;
>> +}
>> +
>> +return link_rate;
>> +}
>> +
>>  static void parse_ddi_port(struct drm_i915_private *dev_priv,
>> struct display_device_data *devdata,
>> u8 bdb_version)
>> @@ -1884,21 +1942,11 @@ 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_HBR3:
>> -info->dp_max_link_rate = 81;
>> -break;
>> -case VBT_DP_MAX_LINK_RATE_HBR2:
>> -info->dp_max_link_rate = 54;
>> -break;
>> -case VBT_DP_MAX_LINK_RATE_HBR:
>> -info->dp_max_link_rate = 27;
>> -break;
>> -case VBT_DP_MAX_LINK_RATE_LBR:
>> -info->dp_max_link_rate = 162000;
>> -break;
>> -}
>> +if (bdb_version >= 230)
>> +info->dp_max_link_rate = 
>> parse_bdb_230_dp_max_link_rate(child->dp_max_link_rate);
>> +else
>> +info->dp_max_link_rate = 
>> +parse_bdb_216_dp_max_link_rate(child->dp_max_link_rate);
>> +
>>  drm_dbg_kms(_priv->drm,
>>  "Port %c VBT DP max link rate: %d\n",
>>  port_name(port), info->dp_max_link_rate); diff 
>> --git 
>> 

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

[Ville Syrjälä]

On Wed, Feb 17, 2021 at 11:39:35PM +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.
> 
> v2: split the mapping table to two for old and new BDB definition.
> v3: return link rate instead of assigning it.
> 
> Cc: Ville Syrjala 
> Cc: Imre Deak 
> Cc: Jani Nikula 
> Cc: Cooper Chiou 
> Cc: William Tseng 
> Signed-off-by: Lee Shawn C 
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 78 +++
>  drivers/gpu/drm/i915/display/intel_vbt_defs.h | 23 --
>  2 files changed, 80 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index 7902d4c2673e..d8305c351b77 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -1759,6 +1759,64 @@ static enum port dvo_port_to_port(struct 
> drm_i915_private *dev_priv,
> dvo_port);
>  }
>  
> +static int parse_bdb_230_dp_max_link_rate(const int vbt_max_link_rate)
> +{
> + int link_rate;

That variable is rather pointless...

> +
> + switch (vbt_max_link_rate) {
> + case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR20:
> + link_rate = 200;
> + break;

... when you can just 'return ' here directly.
Would reduce the noise a bit as well since the break
statements would vanish.

> + case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR13P5:
> + link_rate = 135;
> + break;
> + case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR10:
> + link_rate = 100;
> + break;
> + case BDB_230_VBT_DP_MAX_LINK_RATE_HBR3:
> + link_rate = 81;
> + break;
> + case BDB_230_VBT_DP_MAX_LINK_RATE_HBR2:
> + link_rate = 54;
> + break;
> + case BDB_230_VBT_DP_MAX_LINK_RATE_HBR:
> + link_rate = 27;
> + break;
> + case BDB_230_VBT_DP_MAX_LINK_RATE_LBR:
> + link_rate = 162000;
> + break;
> + case BDB_230_VBT_DP_MAX_LINK_RATE_DEF:
> + default:
> + link_rate = 0;
> + break;
> + }
> +
> + return link_rate;
> +}
> +
> +static int parse_bdb_216_dp_max_link_rate(const int vbt_max_link_rate)
> +{
> + int link_rate;

Same here.

With that changed this is
Reviewed-by: Ville Syrjälä 

> +
> + switch (vbt_max_link_rate) {
> + default:
> + case BDB_216_VBT_DP_MAX_LINK_RATE_HBR3:
> + link_rate = 81;
> + break;
> + case BDB_216_VBT_DP_MAX_LINK_RATE_HBR2:
> + link_rate = 54;
> + break;
> + case BDB_216_VBT_DP_MAX_LINK_RATE_HBR:
> + link_rate = 27;
> + break;
> + case BDB_216_VBT_DP_MAX_LINK_RATE_LBR:
> + link_rate = 162000;
> + break;
> + }
> +
> + return link_rate;
> +}
> +
>  static void parse_ddi_port(struct drm_i915_private *dev_priv,
>  struct display_device_data *devdata,
>  u8 bdb_version)
> @@ -1884,21 +1942,11 @@ 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_HBR3:
> - info->dp_max_link_rate = 81;
> - break;
> - case VBT_DP_MAX_LINK_RATE_HBR2:
> - info->dp_max_link_rate = 54;
> - break;
> - case VBT_DP_MAX_LINK_RATE_HBR:
> - info->dp_max_link_rate = 27;
> - break;
> - case VBT_DP_MAX_LINK_RATE_LBR:
> - info->dp_max_link_rate = 162000;
> - break;
> - }
> + if (bdb_version >= 230)
> + info->dp_max_link_rate = 
> parse_bdb_230_dp_max_link_rate(child->dp_max_link_rate);
> + else
> + info->dp_max_link_rate = 
> parse_bdb_216_dp_max_link_rate(child->dp_max_link_rate);
> +
>   drm_dbg_kms(_priv->drm,
>   "Port %c VBT DP max link rate: %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..0d80b04b34be 100644
> --- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> +++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
> @@ -343,10 +343,21 @@ enum vbt_gmbus_ddi {
>  #define DP_AUX_H 0x80
>  #define 

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

[Lee Shawn C]

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.

v2: split the mapping table to two for old and new BDB definition.
v3: return link rate instead of assigning it.

Cc: Ville Syrjala 
Cc: Imre Deak 
Cc: Jani Nikula 
Cc: Cooper Chiou 
Cc: William Tseng 
Signed-off-by: Lee Shawn C 
---
 drivers/gpu/drm/i915/display/intel_bios.c | 78 +++
 drivers/gpu/drm/i915/display/intel_vbt_defs.h | 23 --
 2 files changed, 80 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
b/drivers/gpu/drm/i915/display/intel_bios.c
index 7902d4c2673e..d8305c351b77 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -1759,6 +1759,64 @@ static enum port dvo_port_to_port(struct 
drm_i915_private *dev_priv,
  dvo_port);
 }
 
+static int parse_bdb_230_dp_max_link_rate(const int vbt_max_link_rate)
+{
+   int link_rate;
+
+   switch (vbt_max_link_rate) {
+   case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR20:
+   link_rate = 200;
+   break;
+   case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR13P5:
+   link_rate = 135;
+   break;
+   case BDB_230_VBT_DP_MAX_LINK_RATE_UHBR10:
+   link_rate = 100;
+   break;
+   case BDB_230_VBT_DP_MAX_LINK_RATE_HBR3:
+   link_rate = 81;
+   break;
+   case BDB_230_VBT_DP_MAX_LINK_RATE_HBR2:
+   link_rate = 54;
+   break;
+   case BDB_230_VBT_DP_MAX_LINK_RATE_HBR:
+   link_rate = 27;
+   break;
+   case BDB_230_VBT_DP_MAX_LINK_RATE_LBR:
+   link_rate = 162000;
+   break;
+   case BDB_230_VBT_DP_MAX_LINK_RATE_DEF:
+   default:
+   link_rate = 0;
+   break;
+   }
+
+   return link_rate;
+}
+
+static int parse_bdb_216_dp_max_link_rate(const int vbt_max_link_rate)
+{
+   int link_rate;
+
+   switch (vbt_max_link_rate) {
+   default:
+   case BDB_216_VBT_DP_MAX_LINK_RATE_HBR3:
+   link_rate = 81;
+   break;
+   case BDB_216_VBT_DP_MAX_LINK_RATE_HBR2:
+   link_rate = 54;
+   break;
+   case BDB_216_VBT_DP_MAX_LINK_RATE_HBR:
+   link_rate = 27;
+   break;
+   case BDB_216_VBT_DP_MAX_LINK_RATE_LBR:
+   link_rate = 162000;
+   break;
+   }
+
+   return link_rate;
+}
+
 static void parse_ddi_port(struct drm_i915_private *dev_priv,
   struct display_device_data *devdata,
   u8 bdb_version)
@@ -1884,21 +1942,11 @@ 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_HBR3:
-   info->dp_max_link_rate = 81;
-   break;
-   case VBT_DP_MAX_LINK_RATE_HBR2:
-   info->dp_max_link_rate = 54;
-   break;
-   case VBT_DP_MAX_LINK_RATE_HBR:
-   info->dp_max_link_rate = 27;
-   break;
-   case VBT_DP_MAX_LINK_RATE_LBR:
-   info->dp_max_link_rate = 162000;
-   break;
-   }
+   if (bdb_version >= 230)
+   info->dp_max_link_rate = 
parse_bdb_230_dp_max_link_rate(child->dp_max_link_rate);
+   else
+   info->dp_max_link_rate = 
parse_bdb_216_dp_max_link_rate(child->dp_max_link_rate);
+
drm_dbg_kms(_priv->drm,
"Port %c VBT DP max link rate: %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..0d80b04b34be 100644
--- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
@@ -343,10 +343,21 @@ 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_HBR   2
-#define VBT_DP_MAX_LINK_RATE_LBR   3
+/* DP max link rate 216+ */
+#define BDB_216_VBT_DP_MAX_LINK_RATE_HBR3  0
+#define BDB_216_VBT_DP_MAX_LINK_RATE_HBR2  1
+#define BDB_216_VBT_DP_MAX_LINK_RATE_HBR   2
+#define BDB_216_VBT_DP_MAX_LINK_RATE_LBR   3
+