[PATCH RESEND v4 0/6] Enhancement to intel_dp_aux_backlight driver

[PATCH RESEND v4 0/6] Enhancement to intel_dp_aux_backlight driver

[Puthikorn Voravootivat]

Rebase since this is not applied cleanly now.

This patch set contain 6 patches.
- First two patches allow enable DPCD backlight control when panel
  can also do that via PWM pin and fix the usage of enable register.
- Next patch adds enable DBC by default
- Next patch makes the driver restore last brightness level after
  turning display off and on.
- Last two patches set the PWM freqency to match data in panel vbt.

Change log:
v2:
- Drop PWM frequency patch
- Addess suggestion from Jani Nikula

v3:
- Add new implementation of PWM frequency patch

v4:
- Rebase / minor typo fix.

Puthikorn Voravootivat (6):
  drm/i915: Add DPCD preferred mode for backlight control
  drm/i915: Correctly enable blacklight adjustment via DPCD
  drm/i915: Support dynamic backlight via DPCD register
  drm/i915: Store brightness level in aux backlight driver
  drm: Add definition for eDP backlight frequency
  drm/i915: Set PWM divider to match desired frequency in vbt

 drivers/gpu/drm/i915/i915_params.c            |   6 +-
 drivers/gpu/drm/i915/i915_params.h            |   2 +-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 143 +++++++++++++++++++++++---
 include/drm/drm_dp_helper.h                   |   2 +
 4 files changed, 133 insertions(+), 20 deletions(-)

-- 
2.12.2.816.g2cccc81164-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND v4 1/6] drm/i915: Add DPCD preferred mode for backlight control

[Puthikorn Voravootivat]

Currently the intel_dp_aux_backlight driver requires eDP panel
to not also support backlight adjustment via PWM pin to use
this driver.

This force the eDP panel that support both ways of backlight
adjustment to do it via PWM pin.

This patch adds the new prefer DPCD mode in the i915_param
to make it enable to prefer DPCD over the PWM via kernel param.

This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
in set_aux_backlight_enable() since the backlight enablement
can be done via BL_ENABLE eDP connector pin in the case that
it does not support doing that via AUX.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/i915_params.c            |  6 ++---
 drivers/gpu/drm/i915/i915_params.h            |  2 +-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 +++++++++++++++++++--------
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e363d076..960393dd5edf 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 = 0,
 	.enable_gvt = false,
 };
 
@@ -246,9 +246,9 @@ 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(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 (0:disable (default), 1:prefer PWM pin, 2: prefer DPCD)");
 
 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..bf6e2c60f697 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -51,6 +51,7 @@
 	func(int, use_mmio_flip); \
 	func(int, mmio_debug); \
 	func(int, edp_vswing); \
+	func(int, enable_dpcd_backlight); \
 	func(unsigned int, inject_load_failure); \
 	/* leave bools at the end to not create holes */ \
 	func(bool, alpha_support); \
@@ -66,7 +67,6 @@
 	func(bool, verbose_state_checks); \
 	func(bool, nuclear_pageflip); \
 	func(bool, enable_dp_mst); \
-	func(bool, 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 6532e226db29..42f73d9a3ccf 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
 {
 	uint8_t reg_val = 0;
 
+	/* Early return when display use other mechanism to enable backlight. */
+	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
+		return;
+
 	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
 			      &reg_val) < 0) {
 		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
@@ -138,27 +142,36 @@ static bool
 intel_dp_aux_display_control_capable(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+	bool supported;
 
 	/* 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
-	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
-	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
-		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
-		return true;
+	switch (i915.enable_dpcd_backlight) {
+	case 1: /* prefer PWM pin */
+		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
+			    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
+			    !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) &&
+			    !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP);
+		break;
+	case 2: /* prefer DPCD */
+		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
+			    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
+		break;
+	default:
+		supported = false;
 	}
-	return false;
+
+	if (supported)
+		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
+
+	return supported;
 }
 
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 {
 	struct intel_panel *panel = &intel_connector->panel;
 
-	if (!i915.enable_dpcd_backlight)
-		return -ENODEV;
-
 	if (!intel_dp_aux_display_control_capable(intel_connector))
 		return -ENODEV;
 
-- 
2.12.2.816.g2cccc81164-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND v4 2/6] drm/i915: Correctly enable blacklight adjustment via DPCD

[Puthikorn Voravootivat]

intel_dp_aux_enable_backlight() assumed that the register
BACKLIGHT_BRIGHTNESS_CONTROL_MODE can only has value 01
(DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET) when initialize.

This patch fixed that by handling all cases of that register.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 29 +++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 42f73d9a3ccf..f06c8381c74e 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -101,15 +101,32 @@ 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;
 
 	set_aux_backlight_enable(intel_dp, true);
 
-	if ((drm_dp_dpcd_readb(&intel_dp->aux,
-			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
-	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
-	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
-				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));
+	if (!drm_dp_dpcd_readb(&intel_dp->aux,
+			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf)) {
+		return;
+	}
+
+	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;
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
+		break;
+
+	/* Do nothing when it is already DPCD mode */
+	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
+	default:
+		break;
+	}
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.12.2.816.g2cccc81164-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND v4 3/6] drm/i915: Support dynamic backlight via DPCD register

[Puthikorn Voravootivat]

This patch enables dynamic backlight by default 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/intel_dp_aux_backlight.c | 31 +++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index f06c8381c74e..ae1b6fe67feb 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -97,10 +97,24 @@ 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) };
+	drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
+			  dbc, sizeof(dbc));
+}
+
 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 new_dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
 
 	set_aux_backlight_enable(intel_dp, true);
@@ -110,16 +124,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;
-		drm_dp_dpcd_writeb(&intel_dp->aux,
-			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
+		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 */
@@ -127,6 +140,16 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	default:
 		break;
 	}
+
+	if (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);
+	}
+
+	if (new_dpcd_buf != dpcd_buf) {
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
+	}
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.12.2.816.g2cccc81164-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND v4 4/6] drm/i915: Store brightness level in aux backlight driver

