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@xxxxxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
