| Message ID | 20170527014209.186331-4-puthik@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
The patch looks good overall, it would have been easier to merge if you'd sent this as the first patch in this version. Some comments inline. On Fri, 2017-05-26 at 18:42 -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 divisor is still > within 25% of the desired value. > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > --- > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 79 +++++++++++++++++++++++++++ > 1 file changed, 79 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > index f55af41ce3bd..a8d485a29f29 100644 > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > @@ -116,6 +116,81 @@ 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 int 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; nit: unnecessary initialization for 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 -1; > + } > + > + fxp = DIV_ROUND_CLOSEST(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 > + * - FxP is within 25% of desired value. > + * Note: 25% is arbitrary value and may need some tweak. > + */ > + 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 -EIO; The error numbers are not propagated outside, so I don't see a real need for them. bool should suffice. > + } > + 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 -EIO; > + } > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > + > + fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4); > + fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4); > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) { > + DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n"); > + return -ERANGE; > + } > + > + for (pn = pn_max; pn >= pn_min; pn--) { > + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << 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"); > + return -EIO; > + } > + 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 -EIO; > + } > + return 0; > +} > + > static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > @@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > } > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) > + if (!intel_dp_aux_set_pwm_freq(connector)) nit: This doesn't read right to me, looking at the function name it seems like you are proceeding when it failed. > + 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) {
On Tue, May 30, 2017 at 8:40 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote: > The patch looks good overall, it would have been easier to merge if > you'd sent this as the first patch in this version. Some comments > inline. > > > Will re-order to make this the first patch in the next version. > > On Fri, 2017-05-26 at 18:42 -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 divisor is still > > within 25% of the desired value. > > > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > > --- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 79 > +++++++++++++++++++++++++++ > > 1 file changed, 79 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > index f55af41ce3bd..a8d485a29f29 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > > @@ -116,6 +116,81 @@ 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 int 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; > > nit: unnecessary initialization for f? > No. We only set f in for (pn = pn_max; pn >= pn_min; pn--) but compiler wouldn't know that pn_max >= pn_min so f might be uninitialized. > > + 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 -1; > > + } > > + > > + fxp = DIV_ROUND_CLOSEST(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 > > + * - FxP is within 25% of desired value. > > + * Note: 25% is arbitrary value and may need some tweak. > > + */ > > + 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 -EIO; > > The error numbers are not propagated outside, so I don't see a real need > for them. bool should suffice. > > Sure. > > > + } > > + 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 -EIO; > > + } > > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + > > + fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4); > > + fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4); > > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) { > > + DRM_DEBUG_KMS("VBT defined backlight frequency out of > range\n"); > > + return -ERANGE; > > + } > > + > > + for (pn = pn_max; pn >= pn_min; pn--) { > > + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << 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"); > > + return -EIO; > > + } > > + 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 -EIO; > > + } > > + return 0; > > +} > > + > > static void intel_dp_aux_enable_backlight(struct intel_connector > *connector) > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(&connector-> > encoder->base); > > @@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(struct > intel_connector *connector) > > DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > > } > > > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) > > + if (!intel_dp_aux_set_pwm_freq(connector)) > > nit: This doesn't read right to me, looking at the function name it > seems like you are proceeding when it failed. > > OK. > > > + 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) { > >
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index f55af41ce3bd..a8d485a29f29 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -116,6 +116,81 @@ 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 int 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 -1; + } + + fxp = DIV_ROUND_CLOSEST(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 + * - FxP is within 25% of desired value. + * Note: 25% is arbitrary value and may need some tweak. + */ + 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 -EIO; + } + 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 -EIO; + } + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; + + fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4); + fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4); + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) { + DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n"); + return -ERANGE; + } + + for (pn = pn_max; pn >= pn_min; pn--) { + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << 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"); + return -EIO; + } + 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 -EIO; + } + return 0; +} + static void intel_dp_aux_enable_backlight(struct intel_connector *connector) { struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); @@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) DRM_DEBUG_KMS("Enable dynamic brightness.\n"); } + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) + if (!intel_dp_aux_set_pwm_freq(connector)) + 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) {
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 divisor is still within 25% of the desired value. Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> --- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 79 +++++++++++++++++++++++++++ 1 file changed, 79 insertions(+)