[Puthikorn Voravootivat]

Some panel will default to zero brightness when turning the
panel off and on again. This patch stores last brightness level
before turning off and set them back when panel is turning on.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index ae1b6fe67feb..f99cf0a6ae44 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -95,6 +95,7 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
 		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
 		return;
 	}
+	connector->panel.backlight.level = level;
 }
 
 /*
@@ -150,6 +151,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
 	}
+	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.12.2.816.g2cccc81164-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND v4 5/6] drm: Add definition for eDP backlight frequency

[Puthikorn Voravootivat]

This patch adds the following definition
- Bit mask for EDP_PWMGEN_BIT_COUNT and min/max cap
  register which only use bit 0:4
- Base frequency (27 MHz) for backlight PWM frequency
  generator.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 include/drm/drm_dp_helper.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c0bd0d7651a9..9aee65ebc54c 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -572,10 +572,12 @@
 #define DP_EDP_PWMGEN_BIT_COUNT             0x724
 #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN     0x725
 #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX     0x726
+# define  DP_EDP_PWMGEN_BIT_COUNT_MASK      (31 << 0)
 
 #define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
 
 #define DP_EDP_BACKLIGHT_FREQ_SET           0x728
+# define DP_EDP_BACKLIGHT_FREQ_BASE         27000000
 
 #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
 #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b
-- 
2.12.2.816.g2cccc81164-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RESEND v4 6/6] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

Read desired PWM frequency from panel vbt and calculate the
value for divider in DPCD address 0x724 and 0x728 to match
that frequency as close as possible.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 56 +++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index f99cf0a6ae44..9adc77bfb515 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -111,12 +111,60 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
 			  dbc, sizeof(dbc));
 }
 
+/*
+ * 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 void 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, 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;
+	fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
+
+	/* Use lowest possible value of Pn to try to make F to be between 1 and
+	 * 255 while still in the range Pn_min and Pn_max
+	 */
+	if (!drm_dp_dpcd_readb(&intel_dp->aux,
+			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)) {
+		return;
+	}
+	if (!drm_dp_dpcd_readb(&intel_dp->aux,
+			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)) {
+		return;
+	}
+	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+	f = fxp / (1 << pn_min);
+	for (pn = pn_min; pn < pn_max && f > 255; pn++)
+		f /= 2;
+
+	/* Cap F to be in the range between 1 and 255. */
+	f = min(f, 255);
+	f = max(f, 1);
+
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET, (u8) f);
+}
+
 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 new_dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
+	bool freq_cap;
 
 	set_aux_backlight_enable(intel_dp, true);
 
@@ -147,10 +195,18 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
 	}
 
+	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
+	if (freq_cap)
+		new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+
 	if (new_dpcd_buf != dpcd_buf) {
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
 	}
+
+	if (freq_cap)
+		intel_dp_aux_set_pwm_freq(connector);
+
 	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
 
-- 
2.12.2.816.g2cccc81164-goog

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for Enhancement to intel_dp_aux_backlight driver (rev3)

[Patchwork]

== Series Details ==

Series: Enhancement to intel_dp_aux_backlight driver (rev3)
URL   : https://patchwork.freedesktop.org/series/21086/
State : warning

== Summary ==

Series 21086v3 Enhancement to intel_dp_aux_backlight driver
https://patchwork.freedesktop.org/api/1.0/series/21086/revisions/3/mbox/

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-bdw-5557u)
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:266  dwarn:1   dfail:0   fail:0   skip:11  time:437s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:435s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:584s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:509s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:558s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:490s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:411s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:402s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:426s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:487s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:464s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:468s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time:568s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:461s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:568s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:455s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:492s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:430s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:410s

7204beb80dcdfd1f8b1cff6a448301407f91a99c drm-tip: 2017y-04m-18d-16h-09m-11s UTC integration manifest
3e64d55 drm/i915: Set PWM divider to match desired frequency in vbt
2d59d47 drm: Add definition for eDP backlight frequency
980d470 drm/i915: Store brightness level in aux backlight driver
e5536b9 drm/i915: Support dynamic backlight via DPCD register
3bc96ce drm/i915: Correctly enable blacklight adjustment via DPCD
e115d05 drm/i915: Add DPCD preferred mode for backlight control

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4515/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 1/6] drm/i915: Add DPCD preferred mode for backlight control

[Pandiyan, Dhinakaran]

Sorry for the wait. This is not a complete review, just some quick
comments for now.


On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> Currently the intel_dp_aux_backlight driver requires eDP panel
> to not also support backlight adjustment via PWM pin to use
> this driver.
> 
> This force the eDP panel that support both ways of backlight
> adjustment to do it via PWM pin.
> 
> This patch adds the new prefer DPCD mode in the i915_param
> to make it enable to prefer DPCD over the PWM via kernel param.
> 
> This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
> in set_aux_backlight_enable() since the backlight enablement
> can be done via BL_ENABLE eDP connector pin in the case that
> it does not support doing that via AUX.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/i915_params.c            |  6 ++---
>  drivers/gpu/drm/i915/i915_params.h            |  2 +-
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 +++++++++++++++++++--------
>  3 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b6a7e363d076..960393dd5edf 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 = 0,
>  	.enable_gvt = false,
>  };
>  
> @@ -246,9 +246,9 @@ 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(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 (0:disable (default), 1:prefer PWM pin, 2: prefer DPCD)");

I looked at other int parameters, -1=default, 0=disable(prefer PWM in
your case), 1=enable(prefer DPCD) seems like the more common way to do
this.


>  
>  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..bf6e2c60f697 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -51,6 +51,7 @@
>  	func(int, use_mmio_flip); \
>  	func(int, mmio_debug); \
>  	func(int, edp_vswing); \
> +	func(int, enable_dpcd_backlight); \
>  	func(unsigned int, inject_load_failure); \
>  	/* leave bools at the end to not create holes */ \
>  	func(bool, alpha_support); \
> @@ -66,7 +67,6 @@
>  	func(bool, verbose_state_checks); \
>  	func(bool, nuclear_pageflip); \
>  	func(bool, enable_dp_mst); \
> -	func(bool, 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 6532e226db29..42f73d9a3ccf 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  {
>  	uint8_t reg_val = 0;
>  
> +	/* Early return when display use other mechanism to enable backlight. */
> +	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> +		return;
> +
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
>  			      &reg_val) < 0) {
>  		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> @@ -138,27 +142,36 @@ static bool
>  intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> +	bool supported;
>  
>  	/* 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> -	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> -	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
> -		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> -		return true;
> +	switch (i915.enable_dpcd_backlight) {
> +	case 1: /* prefer PWM pin */
> +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +			    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> +			    !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) &&
> +			    !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP);
> +		break;
> +	case 2: /* prefer DPCD */
> +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +			    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> +		break;
> +	default:
> +		supported = false;
>  	}
> -	return false;
> +
> +	if (supported)
> +		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> +
> +	return supported;


