[PATCH v5 0/9] Enhancement to intel_dp_aux_backlight driver

[PATCH v5 0/9] Enhancement to intel_dp_aux_backlight driver

[Puthikorn Voravootivat]

This patch set contain 9 patches.
- First five patches fix bug in the driver and allow choosing which
  way to adjust brightness if both PWM pin and AUX are supported
- 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.

v5:
- Split first patch in v4 to 3 patches
- Bump priority for "Correctly enable backlight brightness adjustment via DPCD"
- Make logic clearer for the case that both PWM pin and AUX are supported
- Add more log when write to register fail
- Add log when enable DBC

Puthikorn Voravootivat (9):
  drm/i915: Fix cap check for intel_dp_aux_backlight driver
  drm/i915: Correctly enable backlight brightness adjustment via DPCD
  drm/i915: Drop AUX backlight enable check for backlight control
  drm/i915: Allow choosing how to adjust brightness if both supported
  drm/i915: Set backlight mode before enable backlight
  drm/i915: Support dynamic backlight via DPCD register
  drm/i915: Restore 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 | 161 ++++++++++++++++++++++++--
 include/drm/drm_dp_helper.h                   |   2 +
 4 files changed, 156 insertions(+), 15 deletions(-)

-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 1/9] drm/i915: Fix cap check for intel_dp_aux_backlight driver

[Puthikorn Voravootivat]

intel_dp_aux_backlight driver should check for the
DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP before enable the driver.

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 6532e226db29..24a905d1a74b 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -142,10 +142,9 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
 	/* Check the  eDP Display control capabilities registers to determine if
 	 * the panel can support backlight control over the aux channel
 	 */
-	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
+	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))) {
+	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
 		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
 		return true;
 	}
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 2/9] drm/i915: Correctly enable backlight brightness 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 | 33 ++++++++++++++++++++++-----
 1 file changed, 27 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 24a905d1a74b..ad8560c5f689 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -97,15 +97,36 @@ 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) != 1) {
+		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
+			      DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
+		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;
+		if (drm_dp_dpcd_writeb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) {
+			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
+		}
+		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.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Puthikorn Voravootivat]

There are some panel that
(1) does not support display backlight enable via AUX
(2) support display backlight adjustment via AUX
(3) support display backlight enable via eDP BL_ENABLE pin

The current driver required that (1) must be support to enable (2).
This patch drops that requirement.

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index ad8560c5f689..5b83c9737644 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",
@@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
 	 * 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[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
 		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
 		return true;
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 4/9] drm/i915: Allow choosing how to adjust brightness if both supported

[Puthikorn Voravootivat]

Add option to allow choosing how to adjust brightness if
panel supports both PWM pin and AUX channel.

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 | 17 ++++++++++++++++-
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e363d076..323d12badace 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
 	.huc_firmware_path = NULL,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
-	.enable_dpcd_backlight = false,
+	.enable_dpcd_backlight = -1,
 	.enable_gvt = false,
 };
 
@@ -248,7 +248,9 @@ 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_PARM_DESC(enable_dpcd_backlight,
-	"Enable support for DPCD backlight control (default:false)");
+	"Enable support for DPCD backlight control "
+	"(-1:disable (default), 0:Use PWM pin if both supported, "
+	"1:Use DPCD if both supported");
 
 module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
 MODULE_PARM_DESC(enable_gvt,
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 34148cc8637c..ac02efce6e22 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -66,7 +66,7 @@
 	func(bool, verbose_state_checks); \
 	func(bool, nuclear_pageflip); \
 	func(bool, enable_dp_mst); \
-	func(bool, enable_dpcd_backlight); \
+	func(int, enable_dpcd_backlight); \
 	func(bool, enable_gvt)
 
 #define MEMBER(T, member) T member
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 5b83c9737644..e82f7cb9a7af 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -175,11 +175,26 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
 	return false;
 }
 
+static bool
+intel_dp_pwm_pin_display_control_capable(struct intel_connector *connector)
+{
+	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
+
+	/* Check the  eDP Display control capabilities registers to determine if
+	 * the panel can support backlight control via BL_PWM_DIM eDP pin
+	 */
+	return intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP;
+}
+
 int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector)
 {
 	struct intel_panel *panel = &intel_connector->panel;
 
-	if (!i915.enable_dpcd_backlight)
+	if (i915.enable_dpcd_backlight == -1)
+		return -ENODEV;
+
+	if (i915.enable_dpcd_backlight == 0 &&
+	    intel_dp_pwm_pin_display_control_capable(intel_connector))
 		return -ENODEV;
 
 	if (!intel_dp_aux_display_control_capable(intel_connector))
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 5/9] drm/i915: Set backlight mode before enable backlight

[Puthikorn Voravootivat]

We should set backlight mode register before set register to
enable the backlight.

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index e82f7cb9a7af..5ef3ade7c40e 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -103,8 +103,6 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	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) {
 		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
@@ -131,6 +129,8 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	default:
 		break;
 	}
+
+	set_aux_backlight_enable(intel_dp, true);
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 6/9] 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 | 39 ++++++++++++++++++++++-----
 1 file changed, 33 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 5ef3ade7c40e..7d323af96636 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -97,10 +97,27 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
 	}
 }
 
+/*
+ * Set minimum / maximum dynamic brightness percentage. This value is expressed
+ * as the percentage of normal brightness in 5% increments.
+ */
+static void
+intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
+					   u32 min, u32 max)
+{
+	u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5) };
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
+			  dbc, sizeof(dbc) < 0)) {
+		DRM_DEBUG_KMS("Failed to write aux DBC brightness level\n");
+	}
+}
+
 static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
 	uint8_t dpcd_buf = 0;
+	uint8_t new_dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
@@ -110,18 +127,15 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		return;
 	}
 
