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 --- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 132 ++++++++++++------ 1 file changed, 88 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..4636c8e8ae8a 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,87 @@ 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)) { + 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);
On Mon, 17 Jun 2019, Furquan Shaikh furquan@google.com 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.
The eDP aux backlight is another fine example of simple made difficult. Ugh.
Shawn (Cc'd) has recently submitted patches to this code. Shawn, please also look through this and provide your comments, if any.
One comment inline.
Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 132 ++++++++++++------ 1 file changed, 88 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..4636c8e8ae8a 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");
- 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");- 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,87 @@ 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)) {
pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;max_backlight = (1 << pn) - 1;- }
If the dpcd read fails, pn may be uninitialized; need to check the return value properly here.
Otherwise, seems fine, though unnecessarily complicated but that's hardly your fault...
BR, Jani.
- /* 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);
On Tue, 25 Jun 2019, Jani Nikula jani.nikula@linux.intel.com wrote:
On Mon, 17 Jun 2019, Furquan Shaikh furquan@google.com 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.
The eDP aux backlight is another fine example of simple made difficult. Ugh.
Shawn (Cc'd) has recently submitted patches to this code. Shawn, please also look through this and provide your comments, if any.
From my point for view, this change give the correct backlight.max value.
Just like what driver did in intel_panel.c to retrieve PWM freq from VBT and convert to max backlight value. This should avoid driver configure brightness level that over TCON capability to cause unexpected issue on display backlight.
Best regards, Shawn
One comment inline.
Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-by: Stéphane Marchesin marcheu@chromium.org
drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 132 ++++++++++++------ 1 file changed, 88 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..4636c8e8ae8a 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");
- 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");- 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,87 @@ 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)) {
pn &= DP_EDP_PWMGEN_BIT_COUNT_MASK;max_backlight = (1 << pn) - 1;- }
If the dpcd read fails, pn may be uninitialized; need to check the return value properly here.
Otherwise, seems fine, though unnecessarily complicated but that's hardly your fault...
BR, Jani.
- /* 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);
-- Jani Nikula, Intel Open Source Graphics Center
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 --- 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; + + 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);
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 maximumvalue or
* minimum value will applied automatically. So no need tocheck that.
*/- 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 capmin\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");
- 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 capmax\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 ofrange\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 bitcount\n");
- if (fxp_min > fxp_actual || fxp_actual > fxp_max) {
DRM_DEBUG_KMS("Actual frequency out of range\n");- 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 capmin\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 capmax\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 ofrange\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 bitcount\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
dri-devel@lists.freedesktop.org
-
Furquan Shaikh -
Jani Nikula -
Lee, Shawn C -
Lisovskiy, Stanislav