If i915.enable_dpcd_backlight = 0, aren't you returning an uninitialized
variable here?

-DK


>  }
>  
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
>  {
>  	struct intel_panel *panel = &intel_connector->panel;
>  
> -	if (!i915.enable_dpcd_backlight)
> -		return -ENODEV;
> -
>  	if (!intel_dp_aux_display_control_capable(intel_connector))
>  		return -ENODEV;
>  

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 2/6] drm/i915: Correctly enable blacklight adjustment via DPCD

[Pandiyan, Dhinakaran]

On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> intel_dp_aux_enable_backlight() assumed that the register
> BACKLIGHT_BRIGHTNESS_CONTROL_MODE can only has value 01
> (DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET) when initialize.
> 
> This patch fixed that by handling all cases of that register.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 29 +++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 42f73d9a3ccf..f06c8381c74e 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -101,15 +101,32 @@ 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;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> -	if ((drm_dp_dpcd_readb(&intel_dp->aux,
> -			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
> -	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> -	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> -				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));
> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf)) {
> +		return;
> +	}
> +
> +	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;
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);


You check the return value for _readb() above, but don't check for the
_writeb() here.



> +		break;
> +
> +	/* Do nothing when it is already DPCD mode */
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
> +	default:
> +		break;
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 2/6] drm/i915: Correctly enable blacklight adjustment via DPCD

[Pandiyan, Dhinakaran]

Adjusting "blacklight" probably won't make a lot of difference even if
done correctly:) Typo in the patch subject.

-DK


On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> intel_dp_aux_enable_backlight() assumed that the register
> BACKLIGHT_BRIGHTNESS_CONTROL_MODE can only has value 01
> (DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET) when initialize.
> 
> This patch fixed that by handling all cases of that register.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 29 +++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 42f73d9a3ccf..f06c8381c74e 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -101,15 +101,32 @@ 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;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> -	if ((drm_dp_dpcd_readb(&intel_dp->aux,
> -			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
> -	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> -	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> -				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));
> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf)) {
> +		return;
> +	}
> +
> +	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;
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
> +		break;
> +
> +	/* Do nothing when it is already DPCD mode */
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
> +	default:
> +		break;
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 1/6] drm/i915: Add DPCD preferred mode for backlight control

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 00:54 +0000, Pandiyan, Dhinakaran wrote:
> Sorry for the wait. This is not a complete review, just some quick
> comments for now.
> 
> 
> On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> > Currently the intel_dp_aux_backlight driver requires eDP panel
> > to not also support backlight adjustment via PWM pin to use
> > this driver.
> > 
> > This force the eDP panel that support both ways of backlight
> > adjustment to do it via PWM pin.
> > 
> > This patch adds the new prefer DPCD mode in the i915_param
> > to make it enable to prefer DPCD over the PWM via kernel param.
> > 
> > This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
> > in set_aux_backlight_enable() since the backlight enablement
> > can be done via BL_ENABLE eDP connector pin in the case that
> > it does not support doing that via AUX.
> > 
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/i915_params.c            |  6 ++---
> >  drivers/gpu/drm/i915/i915_params.h            |  2 +-
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 +++++++++++++++++++--------
> >  3 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index b6a7e363d076..960393dd5edf 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 = 0,
> >  	.enable_gvt = false,
> >  };
> >  
> > @@ -246,9 +246,9 @@ 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(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 (0:disable (default), 1:prefer PWM pin, 2: prefer DPCD)");
> 
> I looked at other int parameters, -1=default, 0=disable(prefer PWM in
> your case), 1=enable(prefer DPCD) seems like the more common way to do
> this.
> 
> 
> >  
> >  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..bf6e2c60f697 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -51,6 +51,7 @@
> >  	func(int, use_mmio_flip); \
> >  	func(int, mmio_debug); \
> >  	func(int, edp_vswing); \
> > +	func(int, enable_dpcd_backlight); \
> >  	func(unsigned int, inject_load_failure); \
> >  	/* leave bools at the end to not create holes */ \
> >  	func(bool, alpha_support); \
> > @@ -66,7 +67,6 @@
> >  	func(bool, verbose_state_checks); \
> >  	func(bool, nuclear_pageflip); \
> >  	func(bool, enable_dp_mst); \
> > -	func(bool, 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 6532e226db29..42f73d9a3ccf 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
> >  {
> >  	uint8_t reg_val = 0;
> >  
> > +	/* Early return when display use other mechanism to enable backlight. */
> > +	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> > +		return;
> > +
> >  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
> >  			      &reg_val) < 0) {
> >  		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > @@ -138,27 +142,36 @@ static bool
> >  intel_dp_aux_display_control_capable(struct intel_connector *connector)
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> > +	bool supported;
> >  
> >  	/* 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > -	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> > -	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
> > -		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> > -		return true;
> > +	switch (i915.enable_dpcd_backlight) {
> > +	case 1: /* prefer PWM pin */
> > +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> > +			    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > +			    !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) &&
> > +			    !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP);
> > +		break;
> > +	case 2: /* prefer DPCD */
> > +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> > +			    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> > +		break;
> > +	default:
> > +		supported = false;
> >  	}
> > -	return false;
> > +
> > +	if (supported)
> > +		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> > +
> > +	return supported;
> 
> 
> If i915.enable_dpcd_backlight = 0, aren't you returning an uninitialized
> variable here?
> 
> -DK
> 