+	new_dpcd_buf = dpcd_buf;
 	edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
 
 	switch (edp_backlight_mode) {
 	case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
 	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
 	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
-		dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
-		dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
-		if (drm_dp_dpcd_writeb(&intel_dp->aux,
-			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) {
-			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
-		}
+		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
+		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
 		break;
 
 	/* Do nothing when it is already DPCD mode */
@@ -130,6 +144,19 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		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);
+		DRM_DEBUG_KMS("Enable dynamic brightness.\n");
+	}
+
+	if (new_dpcd_buf != dpcd_buf) {
+		if (drm_dp_dpcd_writeb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
+			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
+		}
+	}
+
 	set_aux_backlight_enable(intel_dp, true);
 }
 
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 7/9] drm/i915: Restore 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 restores last brightness
level back when panel is turning back on.

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 7d323af96636..fc26fea94fd4 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -158,6 +158,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	}
 
 	set_aux_backlight_enable(intel_dp, true);
+	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 8/9] 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..810b7d5d9f2b 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      (0x1f << 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.13.0.rc1.294.g07d810a77f-goog

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

[PATCH v5 9/9] 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 | 71 +++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index fc26fea94fd4..441ad434a82b 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -113,12 +113,76 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
 	}
 }
 
+/*
+ * Set PWM Frequency divider to match desired frequency in vbt.
+ * The PWM Frequency is calculated as 27Mhz / (F x P).
+ * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
+ *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
+ * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
+ *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
+ */
+static 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;
+	DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
+	if (!freq) {
+		DRM_DEBUG_KMS("Use panel default backlight frequency\n");
+		return;
+	}
+
+	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) != 1) {
+		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
+		return;
+	}
+	if (drm_dp_dpcd_readb(&intel_dp->aux,
+			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
+		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
+		return;
+	}
+	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
+
+	f = fxp >> pn_min;
+
+	for (pn = pn_min; pn < pn_max && f > 255; pn++)
+		f >>= 1;
+
+	f = clamp(f, 1, 255);
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux,
+			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
+		DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
+		return;
+	}
+	if (drm_dp_dpcd_writeb(&intel_dp->aux,
+			       DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
+		DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
+		return;
+	}
+}
+
 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;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
@@ -150,6 +214,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		DRM_DEBUG_KMS("Enable dynamic brightness.\n");
 	}
 
+	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) {
 		if (drm_dp_dpcd_writeb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
@@ -157,6 +225,9 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		}
 	}
 
+	if (freq_cap)
+		intel_dp_aux_set_pwm_freq(connector);
+
 	set_aux_backlight_enable(intel_dp, true);
 	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
-- 
2.13.0.rc1.294.g07d810a77f-goog

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

✗ Fi.CI.BAT: failure for Enhancement to intel_dp_aux_backlight driver (rev4)

[Patchwork]

== Series Details ==

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

== Summary ==

make: Entering directory '/home/cidrm/kernel'
  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CHK     include/generated/compile.h
  CHK     kernel/config_data.h
  CC [M]  drivers/gpu/drm/i915/i915_params.o
In file included from ./include/linux/module.h:18:0,
                 from ./include/drm/drmP.h:59,
                 from drivers/gpu/drm/i915/i915_drv.h:47,
                 from drivers/gpu/drm/i915/i915_params.c:26:
drivers/gpu/drm/i915/i915_params.c: In function ‘__check_enable_dpcd_backlight’:
./include/linux/moduleparam.h:344:67: error: return from incompatible pointer type [-Werror=incompatible-pointer-types]
  static inline type __always_unused *__check_##name(void) { return(p); }
                                                                   ^
./include/linux/moduleparam.h:396:35: note: in expansion of macro ‘__param_check’
 #define param_check_bool(name, p) __param_check(name, p, bool)
                                   ^
./include/linux/moduleparam.h:146:2: note: in expansion of macro ‘param_check_bool’
  param_check_##type(name, &(value));       \
  ^
drivers/gpu/drm/i915/i915_params.c:249:1: note: in expansion of macro ‘module_param_named’
 module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
 ^
cc1: all warnings being treated as errors
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_params.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_params.o] Error 1
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1002: recipe for target 'drivers' failed
make: *** [drivers] Error 2
make: Leaving directory '/home/cidrm/kernel'

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

✗ Fi.CI.BAT: failure for Enhancement to intel_dp_aux_backlight driver (rev4)

[Patchwork]

== Series Details ==

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

== Summary ==

  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CHK     include/generated/compile.h
  CHK     kernel/config_data.h
  CC [M]  drivers/gpu/drm/i915/i915_params.o
In file included from ./include/linux/module.h:18:0,
                 from ./include/drm/drmP.h:59,
                 from drivers/gpu/drm/i915/i915_drv.h:47,
                 from drivers/gpu/drm/i915/i915_params.c:26:
drivers/gpu/drm/i915/i915_params.c: In function ‘__check_enable_dpcd_backlight’:
./include/linux/moduleparam.h:344:67: error: return from incompatible pointer type [-Werror=incompatible-pointer-types]
  static inline type __always_unused *__check_##name(void) { return(p); }
                                                                   ^
./include/linux/moduleparam.h:396:35: note: in expansion of macro ‘__param_check’
 #define param_check_bool(name, p) __param_check(name, p, bool)
                                   ^
./include/linux/moduleparam.h:146:2: note: in expansion of macro ‘param_check_bool’
  param_check_##type(name, &(value));       \
  ^
drivers/gpu/drm/i915/i915_params.c:249:1: note: in expansion of macro ‘module_param_named’
 module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
 ^
cc1: all warnings being treated as errors
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/i915_params.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_params.o] Error 1
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1002: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: [PATCH v5 4/9] drm/i915: Allow choosing how to adjust brightness if both supported

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5367 bytes --]

Hi Puthikorn,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on next-20170504]
[cannot apply to v4.11]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Puthikorn-Voravootivat/Enhancement-to-intel_dp_aux_backlight-driver/20170505-003007
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   In file included from include/linux/module.h:18:0,
                    from include/drm/drmP.h:59,
                    from drivers/gpu/drm/i915/i915_drv.h:47,
                    from drivers/gpu/drm/i915/i915_params.c:26:
   drivers/gpu/drm/i915/i915_params.c: In function '__check_enable_dpcd_backlight':
