| Message ID | 20190711004312.248160-1-furquan@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] i915: intel_dp_aux_backlight: Fix max backlight calculations | expand |
On Wed, 2019-07-10 at 17:43 -0700, Furquan Shaikh wrote: > Max backlight value for the panel was being calculated using byte > count i.e. 0xffff if 2 bytes are supported for backlight brightness > and 0xff if 1 byte is supported. However, EDP_PWMGEN_BIT_COUNT > determines the number of active control bits used for the brightness > setting. Thus, even if the panel uses 2 byte setting, it might not > use > all the control bits. Thus, max backlight should be set based on the > value of EDP_PWMGEN_BIT_COUNT instead of assuming 65535 or 255. > > Additionally, EDP_PWMGEN_BIT_COUNT was being updated based on the VBT > frequency which results in a different max backlight value. Thus, > setting of EDP_PWMGEN_BIT_COUNT is moved to setup phase instead of > enable so that max backlight can be calculated correctly. Only the > frequency divider is set during the enable phase using the value of > EDP_PWMGEN_BIT_COUNT. > > Signed-off-by: Furquan Shaikh <furquan@google.com> > Reviewed-by: Stéphane Marchesin <marcheu@chromium.org> Hi, Thank you for your patch. See comments inline. > --- > v2: In case of DPCD failure and pn being uninitialized, return > max_backlight as 0. > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 134 ++++++++++++-- > ---- > 1 file changed, 90 insertions(+), 44 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > index 357136f17f85..b3678b8a5b4d 100644 > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > @@ -110,61 +110,34 @@ static bool 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; > + int freq, fxp, f, fxp_actual, fxp_min, fxp_max; > + u8 pn; > > - /* 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 false; > } > > - 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"); > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, > + &pn) < 0) { > + DRM_DEBUG_KMS("Failed to read aux pwmgen bit count\n"); > return false; > } > - 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 false; > - } > - pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > - pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; > > + fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), > freq); > + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255); > + fxp_actual = f << pn; > + > + /* Ensure frequency is within 25% of desired value */ > 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 false; > - } > > - 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"); > + if (fxp_min > fxp_actual || fxp_actual > fxp_max) { > + DRM_DEBUG_KMS("Actual frequency out of range\n"); > return false; > } > + > 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"); > @@ -224,16 +197,89 @@ static void > intel_dp_aux_disable_backlight(const struct drm_connector_state *old > set_aux_backlight_enable(enc_to_intel_dp(old_conn_state- > >best_encoder), false); > } > > +static u32 intel_dp_aux_calc_max_backlight(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); > + u32 max_backlight = 0; > + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1; > + u8 pn, pn_min, pn_max; > + > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, > + &pn) != 1) > + return max_backlight; Should we really fail here and cause -ENODEV to be returned? We could simply try to go back to old fashion, i.e calculate pn using pn_min to pn_max algorithm as it is done currently, if lets say we couldn't read it or pn was garbage. > + > + /* 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 max_backlight; > + } > + 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 max_backlight; > + } > + 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 max_backlight; > + } If 25% of fxp is anyway an arbitrary value, can we just try to narrow this [fxp_min, fxp_max] to [1 << pn_min, 255 << pn_max] if it happens to be outside of the range? If fxp is within [1 << pn_min, 255 << pn_max] I guess we still can proceed. > + > + 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 max_backlight; > + } > + > + max_backlight = (1 << pn) - 1; > + > + return max_backlight; > +} > + > static int intel_dp_aux_setup_backlight(struct intel_connector > *connector, > enum pipe pipe) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&connector- > >encoder->base); > struct intel_panel *panel = &connector->panel; > > - if (intel_dp->edp_dpcd[2] & > DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > - panel->backlight.max = 0xFFFF; > - else > - panel->backlight.max = 0xFF; > + panel->backlight.max = > intel_dp_aux_calc_max_backlight(connector); > + > + if (!panel->backlight.max) > + return -ENODEV; Can we just fallback to old way here, instead of failure? I see previsouly intel_dp_aux_set_pwm_freq was called only if we had DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP set and it's failure didn't affect program flow that much. My concern is that we have quite a lot of different hardware, so bailing out that way could make our CI life a bit worse. See intel_dp_aux_enable_backlight: 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; > > panel->backlight.min = 0; > panel->backlight.level = intel_dp_aux_get_backlight(connector); Best Regards, Lisovskiy Stanislav
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index 357136f17f85..b3678b8a5b4d 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -110,61 +110,34 @@ static bool 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; + int freq, fxp, f, fxp_actual, fxp_min, fxp_max; + u8 pn; - /* 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 false; } - 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"); + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, + &pn) < 0) { + DRM_DEBUG_KMS("Failed to read aux pwmgen bit count\n"); return false; } - 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 false; - } - pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK; - pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK; + fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq); + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255); + fxp_actual = f << pn; + + /* Ensure frequency is within 25% of desired value */ 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 false; - } - 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"); + if (fxp_min > fxp_actual || fxp_actual > fxp_max) { + DRM_DEBUG_KMS("Actual frequency out of range\n"); return false; } + 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"); @@ -224,16 +197,89 @@ static void intel_dp_aux_disable_backlight(const struct drm_connector_state *old set_aux_backlight_enable(enc_to_intel_dp(old_conn_state->best_encoder), false); } +static u32 intel_dp_aux_calc_max_backlight(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); + u32 max_backlight = 0; + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1; + u8 pn, pn_min, pn_max; + + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, + &pn) != 1) + return max_backlight; + + pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK; + max_backlight = (1 << pn) - 1; + + /* 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 max_backlight; + } + + 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 max_backlight; + } + 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 max_backlight; + } + 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 max_backlight; + } + + 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 max_backlight; + } + + max_backlight = (1 << pn) - 1; + + return max_backlight; +} + static int intel_dp_aux_setup_backlight(struct intel_connector *connector, enum pipe pipe) { struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); struct intel_panel *panel = &connector->panel; - if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) - panel->backlight.max = 0xFFFF; - else - panel->backlight.max = 0xFF; + panel->backlight.max = intel_dp_aux_calc_max_backlight(connector); + + if (!panel->backlight.max) + return -ENODEV; panel->backlight.min = 0; panel->backlight.level = intel_dp_aux_get_backlight(connector);