Ignore that, time for coffee. 

 
> 
> >  }
> >  
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
> >  {
> >  	struct intel_panel *panel = &intel_connector->panel;
> >  
> > -	if (!i915.enable_dpcd_backlight)
> > -		return -ENODEV;
> > -
> >  	if (!intel_dp_aux_display_control_capable(intel_connector))
> >  		return -ENODEV;
> >  
> 
> _______________________________________________
> 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

Re: [PATCH RESEND v4 5/6] drm: Add definition for eDP backlight frequency

[Manasi Navare]

On Tue, Apr 18, 2017 at 04:48:23PM -0700, Puthikorn Voravootivat wrote:

Since this adds definitions in the DRM layer, you need to copy
the dri-devel@lists.freedesktop.org M-L.

> This patch adds the following definition
> - Bit mask for EDP_PWMGEN_BIT_COUNT and min/max cap
>   register which only use bit 0:4
> - Base frequency (27 MHz) for backlight PWM frequency
>   generator.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  include/drm/drm_dp_helper.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index c0bd0d7651a9..9aee65ebc54c 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -572,10 +572,12 @@
>  #define DP_EDP_PWMGEN_BIT_COUNT             0x724
>  #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN     0x725
>  #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX     0x726
> +# define  DP_EDP_PWMGEN_BIT_COUNT_MASK      (31 << 0)
>  
>  #define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
>  
>  #define DP_EDP_BACKLIGHT_FREQ_SET           0x728
> +# define DP_EDP_BACKLIGHT_FREQ_BASE         27000000

Could you use HEX value to define this? Thats the convention around.

Manasi
>  
>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b
> -- 
> 2.12.2.816.g2cccc81164-goog
> 
> _______________________________________________
> 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

Re: [PATCH RESEND v4 2/6] drm/i915: Correctly enable blacklight adjustment via DPCD

[Manasi Navare]

On Tue, Apr 18, 2017 at 04:48:20PM -0700, Puthikorn Voravootivat wrote:
> intel_dp_aux_enable_backlight() assumed that the register
> BACKLIGHT_BRIGHTNESS_CONTROL_MODE can only has value 01
> (DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET) when initialize.
> 
> This patch fixed that by handling all cases of that register.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 29 +++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 42f73d9a3ccf..f06c8381c74e 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -101,15 +101,32 @@ 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;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> -	if ((drm_dp_dpcd_readb(&intel_dp->aux,
> -			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
> -	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> -	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> -				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));
> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf)) {
> +		return;
> +	}
> +
> +	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;
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
> +		break;
> +
> +	/* Do nothing when it is already DPCD mode */
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
> +	default:
> +		break;

Do you really need a break; here?

Manasi
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
> -- 
> 2.12.2.816.g2cccc81164-goog
> 
> _______________________________________________
> 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

Re: [PATCH RESEND v4 6/6] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> Read desired PWM frequency from panel vbt and calculate the
> value for divider in DPCD address 0x724 and 0x728 to match
> that frequency as close as possible.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 56 +++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index f99cf0a6ae44..9adc77bfb515 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -111,12 +111,60 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
>  			  dbc, sizeof(dbc));
>  }
>  
> +/*
> + * 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 void 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, 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;
> +	fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;



How do we know vbt.backlight.pwm_freq_hz isn't zero? Or am I missing
something here.
intel_panel.c: get_backlight_max_vbt() seems to do a check.


-DK

> +
> +	/* Use lowest possible value of Pn to try to make F to be between 1 and
> +	 * 255 while still in the range Pn_min and Pn_max
> +	 */
> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)) {
> +		return;
> +	}
> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)) {
> +		return;
> +	}
> +	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	f = fxp / (1 << pn_min);
> +	for (pn = pn_min; pn < pn_max && f > 255; pn++)
> +		f /= 2;
> +
> +	/* Cap F to be in the range between 1 and 255. */
> +	f = min(f, 255);
> +	f = max(f, 1);
> +
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET, (u8) f);
> +}
> +
>  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 new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
> +	bool freq_cap;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> @@ -147,10 +195,18 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
>  	}
>  
> +	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> +	if (freq_cap)
> +		new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> +
>  	if (new_dpcd_buf != dpcd_buf) {
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
>  	}
> +
> +	if (freq_cap)
> +		intel_dp_aux_set_pwm_freq(connector);
> +
>  	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

Re: [PATCH RESEND v4 3/6] drm/i915: Support dynamic backlight via DPCD register

[Pandiyan, Dhinakaran]

On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> This patch enables dynamic backlight by default for eDP
> panel that supports this feature via DPCD register and
> set minimum / maximum brightness to 0% and 100% of the
> normal brightness.


What does dynamic backlight do? I am trying to understand what allowing
0% minimum brightness means. 


> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 31 +++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index f06c8381c74e..ae1b6fe67feb 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -97,10 +97,24 @@ 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) };
> +	drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
> +			  dbc, sizeof(dbc));
> +}
> +
>  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 new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
>  
>  	set_aux_backlight_enable(intel_dp, true);
> @@ -110,16 +124,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;
> -		drm_dp_dpcd_writeb(&intel_dp->aux,
> -			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
> +		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 */
> @@ -127,6 +140,16 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  	default:
>  		break;
>  	}


Still need a switch here? You are setting mode as
DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD for all four values of mode. 



> +
> +	if (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);
> +	}

Should enabling dynamic backlight control be logged in debug messages?
Could be useful in case this turns out to be buggy.

