| Message ID | 20180131212013.11270-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-01-31 at 13:20 -0800, Rodrigo Vivi wrote: > Since commit 'c4fb60b9aba9 ("drm/i915/bios: add DP max link > rate to VBT child device struct")' we have the new entry > defined for max dp rate that is in use for CNL. > > Let's start using it for all VBT 216+ and > also organize the cnl adjusted rates in terms of rate > and not array size. > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 3 +++ > drivers/gpu/drm/i915/intel_dp.c | 37 +++++++++++++++++++++++++++++-------- > 3 files changed, 33 insertions(+), 8 deletions(-) <snip> > +static int intel_dp_adjusted_max_rate(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + const struct ddi_vbt_port_info *info = > + &dev_priv->vbt.ddi_port_info[dig_port->base.port]; > + > + if (info->dp_max_link_rate) > + return info->dp_max_link_rate; What if this value is higher than cnl_adjusted_max_rate() ? -DK > + > + if (IS_CANNONLAKE(dev_priv)) > + return cnl_adjusted_max_rate(intel_dp); > + > + return INT_MAX; /* No adjusted limit */ > } >
On Thu, Feb 01, 2018 at 02:53:20AM +0000, Pandiyan, Dhinakaran wrote: > > > > On Wed, 2018-01-31 at 13:20 -0800, Rodrigo Vivi wrote: > > Since commit 'c4fb60b9aba9 ("drm/i915/bios: add DP max link > > rate to VBT child device struct")' we have the new entry > > defined for max dp rate that is in use for CNL. > > > > Let's start using it for all VBT 216+ and > > also organize the cnl adjusted rates in terms of rate > > and not array size. > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_bios.c | 3 +++ > > drivers/gpu/drm/i915/intel_dp.c | 37 +++++++++++++++++++++++++++++-------- > > 3 files changed, 33 insertions(+), 8 deletions(-) > > <snip> > > > +static int intel_dp_adjusted_max_rate(struct intel_dp *intel_dp) > > +{ > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > + const struct ddi_vbt_port_info *info = > > + &dev_priv->vbt.ddi_port_info[dig_port->base.port]; > > + > > + if (info->dp_max_link_rate) > > + return info->dp_max_link_rate; > > What if this value is higher than cnl_adjusted_max_rate() ? Then VBT knows something about that SKU that we don't?! *shrug* My understanding is that the purpose of that new VBT field is to workaround bugs they know for certain combination and limit the max rate even more. bugs like the one I had in a part here: https://bugs.freedesktop.org/show_bug.cgi?id=103963 in this case flicker was gone by limiting the max rate. But yeap, maybe vbt is bugged, so are you suggesting min(vbt, bspec) ? > > -DK > > > > + > > + if (IS_CANNONLAKE(dev_priv)) > > + return cnl_adjusted_max_rate(intel_dp); > > + > > + return INT_MAX; /* No adjusted limit */ > > } > > >
On Wed, 2018-01-31 at 21:22 -0800, Rodrigo Vivi wrote: > On Thu, Feb 01, 2018 at 02:53:20AM +0000, Pandiyan, Dhinakaran wrote: > > > > > > > > On Wed, 2018-01-31 at 13:20 -0800, Rodrigo Vivi wrote: > > > Since commit 'c4fb60b9aba9 ("drm/i915/bios: add DP max link > > > rate to VBT child device struct")' we have the new entry > > > defined for max dp rate that is in use for CNL. > > > > > > Let's start using it for all VBT 216+ and > > > also organize the cnl adjusted rates in terms of rate > > > and not array size. > > > > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > > drivers/gpu/drm/i915/intel_bios.c | 3 +++ > > > drivers/gpu/drm/i915/intel_dp.c | 37 +++++++++++++++++++++++++++++-------- > > > 3 files changed, 33 insertions(+), 8 deletions(-) > > > > <snip> > > > > > +static int intel_dp_adjusted_max_rate(struct intel_dp *intel_dp) > > > +{ > > > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > > > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > > > + const struct ddi_vbt_port_info *info = > > > + &dev_priv->vbt.ddi_port_info[dig_port->base.port]; > > > + > > > + if (info->dp_max_link_rate) > > > + return info->dp_max_link_rate; > > > > What if this value is higher than cnl_adjusted_max_rate() ? > > Then VBT knows something about that SKU that we don't?! *shrug* > I have very little idea of how reliable VBT's are on SDP's or if they are always updated with the correct values. Having those fall back checks based on SKU and ports, which we know are definite HW limits, I feel is safer. However, I'll leave this to you as my knowledge is fairly limited. > My understanding is that the purpose of that new VBT field is > to workaround bugs they know for certain combination and limit > the max rate even more. > > bugs like the one I had in a part here: > > https://bugs.freedesktop.org/show_bug.cgi?id=103963 > > in this case flicker was gone by limiting the max rate. > > But yeap, maybe vbt is bugged, so are you suggesting min(vbt, bspec) ? Yes. > > > > > -DK > > > > > > > + > > > + if (IS_CANNONLAKE(dev_priv)) > > > + return cnl_adjusted_max_rate(intel_dp); > > > + > > > + return INT_MAX; /* No adjusted limit */ > > > } > > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 31 Jan 2018, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote: > Since commit 'c4fb60b9aba9 ("drm/i915/bios: add DP max link > rate to VBT child device struct")' we have the new entry > defined for max dp rate that is in use for CNL. > > Let's start using it for all VBT 216+ and > also organize the cnl adjusted rates in terms of rate > and not array size. Rodrigo, I took the liberty of rewriting this into three patches [1]. I hope you don't mind. I had some of this in mind way back when I rewrote the source/sink rate handling. Some notes inline below. I've taken them into account in my series. BR, Jani. [1] http://patchwork.freedesktop.org/patch/msgid/40f37f08cad33234cd86337d39e823ac6e55805f.1517482774.git.jani.nikula@intel.com > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_bios.c | 3 +++ > drivers/gpu/drm/i915/intel_dp.c | 37 +++++++++++++++++++++++++++++-------- > 3 files changed, 33 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index c676269ed843..66dea5c9b10e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1282,6 +1282,7 @@ struct ddi_vbt_port_info { > > uint8_t dp_boost_level; > uint8_t hdmi_boost_level; > + uint8_t dp_max_link_rate; > }; > > enum psr_lines_to_wait { > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c > index cf3f8f1ba6f7..c885daf4cc8d 100644 > --- a/drivers/gpu/drm/i915/intel_bios.c > +++ b/drivers/gpu/drm/i915/intel_bios.c > @@ -1230,6 +1230,9 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port, > info->alternate_aux_channel = aux_channel; > > sanitize_aux_ch(dev_priv, port); > + > + if (bdb_version >= 216 && child->dp_max_link_rate) > + info->dp_max_link_rate = child->dp_max_link_rate; The info is two bits in VBT and needs to be translated to rates. > } > > if (bdb_version >= 158) { > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 03d86ff9b805..1e34d7954355 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -220,7 +220,7 @@ intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp) > return max_dotclk; > } > > -static int cnl_adjusted_max_rate(struct intel_dp *intel_dp, int size) > +static int cnl_adjusted_max_rate(struct intel_dp *intel_dp) > { > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > @@ -230,17 +230,33 @@ static int cnl_adjusted_max_rate(struct intel_dp *intel_dp, int size) > > /* Low voltage SKUs are limited to max of 5.4G */ > if (voltage == VOLTAGE_INFO_0_85V) > - return size - 2; > + return 540000; > > /* For this SKU 8.1G is supported in all ports */ > if (IS_CNL_WITH_PORT_F(dev_priv)) > - return size; > + return 810000; > > /* For other SKUs, max rate on ports A and B is 5.4G */ > if (port == PORT_A || port == PORT_D) > - return size - 2; > + return 540000; > > - return size; > + return 810000; > +} > + > +static int intel_dp_adjusted_max_rate(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > + const struct ddi_vbt_port_info *info = > + &dev_priv->vbt.ddi_port_info[dig_port->base.port]; > + > + if (info->dp_max_link_rate) > + return info->dp_max_link_rate; The VBT value needs mapping, and 0 actually means HBR3 in VBT. > + > + if (IS_CANNONLAKE(dev_priv)) > + return cnl_adjusted_max_rate(intel_dp); Like DK noted, I think this should be a min() of the two. > + > + return INT_MAX; /* No adjusted limit */ I really want functions like this to return the actual max rate. I think returning INT_MAX here is a bit scary. > } > > static void > @@ -249,7 +265,8 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > const int *source_rates; > - int size; > + int i, size; > + int max_rate = intel_dp_adjusted_max_rate(intel_dp); > > /* This should only be done once */ > WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); > @@ -259,7 +276,7 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > size = ARRAY_SIZE(bxt_rates); > } else if (IS_CANNONLAKE(dev_priv)) { > source_rates = cnl_rates; > - size = cnl_adjusted_max_rate(intel_dp, ARRAY_SIZE(cnl_rates)); > + size = ARRAY_SIZE(cnl_rates); > } else if (IS_GEN9_BC(dev_priv)) { > source_rates = skl_rates; > size = ARRAY_SIZE(skl_rates); > @@ -272,8 +289,12 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) > size = ARRAY_SIZE(default_rates) - 1; > } > > + for (i = 0; i < size; i++) > + if (source_rates[i] > max_rate) > + break; We already have code to do this stuff, so I refactored and reused it. > + > + intel_dp->num_source_rates = i; > intel_dp->source_rates = source_rates; > - intel_dp->num_source_rates = size; > } > > static int intersect_rates(const int *source_rates, int source_len,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index c676269ed843..66dea5c9b10e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1282,6 +1282,7 @@ struct ddi_vbt_port_info { uint8_t dp_boost_level; uint8_t hdmi_boost_level; + uint8_t dp_max_link_rate; }; enum psr_lines_to_wait { diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index cf3f8f1ba6f7..c885daf4cc8d 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1230,6 +1230,9 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port, info->alternate_aux_channel = aux_channel; sanitize_aux_ch(dev_priv, port); + + if (bdb_version >= 216 && child->dp_max_link_rate) + info->dp_max_link_rate = child->dp_max_link_rate; } if (bdb_version >= 158) { diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 03d86ff9b805..1e34d7954355 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -220,7 +220,7 @@ intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp) return max_dotclk; } -static int cnl_adjusted_max_rate(struct intel_dp *intel_dp, int size) +static int cnl_adjusted_max_rate(struct intel_dp *intel_dp) { struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); @@ -230,17 +230,33 @@ static int cnl_adjusted_max_rate(struct intel_dp *intel_dp, int size) /* Low voltage SKUs are limited to max of 5.4G */ if (voltage == VOLTAGE_INFO_0_85V) - return size - 2; + return 540000; /* For this SKU 8.1G is supported in all ports */ if (IS_CNL_WITH_PORT_F(dev_priv)) - return size; + return 810000; /* For other SKUs, max rate on ports A and B is 5.4G */ if (port == PORT_A || port == PORT_D) - return size - 2; + return 540000; - return size; + return 810000; +} + +static int intel_dp_adjusted_max_rate(struct intel_dp *intel_dp) +{ + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + const struct ddi_vbt_port_info *info = + &dev_priv->vbt.ddi_port_info[dig_port->base.port]; + + if (info->dp_max_link_rate) + return info->dp_max_link_rate; + + if (IS_CANNONLAKE(dev_priv)) + return cnl_adjusted_max_rate(intel_dp); + + return INT_MAX; /* No adjusted limit */ } static void @@ -249,7 +265,8 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); const int *source_rates; - int size; + int i, size; + int max_rate = intel_dp_adjusted_max_rate(intel_dp); /* This should only be done once */ WARN_ON(intel_dp->source_rates || intel_dp->num_source_rates); @@ -259,7 +276,7 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) size = ARRAY_SIZE(bxt_rates); } else if (IS_CANNONLAKE(dev_priv)) { source_rates = cnl_rates; - size = cnl_adjusted_max_rate(intel_dp, ARRAY_SIZE(cnl_rates)); + size = ARRAY_SIZE(cnl_rates); } else if (IS_GEN9_BC(dev_priv)) { source_rates = skl_rates; size = ARRAY_SIZE(skl_rates); @@ -272,8 +289,12 @@ intel_dp_set_source_rates(struct intel_dp *intel_dp) size = ARRAY_SIZE(default_rates) - 1; } + for (i = 0; i < size; i++) + if (source_rates[i] > max_rate) + break; + + intel_dp->num_source_rates = i; intel_dp->source_rates = source_rates; - intel_dp->num_source_rates = size; } static int intersect_rates(const int *source_rates, int source_len,
Since commit 'c4fb60b9aba9 ("drm/i915/bios: add DP max link rate to VBT child device struct")' we have the new entry defined for max dp rate that is in use for CNL. Let's start using it for all VBT 216+ and also organize the cnl adjusted rates in terms of rate and not array size. Cc: Jani Nikula <jani.nikula@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_bios.c | 3 +++ drivers/gpu/drm/i915/intel_dp.c | 37 +++++++++++++++++++++++++++++-------- 3 files changed, 33 insertions(+), 8 deletions(-)