>> include/linux/moduleparam.h:146:27: error: return from incompatible pointer type [-Werror=incompatible-pointer-types]
     param_check_##type(name, &(value));       \
                              ^
   include/linux/moduleparam.h:344:68: note: in definition of macro '__param_check'
     static inline type __always_unused *__check_##name(void) { return(p); }
                                                                       ^
   include/linux/moduleparam.h:146:2: note: in expansion of macro 'param_check_bool'
     param_check_##type(name, &(value));       \
     ^~~~~~~~~~~~
   drivers/gpu/drm/i915/i915_params.c:249:1: note: in expansion of macro 'module_param_named'
    module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
    ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from include/linux/module.h:18:0,
                    from include/drm/drmP.h:59,
                    from drivers/gpu//drm/i915/i915_drv.h:47,
                    from drivers/gpu//drm/i915/i915_params.c:26:
   drivers/gpu//drm/i915/i915_params.c: In function '__check_enable_dpcd_backlight':
>> include/linux/moduleparam.h:146:27: error: return from incompatible pointer type [-Werror=incompatible-pointer-types]
     param_check_##type(name, &(value));       \
                              ^
   include/linux/moduleparam.h:344:68: note: in definition of macro '__param_check'
     static inline type __always_unused *__check_##name(void) { return(p); }
                                                                       ^
   include/linux/moduleparam.h:146:2: note: in expansion of macro 'param_check_bool'
     param_check_##type(name, &(value));       \
     ^~~~~~~~~~~~
   drivers/gpu//drm/i915/i915_params.c:249:1: note: in expansion of macro 'module_param_named'
    module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
    ^~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +146 include/linux/moduleparam.h

3baee201 Jani Nikula   2014-08-27  130   */
3baee201 Jani Nikula   2014-08-27  131  #define module_param_unsafe(name, type, perm)			\
3baee201 Jani Nikula   2014-08-27  132  	module_param_named_unsafe(name, name, type, perm)
3baee201 Jani Nikula   2014-08-27  133  
3baee201 Jani Nikula   2014-08-27  134  /**
546970bc Rusty Russell 2010-08-11  135   * module_param_named - typesafe helper for a renamed module/cmdline parameter
546970bc Rusty Russell 2010-08-11  136   * @name: a valid C identifier which is the parameter name.
546970bc Rusty Russell 2010-08-11  137   * @value: the actual lvalue to alter.
546970bc Rusty Russell 2010-08-11  138   * @type: the type of the parameter
546970bc Rusty Russell 2010-08-11  139   * @perm: visibility in sysfs.
546970bc Rusty Russell 2010-08-11  140   *
546970bc Rusty Russell 2010-08-11  141   * Usually it's a good idea to have variable names and user-exposed names the
546970bc Rusty Russell 2010-08-11  142   * same, but that's harder if the variable must be non-static or is inside a
546970bc Rusty Russell 2010-08-11  143   * structure.  This allows exposure under a different name.
546970bc Rusty Russell 2010-08-11  144   */
546970bc Rusty Russell 2010-08-11  145  #define module_param_named(name, value, type, perm)			   \
546970bc Rusty Russell 2010-08-11 @146  	param_check_##type(name, &(value));				   \
546970bc Rusty Russell 2010-08-11  147  	module_param_cb(name, &param_ops_##type, &value, perm);		   \
546970bc Rusty Russell 2010-08-11  148  	__MODULE_PARM_TYPE(name, #type)
546970bc Rusty Russell 2010-08-11  149  
546970bc Rusty Russell 2010-08-11  150  /**
3baee201 Jani Nikula   2014-08-27  151   * module_param_named_unsafe - same as module_param_named but taints kernel
3baee201 Jani Nikula   2014-08-27  152   */
3baee201 Jani Nikula   2014-08-27  153  #define module_param_named_unsafe(name, value, type, perm)		\
3baee201 Jani Nikula   2014-08-27  154  	param_check_##type(name, &(value));				\

:::::: The code at line 146 was first introduced by commit
:::::: 546970bc6afc7fb37447fbac09b82c7884662c21 param: add kerneldoc to moduleparam.h

:::::: TO: Rusty Russell <rusty@rustcorp.com.au>
:::::: CC: Rusty Russell <rusty@rustcorp.com.au>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38820 bytes --]

[-- Attachment #3: 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 v5 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 17:28 -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 | 71 +++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index fc26fea94fd4..441ad434a82b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -113,12 +113,76 @@ intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
>  	}
>  }
>  
> +/*
> + * Set PWM Frequency divider to match desired frequency in vbt.
> + * The PWM Frequency is calculated as 27Mhz / (F x P).
> + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the
> + *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the
> + *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> + */
> +static 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;
> +	DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> +	if (!freq) {
> +		DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> +		return;
> +	}
> +
> +	fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
> +
> +	/* Use lowest possible value of Pn to try to make F to be between 1 and

What's the reason to use the lowest possible value of Pn? From what I
understand, choosing a higher value of Pn offers more steps for
brightness control.

-DK

> +	 * 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) != 1) {
> +		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> +		return;
> +	}
> +	if (drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
> +		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
> +		return;
> +	}
> +	pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +	pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> +
> +	f = fxp >> pn_min;
> +
> +	for (pn = pn_min; pn < pn_max && f > 255; pn++)
> +		f >>= 1;
> +
> +	f = clamp(f, 1, 255);
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> +		DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> +		return;
> +	}
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +			       DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
> +		DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
> +		return;
> +	}
> +}
> +
>  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;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
> @@ -150,6 +214,10 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		DRM_DEBUG_KMS("Enable dynamic brightness.\n");
>  	}
>  
> +	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) {
>  		if (drm_dp_dpcd_writeb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
> @@ -157,6 +225,9 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		}
>  	}
>  
> +	if (freq_cap)
> +		intel_dp_aux_set_pwm_freq(connector);
> +
>  	set_aux_backlight_enable(intel_dp, true);
>  	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
>  }

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

Re: [PATCH v5 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat wrote:
> There are some panel that
> (1) does not support display backlight enable via AUX
> (2) support display backlight adjustment via AUX
> (3) support display backlight enable via eDP BL_ENABLE pin
> 
> The current driver required that (1) must be support to enable (2).
> This patch drops that requirement.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index ad8560c5f689..5b83c9737644 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;
> +

How is backlight enabled in this case? 

-DK

>  	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",
> @@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  	 * 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[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>  		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
>  		return true;

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

Re: [PATCH v5 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

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

This is not related to brightness control. This calculation is used to set
the PWM frequency.
Frequency = 27 Mhz / (F * 2^ Pn)
Lower Pn means higher value for F which mean more accuracy for this
calculation.


On Sat, May 6, 2017 at 1:35 AM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Wed, 2017-05-03 at 17:28 -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 | 71
> +++++++++++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index fc26fea94fd4..441ad434a82b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -113,12 +113,76 @@ intel_dp_aux_set_dynamic_backlight_percent(struct
> intel_dp *intel_dp,
> >       }
> >  }
> >
> > +/*
> > + * Set PWM Frequency divider to match desired frequency in vbt.
> > + * The PWM Frequency is calculated as 27Mhz / (F x P).
> > + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0
> of the
> > + *             EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h)
> > + * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of
> the
> > + *             EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h)
> > + */
> > +static 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;
> > +     DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> > +     if (!freq) {
> > +             DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> > +             return;
> > +     }
> > +
> > +     fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
> > +
> > +     /* Use lowest possible value of Pn to try to make F to be between
> 1 and
>
> What's the reason to use the lowest possible value of Pn? From what I
> understand, choosing a higher value of Pn offers more steps for
> brightness control.
>
> -DK
>
> > +      * 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)
> != 1) {
> > +             DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> > +             return;
> > +     }
> > +     if (drm_dp_dpcd_readb(&intel_dp->aux,
> > +                            DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max)
> != 1) {
> > +             DRM_DEBUG_KMS("Failed to read pwmgen bit count cap max\n");
> > +             return;
> > +     }
> > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> > +
> > +     f = fxp >> pn_min;
> > +
> > +     for (pn = pn_min; pn < pn_max && f > 255; pn++)
> > +             f >>= 1;
> > +
> > +     f = clamp(f, 1, 255);
> > +
> > +     if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > +                            DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> > +             DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> > +             return;
> > +     }
> > +     if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > +                            DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) {
> > +             DRM_DEBUG_KMS("Failed to write aux backlight freq\n");
> > +             return;
> > +     }
> > +}
> > +
> >  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;
> >
> >       if (drm_dp_dpcd_readb(&intel_dp->aux,
> >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) !=
> 1) {
> > @@ -150,6 +214,10 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> >       }
> >
> > +     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) {
> >               if (drm_dp_dpcd_writeb(&intel_dp->aux,
> >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf)
> < 0) {
> > @@ -157,6 +225,9 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               }
> >       }
> >
> > +     if (freq_cap)
> > +             intel_dp_aux_set_pwm_freq(connector);
> > +
> >       set_aux_backlight_enable(intel_dp, true);
> >       intel_dp_aux_set_backlight(connector, connector->panel.backlight.
> level);
> >  }
>
>

[-- Attachment #1.2: Type: text/html, Size: 7253 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 v5 1/9] drm/i915: Fix cap check for intel_dp_aux_backlight driver

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat wrote:
> intel_dp_aux_backlight driver should check for the
> DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP before enable the driver.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 6532e226db29..24a905d1a74b 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -142,10 +142,9 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  	/* Check the  eDP Display control capabilities registers to determine if
>  	 * the panel can support backlight control over the aux channel
>  	 */
> -	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> +	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))) {

^Were these two changes intended? The patch claims an additional check
for DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP is being added but also
makes this unrelated change. Aren't you changing how the driver
prioritizes AUX v/s PWM brightness control by removing these lines?

-DK


> +	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>  		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
>  		return true;
>  	}

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

