<div dir="ltr">Actually you are right. Sorry it's my mistake.<div>I was focusing on make the actual frequency match the desired frequency as much as possible. </div><div>But more granularity in backlight adjustment probably more important than that.</div><div> </div><div>Will submit new version of this patch to fix this.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Mon, May 8, 2017 at 11:17 AM, Pandiyan, Dhinakaran <<a href="mailto:dhinakaran.pandiyan@intel.com" target="_blank">dhinakaran.pandiyan@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, 2017-05-08 at 10:49 -0700, Puthikorn Voravootivat wrote: > This is not related to brightness control. This calculation is used to > set the PWM frequency. > Frequency = 27 Mhz / (F * 2^ Pn) > > Lower Pn means higher value for F which mean more accuracy for this > calculation. I am not sure I follow this. Quoting from the spec - "A larger Pn value (meaning more PWM generator control bits) provides a finer backlight adjustment (increased granularity), but also limits the maximum backlight frequency, as described in this section." and then again "Allowing Pn to be adjustable provides the flexibility of backlight dimming granularity vs. maximum backlight frequency." <div class="HOEnZb"><div class="h5"> > > On Sat, May 6, 2017 at 1:35 AM, Pandiyan, Dhinakaran > <<a href="mailto:dhinakaran.pandiyan@intel.com">dhinakaran.pandiyan@intel.com</a>> wrote: > On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat > wrote: > > Read desired PWM frequency from panel vbt and calculate the > > value for divider in DPCD address 0x724 and 0x728 to match > > that frequency as close as possible. > > > > Signed-off-by: Puthikorn Voravootivat <<a href="mailto:puthik@chromium.org">puthik@chromium.org</a>> > > --- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 71 > +++++++++++++++++++++++++++ > > 1 file changed, 71 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > index fc26fea94fd4..441ad434a82b 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > @@ -113,12 +113,76 @@ > intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp > *intel_dp, > > } > > } > > > > +/* > > + * Set PWM Frequency divider to match desired frequency in > vbt. > > + * The PWM Frequency is calculated as 27Mhz / (F x P). > > + * - Where F = PWM Frequency Pre-Divider value programmed > by field 7:0 of the > > + * EDP_BACKLIGHT_FREQ_SET register (DPCD > Address 00728h) > > + * - Where P = 2^Pn, where Pn is the value programmed by > field 4:0 of the > > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address > 00724h) > > + */ > > +static void intel_dp_aux_set_pwm_freq(struct > intel_connector *connector) > > +{ > > + struct drm_i915_private *dev_priv = > to_i915(connector->base.dev); > > + struct intel_dp *intel_dp = > enc_to_intel_dp(&connector->encoder->base); > > + int freq, fxp, f; > > + u8 pn, pn_min, pn_max; > > + > > + /* Find desired value of (F x P) > > + * Note that, if F x P is out of supported range, the > maximum value or > > + * minimum value will applied automatically. So no > need to check that. > > + */ > > + freq = dev_priv->vbt.backlight.pwm_freq_hz; > > + DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz > \n", freq); > > + if (!freq) { > > + DRM_DEBUG_KMS("Use panel default backlight > frequency\n"); > > + return; > > + } > > + > > + fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq; > > + > > + /* Use lowest possible value of Pn to try to make F to > be between 1 and > > > What's the reason to use the lowest possible value of Pn? From > what I > understand, choosing a higher value of Pn offers more steps > for > brightness control. > > -DK > > > + * 255 while still in the range Pn_min and Pn_max > > + */ > > + if (drm_dp_dpcd_readb(&intel_dp->aux, > > + > DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min) != 1) { > > + DRM_DEBUG_KMS("Failed to read pwmgen bit count > cap min\n"); > > + return; > > + } > > + if (drm_dp_dpcd_readb(&intel_dp->aux, > > + > DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) { > > + DRM_DEBUG_KMS("Failed to read pwmgen bit count > cap max\n"); > > + return; > > + } > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + > > + f = fxp >> pn_min; > > + > > + for (pn = pn_min; pn < pn_max && f > 255; pn++) > > + f >>= 1; > > + > > + f = clamp(f, 1, 255); > > + > > + if (drm_dp_dpcd_writeb(&intel_dp->aux, > > + DP_EDP_PWMGEN_BIT_COUNT, pn) < > 0) { > > + DRM_DEBUG_KMS("Failed to write aux pwmgen bit > count\n"); > > + return; > > + } > > + if (drm_dp_dpcd_writeb(&intel_dp->aux, > > + DP_EDP_BACKLIGHT_FREQ_SET, (u8) > f) < 0) { > > + DRM_DEBUG_KMS("Failed to write aux backlight > freq\n"); > > + return; > > + } > > +} > > + > > static void intel_dp_aux_enable_backlight(struct > intel_connector *connector) > > { > > struct intel_dp *intel_dp = > enc_to_intel_dp(&connector->encoder->base); > > uint8_t dpcd_buf = 0; > > uint8_t new_dpcd_buf = 0; > > uint8_t edp_backlight_mode = 0; > > + bool freq_cap; > > > > if (drm_dp_dpcd_readb(&intel_dp->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > &dpcd_buf) != 1) { > > @@ -150,6 +214,10 @@ static void > intel_dp_aux_enable_backlight(struct intel_connector > *connector) > > DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > > } > > > > + freq_cap = intel_dp->edp_dpcd[2] & > DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP; > > + if (freq_cap) > > + new_dpcd_buf |= > DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE; > > + > > if (new_dpcd_buf != dpcd_buf) { > > if (drm_dp_dpcd_writeb(&intel_dp->aux, > > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, > new_dpcd_buf) < 0) { > > @@ -157,6 +225,9 @@ static void > intel_dp_aux_enable_backlight(struct intel_connector > *connector) > > } > > } > > > > + if (freq_cap) > > + intel_dp_aux_set_pwm_freq(connector); > > + > > set_aux_backlight_enable(intel_dp, true); > > intel_dp_aux_set_backlight(connector, > connector->panel.backlight.level); > > } > > > > </div></div><div class="HOEnZb"><div class="h5">> _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> _______________________________________________ Intel-gfx mailing list <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </div></div></blockquote></div> </div>