Re: [Intel-gfx] [PATCH 12/12] drm/i915: Parse DP/eDP max lane count from VBT

[Jani Nikula]

On Thu, 01 Sep 2022, Ville Syrjälä  wrote:
> On Tue, Jul 19, 2022 at 06:39:29AM -0400, Rodrigo Vivi wrote:
>> On Fri, Jul 15, 2022 at 11:20:44PM +0300, Ville Syrjala wrote:
>> > From: Ville Syrjälä 
>> > 
>> > Limit the DP lane count based on the new VBT DP/eDP max
>> > lane count field.
>> > 
>> > Signed-off-by: Ville Syrjälä 
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_bios.c | 16 
>> >  drivers/gpu/drm/i915/display/intel_bios.h |  1 +
>> >  drivers/gpu/drm/i915/display/intel_dp.c   | 13 -
>> >  3 files changed, 29 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
>> > b/drivers/gpu/drm/i915/display/intel_bios.c
>> > index cd86b65055ef..d8063c329b3a 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
>> > @@ -2489,6 +2489,14 @@ static int _intel_bios_dp_max_link_rate(const 
>> > struct intel_bios_encoder_data *de
>> >return 
>> > parse_bdb_216_dp_max_link_rate(devdata->child.dp_max_link_rate);
>> >  }
>> >  
>> > +static int _intel_bios_dp_max_lane_count(const struct 
>> > intel_bios_encoder_data *devdata)
>> > +{
>> > +  if (!devdata || devdata->i915->vbt.version < 244)
>> > +  return 0;
>> > +
>> > +  return devdata->child.dp_max_lane_count + 1;

They just won't learn that non-zero is a really crappy "default" to
set. *sigh*

>> > +}
>> > +
>> >  static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
>> > enum port port)
>> >  {
>> > @@ -3674,6 +3682,14 @@ int intel_bios_dp_max_link_rate(struct 
>> > intel_encoder *encoder)
>> >return _intel_bios_dp_max_link_rate(devdata);
>> >  }
>> >  
>> > +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder)
>> > +{
>> > +  struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> > +  const struct intel_bios_encoder_data *devdata = 
>> > i915->vbt.ports[encoder->port];
>> > +
>> > +  return _intel_bios_dp_max_lane_count(devdata);
>> > +}
>> 
>> do we really need 2 functions here since this one is small and we don't have 
>> any
>> bit switches and all?!
>> or do you plan to reuse this anywhere else later?
>
> This is modelled after the other similar functions. I think Jani had
> some plans for cleaning up a lot of this stuff, but dunno how far we
> are on that path.

A bit stalled. Eventually I'd like all encoders to have encoder->devdata
and callers would pass that instead of encoder, and we wouldn't have
this i915->vbt.ports[encoder->port]; lookup.

But the first function could be used in print_ddi_port() for printing
the info while the latter couldn't. So I kinda prefer the split.

Reviewed-by: Jani Nikula 