Re: [PATCH v5 1/9] drm/i915: Fix cap check for intel_dp_aux_backlight driver

[Puthikorn Voravootivat]

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

I added the option to choose to prioritized AUX or PWM before in last
version of this patch set.
But comment from Jani said that it is better to separate the patch.
The option to prioritized the PWM in now in patch #4

On Mon, May 8, 2017 at 10:55 AM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat wrote:
> > intel_dp_aux_backlight driver should check for the
> > DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP before enable the driver.
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index 6532e226db29..24a905d1a74b 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -142,10 +142,9 @@ intel_dp_aux_display_control_capable(struct
> intel_connector *connector)
> >       /* Check the  eDP Display control capabilities registers to
> determine if
> >        * the panel can support backlight control over the aux channel
> >        */
> > -     if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> &&
> > +     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)))
> {
>
> ^Were these two changes intended? The patch claims an additional check
> for DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP is being added but also
> makes this unrelated change. Aren't you changing how the driver
> prioritizes AUX v/s PWM brightness control by removing these lines?
>
> -DK
>
>
> > +         (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP))
> {
> >               DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> >               return true;
> >       }
>
>

[-- Attachment #1.2: Type: text/html, Size: 2979 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 v5 2/9] drm/i915: Correctly enable backlight brightness adjustment via DPCD

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 17:28 -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>

Spec says both 0x00 and 0x01 are possible power-on defaults. So,

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 ++++++++++++++++++++++-----
>  1 file changed, 27 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 24a905d1a74b..ad8560c5f689 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -97,15 +97,36 @@ 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) != 1) {
> +		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> +			      DP_EDP_BACKLIGHT_MODE_SET_REGISTER);
> +		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;
> +		if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) {
> +			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
> +		}
> +		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 v5 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Pandiyan, Dhinakaran]

On Mon, 2017-05-08 at 10:49 -0700, Puthikorn Voravootivat wrote:
> This is not related to brightness control. This calculation is used to
> set the PWM frequency.
> Frequency = 27 Mhz / (F * 2^ Pn)
> 
> Lower Pn means higher value for F which mean more accuracy for this
> calculation.

I am not sure I follow this. Quoting from the spec - 
"A larger Pn value (meaning more PWM generator control bits) provides a
finer backlight adjustment (increased  granularity),  but  also  limits
the  maximum  backlight  frequency,  as  described in this   section."

and then again

"Allowing Pn to be adjustable provides the flexibility of backlight
dimming granularity vs. maximum backlight frequency."
 