-DK

> +
> +	if (new_dpcd_buf != dpcd_buf) {
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 6/6] drm/i915: Set PWM divider to match desired frequency in vbt

[Manasi Navare]

On Wed, May 03, 2017 at 02:15:06AM +0000, Pandiyan, Dhinakaran wrote:
> On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> > Read desired PWM frequency from panel vbt and calculate the
> > value for divider in DPCD address 0x724 and 0x728 to match
> > that frequency as close as possible.
> > 
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 56 +++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index f99cf0a6ae44..9adc77bfb515 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -111,12 +111,60 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
> >  			  dbc, sizeof(dbc));
> >  }
> >  
> > +/*
> > + * 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 void 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, 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;
> > +	fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
> 
> 
> 
> How do we know vbt.backlight.pwm_freq_hz isn't zero? Or am I missing
> something here.
> intel_panel.c: get_backlight_max_vbt() seems to do a check.
> 
> 
> -DK
>

Yes I had the exact same comment, you should check for if (!vbt.backlight.pwm_freq_hz)

Everything else looks good to me.

Manasi
 
> > +
> > +	/* Use lowest possible value of Pn to try to make F to be between 1 and
> > +	 * 255 while still in the range Pn_min and Pn_max
> > +	 */
> > +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> > +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)) {
> > +		return;
> > +	}
> > +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> > +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)) {
> > +		return;
> > +	}
> > +	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +	f = fxp / (1 << pn_min);
> > +	for (pn = pn_min; pn < pn_max && f > 255; pn++)
> > +		f /= 2;
> > +
> > +	/* Cap F to be in the range between 1 and 255. */
> > +	f = min(f, 255);
> > +	f = max(f, 1);
> > +
> > +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> > +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET, (u8) f);
> > +}
> > +
> >  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 new_dpcd_buf = 0;
> >  	uint8_t edp_backlight_mode = 0;
> > +	bool freq_cap;
> >  
> >  	set_aux_backlight_enable(intel_dp, true);
> >  
> > @@ -147,10 +195,18 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
> >  		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
> >  	}
> >  
> > +	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> > +	if (freq_cap)
> > +		new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> > +
> >  	if (new_dpcd_buf != dpcd_buf) {
> >  		drm_dp_dpcd_writeb(&intel_dp->aux,
> >  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> >  	}
> > +
> > +	if (freq_cap)
> > +		intel_dp_aux_set_pwm_freq(connector);
> > +
> >  	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

Re: [PATCH RESEND v4 1/6] drm/i915: Add DPCD preferred mode for backlight control

[Jani Nikula]

On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> Currently the intel_dp_aux_backlight driver requires eDP panel
> to not also support backlight adjustment via PWM pin to use
> this driver.
>
> This force the eDP panel that support both ways of backlight
> adjustment to do it via PWM pin.

Currently this is by design. But I think we agreed previously that the
driver also has incorrect capability checks for the current design. The
first priority would be to fix those checks. And the patch order in the
series should reflect that.

> This patch adds the new prefer DPCD mode in the i915_param
> to make it enable to prefer DPCD over the PWM via kernel param.
>
> This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
> in set_aux_backlight_enable() since the backlight enablement
> can be done via BL_ENABLE eDP connector pin in the case that
> it does not support doing that via AUX.

"also" in the commit message is a strong clue it should be a separate
patch. What you describe is potentially a fix that should precede this
patch.

>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/i915_params.c            |  6 ++---
>  drivers/gpu/drm/i915/i915_params.h            |  2 +-
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 +++++++++++++++++++--------
>  3 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b6a7e363d076..960393dd5edf 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 = 0,
>  	.enable_gvt = false,
>  };
>  
> @@ -246,9 +246,9 @@ 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(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 (0:disable (default), 1:prefer PWM pin, 2: prefer DPCD)");

See my comments below. I think you need to rethink the options.

>  
>  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..bf6e2c60f697 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -51,6 +51,7 @@
>  	func(int, use_mmio_flip); \
>  	func(int, mmio_debug); \
>  	func(int, edp_vswing); \
> +	func(int, enable_dpcd_backlight); \
>  	func(unsigned int, inject_load_failure); \
>  	/* leave bools at the end to not create holes */ \
>  	func(bool, alpha_support); \
> @@ -66,7 +67,6 @@
>  	func(bool, verbose_state_checks); \
>  	func(bool, nuclear_pageflip); \
>  	func(bool, enable_dp_mst); \
> -	func(bool, 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 6532e226db29..42f73d9a3ccf 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  {
>  	uint8_t reg_val = 0;
>  
> +	/* Early return when display use other mechanism to enable backlight. */
> +	if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> +		return;
> +
>  	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
>  			      &reg_val) < 0) {
>  		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> @@ -138,27 +142,36 @@ static bool
>  intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
> +	bool supported;
>  
>  	/* 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> -	    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> -	      (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
> -		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> -		return true;
> +	switch (i915.enable_dpcd_backlight) {
> +	case 1: /* prefer PWM pin */
> +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +			    (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> +			    !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) &&
> +			    !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP);
> +		break;

This is not right. If we're going to use the PWM pin for backlight
control, it's what's in intel_panel.c, and
intel_dp_aux_init_backlight_funcs() should return -ENODEV. We only
support one or the other.

I think you probably need to have a hard look at Table 10-1 "Summary of
Backlight Control Modes Using DPCD Registers" in eDP 1.4b, and tell us
what modes you really want to support and how. For example, the product
mode or any DPCD/PWM mixed modes aren't very easily achieved with the
current design.

BR,
Jani.


> +	case 2: /* prefer DPCD */
> +		supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +			    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> +		break;
> +	default:
> +		supported = false;
>  	}
> -	return false;
> +
> +	if (supported)
> +		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> +
> +	return supported;
>  }
>  
>  int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
>  {
>  	struct intel_panel *panel = &intel_connector->panel;
>  
> -	if (!i915.enable_dpcd_backlight)
> -		return -ENODEV;
> -
>  	if (!intel_dp_aux_display_control_capable(intel_connector))
>  		return -ENODEV;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 2/6] drm/i915: Correctly enable blacklight adjustment via DPCD

