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 <furq...@google.com>
> Reviewed-by: Stéphane Marchesin <marc...@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