>
>> 
>> > +
>> >  int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder)
>> >  {
>> >struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h 
>> > b/drivers/gpu/drm/i915/display/intel_bios.h
>> > index e47582b0de0a..e375405a7828 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
>> > @@ -258,6 +258,7 @@ bool intel_bios_get_dsc_params(struct intel_encoder 
>> > *encoder,
>> >  int intel_bios_max_tmds_clock(struct intel_encoder *encoder);
>> >  int intel_bios_hdmi_level_shift(struct intel_encoder *encoder);
>> >  int intel_bios_dp_max_link_rate(struct intel_encoder *encoder);
>> > +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder);
>> >  int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder);
>> >  bool intel_bios_port_supports_typec_usb(struct drm_i915_private *i915, 
>> > enum port port);
>> >  bool intel_bios_port_supports_tbt(struct drm_i915_private *i915, enum 
>> > port port);
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
>> > b/drivers/gpu/drm/i915/display/intel_dp.c
>> > index 32292c0be2bd..0370c4c105dc 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> > @@ -286,11 +286,22 @@ static int intel_dp_max_common_rate(struct intel_dp 
>> > *intel_dp)
>> >return intel_dp_common_rate(intel_dp, intel_dp->num_common_rates - 1);
>> >  }
>> >  
>> > +static int intel_dp_max_source_lane_count(struct intel_digital_port 
>> > *dig_port)
>> > +{
>> > +  int vbt_max_lanes = intel_bios_dp_max_lane_count(&dig_port->base);
>> > +  int max_lanes = dig_port->max_lanes;
>> > +
>> > +  if (vbt_max_lanes)
>> > +  max_lanes = min(max_lanes, vbt_max_lanes);
>> > +
>> > +  return max_lanes;
>> > +}
>> > +
>> >  /* Theoretical max between source and sink */
>> >  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
>> >  {
>> >struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> > -  int source_max = dig_port->max_lanes;
>> > +  int source_max = intel_dp_max_source_lane_count(dig_port);
>> >int sink_max = intel_dp->max_sink_lane_count;
>> >int fia_max = intel_tc_port_fi

Re: [Intel-gfx] [PATCH 12/12] drm/i915: Parse DP/eDP max lane count from VBT

[Ville Syrjälä]

On Tue, Jul 19, 2022 at 06:39:29AM -0400, Rodrigo Vivi wrote:
> On Fri, Jul 15, 2022 at 11:20:44PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > Limit the DP lane count based on the new VBT DP/eDP max
> > lane count field.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/display/intel_bios.c | 16 
> >  drivers/gpu/drm/i915/display/intel_bios.h |  1 +
> >  drivers/gpu/drm/i915/display/intel_dp.c   | 13 -
> >  3 files changed, 29 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
> > b/drivers/gpu/drm/i915/display/intel_bios.c
> > index cd86b65055ef..d8063c329b3a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> > @@ -2489,6 +2489,14 @@ static int _intel_bios_dp_max_link_rate(const struct 
> > intel_bios_encoder_data *de
> > return 
> > parse_bdb_216_dp_max_link_rate(devdata->child.dp_max_link_rate);
> >  }
> >  
> > +static int _intel_bios_dp_max_lane_count(const struct 
> > intel_bios_encoder_data *devdata)
> > +{
> > +   if (!devdata || devdata->i915->vbt.version < 244)
> > +   return 0;
> > +
> > +   return devdata->child.dp_max_lane_count + 1;
> > +}
> > +
> >  static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
> >  enum port port)
> >  {
> > @@ -3674,6 +3682,14 @@ int intel_bios_dp_max_link_rate(struct intel_encoder 
> > *encoder)
> > return _intel_bios_dp_max_link_rate(devdata);
> >  }
> >  
> > +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder)
> > +{
> > +   struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > +   const struct intel_bios_encoder_data *devdata = 
> > i915->vbt.ports[encoder->port];
> > +
> > +   return _intel_bios_dp_max_lane_count(devdata);
> > +}
> 
> do we really need 2 functions here since this one is small and we don't have 
> any
> bit switches and all?!
> or do you plan to reuse this anywhere else later?

This is modelled after the other similar functions. I think Jani had
some plans for cleaning up a lot of this stuff, but dunno how far we
are on that path.

> 
> > +
> >  int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder)
> >  {
> > struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> > diff --git a/drivers/gpu/drm/i915/display/intel_bios.h 
> > b/drivers/gpu/drm/i915/display/intel_bios.h
> > index e47582b0de0a..e375405a7828 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bios.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> > @@ -258,6 +258,7 @@ bool intel_bios_get_dsc_params(struct intel_encoder 
> > *encoder,
> >  int intel_bios_max_tmds_clock(struct intel_encoder *encoder);
> >  int intel_bios_hdmi_level_shift(struct intel_encoder *encoder);
> >  int intel_bios_dp_max_link_rate(struct intel_encoder *encoder);
> > +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder);
> >  int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder);
> >  bool intel_bios_port_supports_typec_usb(struct drm_i915_private *i915, 
> > enum port port);
> >  bool intel_bios_port_supports_tbt(struct drm_i915_private *i915, enum port 
> > port);
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 32292c0be2bd..0370c4c105dc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -286,11 +286,22 @@ static int intel_dp_max_common_rate(struct intel_dp 
> > *intel_dp)
> > return intel_dp_common_rate(intel_dp, intel_dp->num_common_rates - 1);
> >  }
> >  
> > +static int intel_dp_max_source_lane_count(struct intel_digital_port 
> > *dig_port)
> > +{
> > +   int vbt_max_lanes = intel_bios_dp_max_lane_count(&dig_port->base);
> > +   int max_lanes = dig_port->max_lanes;
> > +
> > +   if (vbt_max_lanes)
> > +   max_lanes = min(max_lanes, vbt_max_lanes);
> > +
> > +   return max_lanes;
> > +}
> > +
> >  /* Theoretical max between source and sink */
> >  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
> >  {
> > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > -   int source_max = dig_port->max_lanes;
> > +   int source_max = intel_dp_max_source_lane_count(dig_port);
> > int sink_max = intel_dp->max_sink_lane_count;
> > int fia_max = intel_tc_port_fia_max_lane_count(dig_port);
> > int lttpr_max = 
> > drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps);
> > -- 
> > 2.35.1
> > 

