Re: [Intel-gfx] [PATCH 10/10] drm/i915: Drop the redundant hdmi prefix/suffix from a lot of variables

[James Ausmus]

On Wed, Oct 18, 2017 at 08:54:12PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 18, 2017 at 10:43:54AM -0700, James Ausmus wrote:
> > On Mon, Oct 16, 2017 at 05:57:05PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä 
> > > 
> > > A bunch of functions are now exclusively used for HDMI, so naming the
> > > variables with hdmi prefix/suffix is redundant. Also use int rather
> > > than u32 for the translation level consistently.
> > 
> > Is there a case for level to be negative, ever? If not, keeping it u32
> > could help catch cases where we accidently try to set level to -1.
> 
> Is UINT_MAX any better than -1?
> 

I was hoping gcc could be smart enough to throw a warning on an unsigned
underflow, but it doesn't appear to be.

> > 
> > > 
> > > Signed-off-by: Ville Syrjälä 
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c | 61 
> > > +++-
> > >  1 file changed, 29 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index 5f1c546e5e61..cd550aab332b 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -770,41 +770,38 @@ cnl_get_buf_trans_edp(struct drm_i915_private 
> > > *dev_priv, int *n_entries)
> > >  
> > >  static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum 
> > > port port)
> > >  {
> > > - int n_hdmi_entries;
> > > - int hdmi_level;
> > > - int hdmi_default_entry;
> > > + int n_entries, level, default_entry;
> > >  
> > > - hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
> > > + level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
> > >  
> > >   if (IS_CANNONLAKE(dev_priv)) {
> > > - cnl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > > - hdmi_default_entry = n_hdmi_entries - 1;
> > > + cnl_get_buf_trans_hdmi(dev_priv, &n_entries);
> > > + default_entry = n_entries - 1;
> > >   } else if (IS_GEN9_LP(dev_priv)) {
> > > - bxt_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > > - hdmi_default_entry = n_hdmi_entries - 1;
> > > + bxt_get_buf_trans_hdmi(dev_priv, &n_entries);
> > > + default_entry = n_entries - 1;
> > >   } else if (IS_GEN9_BC(dev_priv)) {
> > > - intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > > - hdmi_default_entry = 8;
> > > + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > > + default_entry = 8;
> > >   } else if (IS_BROADWELL(dev_priv)) {
> > > - intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > > - hdmi_default_entry = 7;
> > > + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > > + default_entry = 7;
> > >   } else if (IS_HASWELL(dev_priv)) {
> > > - intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > > - hdmi_default_entry = 6;
> > > + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > > + default_entry = 6;
> > >   } else {
> > >   WARN(1, "ddi translation table missing\n");
> > >   return 0;
> > >   }
> > >  
> > >   /* Choose a good default if VBT is badly populated */
> > > - if (hdmi_level == HDMI_LEVEL_SHIFT_UNKNOWN ||
> > > - hdmi_level >= n_hdmi_entries)
> > > - hdmi_level = hdmi_default_entry;
> > > + if (level == HDMI_LEVEL_SHIFT_UNKNOWN || level >= n_entries)
> > > + level = default_entry;
> > >  
> > > - if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries))
> > > - hdmi_level = n_hdmi_entries - 1;
> > > + if (WARN_ON_ONCE(level >= n_entries))
> > > + level = n_entries - 1;
> > >  
> > > - return hdmi_level;
> > > + return level;
> > >  }
> > >  
> > >  /*
> > > @@ -857,20 +854,20 @@ static void intel_prepare_dp_ddi_buffers(struct 
> > > intel_encoder *encoder)
> > >   * HDMI/DVI use cases.
> > >   */
> > >  static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> > > -int hdmi_level)
> > > +int level)
> > >  {
> > >   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >   u32 iboost_bit = 0;
> > > - int n_hdmi_entries;
> > > + int n_entries;
> > >   enum port port = intel_ddi_get_encoder_port(encoder);
> > > - const struct ddi_buf_trans *ddi_translations_hdmi;
> > > + const struct ddi_buf_trans *ddi_translations;
> > >  
> > > - ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, 
> > > &n_hdmi_entries);
> > > + ddi_translations = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > >  
> > > - if (WARN_ON_ONCE(!ddi_translations_hdmi))
> > > + if (WARN_ON_ONCE(!ddi_translations))
> > >   return;
> > > - if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries))
> > > - hdmi_level = n_hdmi_entries - 1;
> > > + if (WARN_ON_ONCE(level >= n_entries))
> > > + level = n_entries - 1;
> > >  
> > >   /* If we're boosting the current, set bit 31 of trans1 */
> > >   if 

Re: [Intel-gfx] [PATCH 10/10] drm/i915: Drop the redundant hdmi prefix/suffix from a lot of variables

