Re: [PATCH 2/2] drm/i915: Simplify some icl pll calculations

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

On Wed, Apr 10, 2019 at 07:13:48PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2019-04-10 18:59:52)
> > On Mon, Apr 08, 2019 at 06:05:06PM +0100, Chris Wilson wrote:
> > > Quoting Ville Syrjälä (2019-04-08 17:06:01)
> > > > On Mon, Apr 08, 2019 at 04:49:13PM +0100, Chris Wilson wrote:
> > > > > Quoting Ville Syrjala (2019-04-08 16:27:02)
> > > > > > -       /*
> > > > > > -        * Adjust the original formula to delay the division by 2^22 in order to
> > > > > > -        * minimize possible rounding errors.
> > > > > > -        */
> > > > > > -       tmp = (u64)m1 * m2_int * ref_clock +
> > > > > > -             (((u64)m1 * m2_frac * ref_clock) >> 22);
> > > > > > -       tmp = div_u64(tmp, 5 * div1 * div2);
> > > > > > -
> > > > > > -       return tmp;
> > > > > > +       return div_u64(mul_u32_u32(ref_clock * m1, m2),
> > > > > > +                      (5 * div1 * div2) << 22);
> > > > > 
> > > > > You say the denominator here is a u64, so do you not need to cast
> > > > > (u64)(5 * d1 * d2) to ensure it doesn't overflow the shift?
> > > > 
> > > > It should fit into u32. The maximum value should be
> > > > <= (5*0xf*0x7)<<22 based on the number of bits available
> > > 3b * 4b * 3b = 10b. So just fits.
> > > 
> > > Is it worth asserting those limits? Feels like it is running close, and
> > > will be subject to cargo-culting.
> > 
> > I suppose checking for it might be a good idea.
> > 
> > Just 'WARN_ON(5 * div1 * div2 >= 1 << 10)' maybe, or were you thinking
> > of something fancier?
> 
> How about something like
> struct {
> 	unsigned int div1 : 3;
> 	unsigned int div2 : 3;
> } d;
> 
> then with a bit of luck smatch will spot an overflow, and people might
> think twice when copying?
> 
> Even weirder,
> add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-29 (-29)
> Function                                     old     new   delta
> intel_ddi_get_config                        2377    2348     -29
> 
> I dread to look into the function to see how that changed gcc's mind.

I get 48 bytes with a 32bit build, but only if I let it inline that
function. With noinline there is no change apart from a few registers
getting shuffled around. gcc works in mysterious ways.

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