| Message ID | 20171016145705.11780-10-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Oct 16, 2017 at 05:57:04PM +0300, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Handle missing buf trans tables, or out of bounds buf trans levels > the same way everywhere. These should never be hit under normal > conditions, but let's play it safe for now. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index f0a709b66e00..5f1c546e5e61 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -801,6 +801,9 @@ static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port por > hdmi_level >= n_hdmi_entries) > hdmi_level = hdmi_default_entry; > > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > + hdmi_level = n_hdmi_entries - 1; intel_ddi_get_buf_trans_* can set n_entries to 0 on failure, which would cause this to return a level of -1 - maybe those functions need to return 1 on failure instead, similar to the cnl_get_buf_trans_* functions? > + > return hdmi_level; > } > > @@ -864,6 +867,11 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries); > > + if (WARN_ON_ONCE(!ddi_translations_hdmi)) > + return; > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > + hdmi_level = n_hdmi_entries - 1; > + > /* If we're boosting the current, set bit 31 of trans1 */ > if (IS_GEN9_BC(dev_priv) && > dev_priv->vbt.ddi_port_info[port].hdmi_boost_level) > @@ -1847,6 +1855,11 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, > else > ddi_translations = intel_ddi_get_buf_trans_dp(dev_priv, port, &n_entries); > > + if (WARN_ON_ONCE(!ddi_translations)) > + return; > + if (WARN_ON_ONCE(level >= n_entries)) > + level = n_entries - 1; > + > iboost = ddi_translations[level].i_boost; > } > > @@ -1877,6 +1890,11 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder, > else > ddi_translations = bxt_get_buf_trans_dp(dev_priv, &n_entries); > > + if (WARN_ON_ONCE(!ddi_translations)) > + return; > + if (WARN_ON_ONCE(level >= n_entries)) > + level = n_entries - 1; > + > bxt_ddi_phy_set_signal_level(dev_priv, port, > ddi_translations[level].margin, > ddi_translations[level].scale, > @@ -1932,13 +1950,10 @@ static void cnl_ddi_vswing_program(struct intel_encoder *encoder, > else > ddi_translations = cnl_get_buf_trans_dp(dev_priv, &n_entries); > > - if (WARN_ON(ddi_translations == NULL)) > + if (WARN_ON_ONCE(!ddi_translations)) > return; > - > - if (level >= n_entries) { > - DRM_DEBUG_KMS("DDI translation not found for level %d. Using %d instead.", level, n_entries - 1); > + if (WARN_ON_ONCE(level >= n_entries)) > level = n_entries - 1; > - } > > /* Set PORT_TX_DW5 Scaling Mode Sel to 010b. */ > val = I915_READ(CNL_PORT_TX_DW5_LN0(port)); > -- > 2.13.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 18, 2017 at 10:41:44AM -0700, James Ausmus wrote: > On Mon, Oct 16, 2017 at 05:57:04PM +0300, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Handle missing buf trans tables, or out of bounds buf trans levels > > the same way everywhere. These should never be hit under normal > > conditions, but let's play it safe for now. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++++++++++++----- > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index f0a709b66e00..5f1c546e5e61 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -801,6 +801,9 @@ static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port por > > hdmi_level >= n_hdmi_entries) > > hdmi_level = hdmi_default_entry; > > > > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > > + hdmi_level = n_hdmi_entries - 1; > > intel_ddi_get_buf_trans_* can set n_entries to 0 on failure, > which would cause this to return a level of -1 - maybe those functions > need to return 1 on failure instead, similar to the cnl_get_buf_trans_* > functions? Hmm. I guess we could just do if (WARN_ON(hdmi_level == 0)) return; before the other check. That would be equivalent to the !ddi_translations checks done in the other places. > > > + > > return hdmi_level; > > } > > > > @@ -864,6 +867,11 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > > > ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries); > > > > + if (WARN_ON_ONCE(!ddi_translations_hdmi)) > > + return; > > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > > + hdmi_level = n_hdmi_entries - 1; > > + > > /* If we're boosting the current, set bit 31 of trans1 */ > > if (IS_GEN9_BC(dev_priv) && > > dev_priv->vbt.ddi_port_info[port].hdmi_boost_level) > > @@ -1847,6 +1855,11 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, > > else > > ddi_translations = intel_ddi_get_buf_trans_dp(dev_priv, port, &n_entries); > > > > + if (WARN_ON_ONCE(!ddi_translations)) > > + return; > > + if (WARN_ON_ONCE(level >= n_entries)) > > + level = n_entries - 1; > > > > + > > iboost = ddi_translations[level].i_boost; > > } > > > > @@ -1877,6 +1890,11 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder, > > else > > ddi_translations = bxt_get_buf_trans_dp(dev_priv, &n_entries); > > > > + if (WARN_ON_ONCE(!ddi_translations)) > > + return; > > + if (WARN_ON_ONCE(level >= n_entries)) > > + level = n_entries - 1; > > + > > bxt_ddi_phy_set_signal_level(dev_priv, port, > > ddi_translations[level].margin, > > ddi_translations[level].scale, > > @@ -1932,13 +1950,10 @@ static void cnl_ddi_vswing_program(struct intel_encoder *encoder, > > else > > ddi_translations = cnl_get_buf_trans_dp(dev_priv, &n_entries); > > > > - if (WARN_ON(ddi_translations == NULL)) > > + if (WARN_ON_ONCE(!ddi_translations)) > > return; > > - > > - if (level >= n_entries) { > > - DRM_DEBUG_KMS("DDI translation not found for level %d. Using %d instead.", level, n_entries - 1); > > + if (WARN_ON_ONCE(level >= n_entries)) > > level = n_entries - 1; > > - } > > > > /* Set PORT_TX_DW5 Scaling Mode Sel to 010b. */ > > val = I915_READ(CNL_PORT_TX_DW5_LN0(port)); > > -- > > 2.13.6 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 18, 2017 at 08:52:40PM +0300, Ville Syrjälä wrote: > On Wed, Oct 18, 2017 at 10:41:44AM -0700, James Ausmus wrote: > > On Mon, Oct 16, 2017 at 05:57:04PM +0300, Ville Syrjala wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Handle missing buf trans tables, or out of bounds buf trans levels > > > the same way everywhere. These should never be hit under normal > > > conditions, but let's play it safe for now. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++++++++++++----- > > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > index f0a709b66e00..5f1c546e5e61 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -801,6 +801,9 @@ static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port por > > > hdmi_level >= n_hdmi_entries) > > > hdmi_level = hdmi_default_entry; > > > > > > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > > > + hdmi_level = n_hdmi_entries - 1; > > > > intel_ddi_get_buf_trans_* can set n_entries to 0 on failure, > > which would cause this to return a level of -1 - maybe those functions > > need to return 1 on failure instead, similar to the cnl_get_buf_trans_* > > functions? > > Hmm. I guess we could just do > > if (WARN_ON(hdmi_level == 0)) > return; > > before the other check. That would be equivalent to the > !ddi_translations checks done in the other places. Hmm. Actually now that I read things again, the -1 shouldn't actually cause us any problems since all uses of it would do a second !ddi_translations check before indexing anything with the -1. -1 and 0 are both wrong indexes when dealing with a zero sized array, but I suppose in theory 0 is a bit better since it would trip up on firther level>=n_entries checks. So I guess I could try to avoid the negative value. Another way to do that would be just - return hdmi_level; + return max(hdmi_level, 0); Or your other idea about keeping things unsigned would also take care of it. Except having a wild UINT_MAX roaming around seems potentially more dangerous to me. > > > > > + > > > return hdmi_level; > > > } > > > > > > @@ -864,6 +867,11 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > > > > > ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries); > > > > > > + if (WARN_ON_ONCE(!ddi_translations_hdmi)) > > > + return; > > > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > > > + hdmi_level = n_hdmi_entries - 1; > > > + > > > /* If we're boosting the current, set bit 31 of trans1 */ > > > if (IS_GEN9_BC(dev_priv) && > > > dev_priv->vbt.ddi_port_info[port].hdmi_boost_level) > > > @@ -1847,6 +1855,11 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, > > > else > > > ddi_translations = intel_ddi_get_buf_trans_dp(dev_priv, port, &n_entries); > > > > > > + if (WARN_ON_ONCE(!ddi_translations)) > > > + return; > > > + if (WARN_ON_ONCE(level >= n_entries)) > > > + level = n_entries - 1; > > > > > > > + > > > iboost = ddi_translations[level].i_boost; > > > } > > > > > > @@ -1877,6 +1890,11 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder, > > > else > > > ddi_translations = bxt_get_buf_trans_dp(dev_priv, &n_entries); > > > > > > + if (WARN_ON_ONCE(!ddi_translations)) > > > + return; > > > + if (WARN_ON_ONCE(level >= n_entries)) > > > + level = n_entries - 1; > > > + > > > bxt_ddi_phy_set_signal_level(dev_priv, port, > > > ddi_translations[level].margin, > > > ddi_translations[level].scale, > > > @@ -1932,13 +1950,10 @@ static void cnl_ddi_vswing_program(struct intel_encoder *encoder, > > > else > > > ddi_translations = cnl_get_buf_trans_dp(dev_priv, &n_entries); > > > > > > - if (WARN_ON(ddi_translations == NULL)) > > > + if (WARN_ON_ONCE(!ddi_translations)) > > > return; > > > - > > > - if (level >= n_entries) { > > > - DRM_DEBUG_KMS("DDI translation not found for level %d. Using %d instead.", level, n_entries - 1); > > > + if (WARN_ON_ONCE(level >= n_entries)) > > > level = n_entries - 1; > > > - } > > > > > > /* Set PORT_TX_DW5 Scaling Mode Sel to 010b. */ > > > val = I915_READ(CNL_PORT_TX_DW5_LN0(port)); > > > -- > > > 2.13.6 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Oct 18, 2017 at 09:15:09PM +0300, Ville Syrjälä wrote: > On Wed, Oct 18, 2017 at 08:52:40PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 18, 2017 at 10:41:44AM -0700, James Ausmus wrote: > > > On Mon, Oct 16, 2017 at 05:57:04PM +0300, Ville Syrjala wrote: > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > > > Handle missing buf trans tables, or out of bounds buf trans levels > > > > the same way everywhere. These should never be hit under normal > > > > conditions, but let's play it safe for now. > > > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_ddi.c | 25 ++++++++++++++++++++----- > > > > 1 file changed, 20 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > > index f0a709b66e00..5f1c546e5e61 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > @@ -801,6 +801,9 @@ static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port por > > > > hdmi_level >= n_hdmi_entries) > > > > hdmi_level = hdmi_default_entry; > > > > > > > > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > > > > + hdmi_level = n_hdmi_entries - 1; > > > > > > intel_ddi_get_buf_trans_* can set n_entries to 0 on failure, > > > which would cause this to return a level of -1 - maybe those functions > > > need to return 1 on failure instead, similar to the cnl_get_buf_trans_* > > > functions? > > > > Hmm. I guess we could just do > > > > if (WARN_ON(hdmi_level == 0)) > > return; > > > > before the other check. That would be equivalent to the > > !ddi_translations checks done in the other places. > > Hmm. Actually now that I read things again, the -1 shouldn't actually > cause us any problems since all uses of it would do a second > !ddi_translations check before indexing anything with the -1. Yeah, that's true, but it doesn't feel right to have inconsistent error values between cnl and the rest of the world, and ending up with an index of -1 feels wrong as well - OTOH, the cnl functions claiming to have 1 entry when erroring out also feels wrong, so maybe there's not a completely clean solution without over-engineering a simple array & index mechanism. Hmm - maybe keep the switch to int, and on failure have all the paths set n_entries to -1, which makes it an obvious error value, rather than trying to guess error on a potential real or invalid value of 1 or 0. > > -1 and 0 are both wrong indexes when dealing with a zero sized array, > but I suppose in theory 0 is a bit better since it would trip up on > firther level>=n_entries checks. So I guess I could try to avoid > the negative value. > > Another way to do that would be just > - return hdmi_level; > + return max(hdmi_level, 0); > > Or your other idea about keeping things unsigned would also take care > of it. Except having a wild UINT_MAX roaming around seems potentially > more dangerous to me. > > > > > > > > + > > > > return hdmi_level; > > > > } > > > > > > > > @@ -864,6 +867,11 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, > > > > > > > > ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries); > > > > > > > > + if (WARN_ON_ONCE(!ddi_translations_hdmi)) > > > > + return; > > > > + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) > > > > + hdmi_level = n_hdmi_entries - 1; > > > > + > > > > /* If we're boosting the current, set bit 31 of trans1 */ > > > > if (IS_GEN9_BC(dev_priv) && > > > > dev_priv->vbt.ddi_port_info[port].hdmi_boost_level) > > > > @@ -1847,6 +1855,11 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, > > > > else > > > > ddi_translations = intel_ddi_get_buf_trans_dp(dev_priv, port, &n_entries); > > > > > > > > + if (WARN_ON_ONCE(!ddi_translations)) > > > > + return; > > > > + if (WARN_ON_ONCE(level >= n_entries)) > > > > + level = n_entries - 1; > > > > > > > > > > + > > > > iboost = ddi_translations[level].i_boost; > > > > } > > > > > > > > @@ -1877,6 +1890,11 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder, > > > > else > > > > ddi_translations = bxt_get_buf_trans_dp(dev_priv, &n_entries); > > > > > > > > + if (WARN_ON_ONCE(!ddi_translations)) > > > > + return; > > > > + if (WARN_ON_ONCE(level >= n_entries)) > > > > + level = n_entries - 1; > > > > + > > > > bxt_ddi_phy_set_signal_level(dev_priv, port, > > > > ddi_translations[level].margin, > > > > ddi_translations[level].scale, > > > > @@ -1932,13 +1950,10 @@ static void cnl_ddi_vswing_program(struct intel_encoder *encoder, > > > > else > > > > ddi_translations = cnl_get_buf_trans_dp(dev_priv, &n_entries); > > > > > > > > - if (WARN_ON(ddi_translations == NULL)) > > > > + if (WARN_ON_ONCE(!ddi_translations)) > > > > return; > > > > - > > > > - if (level >= n_entries) { > > > > - DRM_DEBUG_KMS("DDI translation not found for level %d. Using %d instead.", level, n_entries - 1); > > > > + if (WARN_ON_ONCE(level >= n_entries)) > > > > level = n_entries - 1; > > > > - } > > > > > > > > /* Set PORT_TX_DW5 Scaling Mode Sel to 010b. */ > > > > val = I915_READ(CNL_PORT_TX_DW5_LN0(port)); > > > > -- > > > > 2.13.6 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index f0a709b66e00..5f1c546e5e61 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -801,6 +801,9 @@ static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port por hdmi_level >= n_hdmi_entries) hdmi_level = hdmi_default_entry; + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) + hdmi_level = n_hdmi_entries - 1; + return hdmi_level; } @@ -864,6 +867,11 @@ static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder, ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries); + if (WARN_ON_ONCE(!ddi_translations_hdmi)) + return; + if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries)) + hdmi_level = n_hdmi_entries - 1; + /* If we're boosting the current, set bit 31 of trans1 */ if (IS_GEN9_BC(dev_priv) && dev_priv->vbt.ddi_port_info[port].hdmi_boost_level) @@ -1847,6 +1855,11 @@ static void skl_ddi_set_iboost(struct intel_encoder *encoder, else ddi_translations = intel_ddi_get_buf_trans_dp(dev_priv, port, &n_entries); + if (WARN_ON_ONCE(!ddi_translations)) + return; + if (WARN_ON_ONCE(level >= n_entries)) + level = n_entries - 1; + iboost = ddi_translations[level].i_boost; } @@ -1877,6 +1890,11 @@ static void bxt_ddi_vswing_sequence(struct intel_encoder *encoder, else ddi_translations = bxt_get_buf_trans_dp(dev_priv, &n_entries); + if (WARN_ON_ONCE(!ddi_translations)) + return; + if (WARN_ON_ONCE(level >= n_entries)) + level = n_entries - 1; + bxt_ddi_phy_set_signal_level(dev_priv, port, ddi_translations[level].margin, ddi_translations[level].scale, @@ -1932,13 +1950,10 @@ static void cnl_ddi_vswing_program(struct intel_encoder *encoder, else ddi_translations = cnl_get_buf_trans_dp(dev_priv, &n_entries); - if (WARN_ON(ddi_translations == NULL)) + if (WARN_ON_ONCE(!ddi_translations)) return; - - if (level >= n_entries) { - DRM_DEBUG_KMS("DDI translation not found for level %d. Using %d instead.", level, n_entries - 1); + if (WARN_ON_ONCE(level >= n_entries)) level = n_entries - 1; - } /* Set PORT_TX_DW5 Scaling Mode Sel to 010b. */ val = I915_READ(CNL_PORT_TX_DW5_LN0(port));