[Ville Syrjälä]

On Wed, Oct 18, 2017 at 10:43:54AM -0700, James Ausmus wrote:
> On Mon, Oct 16, 2017 at 05:57:05PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> > 
> > A bunch of functions are now exclusively used for HDMI, so naming the
> > variables with hdmi prefix/suffix is redundant. Also use int rather
> > than u32 for the translation level consistently.
> 
> Is there a case for level to be negative, ever? If not, keeping it u32
> could help catch cases where we accidently try to set level to -1.

Is UINT_MAX any better than -1?

> 
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c | 61 
> > +++-
> >  1 file changed, 29 insertions(+), 32 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index 5f1c546e5e61..cd550aab332b 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -770,41 +770,38 @@ cnl_get_buf_trans_edp(struct drm_i915_private 
> > *dev_priv, int *n_entries)
> >  
> >  static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum 
> > port port)
> >  {
> > -   int n_hdmi_entries;
> > -   int hdmi_level;
> > -   int hdmi_default_entry;
> > +   int n_entries, level, default_entry;
> >  
> > -   hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
> > +   level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
> >  
> > if (IS_CANNONLAKE(dev_priv)) {
> > -   cnl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > -   hdmi_default_entry = n_hdmi_entries - 1;
> > +   cnl_get_buf_trans_hdmi(dev_priv, &n_entries);
> > +   default_entry = n_entries - 1;
> > } else if (IS_GEN9_LP(dev_priv)) {
> > -   bxt_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > -   hdmi_default_entry = n_hdmi_entries - 1;
> > +   bxt_get_buf_trans_hdmi(dev_priv, &n_entries);
> > +   default_entry = n_entries - 1;
> > } else if (IS_GEN9_BC(dev_priv)) {
> > -   intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > -   hdmi_default_entry = 8;
> > +   intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > +   default_entry = 8;
> > } else if (IS_BROADWELL(dev_priv)) {
> > -   intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > -   hdmi_default_entry = 7;
> > +   intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > +   default_entry = 7;
> > } else if (IS_HASWELL(dev_priv)) {
> > -   intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> > -   hdmi_default_entry = 6;
> > +   intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> > +   default_entry = 6;
> > } else {
> > WARN(1, "ddi translation table missing\n");
> > return 0;
> > }
> >  
> > /* Choose a good default if VBT is badly populated */
> > -   if (hdmi_level == HDMI_LEVEL_SHIFT_UNKNOWN ||
> > -   hdmi_level >= n_hdmi_entries)
> > -   hdmi_level = hdmi_default_entry;
> > +   if (level == HDMI_LEVEL_SHIFT_UNKNOWN || level >= n_entries)
> > +   level = default_entry;
> >  
> > -   if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries))
> > -   hdmi_level = n_hdmi_entries - 1;
> > +   if (WARN_ON_ONCE(level >= n_entries))
> > +   level = n_entries - 1;
> >  
> > -   return hdmi_level;
> > +   return level;
> >  }
> >  
> >  /*
> > @@ -857,20 +854,20 @@ static void intel_prepare_dp_ddi_buffers(struct 
> > intel_encoder *encoder)
> >   * HDMI/DVI use cases.
> >   */
> >  static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> > -  int hdmi_level)
> > +  int level)
> >  {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > u32 iboost_bit = 0;
> > -   int n_hdmi_entries;
> > +   int n_entries;
> > enum port port = intel_ddi_get_encoder_port(encoder);
> > -   const struct ddi_buf_trans *ddi_translations_hdmi;
> > +   const struct ddi_buf_trans *ddi_translations;
> >  
> > -   ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, 
> > &n_hdmi_entries);
> > +   ddi_translations = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> >  
> > -   if (WARN_ON_ONCE(!ddi_translations_hdmi))
> > +   if (WARN_ON_ONCE(!ddi_translations))
> > return;
> > -   if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries))
> > -   hdmi_level = n_hdmi_entries - 1;
> > +   if (WARN_ON_ONCE(level >= n_entries))
> > +   level = n_entries - 1;
> >  
> > /* If we're boosting the current, set bit 31 of trans1 */
> > if (IS_GEN9_BC(dev_priv) &&
> > @@ -879,9 +876,9 @@ static void intel_prepare_hdmi_ddi_buffers(struct 
> > intel_encoder *encoder,
> >  
> > /* Entry 9 is for HDMI: */
> > I915_WRITE(DDI_BUF_TRANS_LO(port, 9),
> > -  ddi_translations_hdmi[hdmi_level].trans1 | i

Re: [Intel-gfx] [PATCH 10/10] drm/i915: Drop the redundant hdmi prefix/suffix from a lot of variables

[James Ausmus]

On Mon, Oct 16, 2017 at 05:57:05PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> A bunch of functions are now exclusively used for HDMI, so naming the
> variables with hdmi prefix/suffix is redundant. Also use int rather
> than u32 for the translation level consistently.

Is there a case for level to be negative, ever? If not, keeping it u32
could help catch cases where we accidently try to set level to -1.

> 
> Signed-off-by: Ville Syrjälä 
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 61 
> +++-
>  1 file changed, 29 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 5f1c546e5e61..cd550aab332b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -770,41 +770,38 @@ cnl_get_buf_trans_edp(struct drm_i915_private 
> *dev_priv, int *n_entries)
>  
>  static int intel_ddi_hdmi_level(struct drm_i915_private *dev_priv, enum port 
> port)
>  {
> - int n_hdmi_entries;
> - int hdmi_level;
> - int hdmi_default_entry;
> + int n_entries, level, default_entry;
>  
> - hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
> + level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
>  
>   if (IS_CANNONLAKE(dev_priv)) {
> - cnl_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> - hdmi_default_entry = n_hdmi_entries - 1;
> + cnl_get_buf_trans_hdmi(dev_priv, &n_entries);
> + default_entry = n_entries - 1;
>   } else if (IS_GEN9_LP(dev_priv)) {
> - bxt_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> - hdmi_default_entry = n_hdmi_entries - 1;
> + bxt_get_buf_trans_hdmi(dev_priv, &n_entries);
> + default_entry = n_entries - 1;
>   } else if (IS_GEN9_BC(dev_priv)) {
> - intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> - hdmi_default_entry = 8;
> + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> + default_entry = 8;
>   } else if (IS_BROADWELL(dev_priv)) {
> - intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> - hdmi_default_entry = 7;
> + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> + default_entry = 7;
>   } else if (IS_HASWELL(dev_priv)) {
> - intel_ddi_get_buf_trans_hdmi(dev_priv, &n_hdmi_entries);
> - hdmi_default_entry = 6;
> + intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
> + default_entry = 6;
>   } else {
>   WARN(1, "ddi translation table missing\n");
>   return 0;
>   }
>  
>   /* Choose a good default if VBT is badly populated */
> - if (hdmi_level == HDMI_LEVEL_SHIFT_UNKNOWN ||
> - hdmi_level >= n_hdmi_entries)
> - hdmi_level = hdmi_default_entry;
> + if (level == HDMI_LEVEL_SHIFT_UNKNOWN || level >= n_entries)
> + level = default_entry;
>  
> - if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries))
> - hdmi_level = n_hdmi_entries - 1;
> + if (WARN_ON_ONCE(level >= n_entries))
> + level = n_entries - 1;
>  
> - return hdmi_level;
> + return level;
>  }
>  
>  /*
> @@ -857,20 +854,20 @@ static void intel_prepare_dp_ddi_buffers(struct 
> intel_encoder *encoder)
>   * HDMI/DVI use cases.
>   */
>  static void intel_prepare_hdmi_ddi_buffers(struct intel_encoder *encoder,
> -int hdmi_level)
> +int level)
>  {
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   u32 iboost_bit = 0;
> - int n_hdmi_entries;
> + int n_entries;
>   enum port port = intel_ddi_get_encoder_port(encoder);
> - const struct ddi_buf_trans *ddi_translations_hdmi;
> + const struct ddi_buf_trans *ddi_translations;
>  
> - ddi_translations_hdmi = intel_ddi_get_buf_trans_hdmi(dev_priv, 
> &n_hdmi_entries);
> + ddi_translations = intel_ddi_get_buf_trans_hdmi(dev_priv, &n_entries);
>  
> - if (WARN_ON_ONCE(!ddi_translations_hdmi))
> + if (WARN_ON_ONCE(!ddi_translations))
>   return;
> - if (WARN_ON_ONCE(hdmi_level >= n_hdmi_entries))
> - hdmi_level = n_hdmi_entries - 1;
> + if (WARN_ON_ONCE(level >= n_entries))
> + level = n_entries - 1;
>  
>   /* If we're boosting the current, set bit 31 of trans1 */
>   if (IS_GEN9_BC(dev_priv) &&
> @@ -879,9 +876,9 @@ static void intel_prepare_hdmi_ddi_buffers(struct 
> intel_encoder *encoder,
>  
>   /* Entry 9 is for HDMI: */
>   I915_WRITE(DDI_BUF_TRANS_LO(port, 9),
> -ddi_translations_hdmi[hdmi_level].trans1 | iboost_bit);
> +ddi_translations[level].trans1 | iboost_bit);
>   I915_WRITE(DDI_BUF_TRANS_HI(port, 9),
> -ddi_translations_hdmi[hdmi_level].trans2);
> +