> 
> On Sat, May 6, 2017 at 1:35 AM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Wed, 2017-05-03 at 17:28 -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 | 71
>         +++++++++++++++++++++++++++
>         >  1 file changed, 71 insertions(+)
>         >
>         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > index fc26fea94fd4..441ad434a82b 100644
>         > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > @@ -113,12 +113,76 @@
>         intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp
>         *intel_dp,
>         >       }
>         >  }
>         >
>         > +/*
>         > + * Set PWM Frequency divider to match desired frequency in
>         vbt.
>         > + * The PWM Frequency is calculated as 27Mhz / (F x P).
>         > + * - Where F = PWM Frequency Pre-Divider value programmed
>         by field 7:0 of the
>         > + *             EDP_BACKLIGHT_FREQ_SET register (DPCD
>         Address 00728h)
>         > + * - Where P = 2^Pn, where Pn is the value programmed by
>         field 4:0 of the
>         > + *             EDP_PWMGEN_BIT_COUNT register (DPCD Address
>         00724h)
>         > + */
>         > +static 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;
>         > +     DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz
>         \n", freq);
>         > +     if (!freq) {
>         > +             DRM_DEBUG_KMS("Use panel default backlight
>         frequency\n");
>         > +             return;
>         > +     }
>         > +
>         > +     fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
>         > +
>         > +     /* Use lowest possible value of Pn to try to make F to
>         be between 1 and
>         
>         
>         What's the reason to use the lowest possible value of Pn? From
>         what I
>         understand, choosing a higher value of Pn offers more steps
>         for
>         brightness control.
>         
>         -DK
>         
>         > +      * 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) != 1) {
>         > +             DRM_DEBUG_KMS("Failed to read pwmgen bit count
>         cap min\n");
>         > +             return;
>         > +     }
>         > +     if (drm_dp_dpcd_readb(&intel_dp->aux,
>         > +
>         DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
>         > +             DRM_DEBUG_KMS("Failed to read pwmgen bit count
>         cap max\n");
>         > +             return;
>         > +     }
>         > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
>         > +
>         > +     f = fxp >> pn_min;
>         > +
>         > +     for (pn = pn_min; pn < pn_max && f > 255; pn++)
>         > +             f >>= 1;
>         > +
>         > +     f = clamp(f, 1, 255);
>         > +
>         > +     if (drm_dp_dpcd_writeb(&intel_dp->aux,
>         > +                            DP_EDP_PWMGEN_BIT_COUNT, pn) <
>         0) {
>         > +             DRM_DEBUG_KMS("Failed to write aux pwmgen bit
>         count\n");
>         > +             return;
>         > +     }
>         > +     if (drm_dp_dpcd_writeb(&intel_dp->aux,
>         > +                            DP_EDP_BACKLIGHT_FREQ_SET, (u8)
>         f) < 0) {
>         > +             DRM_DEBUG_KMS("Failed to write aux backlight
>         freq\n");
>         > +             return;
>         > +     }
>         > +}
>         > +
>         >  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;
>         >
>         >       if (drm_dp_dpcd_readb(&intel_dp->aux,
>         >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         &dpcd_buf) != 1) {
>         > @@ -150,6 +214,10 @@ static void
>         intel_dp_aux_enable_backlight(struct intel_connector
>         *connector)
>         >               DRM_DEBUG_KMS("Enable dynamic brightness.\n");
>         >       }
>         >
>         > +     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) {
>         >               if (drm_dp_dpcd_writeb(&intel_dp->aux,
>         >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
>         new_dpcd_buf) < 0) {
>         > @@ -157,6 +225,9 @@ static void
>         intel_dp_aux_enable_backlight(struct intel_connector
>         *connector)
>         >               }
>         >       }
>         >
>         > +     if (freq_cap)
>         > +             intel_dp_aux_set_pwm_freq(connector);
>         > +
>         >       set_aux_backlight_enable(intel_dp, true);
>         >       intel_dp_aux_set_backlight(connector,
>         connector->panel.backlight.level);
>         >  }
>         
>         
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v5 5/9] drm/i915: Set backlight mode before enable backlight

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat wrote:
> We should set backlight mode register before set register to
> enable the backlight.
> 

Sounds reasonable, although I did not find anything in the spec. that
says we should do this. If you've tested and it works, 
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>




> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index e82f7cb9a7af..5ef3ade7c40e 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -103,8 +103,6 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  	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) {
>  		DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n",
> @@ -131,6 +129,8 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  	default:
>  		break;
>  	}
> +
> +	set_aux_backlight_enable(intel_dp, true);
>  }
>  
>  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 v5 9/9] drm/i915: Set PWM divider to match desired frequency in vbt

[Puthikorn Voravootivat]

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

Actually you are right. Sorry it's my mistake.
I was focusing on make the actual frequency match the desired frequency as
much as possible.
But more granularity in backlight adjustment probably more important than
that.

Will submit new version of this patch to fix this.

On Mon, May 8, 2017 at 11:17 AM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Mon, 2017-05-08 at 10:49 -0700, Puthikorn Voravootivat wrote:
> > This is not related to brightness control. This calculation is used to
> > set the PWM frequency.
> > Frequency = 27 Mhz / (F * 2^ Pn)
> >
> > Lower Pn means higher value for F which mean more accuracy for this
> > calculation.
>
> I am not sure I follow this. Quoting from the spec -
> "A larger Pn value (meaning more PWM generator control bits) provides a
> finer backlight adjustment (increased  granularity),  but  also  limits
> the  maximum  backlight  frequency,  as  described in this   section."
>
> and then again
>
> "Allowing Pn to be adjustable provides the flexibility of backlight
> dimming granularity vs. maximum backlight frequency."
>
> >
> > On Sat, May 6, 2017 at 1:35 AM, Pandiyan, Dhinakaran
> > <dhinakaran.pandiyan@intel.com> wrote:
> >         On Wed, 2017-05-03 at 17:28 -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 | 71
> >         +++++++++++++++++++++++++++
> >         >  1 file changed, 71 insertions(+)
> >         >
> >         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         > index fc26fea94fd4..441ad434a82b 100644
> >         > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >         > @@ -113,12 +113,76 @@
> >         intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp
> >         *intel_dp,
> >         >       }
> >         >  }
> >         >
> >         > +/*
> >         > + * Set PWM Frequency divider to match desired frequency in
> >         vbt.
> >         > + * The PWM Frequency is calculated as 27Mhz / (F x P).
> >         > + * - Where F = PWM Frequency Pre-Divider value programmed
> >         by field 7:0 of the
> >         > + *             EDP_BACKLIGHT_FREQ_SET register (DPCD
> >         Address 00728h)
> >         > + * - Where P = 2^Pn, where Pn is the value programmed by
> >         field 4:0 of the
> >         > + *             EDP_PWMGEN_BIT_COUNT register (DPCD Address
> >         00724h)
> >         > + */
> >         > +static 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;
> >         > +     DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz
> >         \n", freq);
> >         > +     if (!freq) {
> >         > +             DRM_DEBUG_KMS("Use panel default backlight
> >         frequency\n");
> >         > +             return;
> >         > +     }
> >         > +
> >         > +     fxp = DP_EDP_BACKLIGHT_FREQ_BASE / freq;
> >         > +
> >         > +     /* Use lowest possible value of Pn to try to make F to
> >         be between 1 and
> >
> >
> >         What's the reason to use the lowest possible value of Pn? From
> >         what I
> >         understand, choosing a higher value of Pn offers more steps
> >         for
> >         brightness control.
> >
> >         -DK
> >
> >         > +      * 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) != 1) {
> >         > +             DRM_DEBUG_KMS("Failed to read pwmgen bit count
> >         cap min\n");
> >         > +             return;
> >         > +     }
> >         > +     if (drm_dp_dpcd_readb(&intel_dp->aux,
> >         > +
> >         DP_EDP_PWMGEN_BIT_COUNT_CAP_MAX, &pn_max) != 1) {
> >         > +             DRM_DEBUG_KMS("Failed to read pwmgen bit count
> >         cap max\n");
> >         > +             return;
> >         > +     }
> >         > +     pn_min &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         > +     pn_max &= DP_EDP_PWMGEN_BIT_COUNT_MASK;
> >         > +
> >         > +     f = fxp >> pn_min;
> >         > +
> >         > +     for (pn = pn_min; pn < pn_max && f > 255; pn++)
> >         > +             f >>= 1;
> >         > +
> >         > +     f = clamp(f, 1, 255);
> >         > +
> >         > +     if (drm_dp_dpcd_writeb(&intel_dp->aux,
> >         > +                            DP_EDP_PWMGEN_BIT_COUNT, pn) <
> >         0) {
> >         > +             DRM_DEBUG_KMS("Failed to write aux pwmgen bit
> >         count\n");
> >         > +             return;
> >         > +     }
> >         > +     if (drm_dp_dpcd_writeb(&intel_dp->aux,
> >         > +                            DP_EDP_BACKLIGHT_FREQ_SET, (u8)
> >         f) < 0) {
> >         > +             DRM_DEBUG_KMS("Failed to write aux backlight
> >         freq\n");
> >         > +             return;
> >         > +     }
> >         > +}
> >         > +
> >         >  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;
> >         >
> >         >       if (drm_dp_dpcd_readb(&intel_dp->aux,
> >         >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> >         &dpcd_buf) != 1) {
> >         > @@ -150,6 +214,10 @@ static void
> >         intel_dp_aux_enable_backlight(struct intel_connector
> >         *connector)
> >         >               DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> >         >       }
> >         >
> >         > +     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) {
> >         >               if (drm_dp_dpcd_writeb(&intel_dp->aux,
> >         >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> >         new_dpcd_buf) < 0) {
> >         > @@ -157,6 +225,9 @@ static void
> >         intel_dp_aux_enable_backlight(struct intel_connector
> >         *connector)
> >         >               }
> >         >       }
> >         >
> >         > +     if (freq_cap)
> >         > +             intel_dp_aux_set_pwm_freq(connector);
> >         > +
> >         >       set_aux_backlight_enable(intel_dp, true);
> >         >       intel_dp_aux_set_backlight(connector,
> >         connector->panel.backlight.level);
> >         >  }
> >
> >
> >
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 12176 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 v5 1/9] drm/i915: Fix cap check for intel_dp_aux_backlight driver

[Pandiyan, Dhinakaran]

On Mon, 2017-05-08 at 11:06 -0700, Puthikorn Voravootivat wrote:
> I added the option to choose to prioritized AUX or PWM before in last
> version of this patch set.
> But comment from Jani said that it is better to separate the patch.
> The option to prioritized the PWM in now in patch #4

You are still modifying priorities in this patch by removing the check
for !(_PIN_ENABLE_CAP || _BRIGHTNESS_PWM_PIN_CAP)

I can see why it made sense to split the previous version, but shouldn't
this patch be just about fixing the CAPS check for the existing design.

i.e.,
+(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {


You can then make the PWM PIN/AUX preference changes in patch 4.


Jani,
please correct me if I'm wrong here.

-DK

> 
> 
> On Mon, May 8, 2017 at 10:55 AM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat
>         wrote:
>         > intel_dp_aux_backlight driver should check for the
>         > DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP before enable the
>         driver.
>         >
>         > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>         > ---
>         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++---
>         >  1 file changed, 2 insertions(+), 3 deletions(-)
>         >
>         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > index 6532e226db29..24a905d1a74b 100644
>         > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > @@ -142,10 +142,9 @@
>         intel_dp_aux_display_control_capable(struct intel_connector
>         *connector)
>         >       /* Check the  eDP Display control capabilities
>         registers to determine if
>         >        * the panel can support backlight control over the
>         aux channel
>         >        */
>         > -     if (intel_dp->edp_dpcd[1] &
>         DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
>         > +     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))) {
>         
>         ^Were these two changes intended? The patch claims an
>         additional check
>         for DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP is being added but
>         also
>         makes this unrelated change. Aren't you changing how the
>         driver
>         prioritizes AUX v/s PWM brightness control by removing these
>         lines?
>         
>         -DK
>         
>         
>         > +         (intel_dp->edp_dpcd[2] &
>         DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>         >               DRM_DEBUG_KMS("AUX Backlight Control
>         Supported!\n");
>         >               return true;
>         >       }
>         
>         
> 
> 
> _______________________________________________
> 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 v5 7/9] drm/i915: Restore brightness level in aux backlight driver

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat wrote:
> Some panel will default to zero brightness when turning the
> panel off and on again. This patch restores last brightness
> level back when panel is turning back on.
> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>

Looks correct and all other _enable_backlight() functions are doing it
too. 
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 7d323af96636..fc26fea94fd4 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -158,6 +158,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  	}
>  
>  	set_aux_backlight_enable(intel_dp, true);
> +	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
>  }
>  
>  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 v5 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Puthikorn Voravootivat]

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