-- 
Ville Syrjälä
Intel

Re: [Intel-gfx] [PATCH 12/12] drm/i915: Parse DP/eDP max lane count from VBT

[Rodrigo Vivi]

On Fri, Jul 15, 2022 at 11:20:44PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> Limit the DP lane count based on the new VBT DP/eDP max
> lane count field.
> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/display/intel_bios.c | 16 
>  drivers/gpu/drm/i915/display/intel_bios.h |  1 +
>  drivers/gpu/drm/i915/display/intel_dp.c   | 13 -
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
> b/drivers/gpu/drm/i915/display/intel_bios.c
> index cd86b65055ef..d8063c329b3a 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.c
> +++ b/drivers/gpu/drm/i915/display/intel_bios.c
> @@ -2489,6 +2489,14 @@ static int _intel_bios_dp_max_link_rate(const struct 
> intel_bios_encoder_data *de
>   return 
> parse_bdb_216_dp_max_link_rate(devdata->child.dp_max_link_rate);
>  }
>  
> +static int _intel_bios_dp_max_lane_count(const struct 
> intel_bios_encoder_data *devdata)
> +{
> + if (!devdata || devdata->i915->vbt.version < 244)
> + return 0;
> +
> + return devdata->child.dp_max_lane_count + 1;
> +}
> +
>  static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
>enum port port)
>  {
> @@ -3674,6 +3682,14 @@ int intel_bios_dp_max_link_rate(struct intel_encoder 
> *encoder)
>   return _intel_bios_dp_max_link_rate(devdata);
>  }
>  
> +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder)
> +{
> + struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> + const struct intel_bios_encoder_data *devdata = 
> i915->vbt.ports[encoder->port];
> +
> + return _intel_bios_dp_max_lane_count(devdata);
> +}

do we really need 2 functions here since this one is small and we don't have any
bit switches and all?!
or do you plan to reuse this anywhere else later?

> +
>  int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder)
>  {
>   struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_bios.h 
> b/drivers/gpu/drm/i915/display/intel_bios.h
> index e47582b0de0a..e375405a7828 100644
> --- a/drivers/gpu/drm/i915/display/intel_bios.h
> +++ b/drivers/gpu/drm/i915/display/intel_bios.h
> @@ -258,6 +258,7 @@ bool intel_bios_get_dsc_params(struct intel_encoder 
> *encoder,
>  int intel_bios_max_tmds_clock(struct intel_encoder *encoder);
>  int intel_bios_hdmi_level_shift(struct intel_encoder *encoder);
>  int intel_bios_dp_max_link_rate(struct intel_encoder *encoder);
> +int intel_bios_dp_max_lane_count(struct intel_encoder *encoder);
>  int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder);
>  bool intel_bios_port_supports_typec_usb(struct drm_i915_private *i915, enum 
> port port);
>  bool intel_bios_port_supports_tbt(struct drm_i915_private *i915, enum port 
> port);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 32292c0be2bd..0370c4c105dc 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -286,11 +286,22 @@ static int intel_dp_max_common_rate(struct intel_dp 
> *intel_dp)
>   return intel_dp_common_rate(intel_dp, intel_dp->num_common_rates - 1);
>  }
>  
> +static int intel_dp_max_source_lane_count(struct intel_digital_port 
> *dig_port)
> +{
> + int vbt_max_lanes = intel_bios_dp_max_lane_count(&dig_port->base);
> + int max_lanes = dig_port->max_lanes;
> +
> + if (vbt_max_lanes)
> + max_lanes = min(max_lanes, vbt_max_lanes);
> +
> + return max_lanes;
> +}
> +
>  /* Theoretical max between source and sink */
>  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
>  {
>   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> - int source_max = dig_port->max_lanes;
> + int source_max = intel_dp_max_source_lane_count(dig_port);
>   int sink_max = intel_dp->max_sink_lane_count;
>   int fia_max = intel_tc_port_fia_max_lane_count(dig_port);
>   int lttpr_max = 
> drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps);
> -- 
> 2.35.1
> 