[Jani Nikula]

On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> intel_dp_aux_enable_backlight() assumed that the register
> BACKLIGHT_BRIGHTNESS_CONTROL_MODE can only has value 01
> (DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET) when initialize.
>
> This patch fixed that by handling all cases of that register.

Fixes like this should precede the functional changes, i.e. this should
be earlier in the series.

>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 29 +++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 42f73d9a3ccf..f06c8381c74e 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -101,15 +101,32 @@ 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;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> -	if ((drm_dp_dpcd_readb(&intel_dp->aux,
> -			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
> -	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> -	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> -				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));

Ugh, that's really totally busted, since this actually enables the
product mode because it doesn't mask out the preset first. Thanks for
fixing this.

> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf)) {

I don't think there are any valid scenarious where this call would
return 0. if (... != 1) is the right check.

> +		return;
> +	}
> +
> +	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;
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);

I wonder if the right order would be to set the mode first, and call
set_aux_backlight_enable() after that. I couldn't find anything about
that in the spec, but gut feeling says that would be better. Needs to be
a separate change though.

BR,
Jani.

> +		break;
> +
> +	/* Do nothing when it is already DPCD mode */
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
> +	default:
> +		break;
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 4/6] drm/i915: Store brightness level in aux backlight driver

[Jani Nikula]

On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> Some panel will default to zero brightness when turning the
> panel off and on again. This patch stores last brightness level
> before turning off and set them back when panel is turning on.
>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index ae1b6fe67feb..f99cf0a6ae44 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -95,6 +95,7 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
>  		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
>  		return;
>  	}
> +	connector->panel.backlight.level = level;

This gets already done in intel_panel_set_backlight(), and you shouldn't
touch it here.

>  }
>  
>  /*
> @@ -150,6 +151,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
>  	}
> +	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);

Seems like a sensible change.

BR,
Jani.

>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 5/6] drm: Add definition for eDP backlight frequency

[Jani Nikula]

On Tue, 02 May 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Tue, Apr 18, 2017 at 04:48:23PM -0700, Puthikorn Voravootivat wrote:
>
> Since this adds definitions in the DRM layer, you need to copy
> the dri-devel@lists.freedesktop.org M-L.
>
>> This patch adds the following definition
>> - Bit mask for EDP_PWMGEN_BIT_COUNT and min/max cap
>>   register which only use bit 0:4
>> - Base frequency (27 MHz) for backlight PWM frequency
>>   generator.
>> 
>> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>> ---
>>  include/drm/drm_dp_helper.h | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
>> index c0bd0d7651a9..9aee65ebc54c 100644
>> --- a/include/drm/drm_dp_helper.h
>> +++ b/include/drm/drm_dp_helper.h
>> @@ -572,10 +572,12 @@
>>  #define DP_EDP_PWMGEN_BIT_COUNT             0x724
>>  #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN     0x725
>>  #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX     0x726
>> +# define  DP_EDP_PWMGEN_BIT_COUNT_MASK      (31 << 0)

For bit masks like this hex is preferred. 0x1f is way more intuitive
than a decimal number.

>>  
>>  #define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
>>  
>>  #define DP_EDP_BACKLIGHT_FREQ_SET           0x728
>> +# define DP_EDP_BACKLIGHT_FREQ_BASE         27000000
>
> Could you use HEX value to define this? Thats the convention around.

However I think this is fine as decimal.

BR,
Jani.


>
> Manasi
>>  
>>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
>>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b
>> -- 
>> 2.12.2.816.g2cccc81164-goog
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 6/6] drm/i915: Set PWM divider to match desired frequency in vbt

[Jani Nikula]

On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> Read desired PWM frequency from panel vbt and calculate the
> value for divider in DPCD address 0x724 and 0x728 to match
> that frequency as close as possible.
>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 56 +++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index f99cf0a6ae44..9adc77bfb515 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -111,12 +111,60 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
>  			  dbc, sizeof(dbc));
>  }
>  
> +/*
> + * 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 void 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, 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;
> +	fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
> +
> +	/* Use lowest possible value of Pn to try to make F to be between 1 and
> +	 * 255 while still in the range Pn_min and Pn_max
> +	 */
> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)) {
> +		return;
> +	}
> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)) {
> +		return;
> +	}
> +	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	f = fxp / (1 << pn_min);
> +	for (pn = pn_min; pn < pn_max && f > 255; pn++)

pn <= pn_max

> +		f /= 2;
> +
> +	/* Cap F to be in the range between 1 and 255. */
> +	f = min(f, 255);
> +	f = max(f, 1);

See clamp().

All in all the above is a rather complicated way to figure out how many
bits you have to shift (F * P) right to fit it in 8 bits. Observe that
F = (27 MHz / pwm_frew_hz) >> Pn.


BR,
Jani.

> +
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> +	drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET, (u8) f);
> +}
> +
>  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 new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
> +	bool freq_cap;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> @@ -147,10 +195,18 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
>  	}
>  
> +	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> +	if (freq_cap)
> +		new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> +
>  	if (new_dpcd_buf != dpcd_buf) {
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
>  	}
> +
> +	if (freq_cap)
> +		intel_dp_aux_set_pwm_freq(connector);
> +
>  	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
>  }

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 5/6] drm: Add definition for eDP backlight frequency

[Manasi Navare]

