Re: [bug report] drm/i915: Make sure we have enough memory bandwidth on ICL

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, May 29, 2019 at 01:03:35PM +0300, Dan Carpenter wrote:
> Hi Syrjälä,
> 
> I had a question about commit c457d9cf256e: ("drm/i915: Make sure we have
> enough memory bandwidth on ICL").
> 
> drivers/gpu/drm/i915/intel_bw.c
>     64  static int icl_pcode_read_qgv_point_info(struct drm_i915_private *dev_priv,
>     65                                           struct intel_qgv_point *sp,
>     66                                           int point)
>     67  {
>     68          u32 val = 0, val2;
>                              ^^^^
> "val2" is uninitialized.
> 
>     69          int ret;
>     70  
>     71          ret = sandybridge_pcode_read(dev_priv,
>     72                                       ICL_PCODE_MEM_SUBSYSYSTEM_INFO |
>     73                                       ICL_PCODE_MEM_SS_READ_QGV_POINT_INFO(point),
>     74                                       &val, &val2);
>     75          if (ret)
>     76                  return ret;
>     77  
>     78          sp->dclk = val & 0xffff;
>     79          sp->t_rp = (val & 0xff0000) >> 16;
>     80          sp->t_rcd = (val & 0xff000000) >> 24;
>     81  
>     82          sp->t_rdpre = val2 & 0xff;
>     83          sp->t_ras = (val2 & 0xff00) >> 8;
>     84  
>     85          sp->t_rc = sp->t_rp + sp->t_ras;
>     86  
>     87          return 0;
>     88  }
> 
> drivers/gpu/drm/i915/intel_sideband.c
>    376  static int __sandybridge_pcode_rw(struct drm_i915_private *i915,
>    377                                    u32 mbox, u32 *val, u32 *val1,
>    378                                    int fast_timeout_us,
>    379                                    int slow_timeout_ms,
>    380                                    bool is_read)
>    381  {
>    382          struct intel_uncore *uncore = &i915->uncore;
>    383  
>    384          lockdep_assert_held(&i915->sb_lock);
>    385  
>    386          /*
>    387           * GEN6_PCODE_* are outside of the forcewake domain, we can
>    388           * use te fw I915_READ variants to reduce the amount of work
>    389           * required when reading/writing.
>    390           */
>    391  
>    392          if (intel_uncore_read_fw(uncore, GEN6_PCODE_MAILBOX) & GEN6_PCODE_READY)
>    393                  return -EAGAIN;
>    394  
>    395          intel_uncore_write_fw(uncore, GEN6_PCODE_DATA, *val);
>    396          intel_uncore_write_fw(uncore, GEN6_PCODE_DATA1, val1 ? *val1 : 0);
>                                                                        ^^^^^
> We write uninitialized value out here.  I'm sort of surprised that
> UBSan doesn't complain.  I don't know the code well enough to say if
> this is a problem.

Thanks for catching this. I suspect this pcode command doesn't care
about the initial value in that register, but I can't be 100% sure.
So we should initialize it just in case.

> 
>    397          intel_uncore_write_fw(uncore,
>    398                                GEN6_PCODE_MAILBOX, GEN6_PCODE_READY | mbox);
>    399  
>    400          if (__intel_wait_for_register_fw(uncore,
>    401                                           GEN6_PCODE_MAILBOX,
>    402                                           GEN6_PCODE_READY, 0,
>    403                                           fast_timeout_us,
>    404                                           slow_timeout_ms,
>    405                                           &mbox))
>    406                  return -ETIMEDOUT;
> 
> regards,
> dan carpenter

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx