<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 30, 2017 at 8:40 PM, Pandiyan, Dhinakaran <span dir="ltr"><<a href="mailto:dhinakaran.pandiyan@intel.com" target="_blank" class="gmail-cremed gmail-cremed cremed">dhinakaran.pandiyan@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The patch looks good overall, it would have been easier to merge if<br>
you'd sent this as the first patch in this version. Some comments<br>
inline.<br>
<div><div class="gmail-h5"><br>
<br></div></div></blockquote><div>Will re-order to make this the first patch in the next version. </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">
<br>
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:<br>
> Read desired PWM frequency from panel vbt and calculate the<br>
> value for divider in DPCD address 0x724 and 0x728 to have<br>
> as many bits as possible for PWM duty cyle for granularity of<br>
> brightness adjustment while the frequency divisor is still<br>
> within 25% of the desired value.<br>
><br>
> Signed-off-by: Puthikorn Voravootivat <<a href="mailto:puthik@chromium.org" class="gmail-cremed gmail-cremed cremed">puthik@chromium.org</a>><br>
> ---<br>
> drivers/gpu/drm/i915/intel_dp_<wbr>aux_backlight.c | 79 +++++++++++++++++++++++++++<br>
> 1 file changed, 79 insertions(+)<br>
><br>
> diff --git a/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> index f55af41ce3bd..a8d485a29f29 100644<br>
> --- a/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> +++ b/drivers/gpu/drm/i915/intel_<wbr>dp_aux_backlight.c<br>
> @@ -116,6 +116,81 @@ intel_dp_aux_set_dynamic_<wbr>backlight_percent(struct intel_dp *intel_dp,<br>
> }<br>
> }<br>
><br>
> +/*<br>
> + * Set PWM Frequency divider to match desired frequency in vbt.<br>
> + * The PWM Frequency is calculated as 27Mhz / (F x P).<br>
> + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the<br>
> + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)<br>
> + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the<br>
> + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)<br>
> + */<br>
> +static int intel_dp_aux_set_pwm_freq(<wbr>struct intel_connector *connector)<br>
> +{<br>
> + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);<br>
> + struct intel_dp *intel_dp = enc_to_intel_dp(&connector-><wbr>encoder->base);<br>
> + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;<br>
<br>
</div></div>nit: unnecessary initialization for f?<br></blockquote><div>No. We only set f in <span style="color:rgb(80,0,80)">for (pn = pn_max; pn >= pn_min; pn--)</span><span style="color:rgb(80,0,80)"> </span>but compiler wouldn't</div><div>know that pn_max >= pn_min so f might be uninitialized.</div><div><br></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"><br>
> + u8 pn, pn_min, pn_max;<br>
> +<br>
> + /* Find desired value of (F x P)<br>
> + * Note that, if F x P is out of supported range, the maximum value or<br>
> + * minimum value will applied automatically. So no need to check that.<br>
> + */<br>
> + freq = dev_priv->vbt.backlight.pwm_<wbr>freq_hz;<br>
> + DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);<br>
> + if (!freq) {<br>
> + DRM_DEBUG_KMS("Use panel default backlight frequency\n");<br>
> + return -1;<br>
> + }<br>
> +<br>
> + fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_<wbr>BACKLIGHT_FREQ_BASE_KHZ), freq);<br>
> +<br>
> + /* Use highest possible value of Pn for more granularity of brightness<br>
> + * adjustment while satifying the conditions below.<br>
> + * - Pn is in the range of Pn_min and Pn_max<br>
> + * - F is in the range of 1 and 255<br>
> + * - FxP is within 25% of desired value.<br>
> + * Note: 25% is arbitrary value and may need some tweak.<br>
> + */<br>
> + if (drm_dp_dpcd_readb(&intel_dp-><wbr>aux,<br>
> + DP_EDP_PWMGEN_BIT_COUNT_CAP_<wbr>MIN, &pn_min) != 1) {<br>
> + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");<br>
> + return -EIO;<br>
<br>
</div></div>The error numbers are not propagated outside, so I don't see a real need<br>
for them. bool should suffice.<br>
<div><div class="gmail-h5"><br></div></div></blockquote><div>Sure. </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">
<br>
> + }<br>
> + if (drm_dp_dpcd_readb(&intel_dp-><wbr>aux,<br>
> + DP_EDP_PWMGEN_BIT_COUNT_CAP_<wbr>MAX, &pn_max) != 1) {<br>
> + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");<br>
> + return -EIO;<br>
> + }<br>
> + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;<br>
> + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;<br>
> +<br>
> + fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);<br>
> + fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);<br>
> + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {<br>
> + DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n");<br>
> + return -ERANGE;<br>
> + }<br>
> +<br>
> + for (pn = pn_max; pn >= pn_min; pn--) {<br>
> + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);<br>
> + fxp_actual = f << pn;<br>
> + if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)<br>
> + break;<br>
> + }<br>
> +<br>
> + if (drm_dp_dpcd_writeb(&intel_dp-<wbr>>aux,<br>
> + DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {<br>
> + DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");<br>
> + return -EIO;<br>
> + }<br>
> + if (drm_dp_dpcd_writeb(&intel_dp-<wbr>>aux,<br>
> + DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {<br>
> + DRM_DEBUG_KMS("Failed to write aux backlight freq\n");<br>
> + return -EIO;<br>
> + }<br>
> + return 0;<br>
> +}<br>
> +<br>
> static void intel_dp_aux_enable_backlight(<wbr>struct intel_connector *connector)<br>
> {<br>
> struct intel_dp *intel_dp = enc_to_intel_dp(&connector-><wbr>encoder->base);<br>
> @@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(<wbr>struct intel_connector *connector)<br>
> DRM_DEBUG_KMS("Enable dynamic brightness.\n");<br>
> }<br>
><br>
> + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_<wbr>CAP)<br>
> + if (!intel_dp_aux_set_pwm_freq(<wbr>connector))<br>
<br>
</div></div>nit: This doesn't read right to me, looking at the function name it<br>
seems like you are proceeding when it failed.<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br></div></div></blockquote><div>OK. </div><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">
<br>
> + new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_<wbr>ENABLE;<br>
> +<br>
> if (new_dpcd_buf != dpcd_buf) {<br>
> if (drm_dp_dpcd_writeb(&intel_dp-<wbr>>aux,<br>
> DP_EDP_BACKLIGHT_MODE_SET_<wbr>REGISTER, new_dpcd_buf) < 0) {<br>
<br>
</div></div></blockquote></div><br></div></div>