> How is backlight enabled in this case?
Using eDP BL_ENABLE pin

On Sat, May 6, 2017 at 1:59 AM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat wrote:
> > There are some panel that
> > (1) does not support display backlight enable via AUX
> > (2) support display backlight adjustment via AUX
> > (3) support display backlight enable via eDP BL_ENABLE pin
> >
> > The current driver required that (1) must be support to enable (2).
> > This patch drops that requirement.
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index ad8560c5f689..5b83c9737644 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;
> > +
>
> How is backlight enabled in this case?
>
> -DK
>
> >       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",
> > @@ -164,7 +168,6 @@ intel_dp_aux_display_control_capable(struct
> intel_connector *connector)
> >        * 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[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP))
> {
> >               DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
> >               return true;
>
>

[-- Attachment #1.2: Type: text/html, Size: 3066 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 v5 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Pandiyan, Dhinakaran]

On Tue, 2017-05-09 at 16:39 -0700, Puthikorn Voravootivat wrote:
> > How is backlight enabled in this case?
> Using eDP BL_ENABLE pin


Sure, but I am not seeing how this falls back to one of the
[bxt,lpt]_enable_backlight() functions in intel_panel.c. Apologies if I
am missing something very obvious.

If intel_dp_aux_init_backlight_funcs() returned -ENODEV, then one of the
platform specific PWM enable callbacks would be called. But in this
case, dp_aux_enable_backlight() just returns without doing anything.


-DK
> 
> 
> On Sat, May 6, 2017 at 1:59 AM, Pandiyan, Dhinakaran
> <dhinakaran.pandiyan@intel.com> wrote:
>         On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat
>         wrote:
>         > There are some panel that
>         > (1) does not support display backlight enable via AUX
>         > (2) support display backlight adjustment via AUX
>         > (3) support display backlight enable via eDP BL_ENABLE pin
>         >
>         > The current driver required that (1) must be support to
>         enable (2).
>         > This patch drops that requirement.
>         >
>         > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>         > ---
>         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
>         >  1 file changed, 4 insertions(+), 1 deletion(-)
>         >
>         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>         > index ad8560c5f689..5b83c9737644 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;
>         > +
>         
>         How is backlight enabled in this case?
>         
>         -DK
>         
>         >       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",
>         > @@ -164,7 +168,6 @@
>         intel_dp_aux_display_control_capable(struct intel_connector
>         *connector)
>         >        * 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[2] &
>         DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>         >               DRM_DEBUG_KMS("AUX Backlight Control
>         Supported!\n");
>         >               return true;
>         
>         
> 
> 
> _______________________________________________
> 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 v5 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Jani Nikula]

On Wed, 10 May 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
> On Tue, 2017-05-09 at 16:39 -0700, Puthikorn Voravootivat wrote:
>> > How is backlight enabled in this case?
>> Using eDP BL_ENABLE pin
>
>
> Sure, but I am not seeing how this falls back to one of the
> [bxt,lpt]_enable_backlight() functions in intel_panel.c. Apologies if I
> am missing something very obvious.
>
> If intel_dp_aux_init_backlight_funcs() returned -ENODEV, then one of the
> platform specific PWM enable callbacks would be called. But in this
> case, dp_aux_enable_backlight() just returns without doing anything.

The eDP BL_ENABLE pin (in the physical eDP connector) is controlled by
the panel power sequencer, independent from the PWM control. See
intel_edp_backlight_* functions in intel_dp.c.

BR,
Jani.


>
>
> -DK
>> 
>> 
>> On Sat, May 6, 2017 at 1:59 AM, Pandiyan, Dhinakaran
>> <dhinakaran.pandiyan@intel.com> wrote:
>>         On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat
>>         wrote:
>>         > There are some panel that
>>         > (1) does not support display backlight enable via AUX
>>         > (2) support display backlight adjustment via AUX
>>         > (3) support display backlight enable via eDP BL_ENABLE pin
>>         >
>>         > The current driver required that (1) must be support to
>>         enable (2).
>>         > This patch drops that requirement.
>>         >
>>         > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>>         > ---
>>         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
>>         >  1 file changed, 4 insertions(+), 1 deletion(-)
>>         >
>>         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>>         > index ad8560c5f689..5b83c9737644 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;
>>         > +
>>         
>>         How is backlight enabled in this case?
>>         
>>         -DK
>>         
>>         >       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",
>>         > @@ -164,7 +168,6 @@
>>         intel_dp_aux_display_control_capable(struct intel_connector
>>         *connector)
>>         >        * 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[2] &
>>         DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>>         >               DRM_DEBUG_KMS("AUX Backlight Control
>>         Supported!\n");
>>         >               return true;
>>         
>>         
>> 
>> 
>> _______________________________________________
>> 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 v5 6/9] drm/i915: Support dynamic backlight via DPCD register

[Pandiyan, Dhinakaran]

On Wed, 2017-05-03 at 17:28 -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.


I read the link that you shared last time, should there be a switch for
a feature like this that can affect image quality? Should this be a
decision in the kernel with no provision to turn off/on? 


-DK

> 
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 39 ++++++++++++++++++++++-----
>  1 file changed, 33 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 5ef3ade7c40e..7d323af96636 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -97,10 +97,27 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
>  	}
>  }
>  
> +/*
> + * Set minimum / maximum dynamic brightness percentage. This value is expressed
> + * as the percentage of normal brightness in 5% increments.
> + */
> +static void
> +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
> +					   u32 min, u32 max)
> +{
> +	u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5) };
> +
> +	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
> +			  dbc, sizeof(dbc) < 0)) {
> +		DRM_DEBUG_KMS("Failed to write aux DBC brightness level\n");
> +	}
> +}
> +
>  static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
>  	uint8_t dpcd_buf = 0;
> +	uint8_t new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
> @@ -110,18 +127,15 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		return;
>  	}
>  
> +	new_dpcd_buf = dpcd_buf;
>  	edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
>  
>  	switch (edp_backlight_mode) {
>  	case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
>  	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
>  	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
> -		dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> -		dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> -		if (drm_dp_dpcd_writeb(&intel_dp->aux,
> -			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) {
> -			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
> -		}
> +		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> +		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>  		break;
>  
>  	/* Do nothing when it is already DPCD mode */
> @@ -130,6 +144,19 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		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);
> +		DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> +	}
> +
> +	if (new_dpcd_buf != dpcd_buf) {
> +		if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
> +			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
> +		}
> +	}
> +
>  	set_aux_backlight_enable(intel_dp, true);
>  }
>  

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