On Wed, May 03, 2017 at 03:49:23PM +0300, Jani Nikula wrote:
> On Tue, 02 May 2017, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > On Tue, Apr 18, 2017 at 04:48:23PM -0700, Puthikorn Voravootivat wrote:
> >
> > Since this adds definitions in the DRM layer, you need to copy
> > the dri-devel@lists.freedesktop.org M-L.
> >
> >> This patch adds the following definition
> >> - Bit mask for EDP_PWMGEN_BIT_COUNT and min/max cap
> >>   register which only use bit 0:4
> >> - Base frequency (27 MHz) for backlight PWM frequency
> >>   generator.
> >> 
> >> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> >> ---
> >>  include/drm/drm_dp_helper.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> >> index c0bd0d7651a9..9aee65ebc54c 100644
> >> --- a/include/drm/drm_dp_helper.h
> >> +++ b/include/drm/drm_dp_helper.h
> >> @@ -572,10 +572,12 @@
> >>  #define DP_EDP_PWMGEN_BIT_COUNT             0x724
> >>  #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN     0x725
> >>  #define DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX     0x726
> >> +# define  DP_EDP_PWMGEN_BIT_COUNT_MASK      (31 << 0)
> 
> For bit masks like this hex is preferred. 0x1f is way more intuitive
> than a decimal number.
> 
> >>  
> >>  #define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
> >>  
> >>  #define DP_EDP_BACKLIGHT_FREQ_SET           0x728
> >> +# define DP_EDP_BACKLIGHT_FREQ_BASE         27000000
> >
> > Could you use HEX value to define this? Thats the convention around.
> 
> However I think this is fine as decimal.
> 
> BR,
> Jani.
> 
>

Yea it is the direct representation of 27MHz so I guess decimal
is more intuitive.

Manasi

> >
> > Manasi
> >>  
> >>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
> >>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b
> >> -- 
> >> 2.12.2.816.g2cccc81164-goog
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RESEND v4 3/6] drm/i915: Support dynamic backlight via DPCD register

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 4053 bytes --]

On Tue, May 2, 2017 at 8:00 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote:
> > This patch enables dynamic backlight by default for eDP
> > panel that supports this feature via DPCD register and
> > set minimum / maximum brightness to 0% and 100% of the
> > normal brightness.
>
>
> What does dynamic backlight do? I am trying to understand what allowing
> 0% minimum brightness means.
>
> Display will make backlight dimmer when content shown on screen is darker.
See
https://www.ecnmag.com/article/2010/04/content-adaptive-lcd-backlight-control

>
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 31
> +++++++++++++++++++++++----
> >  1 file changed, 27 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index f06c8381c74e..ae1b6fe67feb 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -97,10 +97,24 @@ 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)
> };
> > +     drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_
> SET,
> > +                       dbc, sizeof(dbc));
> > +}
> > +
> >  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 new_dpcd_buf = 0;
> >       uint8_t edp_backlight_mode = 0;
> >
> >       set_aux_backlight_enable(intel_dp, true);
> > @@ -110,16 +124,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;
> > -             drm_dp_dpcd_writeb(&intel_dp->aux,
> > -                     DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
> > +             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 */
> > @@ -127,6 +140,16 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >       default:
> >               break;
> >       }
>
>
> Still need a switch here? You are setting mode as
> DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD for all four values of mode.
>
>
>
> > +
> > +     if (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);
> > +     }
>
> Should enabling dynamic backlight control be logged in debug messages?
> Could be useful in case this turns out to be buggy.
>
Sure

>
> -DK
>
> > +
> > +     if (new_dpcd_buf != dpcd_buf) {
> > +             drm_dp_dpcd_writeb(&intel_dp->aux,
> > +                     DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> > +     }
> >  }
> >
> >  static void intel_dp_aux_disable_backlight(struct intel_connector
> *connector)
>
>

[-- Attachment #1.2: Type: text/html, Size: 5986 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

Re: [PATCH RESEND v4 6/6] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 4797 bytes --]

On Wed, May 3, 2017 at 7:12 AM, Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> > Read desired PWM frequency from panel vbt and calculate the
> > value for divider in DPCD address 0x724 and 0x728 to match
> > that frequency as close as possible.
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 56
> +++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index f99cf0a6ae44..9adc77bfb515 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -111,12 +111,60 @@ intel_dp_aux_set_dynamic_backlight_percent(struct
> intel_dp *intel_dp,
> >                         dbc, sizeof(dbc));
> >  }
> >
> > +/*
> > + * 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 void 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, 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;
> > +     fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
> > +
> > +     /* Use lowest possible value of Pn to try to make F to be between
> 1 and
> > +      * 255 while still in the range Pn_min and Pn_max
> > +      */
> > +     if (!drm_dp_dpcd_readb(&intel_dp->aux,
> > +                            DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min))
> {
> > +             return;
> > +     }
> > +     if (!drm_dp_dpcd_readb(&intel_dp->aux,
> > +                            DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max))
> {
> > +             return;
> > +     }
> > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +     f = fxp / (1 << pn_min);
> > +     for (pn = pn_min; pn < pn_max && f > 255; pn++)
>
> pn <= pn_max
>
|pn < pn_max| is correct because if f is very high it is possible to get pn
== pn_max+1 if we use |pn <= pn_max|

>
> > +             f /= 2;
> > +
> > +     /* Cap F to be in the range between 1 and 255. */
> > +     f = min(f, 255);
> > +     f = max(f, 1);
>
> See clamp().
>
Will use this in next patch set.

>
> All in all the above is a rather complicated way to figure out how many
> bits you have to shift (F * P) right to fit it in 8 bits. Observe that
> F = (27 MHz / pwm_frew_hz) >> Pn.

Agree that it is quite complicate way but I can't think of more simple way
for this that still
won't get weird result if f is very high or very low.

>
> BR,
> Jani.
>
> > +
> > +     drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_PWMGEN_BIT_COUNT, pn);
> > +     drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
> (u8) f);
> > +}
> > +
> >  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 new_dpcd_buf = 0;
> >       uint8_t edp_backlight_mode = 0;
> > +     bool freq_cap;
> >
> >       set_aux_backlight_enable(intel_dp, true);
> >
> > @@ -147,10 +195,18 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0,
> 100);
> >       }
> >
> > +     freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_
> CAP;
> > +     if (freq_cap)
> > +             new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> > +
> >       if (new_dpcd_buf != dpcd_buf) {
> >               drm_dp_dpcd_writeb(&intel_dp->aux,
> >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> >       }
> > +
> > +     if (freq_cap)
> > +             intel_dp_aux_set_pwm_freq(connector);
> > +
> >       intel_dp_aux_set_backlight(connector, connector->panel.backlight.
> level);
> >  }
>
> --
> Jani Nikula, Intel Open Source Technology Center
>

