This patch set contain 3 patches. Another 6 patches in previous version was already merged in v7 and v9. - First patch adds heuristic to determine whether we should use AUX or PWM pin to adjust panel backlight brightness. - Next patch adds support for dynamic brightness. - Last patch sets the PWM freqency to match data in panel vbt.
Change log: v10: - Add heuristic in patch #1 - Add _unsafe mod option in patch #1, #2 - handle frequency set error in patch #3
v9: - Fix nits in v8
v8: - Drop 4 patches that was already merged - Move DP_EDP_BACKLIGHT_AUX_ENABLE_CAP check to patch #2 to avoid behavior change if only apply patch #1 - Add TODO to warn about enable backlight twice in patch #2 - Use DIV_ROUND_CLOSEST instead of just "/" in patch #5 - Fix bug calculate pn in patch #5 - Clarify commit message / code comment in patch #5
v7: - Add check in intel_dp_pwm_pin_display_control_capable in patch #4 - Add option in patch #6 to enable DPCD or not - Change definition in patch #8 and implementation in #9 to use Khz - Fix compiler warning from build bot in patch #9
v6: - Address review from Dhinakaran - Make PWM frequency to have highest value of Pn that make the frequency still within 25% of desired frequency.
v5: - Split first patch in v4 to 3 patches - Bump priority for "Correctly enable backlight brightness adjustment via DPCD" - Make logic clearer for the case that both PWM pin and AUX are supported - Add more log when write to register fail - Add log when enable DBC
v4: - Rebase / minor typo fix.
v3: - Add new implementation of PWM frequency patch
v2: - Drop PWM frequency patch - Address suggestion from Jani Nikula
Puthikorn Voravootivat (3): drm/i915: Add heuristic to determine better way to adjust brightness drm/i915: Add option to support dynamic backlight via DPCD drm/i915: Set PWM divider to match desired frequency in vbt
drivers/gpu/drm/i915/i915_params.c | 12 +- drivers/gpu/drm/i915/i915_params.h | 5 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 185 ++++++++++++++++++++++++-- 3 files changed, 186 insertions(+), 16 deletions(-)
Add heuristic to decide that AUX or PWM pin should use for backlight brightness adjustment and modify i915 param description to have auto, force disable, and force enable.
The heuristic to determine that using AUX pin is better than using PWM pin is that the panel support any of the feature list here. - Regional backlight brightness adjustment - Backlight PWM frequency set - More than 8 bits resolution of brightness level - Backlight enablement via AUX and not by BL_ENABLE pin
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org --- drivers/gpu/drm/i915/i915_params.c | 7 +-- drivers/gpu/drm/i915/i915_params.h | 2 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b6a7e363d076..3758ae1f11b4 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = { .huc_firmware_path = NULL, .enable_dp_mst = true, .inject_load_failure = 0, - .enable_dpcd_backlight = false, + .enable_dpcd_backlight = -1, .enable_gvt = false, };
@@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst, module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400); MODULE_PARM_DESC(inject_load_failure, "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600); +module_param_named_unsafe(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600); MODULE_PARM_DESC(enable_dpcd_backlight, - "Enable support for DPCD backlight control (default:false)"); + "Enable support for DPCD backlight control " + "(-1:auto (default), 0:force disable, 1:force enabled if supported");
module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 34148cc8637c..ac02efce6e22 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -66,7 +66,7 @@ func(bool, verbose_state_checks); \ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ - func(bool, enable_dpcd_backlight); \ + func(int, enable_dpcd_backlight); \ func(bool, enable_gvt)
#define MEMBER(T, member) T member diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index a0995c00fc84..c89aae804659 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable) else reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
+ /* TODO: If the panel also support enabling backlight via BL_ENABLE pin, + * the backlight will be enabled again in _intel_edp_backlight_on() + */ if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val) != 1) { DRM_DEBUG_KMS("Failed to %s aux backlight\n", @@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector) /* Check the eDP Display control capabilities registers to determine if * the panel can support backlight control over the aux channel */ - if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP && - (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) && - !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) { + if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) && + (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) { DRM_DEBUG_KMS("AUX Backlight Control Supported!\n"); return true; } return false; }
+/* + * Heuristic function whether we should use AUX for backlight adjustment or not. + * + * We should use AUX for backlight brightness adjustment if panel doesn't this + * via PWM pin or using AUX is better than using PWM pin. + * + * The heuristic to determine that using AUX pin is better than using PWM pin is + * that the panel support any of the feature list here. + * - Regional backlight brightness adjustment + * - Backlight PWM frequency set + * - More than 8 bits resolution of brightness level + * - Backlight enablement via AUX and not by BL_ENABLE pin + * + * If all above are not true, assume that using PWM pin is better. + */ +static bool +intel_dp_aux_display_control_heuristic(struct intel_connector *connector) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); + uint8_t reg_val; + + /* Panel doesn't support adjusting backlight brightness via PWN pin */ + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) + return true; + + /* Panel supports regional backlight brightness adjustment */ + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3, + ®_val) != 1) { + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n", + DP_EDP_GENERAL_CAP_3); + return false; + } + if (reg_val > 0) + return true; + + /* Panel supports backlight PWM frequency set */ + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) + return true; + + /* Panel supports more than 8 bits resolution of brightness level */ + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) + return true; + + /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */ + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)) + return true; + + return false; + +} + int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) { struct intel_panel *panel = &intel_connector->panel; @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) if (!intel_dp_aux_display_control_capable(intel_connector)) return -ENODEV;
+ if (i915.enable_dpcd_backlight == -1 && + !intel_dp_aux_display_control_heuristic(intel_connector)) + return -ENODEV; + panel->backlight.setup = intel_dp_aux_setup_backlight; panel->backlight.enable = intel_dp_aux_enable_backlight; panel->backlight.disable = intel_dp_aux_disable_backlight;
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
Add heuristic to decide that AUX or PWM pin should use for backlight brightness adjustment and modify i915 param description to have auto, force disable, and force enable.
The heuristic to determine that using AUX pin is better than using PWM pin is that the panel support any of the feature list here.
- Regional backlight brightness adjustment
- Backlight PWM frequency set
- More than 8 bits resolution of brightness level
- Backlight enablement via AUX and not by BL_ENABLE pin
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org
drivers/gpu/drm/i915/i915_params.c | 7 +-- drivers/gpu/drm/i915/i915_params.h | 2 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b6a7e363d076..3758ae1f11b4 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = { .huc_firmware_path = NULL, .enable_dp_mst = true, .inject_load_failure = 0,
- .enable_dpcd_backlight = false,
- .enable_dpcd_backlight = -1, .enable_gvt = false,
};
@@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst, module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400); MODULE_PARM_DESC(inject_load_failure, "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600); +module_param_named_unsafe(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600); MODULE_PARM_DESC(enable_dpcd_backlight,
- "Enable support for DPCD backlight control (default:false)");
- "Enable support for DPCD backlight control "
- "(-1:auto (default), 0:force disable, 1:force enabled if supported");
module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 34148cc8637c..ac02efce6e22 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -66,7 +66,7 @@ func(bool, verbose_state_checks); \ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \
- func(bool, enable_dpcd_backlight); \
- func(int, enable_dpcd_backlight); \ func(bool, enable_gvt)
#define MEMBER(T, member) T member diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index a0995c00fc84..c89aae804659 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable) else reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
- /* TODO: If the panel also support enabling backlight via BL_ENABLE pin,
* the backlight will be enabled again in _intel_edp_backlight_on()*/
Unrelated hunk, please remove. This should have been included in one of the previous patches.
if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val) != 1) { DRM_DEBUG_KMS("Failed to %s aux backlight\n", @@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector) /* Check the eDP Display control capabilities registers to determine if * the panel can support backlight control over the aux channel */
- if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
- if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {}
+/*
- Heuristic function whether we should use AUX for backlight adjustment or not.
- We should use AUX for backlight brightness adjustment if panel doesn't this
- via PWM pin or using AUX is better than using PWM pin.
- The heuristic to determine that using AUX pin is better than using PWM pin is
- that the panel support any of the feature list here.
- Regional backlight brightness adjustment
- Backlight PWM frequency set
- More than 8 bits resolution of brightness level
- Backlight enablement via AUX and not by BL_ENABLE pin
- If all above are not true, assume that using PWM pin is better.
- */
+static bool +intel_dp_aux_display_control_heuristic(struct intel_connector *connector) +{
- struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
- uint8_t reg_val;
- /* Panel doesn't support adjusting backlight brightness via PWN pin */
- if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ?
return true;- /* Panel supports regional backlight brightness adjustment */
- if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
Spec says this register is new to eDP 1.4. I don't see a check for version.
®_val) != 1) {DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",DP_EDP_GENERAL_CAP_3);return false;- }
- if (reg_val > 0)
return true;- /* Panel supports backlight PWM frequency set */
- if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
return true;- /* Panel supports more than 8 bits resolution of brightness level */
- if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
return true;
I don't know if more than 8 bits is relatively better than the resolution PWM pin allows. I guess, we could fix that later by comparing the number of brightness levels.
- /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */
- if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP))return true;- return false;
+}
int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) { struct intel_panel *panel = &intel_connector->panel; @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) if (!intel_dp_aux_display_control_capable(intel_connector)) return -ENODEV;
- if (i915.enable_dpcd_backlight == -1 &&
!intel_dp_aux_display_control_heuristic(intel_connector))return -ENODEV;- panel->backlight.setup = intel_dp_aux_setup_backlight; panel->backlight.enable = intel_dp_aux_enable_backlight; panel->backlight.disable = intel_dp_aux_disable_backlight;
On Fri, 2017-06-02 at 17:42 +0000, Pandiyan, Dhinakaran wrote:
Somehow the CC's got removed in my previous reply, adding them back. See one additional comment below.
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
Add heuristic to decide that AUX or PWM pin should use for backlight brightness adjustment and modify i915 param description to have auto, force disable, and force enable.
The heuristic to determine that using AUX pin is better than using PWM pin is that the panel support any of the feature list here.
- Regional backlight brightness adjustment
- Backlight PWM frequency set
- More than 8 bits resolution of brightness level
- Backlight enablement via AUX and not by BL_ENABLE pin
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org
drivers/gpu/drm/i915/i915_params.c | 7 +-- drivers/gpu/drm/i915/i915_params.h | 2 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b6a7e363d076..3758ae1f11b4 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = { .huc_firmware_path = NULL, .enable_dp_mst = true, .inject_load_failure = 0,
- .enable_dpcd_backlight = false,
- .enable_dpcd_backlight = -1, .enable_gvt = false,
};
@@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst, module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400); MODULE_PARM_DESC(inject_load_failure, "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600); +module_param_named_unsafe(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600); MODULE_PARM_DESC(enable_dpcd_backlight,
- "Enable support for DPCD backlight control (default:false)");
- "Enable support for DPCD backlight control "
- "(-1:auto (default), 0:force disable, 1:force enabled if supported");
module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 34148cc8637c..ac02efce6e22 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -66,7 +66,7 @@ func(bool, verbose_state_checks); \ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \
- func(bool, enable_dpcd_backlight); \
- func(int, enable_dpcd_backlight); \
Please move this above the bools, see comment in code that's a few lines above.
-DK
func(bool, enable_gvt)
#define MEMBER(T, member) T member diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index a0995c00fc84..c89aae804659 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable) else reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
- /* TODO: If the panel also support enabling backlight via BL_ENABLE pin,
* the backlight will be enabled again in _intel_edp_backlight_on()*/Unrelated hunk, please remove. This should have been included in one of the previous patches.
if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val) != 1) { DRM_DEBUG_KMS("Failed to %s aux backlight\n", @@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector) /* Check the eDP Display control capabilities registers to determine if * the panel can support backlight control over the aux channel */
- if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
- if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {}
+/*
- Heuristic function whether we should use AUX for backlight adjustment or not.
- We should use AUX for backlight brightness adjustment if panel doesn't this
- via PWM pin or using AUX is better than using PWM pin.
- The heuristic to determine that using AUX pin is better than using PWM pin is
- that the panel support any of the feature list here.
- Regional backlight brightness adjustment
- Backlight PWM frequency set
- More than 8 bits resolution of brightness level
- Backlight enablement via AUX and not by BL_ENABLE pin
- If all above are not true, assume that using PWM pin is better.
- */
+static bool +intel_dp_aux_display_control_heuristic(struct intel_connector *connector) +{
- struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
- uint8_t reg_val;
- /* Panel doesn't support adjusting backlight brightness via PWN pin */
- if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ?
return true;- /* Panel supports regional backlight brightness adjustment */
- if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
Spec says this register is new to eDP 1.4. I don't see a check for version.
®_val) != 1) {DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",DP_EDP_GENERAL_CAP_3);return false;- }
- if (reg_val > 0)
return true;- /* Panel supports backlight PWM frequency set */
- if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
return true;- /* Panel supports more than 8 bits resolution of brightness level */
- if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT)
return true;I don't know if more than 8 bits is relatively better than the resolution PWM pin allows. I guess, we could fix that later by comparing the number of brightness levels.
- /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */
- if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP))return true;- return false;
+}
int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) { struct intel_panel *panel = &intel_connector->panel; @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) if (!intel_dp_aux_display_control_capable(intel_connector)) return -ENODEV;
- if (i915.enable_dpcd_backlight == -1 &&
!intel_dp_aux_display_control_heuristic(intel_connector))return -ENODEV;- panel->backlight.setup = intel_dp_aux_setup_backlight; panel->backlight.enable = intel_dp_aux_enable_backlight; panel->backlight.disable = intel_dp_aux_disable_backlight;
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jun 2, 2017 at 10:56 AM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote:
On Fri, 2017-06-02 at 17:42 +0000, Pandiyan, Dhinakaran wrote:
Somehow the CC's got removed in my previous reply, adding them back. See one additional comment below.
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
Add heuristic to decide that AUX or PWM pin should use for backlight brightness adjustment and modify i915 param description to have auto, force disable, and force enable.
The heuristic to determine that using AUX pin is better than using PWM pin is that the panel support any of the feature list here.
- Regional backlight brightness adjustment
- Backlight PWM frequency set
- More than 8 bits resolution of brightness level
- Backlight enablement via AUX and not by BL_ENABLE pin
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org
drivers/gpu/drm/i915/i915_params.c | 7 +-- drivers/gpu/drm/i915/i915_params.h | 2 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64
+++++++++++++++++++++++++--
3 files changed, 66 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index b6a7e363d076..3758ae1f11b4 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = { .huc_firmware_path = NULL, .enable_dp_mst = true, .inject_load_failure = 0,
- .enable_dpcd_backlight = false,
- .enable_dpcd_backlight = -1, .enable_gvt = false,
};
@@ -246,9 +246,10 @@ MODULE_PARM_DESC(enable_dp_mst, module_param_named_unsafe(inject_load_failure,
i915.inject_load_failure, uint, 0400);
MODULE_PARM_DESC(inject_load_failure, "Force an error after a number of failure check points (0:disabled
(default), N:force failure at the Nth failure check point)");
-module_param_named(enable_dpcd_backlight,
i915.enable_dpcd_backlight, bool, 0600);
+module_param_named_unsafe(enable_dpcd_backlight,
i915.enable_dpcd_backlight, int, 0600);
MODULE_PARM_DESC(enable_dpcd_backlight,
- "Enable support for DPCD backlight control (default:false)");
- "Enable support for DPCD backlight control "
- "(-1:auto (default), 0:force disable, 1:force enabled if
supported");
module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, diff --git a/drivers/gpu/drm/i915/i915_params.h
b/drivers/gpu/drm/i915/i915_params.h
index 34148cc8637c..ac02efce6e22 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -66,7 +66,7 @@ func(bool, verbose_state_checks); \ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \
- func(bool, enable_dpcd_backlight); \
- func(int, enable_dpcd_backlight); \
Please move this above the bools, see comment in code that's a few lines above.
Will do
-DK
func(bool, enable_gvt)#define MEMBER(T, member) T member diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index a0995c00fc84..c89aae804659 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct
intel_dp *intel_dp, bool enable)
else reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
- /* TODO: If the panel also support enabling backlight via
BL_ENABLE pin,
- the backlight will be enabled again in _intel_edp_backlight_on()
- */
Unrelated hunk, please remove. This should have been included in one of the previous patches.
Removed
if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
reg_val) != 1) { DRM_DEBUG_KMS("Failed to %s aux backlight\n",@@ -168,15 +171,66 @@ intel_dp_aux_display_control_capable(struct
intel_connector *connector)
/* Check the eDP Display control capabilities registers todetermine if
* the panel can support backlight control over the aux channel */
- if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
&&
(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)&&
!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)){
- if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP)
&&
(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)){
DRM_DEBUG_KMS("AUX Backlight Control Supported!\n"); return true; } return false;}
+/*
- Heuristic function whether we should use AUX for backlight
adjustment or not.
- We should use AUX for backlight brightness adjustment if panel
doesn't this
- via PWM pin or using AUX is better than using PWM pin.
- The heuristic to determine that using AUX pin is better than using
PWM pin is
- that the panel support any of the feature list here.
- Regional backlight brightness adjustment
- Backlight PWM frequency set
- More than 8 bits resolution of brightness level
- Backlight enablement via AUX and not by BL_ENABLE pin
- If all above are not true, assume that using PWM pin is better.
- */
+static bool +intel_dp_aux_display_control_heuristic(struct intel_connector
*connector)
+{
- struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
encoder->base);
- uint8_t reg_val;
- /* Panel doesn't support adjusting backlight brightness via PWN
pin */
- if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_
PWM_PIN_CAP))
Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ?
Thank you for catching this.
return true;- /* Panel supports regional backlight brightness adjustment */
- if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
Spec says this register is new to eDP 1.4. I don't see a check for version.
There is no need to check this. In not supported eDP panel, this register will read all 0s.
®_val) != 1) {DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",DP_EDP_GENERAL_CAP_3);return false;- }
- if (reg_val > 0)
return true;- /* Panel supports backlight PWM frequency set */
- if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
return true;- /* Panel supports more than 8 bits resolution of brightness level
*/
- if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_
BYTE_COUNT)
return true;I don't know if more than 8 bits is relatively better than the resolution PWM pin allows. I guess, we could fix that later by comparing the number of brightness levels.
- /* Panel supports enabling backlight via AUX but not by BL_ENABLE
pin */
- if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP))return true;- return false;
+}
int intel_dp_aux_init_backlight_funcs(struct intel_connector
*intel_connector)
{ struct intel_panel *panel = &intel_connector->panel; @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct
intel_connector *intel_connector)
if (!intel_dp_aux_display_control_capable(intel_connector)) return -ENODEV;
- if (i915.enable_dpcd_backlight == -1 &&
!intel_dp_aux_display_control_heuristic(intel_connector))return -ENODEV;- panel->backlight.setup = intel_dp_aux_setup_backlight; panel->backlight.enable = intel_dp_aux_enable_backlight; panel->backlight.disable = intel_dp_aux_disable_backlight;
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
This patch adds option to enable dynamic backlight for eDP panel that supports this feature via DPCD register and set minimum / maximum brightness to 0% and 100% of the normal brightness.
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org --- drivers/gpu/drm/i915/i915_params.c | 5 ++++ drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42 ++++++++++++++++++++++----- 3 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 3758ae1f11b4..ce033d58134e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = { .inject_load_failure = 0, .enable_dpcd_backlight = -1, .enable_gvt = false, + .enable_dbc = false, };
module_param_named(modeset, i915.modeset, int, 0400); @@ -254,3 +255,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, "Enable support for Intel GVT-g graphics virtualization host support(default:false)"); + +module_param_named_unsafe(enable_dbc, i915.enable_dbc, bool, 0600); +MODULE_PARM_DESC(enable_dbc, + "Enable support for dynamic backlight control (default:false)"); diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index ac02efce6e22..2de3e2850b54 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -67,7 +67,8 @@ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ func(int, enable_dpcd_backlight); \ - func(bool, enable_gvt) + func(bool, enable_gvt); \ + func(bool, enable_dbc)
#define MEMBER(T, member) T member struct i915_params { diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index c89aae804659..f55af41ce3bd 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level) } }
+/* + * Set minimum / maximum dynamic brightness percentage. This value is expressed + * as the percentage of normal brightness in 5% increments. + */ +static void +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp, + u32 min, u32 max) +{ + u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5) }; + + if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET, + dbc, sizeof(dbc)) < 0) { + DRM_DEBUG_KMS("Failed to write aux DBC brightness level\n"); + } +} + static void intel_dp_aux_enable_backlight(struct intel_connector *connector) { struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); - uint8_t dpcd_buf = 0; - uint8_t edp_backlight_mode = 0; + uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode;
if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) { @@ -113,18 +128,15 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) return; }
+ new_dpcd_buf = dpcd_buf; edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
switch (edp_backlight_mode) { case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT: - dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; - dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; - if (drm_dp_dpcd_writeb(&intel_dp->aux, - DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) { - DRM_DEBUG_KMS("Failed to write aux backlight mode\n"); - } + new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; + new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; break;
/* Do nothing when it is already DPCD mode */ @@ -133,6 +145,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) break; }
+ if (i915.enable_dbc && + (intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP)) { + new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE; + intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100); + DRM_DEBUG_KMS("Enable dynamic brightness.\n"); + } + + if (new_dpcd_buf != dpcd_buf) { + if (drm_dp_dpcd_writeb(&intel_dp->aux, + DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) { + DRM_DEBUG_KMS("Failed to write aux backlight mode\n"); + } + } + set_aux_backlight_enable(intel_dp, true); intel_dp_aux_set_backlight(connector, connector->panel.backlight.level); }
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
This patch adds option to enable dynamic backlight for eDP panel that supports this feature via DPCD register and set minimum / maximum brightness to 0% and 100% of the normal brightness.
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org
drivers/gpu/drm/i915/i915_params.c | 5 ++++ drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42 ++++++++++++++++++++++----- 3 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 3758ae1f11b4..ce033d58134e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = { .inject_load_failure = 0, .enable_dpcd_backlight = -1, .enable_gvt = false,
- .enable_dbc = false,
Based on Daniel's earlier comments on module parameters, shouldn't this be enabled by default too?
Or even more importantly, is this the right approach to enable/disable dynamic back light control? The reason I recommended having some sort of control to disable/enable is that the eDP spec. says the feature can have user visible impact.
Table 10-3 Display Control Capabilities
"While the DBC control bit’s function is defined in this Standard, DBC implementation specifics are not defined, including interaction with other DPCD register settings. The DBC implementation, visual performance, and power savings characteristics can differ between specific panels."
-DK
};
module_param_named(modeset, i915.modeset, int, 0400); @@ -254,3 +255,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, "Enable support for Intel GVT-g graphics virtualization host support(default:false)");
+module_param_named_unsafe(enable_dbc, i915.enable_dbc, bool, 0600); +MODULE_PARM_DESC(enable_dbc,
- "Enable support for dynamic backlight control (default:false)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index ac02efce6e22..2de3e2850b54 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -67,7 +67,8 @@ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ func(int, enable_dpcd_backlight); \
- func(bool, enable_gvt)
- func(bool, enable_gvt); \
- func(bool, enable_dbc)
#define MEMBER(T, member) T member struct i915_params { diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index c89aae804659..f55af41ce3bd 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level) } }
+/*
- Set minimum / maximum dynamic brightness percentage. This value is expressed
- as the percentage of normal brightness in 5% increments.
- */
+static void +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
u32 min, u32 max)+{
- u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5) };
- if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
dbc, sizeof(dbc)) < 0) {DRM_DEBUG_KMS("Failed to write aux DBC brightness level\n");- }
+}
static void intel_dp_aux_enable_backlight(struct intel_connector *connector) { struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
- uint8_t dpcd_buf = 0;
- uint8_t edp_backlight_mode = 0;
uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode;
if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
@@ -113,18 +128,15 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) return; }
new_dpcd_buf = dpcd_buf; edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
switch (edp_backlight_mode) { case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) {DRM_DEBUG_KMS("Failed to write aux backlight mode\n");}
new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;break;
/* Do nothing when it is already DPCD mode */
@@ -133,6 +145,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) break; }
- if (i915.enable_dbc &&
(intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP)) {new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);DRM_DEBUG_KMS("Enable dynamic brightness.\n");- }
- if (new_dpcd_buf != dpcd_buf) {
if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {DRM_DEBUG_KMS("Failed to write aux backlight mode\n");}- }
- set_aux_backlight_enable(intel_dp, true); intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
}
On Tue, May 30, 2017 at 9:18 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote:
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
This patch adds option to enable dynamic backlight for eDP panel that supports this feature via DPCD register and set minimum / maximum brightness to 0% and 100% of the normal brightness.
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org
drivers/gpu/drm/i915/i915_params.c | 5 ++++ drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42
++++++++++++++++++++++-----
3 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 3758ae1f11b4..ce033d58134e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = { .inject_load_failure = 0, .enable_dpcd_backlight = -1, .enable_gvt = false,
.enable_dbc = false,Based on Daniel's earlier comments on module parameters, shouldn't this be enabled by default too?
Yes. Will do.
Or even more importantly, is this the right approach to enable/disable dynamic back light control? The reason I recommended having some sort of control to disable/enable is that the eDP spec. says the feature can have user visible impact.
I don't think we should expect end user to set this correctly. For power user, I think the i915_params is adequate. I don't want to add more complication to the driver.
Table 10-3 Display Control Capabilities
"While the DBC control bit’s function is defined in this Standard, DBC implementation specifics are not defined, including interaction with other DPCD register settings. The DBC implementation, visual performance, and power savings characteristics can differ between specific panels."
-DK
};
module_param_named(modeset, i915.modeset, int, 0400); @@ -254,3 +255,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, "Enable support for Intel GVT-g graphics virtualization host
support(default:false)");
+module_param_named_unsafe(enable_dbc, i915.enable_dbc, bool, 0600); +MODULE_PARM_DESC(enable_dbc,
"Enable support for dynamic backlight control (default:false)");diff --git a/drivers/gpu/drm/i915/i915_params.h
b/drivers/gpu/drm/i915/i915_params.h
index ac02efce6e22..2de3e2850b54 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -67,7 +67,8 @@ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ func(int, enable_dpcd_backlight); \
func(bool, enable_gvt)
func(bool, enable_gvt); \func(bool, enable_dbc)#define MEMBER(T, member) T member struct i915_params { diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index c89aae804659..f55af41ce3bd 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct intel_connector
*connector, u32 level)
}}
+/*
- Set minimum / maximum dynamic brightness percentage. This value is
expressed
- as the percentage of normal brightness in 5% increments.
- */
+static void +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
u32 min, u32 max)+{
u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5)};
if (drm_dp_dpcd_write(&intel_dp->aux,DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
dbc, sizeof(dbc)) < 0) {DRM_DEBUG_KMS("Failed to write aux DBC brightnesslevel\n");
}+}
static void intel_dp_aux_enable_backlight(struct intel_connector
*connector)
{ struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
encoder->base);
uint8_t dpcd_buf = 0;uint8_t edp_backlight_mode = 0;
uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode; if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) !=
- {
@@ -113,18 +128,15 @@ static void intel_dp_aux_enable_backlight(struct
intel_connector *connector)
return; }
new_dpcd_buf = dpcd_buf; edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
switch (edp_backlight_mode) { case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) <
- {
DRM_DEBUG_KMS("Failed to write aux backlightmode\n");
}
new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; break; /* Do nothing when it is already DPCD mode */@@ -133,6 +145,20 @@ static void intel_dp_aux_enable_backlight(struct
intel_connector *connector)
break; }
if (i915.enable_dbc &&(intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP)) {new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0,100);
DRM_DEBUG_KMS("Enable dynamic brightness.\n");}if (new_dpcd_buf != dpcd_buf) {if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf)< 0) {
DRM_DEBUG_KMS("Failed to write aux backlightmode\n");
}}set_aux_backlight_enable(intel_dp, true); intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
}
On Wed, 2017-05-31 at 14:37 -0700, Puthikorn Voravootivat wrote:
On Tue, May 30, 2017 at 9:18 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote: On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote: > This patch adds option to enable dynamic backlight for eDP > panel that supports this feature via DPCD register and > set minimum / maximum brightness to 0% and 100% of the > normal brightness. > > Signed-off-by: Puthikorn Voravootivat puthik@chromium.org > --- > drivers/gpu/drm/i915/i915_params.c | 5 ++++ > drivers/gpu/drm/i915/i915_params.h | 3 +- > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42 ++++++++++++++++++++++----- > 3 files changed, 41 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 3758ae1f11b4..ce033d58134e 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = { > .inject_load_failure = 0, > .enable_dpcd_backlight = -1, > .enable_gvt = false, > + .enable_dbc = false,
Based on Daniel's earlier comments on module parameters, shouldn't this be enabled by default too?Yes. Will do.
Or even more importantly, is this the right approach to enable/disable dynamic back light control? The reason I recommended having some sort of control to disable/enable is that the eDP spec. says the feature can have user visible impact.I don't think we should expect end user to set this correctly. For power user, I think the i915_params is adequate. I don't want to add more complication to the driver.
Sure, I am not requesting any changes here :) I meant to address the question to Daniel in case he had any feedback.
-DK
Table 10-3 Display Control Capabilities "While the DBC control bit’s function is defined in this Standard, DBC implementation specifics are not defined, including interaction with other DPCD register settings. The DBC implementation, visual performance, and power savings characteristics can differ between specific panels." -DK > }; > > module_param_named(modeset, i915.modeset, int, 0400); > @@ -254,3 +255,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, > module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); > MODULE_PARM_DESC(enable_gvt, > "Enable support for Intel GVT-g graphics virtualization host support(default:false)"); > + > +module_param_named_unsafe(enable_dbc, i915.enable_dbc, bool, 0600); > +MODULE_PARM_DESC(enable_dbc, > + "Enable support for dynamic backlight control (default:false)"); > diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h > index ac02efce6e22..2de3e2850b54 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -67,7 +67,8 @@ > func(bool, nuclear_pageflip); \ > func(bool, enable_dp_mst); \ > func(int, enable_dpcd_backlight); \ > - func(bool, enable_gvt) > + func(bool, enable_gvt); \ > + func(bool, enable_dbc) > > #define MEMBER(T, member) T member > struct i915_params { > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > index c89aae804659..f55af41ce3bd 100644 > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c > @@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level) > } > } > > +/* > + * Set minimum / maximum dynamic brightness percentage. This value is expressed > + * as the percentage of normal brightness in 5% increments. > + */ > +static void > +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp, > + u32 min, u32 max) > +{ > + u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5) }; > + > + if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET, > + dbc, sizeof(dbc)) < 0) { > + DRM_DEBUG_KMS("Failed to write aux DBC brightness level\n"); > + } > +} > + > static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > - uint8_t dpcd_buf = 0; > - uint8_t edp_backlight_mode = 0; > + uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode; > > if (drm_dp_dpcd_readb(&intel_dp->aux, > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) { > @@ -113,18 +128,15 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > return; > } > > + new_dpcd_buf = dpcd_buf; > edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; > > switch (edp_backlight_mode) { > case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM: > case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET: > case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT: > - dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; > - dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > - if (drm_dp_dpcd_writeb(&intel_dp->aux, > - DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) { > - DRM_DEBUG_KMS("Failed to write aux backlight mode\n"); > - } > + new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK; > + new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; > break; > > /* Do nothing when it is already DPCD mode */ > @@ -133,6 +145,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) > break; > } > > + if (i915.enable_dbc && > + (intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP)) { > + new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE; > + intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100); > + DRM_DEBUG_KMS("Enable dynamic brightness.\n"); > + } > + > + if (new_dpcd_buf != dpcd_buf) { > + if (drm_dp_dpcd_writeb(&intel_dp->aux, > + DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) { > + DRM_DEBUG_KMS("Failed to write aux backlight mode\n"); > + } > + } > + > set_aux_backlight_enable(intel_dp, true); > intel_dp_aux_set_backlight(connector, connector->panel.backlight.level); > }
Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
This patch adds option to enable dynamic backlight for eDP panel that supports this feature via DPCD register and set minimum / maximum brightness to 0% and 100% of the normal brightness.
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org
drivers/gpu/drm/i915/i915_params.c | 5 ++++ drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42 ++++++++++++++++++++++----- 3 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 3758ae1f11b4..ce033d58134e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = { .inject_load_failure = 0, .enable_dpcd_backlight = -1, .enable_gvt = false,
- .enable_dbc = false,
};
module_param_named(modeset, i915.modeset, int, 0400); @@ -254,3 +255,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, "Enable support for Intel GVT-g graphics virtualization host support(default:false)");
+module_param_named_unsafe(enable_dbc, i915.enable_dbc, bool, 0600); +MODULE_PARM_DESC(enable_dbc,
- "Enable support for dynamic backlight control (default:false)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index ac02efce6e22..2de3e2850b54 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -67,7 +67,8 @@ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ func(int, enable_dpcd_backlight); \
- func(bool, enable_gvt)
- func(bool, enable_gvt); \
- func(bool, enable_dbc)
#define MEMBER(T, member) T member struct i915_params { diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index c89aae804659..f55af41ce3bd 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level) } }
+/*
- Set minimum / maximum dynamic brightness percentage. This value is expressed
- as the percentage of normal brightness in 5% increments.
- */
+static void +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
u32 min, u32 max)+{
- u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5) };
- if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
dbc, sizeof(dbc)) < 0) {DRM_DEBUG_KMS("Failed to write aux DBC brightness level\n");- }
+}
static void intel_dp_aux_enable_backlight(struct intel_connector *connector) { struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
- uint8_t dpcd_buf = 0;
- uint8_t edp_backlight_mode = 0;
uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode;
if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
@@ -113,18 +128,15 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) return; }
new_dpcd_buf = dpcd_buf; edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
switch (edp_backlight_mode) { case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) {DRM_DEBUG_KMS("Failed to write aux backlight mode\n");}
new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;break;
/* Do nothing when it is already DPCD mode */
@@ -133,6 +145,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector) break; }
- if (i915.enable_dbc &&
(intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP)) {new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);DRM_DEBUG_KMS("Enable dynamic brightness.\n");- }
Just curious, does DBC require DP AUX brightness control?
-DK
- if (new_dpcd_buf != dpcd_buf) {
if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {DRM_DEBUG_KMS("Failed to write aux backlight mode\n");}- }
- set_aux_backlight_enable(intel_dp, true); intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
}
On Fri, Jun 2, 2017 at 11:25 AM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote:
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
This patch adds option to enable dynamic backlight for eDP panel that supports this feature via DPCD register and set minimum / maximum brightness to 0% and 100% of the normal brightness.
Signed-off-by: Puthikorn Voravootivat puthik@chromium.org
drivers/gpu/drm/i915/i915_params.c | 5 ++++ drivers/gpu/drm/i915/i915_params.h | 3 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42
++++++++++++++++++++++-----
3 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c
b/drivers/gpu/drm/i915/i915_params.c
index 3758ae1f11b4..ce033d58134e 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = { .inject_load_failure = 0, .enable_dpcd_backlight = -1, .enable_gvt = false,
.enable_dbc = false,};
module_param_named(modeset, i915.modeset, int, 0400); @@ -254,3 +255,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight, module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, "Enable support for Intel GVT-g graphics virtualization host
support(default:false)");
+module_param_named_unsafe(enable_dbc, i915.enable_dbc, bool, 0600); +MODULE_PARM_DESC(enable_dbc,
"Enable support for dynamic backlight control (default:false)");diff --git a/drivers/gpu/drm/i915/i915_params.h
b/drivers/gpu/drm/i915/i915_params.h
index ac02efce6e22..2de3e2850b54 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -67,7 +67,8 @@ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ func(int, enable_dpcd_backlight); \
func(bool, enable_gvt)
func(bool, enable_gvt); \func(bool, enable_dbc)#define MEMBER(T, member) T member struct i915_params { diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index c89aae804659..f55af41ce3bd 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct intel_connector
*connector, u32 level)
}}
+/*
- Set minimum / maximum dynamic brightness percentage. This value is
expressed
- as the percentage of normal brightness in 5% increments.
- */
+static void +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
u32 min, u32 max)+{
u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5)};
if (drm_dp_dpcd_write(&intel_dp->aux,DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
dbc, sizeof(dbc)) < 0) {DRM_DEBUG_KMS("Failed to write aux DBC brightnesslevel\n");
}+}
static void intel_dp_aux_enable_backlight(struct intel_connector
*connector)
{ struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
encoder->base);
uint8_t dpcd_buf = 0;uint8_t edp_backlight_mode = 0;
uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode; if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) !=
- {
@@ -113,18 +128,15 @@ static void intel_dp_aux_enable_backlight(struct
intel_connector *connector)
return; }
new_dpcd_buf = dpcd_buf; edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
switch (edp_backlight_mode) { case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET: case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) <
- {
DRM_DEBUG_KMS("Failed to write aux backlightmode\n");
}
new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD; break; /* Do nothing when it is already DPCD mode */@@ -133,6 +145,20 @@ static void intel_dp_aux_enable_backlight(struct
intel_connector *connector)
break; }
if (i915.enable_dbc &&(intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP)) {new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0,100);
DRM_DEBUG_KMS("Enable dynamic brightness.\n");}Just curious, does DBC require DP AUX brightness control?
No. Here is quote from the spec.
"The Dynamic Backlight Control (DBC) and color engine modes are not listed in Table 10-1, because when they are available, they are available for all supported modes."
-DK
if (new_dpcd_buf != dpcd_buf) {if (drm_dp_dpcd_writeb(&intel_dp->aux,DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf)< 0) {
DRM_DEBUG_KMS("Failed to write aux backlightmode\n");
}}set_aux_backlight_enable(intel_dp, true); intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
}
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; + 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) {
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 maximumvalue or
* minimum value will applied automatically. So no need to checkthat.
*/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 ofbrightness
* 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 ofrange\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) {
dri-devel@lists.freedesktop.org
-
Pandiyan, Dhinakaran -
Puthikorn Voravootivat