<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, May 15, 2017 at 4:07 PM, Pandiyan, Dhinakaran <<a href="mailto:dhinakaran.pandiyan@intel.com" target="_blank" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Fri, 2017-05-12 at 17:31 -0700, Puthikorn Voravootivat wrote: > > > > On Fri, May 12, 2017 at 5:12 PM, Pandiyan, Dhinakaran > <<a href="mailto:dhinakaran.pandiyan@intel.com" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>> wrote: > On Thu, 2017-05-11 at 16:02 -0700, Puthikorn Voravootivat > wrote: > > Read desired PWM frequency from panel vbt and calculate the > > value for divider in DPCD address 0x724 and 0x728 to have > > as many bits as possible for PWM duty cyle for granularity > of > > brightness adjustment while the frequency is still within > 25% > > of the desired frequency. > > I read a few eDP panel data sheets, the PWM frequencies all > start from > ~200Hz. If the VBT chooses this lowest value to allow for more > brightness control, and then this patch lowers the value by > another 25%, > we'll end up below the panel allowed PWM frequency. > > In fact, one of the systems I checked had PWM frequency as > 200Hz in VBT > and the panel datasheet also had PWM frequency range starting > from > 200Hz. Have you considered this case? > > The spec said "A given LCD panel typically has a limited range of > backlight frequency capability. > To limit the programmable frequency range, limitations are placed on > the allowable total divider ratio with the Sink device" > So I think it should be auto cap to 200Hz in this case. > > -DK > > > > Signed-off-by: Puthikorn Voravootivat <<a href="mailto:puthik@chromium.org" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">puthik@chromium.org</a>> > > --- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 81 > +++++++++++++++++++++++++++ > > 1 file changed, 81 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > index 0b48851013cc..6f10a2f1ab76 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > @@ -113,12 +113,86 @@ > 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, fxp_min, fxp_max, fxp_actual, f = 1; > > + 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 = KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ) / freq; > > + > > + /* Use highest possible value of Pn for more > granularity of brightness > > + * adjustment while satifying the conditions below. > > + * - Pn is in the range of Pn_min and Pn_max > > + * - F is in the range of 1 and 255 > > + * - Effective frequency is within 25% of desired > frequency. > > + */ > > + 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; > > + > > + fxp_min = fxp * 3 / 4; > > + fxp_max = fxp * 5 / 4; > > > You are allowing fxp between +/- 25% of the actual. This isn't > same as > the "Effective frequency is within 25% of desired frequency." > right? The > frequency can vary between -20% and +33%. > > > You are right. > You want me to change commit message to reflect this or change the > code to > match the commit message? </div></div>I am okay with fixing the comment and commit message. Is the 25% arbitrary or based on the distances between the possible values? Please make a note in the comment if it's the former. > > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < > fxp_max) { > > + DRM_DEBUG_KMS("VBT defined backlight frequency > out of range\n"); > > + return; > > + } > > + > > + for (pn = pn_max; pn > pn_min; pn--) { Is there a reason this is not pn >= pn_min? </blockquote><div>This is bug because f value will be incorrect in the case that pn == pn_min.</div><div>Thanks for catching this.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > > + f = clamp(fxp >> pn, 1, 255); > > + fxp_actual = f << pn; > > + if (fxp_min <= fxp_actual && fxp_actual <= > fxp_max) > > + break; > > + } > > + > > + 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"); If the number of brightness control bits are changing, the max. brightness value changes too. Please see intel_dp_aux_setup_backlight(). <div><div class="gmail-h5"> </div></div></blockquote><div>I think this is fine because</div><div>- intel_dp_aux_setup_backlight() will called intel_dp_aux_enable_backlight() which </div><div>called intel_dp_aux_set_pwm_freq() first before determine the max brightness value.</div><div>- Also, the panel I tested does not change BACKLIGHT_BRIGHTNESS_BYTE_COUNT </div><div>when changing the frequency. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail-h5"> > > + 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) { > > @@ -151,6 +225,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) { > > @@ -158,6 +236,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>> _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">Intel-gfx@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="gmail-cremed gmail-cremed gmail-cremed gmail-cremed cremed">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </blockquote></div> </div></div>