Re: [PATCH v5 3/9] drm/i915: Drop AUX backlight enable check for backlight control

[Pandiyan, Dhinakaran]

On Wed, 2017-05-10 at 13:05 +0300, Jani Nikula wrote:
> On Wed, 10 May 2017, "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com> wrote:
> > On Tue, 2017-05-09 at 16:39 -0700, Puthikorn Voravootivat wrote:
> >> > How is backlight enabled in this case?
> >> Using eDP BL_ENABLE pin
> >
> >
> > Sure, but I am not seeing how this falls back to one of the
> > [bxt,lpt]_enable_backlight() functions in intel_panel.c. Apologies if I
> > am missing something very obvious.
> >
> > If intel_dp_aux_init_backlight_funcs() returned -ENODEV, then one of the
> > platform specific PWM enable callbacks would be called. But in this
> > case, dp_aux_enable_backlight() just returns without doing anything.
> 
> The eDP BL_ENABLE pin (in the physical eDP connector) is controlled by
> the panel power sequencer, independent from the PWM control. See
> intel_edp_backlight_* functions in intel_dp.c.
> 
> BR,
> Jani.
> 
> 

Ah! Thanks, that makes sense. The naming is a bit confusing though. The
_enable_backlight() functions in intel_panel.c don't enable backlight
but enable PWM.  