[Intel-gfx] [PATCH 12/12] drm/i915: Parse DP/eDP max lane count from VBT

[Ville Syrjala]

From: Ville Syrjälä 

Limit the DP lane count based on the new VBT DP/eDP max
lane count field.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_bios.c | 16 
 drivers/gpu/drm/i915/display/intel_bios.h |  1 +
 drivers/gpu/drm/i915/display/intel_dp.c   | 13 -
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c 
b/drivers/gpu/drm/i915/display/intel_bios.c
index cd86b65055ef..d8063c329b3a 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -2489,6 +2489,14 @@ static int _intel_bios_dp_max_link_rate(const struct 
intel_bios_encoder_data *de
return 
parse_bdb_216_dp_max_link_rate(devdata->child.dp_max_link_rate);
 }
 
+static int _intel_bios_dp_max_lane_count(const struct intel_bios_encoder_data 
*devdata)
+{
+   if (!devdata || devdata->i915->vbt.version < 244)
+   return 0;
+
+   return devdata->child.dp_max_lane_count + 1;
+}
+
 static void sanitize_device_type(struct intel_bios_encoder_data *devdata,
 enum port port)
 {
@@ -3674,6 +3682,14 @@ int intel_bios_dp_max_link_rate(struct intel_encoder 
*encoder)
return _intel_bios_dp_max_link_rate(devdata);
 }
 
+int intel_bios_dp_max_lane_count(struct intel_encoder *encoder)
+{
+   struct drm_i915_private *i915 = to_i915(encoder->base.dev);
+   const struct intel_bios_encoder_data *devdata = 
i915->vbt.ports[encoder->port];
+
+   return _intel_bios_dp_max_lane_count(devdata);
+}
+
 int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder)
 {
struct drm_i915_private *i915 = to_i915(encoder->base.dev);
diff --git a/drivers/gpu/drm/i915/display/intel_bios.h 
b/drivers/gpu/drm/i915/display/intel_bios.h
index e47582b0de0a..e375405a7828 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.h
+++ b/drivers/gpu/drm/i915/display/intel_bios.h
@@ -258,6 +258,7 @@ bool intel_bios_get_dsc_params(struct intel_encoder 
*encoder,
 int intel_bios_max_tmds_clock(struct intel_encoder *encoder);
 int intel_bios_hdmi_level_shift(struct intel_encoder *encoder);
 int intel_bios_dp_max_link_rate(struct intel_encoder *encoder);
+int intel_bios_dp_max_lane_count(struct intel_encoder *encoder);
 int intel_bios_alternate_ddc_pin(struct intel_encoder *encoder);
 bool intel_bios_port_supports_typec_usb(struct drm_i915_private *i915, enum 
port port);
 bool intel_bios_port_supports_tbt(struct drm_i915_private *i915, enum port 
port);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index 32292c0be2bd..0370c4c105dc 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -286,11 +286,22 @@ static int intel_dp_max_common_rate(struct intel_dp 
*intel_dp)
return intel_dp_common_rate(intel_dp, intel_dp->num_common_rates - 1);
 }
 
+static int intel_dp_max_source_lane_count(struct intel_digital_port *dig_port)
+{
+   int vbt_max_lanes = intel_bios_dp_max_lane_count(&dig_port->base);
+   int max_lanes = dig_port->max_lanes;
+
+   if (vbt_max_lanes)
+   max_lanes = min(max_lanes, vbt_max_lanes);
+
+   return max_lanes;
+}
+
 /* Theoretical max between source and sink */
 static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
 {
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-   int source_max = dig_port->max_lanes;
+   int source_max = intel_dp_max_source_lane_count(dig_port);
int sink_max = intel_dp->max_sink_lane_count;
int fia_max = intel_tc_port_fia_max_lane_count(dig_port);
int lttpr_max = 
drm_dp_lttpr_max_lane_count(intel_dp->lttpr_common_caps);
-- 
2.35.1