[-- Attachment #1.2: Type: text/html, Size: 6803 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

Re: [PATCH RESEND v4 1/6] drm/i915: Add DPCD preferred mode for backlight control

[Puthikorn Voravootivat]

[-- Attachment #1.1: Type: text/plain, Size: 8056 bytes --]

On Wed, May 3, 2017 at 2:11 AM, Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> > Currently the intel_dp_aux_backlight driver requires eDP panel
> > to not also support backlight adjustment via PWM pin to use
> > this driver.
> >
> > This force the eDP panel that support both ways of backlight
> > adjustment to do it via PWM pin.
>
> Currently this is by design. But I think we agreed previously that the
> driver also has incorrect capability checks for the current design. The
> first priority would be to fix those checks. And the patch order in the
> series should reflect that.
>
> > This patch adds the new prefer DPCD mode in the i915_param
> > to make it enable to prefer DPCD over the PWM via kernel param.
> >
> > This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
> > in set_aux_backlight_enable() since the backlight enablement
> > can be done via BL_ENABLE eDP connector pin in the case that
> > it does not support doing that via AUX.
>
> "also" in the commit message is a strong clue it should be a separate
> patch. What you describe is potentially a fix that should precede this
> patch.
>
I will split this into 3 patches.
1. Fix cap check
2. Drop AUX backlight enable requirement
3. Allow choosing to use PWM pin or AUX if both supported


> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/i915_params.c            |  6 ++---
> >  drivers/gpu/drm/i915/i915_params.h            |  2 +-
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33
> +++++++++++++++++++--------
> >  3 files changed, 27 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> > index b6a7e363d076..960393dd5edf 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 = 0,
> >       .enable_gvt = false,
> >  };
> >
> > @@ -246,9 +246,9 @@ 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(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 (0:disable (default),
> 1:prefer PWM pin, 2: prefer DPCD)");
>
> See my comments below. I think you need to rethink the options.
>
> >
> >  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..bf6e2c60f697 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -51,6 +51,7 @@
> >       func(int, use_mmio_flip); \
> >       func(int, mmio_debug); \
> >       func(int, edp_vswing); \
> > +     func(int, enable_dpcd_backlight); \
> >       func(unsigned int, inject_load_failure); \
> >       /* leave bools at the end to not create holes */ \
> >       func(bool, alpha_support); \
> > @@ -66,7 +67,6 @@
> >       func(bool, verbose_state_checks); \
> >       func(bool, nuclear_pageflip); \
> >       func(bool, enable_dp_mst); \
> > -     func(bool, 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 6532e226db29..42f73d9a3ccf 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -28,6 +28,10 @@ static void set_aux_backlight_enable(struct intel_dp
> *intel_dp, bool enable)
> >  {
> >       uint8_t reg_val = 0;
> >
> > +     /* Early return when display use other mechanism to enable
> backlight. */
> > +     if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP))
> > +             return;
> > +
> >       if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_
> REGISTER,
> >                             &reg_val) < 0) {
> >               DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> > @@ -138,27 +142,36 @@ static bool
> >  intel_dp_aux_display_control_capable(struct intel_connector *connector)
> >  {
> >       struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> > +     bool supported;
> >
> >       /* 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[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > -         !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
> > -           (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)))
> {
> > -             DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> > -             return true;
> > +     switch (i915.enable_dpcd_backlight) {
> > +     case 1: /* prefer PWM pin */
> > +             supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP)
> &&
> > +                         (intel_dp->edp_dpcd[1] &
> DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
> > +                         !(intel_dp->edp_dpcd[1] &
> DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) &&
> > +                         !(intel_dp->edp_dpcd[2] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP);
> > +             break;
>
> This is not right. If we're going to use the PWM pin for backlight
> control, it's what's in intel_panel.c, and
> intel_dp_aux_init_backlight_funcs() should return -ENODEV. We only
> support one or the other.
>
Fixed in next patch set.

>
> I think you probably need to have a hard look at Table 10-1 "Summary of
> Backlight Control Modes Using DPCD Registers" in eDP 1.4b, and tell us
> what modes you really want to support and how. For example, the product
> mode or any DPCD/PWM mixed modes aren't very easily achieved with the
> current design.
>


What we actually need is that we have panel that
- does not support display backlight enable via AUX
- support display backlight adjustment via AUX
- support display backlight enable via eDP BL_ENABLE pin
- support display backlight adjustment via eDP PWM pin but that pin is not
connected
- support display backlight frequency adjustment via AUX

and we want to
- enable backlight via eDP BL_ENABLE pin
- adjust backlight brightness via AUX
- adjust backlight frequency via AUX

>
> BR,
> Jani.
>
>
> > +     case 2: /* prefer DPCD */
> > +             supported = (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP)
> &&
> > +                         (intel_dp->edp_dpcd[2] &
> DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP);
> > +             break;
> > +     default:
> > +             supported = false;
> >       }
> > -     return false;
> > +
> > +     if (supported)
> > +             DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> > +
> > +     return supported;
> >  }
> >
> >  int intel_dp_aux_init_backlight_funcs(struct intel_connector
> *intel_connector)
> >  {
> >       struct intel_panel *panel = &intel_connector->panel;
> >
> > -     if (!i915.enable_dpcd_backlight)
> > -             return -ENODEV;
> > -
> >       if (!intel_dp_aux_display_control_capable(intel_connector))
> >               return -ENODEV;
>
> --
> Jani Nikula, Intel Open Source Technology Center
>

[-- Attachment #1.2: Type: text/html, Size: 11048 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