* [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver
@ 2017-05-27 1:42 Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-27 1:42 UTC (permalink / raw)
To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula, Daniel Vetter
Cc: Puthikorn Voravootivat, dri-devel
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(-)
--
2.13.0.219.gdb65acc882-goog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
@ 2017-05-27 1:42 ` Puthikorn Voravootivat
2017-06-02 17:42 ` Pandiyan, Dhinakaran
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-27 1:42 UTC (permalink / raw)
To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula, Daniel Vetter
Cc: Puthikorn Voravootivat, dri-devel
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;
--
2.13.0.219.gdb65acc882-goog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
@ 2017-05-27 1:42 ` Puthikorn Voravootivat
2017-05-31 4:18 ` Pandiyan, Dhinakaran
2017-06-02 18:25 ` Pandiyan, Dhinakaran
2017-05-27 1:42 ` [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
2017-05-27 2:01 ` ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev9) Patchwork
3 siblings, 2 replies; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-27 1:42 UTC (permalink / raw)
To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula, Daniel Vetter
Cc: Puthikorn Voravootivat, dri-devel
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);
}
--
2.13.0.219.gdb65acc882-goog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
@ 2017-05-27 1:42 ` Puthikorn Voravootivat
2017-05-31 3:40 ` Pandiyan, Dhinakaran
2017-05-27 2:01 ` ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev9) Patchwork
3 siblings, 1 reply; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-27 1:42 UTC (permalink / raw)
To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula, Daniel Vetter
Cc: Puthikorn Voravootivat, dri-devel
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) {
--
2.13.0.219.gdb65acc882-goog
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev9)
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
` (2 preceding siblings ...)
2017-05-27 1:42 ` [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
@ 2017-05-27 2:01 ` Patchwork
3 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2017-05-27 2:01 UTC (permalink / raw)
To: Puthikorn Voravootivat; +Cc: intel-gfx
== Series Details ==
Series: Enhancement to intel_dp_aux_backlight driver (rev9)
URL : https://patchwork.freedesktop.org/series/21086/
State : success
== Summary ==
Series 21086v9 Enhancement to intel_dp_aux_backlight driver
https://patchwork.freedesktop.org/api/1.0/series/21086/revisions/9/mbox/
Test kms_flip:
Subgroup basic-flip-vs-dpms:
dmesg-warn -> PASS (fi-skl-6700hq) fdo#101144
fdo#101144 https://bugs.freedesktop.org/show_bug.cgi?id=101144
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time:442s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time:434s
fi-bsw-n3050 total:278 pass:242 dwarn:0 dfail:0 fail:0 skip:36 time:587s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time:520s
fi-byt-j1900 total:278 pass:254 dwarn:0 dfail:0 fail:0 skip:24 time:491s
fi-byt-n2820 total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:485s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:432s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time:414s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time:416s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:489s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time:467s
fi-kbl-7500u total:278 pass:255 dwarn:5 dfail:0 fail:0 skip:18 time:462s
fi-kbl-7560u total:278 pass:263 dwarn:5 dfail:0 fail:0 skip:10 time:568s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:460s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time:561s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time:465s
fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time:499s
fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time:437s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time:530s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time:402s
bf9722dc7ed767dd56571e82950e533f69d9318e drm-tip: 2017y-05m-26d-23h-15m-13s UTC integration manifest
b57c3ff drm/i915: Set PWM divider to match desired frequency in vbt
a4edbd5 drm/i915: Add option to support dynamic backlight via DPCD
bc40f50 drm/i915: Add heuristic to determine better way to adjust brightness
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4824/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt
2017-05-27 1:42 ` [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
@ 2017-05-31 3:40 ` Pandiyan, Dhinakaran
2017-05-31 21:44 ` Puthikorn Voravootivat
0 siblings, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-31 3:40 UTC (permalink / raw)
To: puthik; +Cc: dri-devel, intel-gfx
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) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
@ 2017-05-31 4:18 ` Pandiyan, Dhinakaran
2017-05-31 21:37 ` Puthikorn Voravootivat
2017-06-02 18:25 ` Pandiyan, Dhinakaran
1 sibling, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-31 4:18 UTC (permalink / raw)
To: puthik; +Cc: dri-devel, 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,
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);
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-31 4:18 ` Pandiyan, Dhinakaran
@ 2017-05-31 21:37 ` Puthikorn Voravootivat
2017-05-31 21:52 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-31 21:37 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: puthik, intel-gfx, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 6786 bytes --]
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 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);
> > }
>
>
[-- Attachment #1.2: Type: text/html, Size: 8878 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt
2017-05-31 3:40 ` Pandiyan, Dhinakaran
@ 2017-05-31 21:44 ` Puthikorn Voravootivat
0 siblings, 0 replies; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-31 21:44 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: puthik, intel-gfx, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 5959 bytes --]
On Tue, May 30, 2017 at 8:40 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:
> The patch looks good overall, it would have been easier to merge if
> you'd sent this as the first patch in this version. Some comments
> inline.
>
>
> Will re-order to make this the first patch in the next version.
>
> On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote:
> > Read desired PWM frequency from panel vbt and calculate the
> > value for divider in DPCD address 0x724 and 0x728 to have
> > as many bits as possible for PWM duty cyle for granularity of
> > brightness adjustment while the frequency divisor is still
> > within 25% of the desired value.
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 79
> +++++++++++++++++++++++++++
> > 1 file changed, 79 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index f55af41ce3bd..a8d485a29f29 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -116,6 +116,81 @@ intel_dp_aux_set_dynamic_backlight_percent(struct
> intel_dp *intel_dp,
> > }
> > }
> >
> > +/*
> > + * Set PWM Frequency divider to match desired frequency in vbt.
> > + * The PWM Frequency is calculated as 27Mhz / (F x P).
> > + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0
> of the
> > + * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> > + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of
> the
> > + * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> > + */
> > +static int intel_dp_aux_set_pwm_freq(struct intel_connector *connector)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> > + int freq, fxp, fxp_min, fxp_max, fxp_actual, f = 1;
>
> nit: unnecessary initialization for f?
>
No. We only set f in for (pn = pn_max; pn >= pn_min; pn--) but compiler
wouldn't
know that pn_max >= pn_min so f might be uninitialized.
> > + u8 pn, pn_min, pn_max;
> > +
> > + /* Find desired value of (F x P)
> > + * Note that, if F x P is out of supported range, the maximum
> value or
> > + * minimum value will applied automatically. So no need to check
> that.
> > + */
> > + freq = dev_priv->vbt.backlight.pwm_freq_hz;
> > + DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> > + if (!freq) {
> > + DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> > + return -1;
> > + }
> > +
> > + fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ),
> freq);
> > +
> > + /* Use highest possible value of Pn for more granularity of
> brightness
> > + * adjustment while satifying the conditions below.
> > + * - Pn is in the range of Pn_min and Pn_max
> > + * - F is in the range of 1 and 255
> > + * - FxP is within 25% of desired value.
> > + * Note: 25% is arbitrary value and may need some tweak.
> > + */
> > + if (drm_dp_dpcd_readb(&intel_dp->aux,
> > + DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)
> != 1) {
> > + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> > + return -EIO;
>
> The error numbers are not propagated outside, so I don't see a real need
> for them. bool should suffice.
>
> Sure.
>
> > + }
> > + if (drm_dp_dpcd_readb(&intel_dp->aux,
> > + DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)
> != 1) {
> > + DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
> > + return -EIO;
> > + }
> > + pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > + pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +
> > + fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> > + fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> > + if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > + DRM_DEBUG_KMS("VBT defined backlight frequency out of
> range\n");
> > + return -ERANGE;
> > + }
> > +
> > + for (pn = pn_max; pn >= pn_min; pn--) {
> > + f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255);
> > + fxp_actual = f << pn;
> > + if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > + break;
> > + }
> > +
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > + DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> > + DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> > + return -EIO;
> > + }
> > + if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > + DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
> > + DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
> > + return -EIO;
> > + }
> > + return 0;
> > +}
> > +
> > static void intel_dp_aux_enable_backlight(struct intel_connector
> *connector)
> > {
> > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> > @@ -152,6 +227,10 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> > DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> > }
> >
> > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP)
> > + if (!intel_dp_aux_set_pwm_freq(connector))
>
> nit: This doesn't read right to me, looking at the function name it
> seems like you are proceeding when it failed.
>
> OK.
>
> > + new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_
> ENABLE;
> > +
> > if (new_dpcd_buf != dpcd_buf) {
> > if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf)
> < 0) {
>
>
[-- Attachment #1.2: Type: text/html, Size: 8823 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-31 21:37 ` Puthikorn Voravootivat
@ 2017-05-31 21:52 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-05-31 21:52 UTC (permalink / raw)
To: puthik; +Cc: daniel.vetter, intel-gfx, dri-devel
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
@ 2017-06-02 17:42 ` Pandiyan, Dhinakaran
2017-06-02 17:56 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-02 17:42 UTC (permalink / raw)
To: puthik; +Cc: dri-devel, intel-gfx
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)) {
> 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] ?
> + 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-06-02 17:42 ` Pandiyan, Dhinakaran
@ 2017-06-02 17:56 ` Pandiyan, Dhinakaran
2017-06-03 1:35 ` Puthikorn Voravootivat
0 siblings, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-02 17:56 UTC (permalink / raw)
To: puthik; +Cc: daniel.vetter, intel-gfx, dri-devel
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)) {
> > 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] ?
>
>
> > + 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
2017-05-31 4:18 ` Pandiyan, Dhinakaran
@ 2017-06-02 18:25 ` Pandiyan, Dhinakaran
2017-06-03 1:56 ` Puthikorn Voravootivat
1 sibling, 1 reply; 15+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-06-02 18:25 UTC (permalink / raw)
To: puthik; +Cc: dri-devel, 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);
> }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness
2017-06-02 17:56 ` Pandiyan, Dhinakaran
@ 2017-06-03 1:35 ` Puthikorn Voravootivat
0 siblings, 0 replies; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-06-03 1:35 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: puthik, daniel.vetter, intel-gfx, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 8851 bytes --]
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 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))
> >
> > 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
>
>
[-- Attachment #1.2: Type: text/html, Size: 12387 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD
2017-06-02 18:25 ` Pandiyan, Dhinakaran
@ 2017-06-03 1:56 ` Puthikorn Voravootivat
0 siblings, 0 replies; 15+ messages in thread
From: Puthikorn Voravootivat @ 2017-06-03 1:56 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: puthik, intel-gfx, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 5982 bytes --]
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 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?
>
> 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 backlight
> mode\n");
> > + }
> > + }
> > +
> > set_aux_backlight_enable(intel_dp, true);
> > intel_dp_aux_set_backlight(connector, connector->panel.backlight.
> level);
> > }
>
>
[-- Attachment #1.2: Type: text/html, Size: 8151 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-06-03 1:56 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-27 1:42 [PATCH v10 0/3] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 1/3] drm/i915: Add heuristic to determine better way to adjust brightness Puthikorn Voravootivat
2017-06-02 17:42 ` Pandiyan, Dhinakaran
2017-06-02 17:56 ` Pandiyan, Dhinakaran
2017-06-03 1:35 ` Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 2/3] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
2017-05-31 4:18 ` Pandiyan, Dhinakaran
2017-05-31 21:37 ` Puthikorn Voravootivat
2017-05-31 21:52 ` Pandiyan, Dhinakaran
2017-06-02 18:25 ` Pandiyan, Dhinakaran
2017-06-03 1:56 ` Puthikorn Voravootivat
2017-05-27 1:42 ` [PATCH v10 3/3] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
2017-05-31 3:40 ` Pandiyan, Dhinakaran
2017-05-31 21:44 ` Puthikorn Voravootivat
2017-05-27 2:01 ` ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev9) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.