> >
> >
> > -DK
> >> 
> >> 
> >> On Sat, May 6, 2017 at 1:59 AM, Pandiyan, Dhinakaran
> >> <dhinakaran.pandiyan@intel.com> wrote:
> >>         On Wed, 2017-05-03 at 17:28 -0700, Puthikorn Voravootivat
> >>         wrote:
> >>         > There are some panel that
> >>         > (1) does not support display backlight enable via AUX
> >>         > (2) support display backlight adjustment via AUX
> >>         > (3) support display backlight enable via eDP BL_ENABLE pin
> >>         >
> >>         > The current driver required that (1) must be support to
> >>         enable (2).
> >>         > This patch drops that requirement.
> >>         >
> >>         > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> >>         > ---
> >>         >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 5 ++++-
> >>         >  1 file changed, 4 insertions(+), 1 deletion(-)
> >>         >
> >>         > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >>         b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> >>         > index ad8560c5f689..5b83c9737644 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;
> >>         > +
> >>         
> >>         How is backlight enabled in this case?
> >>         
> >>         -DK
> >>         
> >>         >       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",
> >>         > @@ -164,7 +168,6 @@
> >>         intel_dp_aux_display_control_capable(struct intel_connector
> >>         *connector)
> >>         >        * 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[2] &
> >>         DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
> >>         >               DRM_DEBUG_KMS("AUX Backlight Control
> >>         Supported!\n");
> >>         >               return true;
> >>         
> >>         
> >> 
> >> 
> >> _______________________________________________
> >> 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 v5 6/9] drm/i915: Support dynamic backlight via DPCD register

[Puthikorn Voravootivat]

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

Fair enough. Will add kernel switch in next version.

On Wed, May 10, 2017 at 6:26 PM, Pandiyan, Dhinakaran <
dhinakaran.pandiyan@intel.com> wrote:

> On Wed, 2017-05-03 at 17:28 -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.
>
>
> I read the link that you shared last time, should there be a switch for
> a feature like this that can affect image quality? Should this be a
> decision in the kernel with no provision to turn off/on?
>
>
> -DK
>
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 39
> ++++++++++++++++++++++-----
> >  1 file changed, 33 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 5ef3ade7c40e..7d323af96636 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -97,10 +97,27 @@ intel_dp_aux_set_backlight(struct intel_connector
> *connector, u32 level)
> >       }
> >  }
> >
> > +/*
> > + * Set minimum / maximum dynamic brightness percentage. This value is
> expressed
> > + * as the percentage of normal brightness in 5% increments.
> > + */
> > +static void
> > +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
> > +                                        u32 min, u32 max)
> > +{
> > +     u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5)
> };
> > +
> > +     if (drm_dp_dpcd_write(&intel_dp->aux,
> DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
> > +                       dbc, sizeof(dbc) < 0)) {
> > +             DRM_DEBUG_KMS("Failed to write aux DBC brightness
> level\n");
> > +     }
> > +}
> > +
> >  static void intel_dp_aux_enable_backlight(struct intel_connector
> *connector)
> >  {
> >       struct intel_dp *intel_dp = enc_to_intel_dp(&connector->
> encoder->base);
> >       uint8_t dpcd_buf = 0;
> > +     uint8_t new_dpcd_buf = 0;
> >       uint8_t edp_backlight_mode = 0;
> >
> >       if (drm_dp_dpcd_readb(&intel_dp->aux,
> > @@ -110,18 +127,15 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               return;
> >       }
> >
> > +     new_dpcd_buf = dpcd_buf;
> >       edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_
> MASK;
> >
> >       switch (edp_backlight_mode) {
> >       case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
> >       case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
> >       case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
> > -             dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> > -             dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> > -             if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > -                     DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) <
> 0) {
> > -                     DRM_DEBUG_KMS("Failed to write aux backlight
> mode\n");
> > -             }
> > +             new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> > +             new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> >               break;
> >
> >       /* Do nothing when it is already DPCD mode */
> > @@ -130,6 +144,19 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               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);
> > +             DRM_DEBUG_KMS("Enable dynamic brightness.\n");
> > +     }
> > +
> > +     if (new_dpcd_buf != dpcd_buf) {
> > +             if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > +                     DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf)
> < 0) {
> > +                     DRM_DEBUG_KMS("Failed to write aux backlight
> mode\n");
> > +             }
> > +     }
> > +
> >       set_aux_backlight_enable(intel_dp, true);
> >  }
> >
>
>

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