| Message ID | 1455738073-14502-5-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Qua, 2016-02-17 às 21:41 +0200, ville.syrjala@linux.intel.com escreveu: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The reason for spcial casing 20MHz in the iclkip calculations is that > it would overflow the 7 bit divisor value. Let's rewrite the special > case to check for just that, and bump up auxdiv when needed. This > makes > the code work for freqeuencies close to but not exactly 20MHz. The > real > lower limit for auxdiv=0 is actually: > 172800000/(0x7f+2)*64)=~20930 kHz, and below that we must resort to > auxdiv=1. > > Actually this is all very theoretical since we limit the dotclock to > min 25MHz with CRT on all platforms. 25Mhz is actually the documented > limit in Bspec, so it seems we ought to never need to worry about the > auxdiv=1 case. But no harm in having it. I like the patch and it looks correct. The main "advantage" of the previous version is that it matched the spec exactly, so future code readers may be confused now. Maybe a little comment more explicitly explaining why it doesn't match the spec now would help: possibly some shorter version of your commit message, with the typos fixed. Anyway, git-bisect should be enough and I'm fine even without the comment: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++--------- > ---------- > 1 file changed, 19 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a3c959cd8b3b..5e7b22a31098 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3957,37 +3957,35 @@ static void lpt_disable_iclkip(struct > drm_i915_private *dev_priv) > /* Program iCLKIP clock to the desired frequency */ > static void lpt_program_iclkip(struct drm_crtc *crtc) > { > - struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > int clock = to_intel_crtc(crtc)->config- > >base.adjusted_mode.crtc_clock; > u32 divsel, phaseinc, auxdiv, phasedir = 0; > u32 temp; > > lpt_disable_iclkip(dev_priv); > > - /* 20MHz is a corner case which is out of range for the 7- > bit divisor */ > - if (clock == 20000) { > - auxdiv = 1; > - divsel = 0x41; > - phaseinc = 0x20; > - } else { > - /* The iCLK virtual clock root frequency is in MHz, > - * but the adjusted_mode->crtc_clock in in KHz. To > get the > - * divisors, it is necessary to divide one by > another, so we > - * convert the virtual clock precision to KHz here > for higher > - * precision. > - */ > + /* The iCLK virtual clock root frequency is in MHz, > + * but the adjusted_mode->crtc_clock in in KHz. To get the > + * divisors, it is necessary to divide one by another, so we > + * convert the virtual clock precision to KHz here for > higher > + * precision. > + */ > + for (auxdiv = 0; auxdiv < 2; auxdiv++) { > u32 iclk_virtual_root_freq = 172800 * 1000; > u32 iclk_pi_range = 64; > - u32 desired_divisor, msb_divisor_value, pi_value; > + u32 desired_divisor; > > - desired_divisor = > DIV_ROUND_CLOSEST(iclk_virtual_root_freq, clock); > - msb_divisor_value = desired_divisor / iclk_pi_range; > - pi_value = desired_divisor % iclk_pi_range; > + desired_divisor = > DIV_ROUND_CLOSEST(iclk_virtual_root_freq, > + clock << > auxdiv); > + divsel = (desired_divisor / iclk_pi_range) - 2; > + phaseinc = desired_divisor % iclk_pi_range; > > - auxdiv = 0; > - divsel = msb_divisor_value - 2; > - phaseinc = pi_value; > + /* > + * Near 20MHz is a corner case which is > + * out of range for the 7-bit divisor > + */ > + if (divsel <= 0x7f) > + break; > } > > /* This should not happen with any sane values */
On ke, 2016-02-17 at 21:41 +0200, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The reason for spcial casing 20MHz in the iclkip calculations is that > it would overflow the 7 bit divisor value. Let's rewrite the special > case to check for just that, and bump up auxdiv when needed. This > makes > the code work for freqeuencies close to but not exactly 20MHz. The > real > lower limit for auxdiv=0 is actually: > 172800000/(0x7f+2)*64)=~20930 kHz, and below that we must resort to > auxdiv=1. > > Actually this is all very theoretical since we limit the dotclock to > min 25MHz with CRT on all platforms. 25Mhz is actually the documented > limit in Bspec, so it seems we ought to never need to worry about the > auxdiv=1 case. But no harm in having it. > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Yep, fewer special casing is better. Btw, the spec could have just as well describe how to calculate aux_div instead of providing special cases and tables, I wonder why this is done at places. The patch looks good: Reviewed-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++--------- > ---------- > 1 file changed, 19 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a3c959cd8b3b..5e7b22a31098 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3957,37 +3957,35 @@ static void lpt_disable_iclkip(struct > drm_i915_private *dev_priv) > /* Program iCLKIP clock to the desired frequency */ > static void lpt_program_iclkip(struct drm_crtc *crtc) > { > - struct drm_device *dev = crtc->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > + struct drm_i915_private *dev_priv = to_i915(crtc->dev); > int clock = to_intel_crtc(crtc)->config- > >base.adjusted_mode.crtc_clock; > u32 divsel, phaseinc, auxdiv, phasedir = 0; > u32 temp; > > lpt_disable_iclkip(dev_priv); > > - /* 20MHz is a corner case which is out of range for the 7- > bit divisor */ > - if (clock == 20000) { > - auxdiv = 1; > - divsel = 0x41; > - phaseinc = 0x20; > - } else { > - /* The iCLK virtual clock root frequency is in MHz, > - * but the adjusted_mode->crtc_clock in in KHz. To > get the > - * divisors, it is necessary to divide one by > another, so we > - * convert the virtual clock precision to KHz here > for higher > - * precision. > - */ > + /* The iCLK virtual clock root frequency is in MHz, > + * but the adjusted_mode->crtc_clock in in KHz. To get the > + * divisors, it is necessary to divide one by another, so we > + * convert the virtual clock precision to KHz here for > higher > + * precision. > + */ > + for (auxdiv = 0; auxdiv < 2; auxdiv++) { > u32 iclk_virtual_root_freq = 172800 * 1000; > u32 iclk_pi_range = 64; > - u32 desired_divisor, msb_divisor_value, pi_value; > + u32 desired_divisor; > > - desired_divisor = > DIV_ROUND_CLOSEST(iclk_virtual_root_freq, clock); > - msb_divisor_value = desired_divisor / iclk_pi_range; > - pi_value = desired_divisor % iclk_pi_range; > + desired_divisor = > DIV_ROUND_CLOSEST(iclk_virtual_root_freq, > + clock << > auxdiv); > + divsel = (desired_divisor / iclk_pi_range) - 2; > + phaseinc = desired_divisor % iclk_pi_range; > > - auxdiv = 0; > - divsel = msb_divisor_value - 2; > - phaseinc = pi_value; > + /* > + * Near 20MHz is a corner case which is > + * out of range for the 7-bit divisor > + */ > + if (divsel <= 0x7f) > + break; > } > > /* This should not happen with any sane values */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index a3c959cd8b3b..5e7b22a31098 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3957,37 +3957,35 @@ static void lpt_disable_iclkip(struct drm_i915_private *dev_priv) /* Program iCLKIP clock to the desired frequency */ static void lpt_program_iclkip(struct drm_crtc *crtc) { - struct drm_device *dev = crtc->dev; - struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_i915_private *dev_priv = to_i915(crtc->dev); int clock = to_intel_crtc(crtc)->config->base.adjusted_mode.crtc_clock; u32 divsel, phaseinc, auxdiv, phasedir = 0; u32 temp; lpt_disable_iclkip(dev_priv); - /* 20MHz is a corner case which is out of range for the 7-bit divisor */ - if (clock == 20000) { - auxdiv = 1; - divsel = 0x41; - phaseinc = 0x20; - } else { - /* The iCLK virtual clock root frequency is in MHz, - * but the adjusted_mode->crtc_clock in in KHz. To get the - * divisors, it is necessary to divide one by another, so we - * convert the virtual clock precision to KHz here for higher - * precision. - */ + /* The iCLK virtual clock root frequency is in MHz, + * but the adjusted_mode->crtc_clock in in KHz. To get the + * divisors, it is necessary to divide one by another, so we + * convert the virtual clock precision to KHz here for higher + * precision. + */ + for (auxdiv = 0; auxdiv < 2; auxdiv++) { u32 iclk_virtual_root_freq = 172800 * 1000; u32 iclk_pi_range = 64; - u32 desired_divisor, msb_divisor_value, pi_value; + u32 desired_divisor; - desired_divisor = DIV_ROUND_CLOSEST(iclk_virtual_root_freq, clock); - msb_divisor_value = desired_divisor / iclk_pi_range; - pi_value = desired_divisor % iclk_pi_range; + desired_divisor = DIV_ROUND_CLOSEST(iclk_virtual_root_freq, + clock << auxdiv); + divsel = (desired_divisor / iclk_pi_range) - 2; + phaseinc = desired_divisor % iclk_pi_range; - auxdiv = 0; - divsel = msb_divisor_value - 2; - phaseinc = pi_value; + /* + * Near 20MHz is a corner case which is + * out of range for the 7-bit divisor + */ + if (divsel <= 0x7f) + break; } /* This should not happen with any sane values */