intel-gfx.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver
@ 2017-05-23 22:38 Puthikorn Voravootivat
  2017-05-23 22:38 ` [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control Puthikorn Voravootivat
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-23 22:38 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula
  Cc: Puthikorn Voravootivat, dri-devel

This patch set contain 5 patches. Another 4 patches in previous version
was already merged in v7.
- First two patches allow choosing which way to adjust brightness
  if both PWM pin and AUX are supported
- Next patch adds support for dynamic brightness.
- Last two patches set the PWM freqency to match data in panel vbt.

Change log:
v9:
- Fix nits in v8

v8:
- Drop 4 patches that was already merged
- Move DP_EDP_BACKLIGHT_AUX_ENABLE_CAP check to patch #2 to avoid
  behavior change if only apply patch #1
- Add TODO to warn about enable backlight twice in patch #2
- Use DIV_ROUND_CLOSEST instead of just "/" in patch #5
- Fix bug calculate pn in patch #5
- Clarify commit  message / code comment in patch #5

v7:
- Add check in intel_dp_pwm_pin_display_control_capable in patch #4
- Add option in patch #6 to enable DPCD or not
- Change definition in patch #8 and implementation in #9 to use Khz
- Fix compiler warning from build bot in patch #9

v6:
- Address review from Dhinakaran
- Make PWM frequency to have highest value of Pn that make the
  frequency still within 25% of desired frequency.

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

v4:
- Rebase / minor typo fix.

v3:
- Add new implementation of PWM frequency patch

v2:
- Drop PWM frequency patch
- Address suggestion from Jani Nikula

Puthikorn Voravootivat (5):
  drm/i915: Drop AUX backlight enable check for backlight control
  drm/i915: Allow choosing how to adjust brightness if both supported
  drm/i915: Add option to support dynamic backlight via DPCD
  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            |  13 ++-
 drivers/gpu/drm/i915/i915_params.h            |   5 +-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 156 +++++++++++++++++++++++---
 include/drm/drm_dp_helper.h                   |   2 +
 4 files changed, 157 insertions(+), 19 deletions(-)

-- 
2.13.0.219.gdb65acc882-goog

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control
  2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
@ 2017-05-23 22:38 ` Puthikorn Voravootivat
  2017-05-24  8:16   ` Jani Nikula
  2017-05-23 22:38 ` [PATCH v9 2/5] drm/i915: Allow choosing how to adjust brightness if both supported Puthikorn Voravootivat
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-23 22:38 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula
  Cc: Puthikorn Voravootivat, dri-devel

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 | 8 +++++---
 1 file changed, 5 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 b87c5a381d6a..a0995c00fc84 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",
@@ -165,10 +169,8 @@ 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) &&
-	    !((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_PWM_PIN_CAP)) {
 		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
 		return true;
 	}
-- 
2.13.0.219.gdb65acc882-goog

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v9 2/5] drm/i915: Allow choosing how to adjust brightness if both supported
  2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
  2017-05-23 22:38 ` [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control Puthikorn Voravootivat
@ 2017-05-23 22:38 ` Puthikorn Voravootivat
  2017-05-24  8:26   ` [Intel-gfx] " Daniel Vetter
  2017-05-23 22:38 ` [PATCH v9 3/5] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-23 22:38 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula
  Cc: Puthikorn Voravootivat, dri-devel

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            |  8 +++++---
 drivers/gpu/drm/i915/i915_params.h            |  2 +-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 26 ++++++++++++++++++++++----
 3 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6a7e363d076..13cf3f1572ab 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
 	.huc_firmware_path = NULL,
 	.enable_dp_mst = true,
 	.inject_load_failure = 0,
-	.enable_dpcd_backlight = false,
+	.enable_dpcd_backlight = -1,
 	.enable_gvt = false,
 };
 
@@ -246,9 +246,11 @@ MODULE_PARM_DESC(enable_dp_mst,
 module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400);
 MODULE_PARM_DESC(inject_load_failure,
 	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
-module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
+module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);
 MODULE_PARM_DESC(enable_dpcd_backlight,
-	"Enable support for DPCD backlight control (default:false)");
+	"Enable support for DPCD backlight control "
+	"(-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 a0995c00fc84..16ba1924308d 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
 	else
 		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
 
+	/* TODO: If the panel also support enabling backlight via BL_ENABLE pin,
+	 * the backlight will be enabled again in _intel_edp_backlight_on()
+	 */
 	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
 			       reg_val) != 1) {
 		DRM_DEBUG_KMS("Failed to %s aux backlight\n",
@@ -168,20 +171,35 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
 	/* Check the  eDP Display control capabilities registers to determine if
 	 * the panel can support backlight control over the aux channel
 	 */
-	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
-	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
-	    !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
+	if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
+	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
 		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
 		return true;
 	}
 	return false;
 }
 
+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[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
+	       (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.219.gdb65acc882-goog

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v9 3/5] drm/i915: Add option to support dynamic backlight via DPCD
  2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
  2017-05-23 22:38 ` [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control Puthikorn Voravootivat
  2017-05-23 22:38 ` [PATCH v9 2/5] drm/i915: Allow choosing how to adjust brightness if both supported Puthikorn Voravootivat
@ 2017-05-23 22:38 ` Puthikorn Voravootivat
  2017-05-23 22:38 ` [PATCH v9 4/5] drm: Add definition for eDP backlight frequency Puthikorn Voravootivat
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-23 22:38 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula
  Cc: Puthikorn Voravootivat, dri-devel

This patch adds option to enable dynamic backlight for eDP
panel that supports this feature via DPCD register and
set minimum / maximum brightness to 0% and 100% of the
normal brightness.

Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
---
 drivers/gpu/drm/i915/i915_params.c            |  5 ++++
 drivers/gpu/drm/i915/i915_params.h            |  3 +-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 42 ++++++++++++++++++++++-----
 3 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 13cf3f1572ab..6eaf660e74da 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -65,6 +65,7 @@ struct i915_params i915 __read_mostly = {
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = -1,
 	.enable_gvt = false,
+	.enable_dbc = false,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -255,3 +256,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
 module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
 MODULE_PARM_DESC(enable_gvt,
 	"Enable support for Intel GVT-g graphics virtualization host support(default:false)");
+
+module_param_named(enable_dbc, i915.enable_dbc, bool, 0600);
+MODULE_PARM_DESC(enable_dbc,
+	"Enable support for dynamic backlight control (default:false)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index ac02efce6e22..2de3e2850b54 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -67,7 +67,8 @@
 	func(bool, nuclear_pageflip); \
 	func(bool, enable_dp_mst); \
 	func(int, enable_dpcd_backlight); \
-	func(bool, enable_gvt)
+	func(bool, enable_gvt); \
+	func(bool, enable_dbc)
 
 #define MEMBER(T, member) T member
 struct i915_params {
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 16ba1924308d..f1b7855a2d2a 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -100,11 +100,26 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
 	}
 }
 
+/*
+ * Set minimum / maximum dynamic brightness percentage. This value is expressed
+ * as the percentage of normal brightness in 5% increments.
+ */
+static void
+intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
+					   u32 min, u32 max)
+{
+	u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5) };
+
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
+			  dbc, sizeof(dbc)) < 0) {
+		DRM_DEBUG_KMS("Failed to write aux DBC brightness level\n");
+	}
+}
+
 static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
-	uint8_t dpcd_buf = 0;
-	uint8_t edp_backlight_mode = 0;
+	uint8_t dpcd_buf, new_dpcd_buf, edp_backlight_mode;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
@@ -113,18 +128,15 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		return;
 	}
 
+	new_dpcd_buf = dpcd_buf;
 	edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
 
 	switch (edp_backlight_mode) {
 	case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
 	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
 	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
-		dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
-		dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
-		if (drm_dp_dpcd_writeb(&intel_dp->aux,
-			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf) < 0) {
-			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
-		}
+		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
+		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
 		break;
 
 	/* Do nothing when it is already DPCD mode */
@@ -133,6 +145,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		break;
 	}
 
+	if (i915.enable_dbc &&
+	    (intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP)) {
+		new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;
+		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
+		DRM_DEBUG_KMS("Enable dynamic brightness.\n");
+	}
+
+	if (new_dpcd_buf != dpcd_buf) {
+		if (drm_dp_dpcd_writeb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf) < 0) {
+			DRM_DEBUG_KMS("Failed to write aux backlight mode\n");
+		}
+	}
+
 	set_aux_backlight_enable(intel_dp, true);
 	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
-- 
2.13.0.219.gdb65acc882-goog

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v9 4/5] drm: Add definition for eDP backlight frequency
  2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
                   ` (2 preceding siblings ...)
  2017-05-23 22:38 ` [PATCH v9 3/5] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
@ 2017-05-23 22:38 ` Puthikorn Voravootivat
  2017-05-24  8:40   ` Jani Nikula
  2017-05-23 22:38 ` [PATCH v9 5/5] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 16+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-23 22:38 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula
  Cc: Puthikorn Voravootivat, dri-devel

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>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 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..eaa307f6ae8c 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_KHZ     27000
 
 #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
 #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b
-- 
2.13.0.219.gdb65acc882-goog

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v9 5/5] drm/i915: Set PWM divider to match desired frequency in vbt
  2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
                   ` (3 preceding siblings ...)
  2017-05-23 22:38 ` [PATCH v9 4/5] drm: Add definition for eDP backlight frequency Puthikorn Voravootivat
@ 2017-05-23 22:38 ` Puthikorn Voravootivat
  2017-05-26 12:49   ` Jani Nikula
  2017-05-23 22:51 ` ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev8) Patchwork
  2017-05-24  8:15 ` Patchwork
  6 siblings, 1 reply; 16+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-23 22:38 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan, Jani Nikula
  Cc: Puthikorn Voravootivat, dri-devel

Read desired PWM frequency from panel vbt and calculate the
value for divider in DPCD address 0x724 and 0x728 to have
as many bits as possible for PWM duty cyle for granularity of
brightness adjustment while the frequency divisor is still
within 25% of the desired value.

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index f1b7855a2d2a..b7cd44550127 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -116,10 +116,85 @@ 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, fxp_min, fxp_max, fxp_actual, f = 1;
+	u8 pn, pn_min, pn_max;
+
+	/* Find desired value of (F x P)
+	 * Note that, if F x P is out of supported range, the maximum value or
+	 * minimum value will applied automatically. So no need to check that.
+	 */
+	freq = dev_priv->vbt.backlight.pwm_freq_hz;
+	DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
+	if (!freq) {
+		DRM_DEBUG_KMS("Use panel default backlight frequency\n");
+		return;
+	}
+
+	fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
+
+	/* Use highest possible value of Pn for more granularity of brightness
+	 * adjustment while satifying the conditions below.
+	 * - Pn is in the range of Pn_min and Pn_max
+	 * - F is in the range of 1 and 255
+	 * - FxP is within 25% of desired value.
+	 *   Note: 25% is arbitrary value and may need some tweak.
+	 */
+	if (drm_dp_dpcd_readb(&intel_dp->aux,
+			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min) != 1) {
+		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
+		return;
+	}
+	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;
+
+	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
+	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
+	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
+		DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n");
+		return;
+	}
+
+	for (pn = pn_max; pn >= pn_min; pn--) {
+		f = clamp(DIV_ROUND_CLOSEST(fxp , 1 << pn), 1, 255);
+		fxp_actual = f << pn;
+		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
+			break;
+	}
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux,
+			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
+		DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
+		return;
+	}
+	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, new_dpcd_buf, edp_backlight_mode;
+	bool freq_cap;
 
 	if (drm_dp_dpcd_readb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
@@ -152,6 +227,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) {
@@ -159,6 +238,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.219.gdb65acc882-goog

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev8)
  2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
                   ` (4 preceding siblings ...)
  2017-05-23 22:38 ` [PATCH v9 5/5] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
@ 2017-05-23 22:51 ` Patchwork
  2017-05-24  5:34   ` Saarinen, Jani
  2017-05-24  8:15 ` Patchwork
  6 siblings, 1 reply; 16+ messages in thread
From: Patchwork @ 2017-05-23 22:51 UTC (permalink / raw)
  To: Puthikorn Voravootivat; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

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

fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:431s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:438s
fi-bdw-5557u failed to collect. IGT log at Patchwork_4792/fi-bdw-5557u/igt.log
fi-bsw-n3050 failed to collect. IGT log at Patchwork_4792/fi-bsw-n3050/igt.log
fi-bxt-j4205 failed to collect. IGT log at Patchwork_4792/fi-bxt-j4205/igt.log
fi-byt-j1900 failed to collect. IGT log at Patchwork_4792/fi-byt-j1900/igt.log
fi-byt-n2820 failed to collect. IGT log at Patchwork_4792/fi-byt-n2820/igt.log
fi-hsw-4770 failed to collect. IGT log at Patchwork_4792/fi-hsw-4770/igt.log
fi-hsw-4770r failed to collect. IGT log at Patchwork_4792/fi-hsw-4770r/igt.log
fi-ilk-650 failed to collect. IGT log at Patchwork_4792/fi-ilk-650/igt.log
fi-ivb-3520m failed to collect. IGT log at Patchwork_4792/fi-ivb-3520m/igt.log
fi-ivb-3770 failed to collect. IGT log at Patchwork_4792/fi-ivb-3770/igt.log
fi-kbl-7500u failed to collect. IGT log at Patchwork_4792/fi-kbl-7500u/igt.log
fi-kbl-7560u failed to collect. IGT log at Patchwork_4792/fi-kbl-7560u/igt.log
fi-skl-6260u failed to collect. IGT log at Patchwork_4792/fi-skl-6260u/igt.log
fi-skl-6700hq failed to collect. IGT log at Patchwork_4792/fi-skl-6700hq/igt.log
fi-skl-6700k failed to collect. IGT log at Patchwork_4792/fi-skl-6700k/igt.log
fi-skl-6770hq failed to collect. IGT log at Patchwork_4792/fi-skl-6770hq/igt.log
fi-snb-2520m failed to collect. IGT log at Patchwork_4792/fi-snb-2520m/igt.log
fi-snb-2600 failed to collect. IGT log at Patchwork_4792/fi-snb-2600/igt.log

26f8446d6f098e0f05c0aaaa09f29f7f086decf8 drm-tip: 2017y-05m-23d-21h-09m-34s UTC integration manifest
9306eb3 drm/i915: Set PWM divider to match desired frequency in vbt
45be0dd drm: Add definition for eDP backlight frequency
98ab393 drm/i915: Add option to support dynamic backlight via DPCD
51873e2 drm/i915: Allow choosing how to adjust brightness if both supported
a782c81 drm/i915: Drop AUX backlight enable check for backlight control

== Logs ==

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev8)
  2017-05-23 22:51 ` ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev8) Patchwork
@ 2017-05-24  5:34   ` Saarinen, Jani
  0 siblings, 0 replies; 16+ messages in thread
From: Saarinen, Jani @ 2017-05-24  5:34 UTC (permalink / raw)
  To: intel-gfx@lists.freedesktop.org, Puthikorn Voravootivat

Hi,

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Patchwork
> Sent: Wednesday, May 24, 2017 1:52 AM
> To: Puthikorn Voravootivat <puthik@chromium.org>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] ✓ Fi.CI.BAT: success for Enhancement to
> intel_dp_aux_backlight driver (rev8)
> 
> == Series Details ==
> 
> Series: Enhancement to intel_dp_aux_backlight driver (rev8)
> URL   : https://patchwork.freedesktop.org/series/21086/
> State : success
> 
> == Summary ==
> 
> Series 21086v8 Enhancement to intel_dp_aux_backlight driver
> https://patchwork.freedesktop.org/api/1.0/series/21086/revisions/8/mbox/
> 
> fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:431s
> fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:438s
And to be investigated...why not logs. Resending when clear. 

> fi-bdw-5557u failed to collect. IGT log at Patchwork_4792/fi-bdw-5557u/igt.log
> fi-bsw-n3050 failed to collect. IGT log at Patchwork_4792/fi-bsw-n3050/igt.log
> fi-bxt-j4205 failed to collect. IGT log at Patchwork_4792/fi-bxt-j4205/igt.log
> fi-byt-j1900 failed to collect. IGT log at Patchwork_4792/fi-byt-j1900/igt.log
> fi-byt-n2820 failed to collect. IGT log at Patchwork_4792/fi-byt-n2820/igt.log
> fi-hsw-4770 failed to collect. IGT log at Patchwork_4792/fi-hsw-4770/igt.log
> fi-hsw-4770r failed to collect. IGT log at Patchwork_4792/fi-hsw-4770r/igt.log
> fi-ilk-650 failed to collect. IGT log at Patchwork_4792/fi-ilk-650/igt.log fi-ivb-
> 3520m failed to collect. IGT log at Patchwork_4792/fi-ivb-3520m/igt.log
> fi-ivb-3770 failed to collect. IGT log at Patchwork_4792/fi-ivb-3770/igt.log
> fi-kbl-7500u failed to collect. IGT log at Patchwork_4792/fi-kbl-7500u/igt.log
> fi-kbl-7560u failed to collect. IGT log at Patchwork_4792/fi-kbl-7560u/igt.log
> fi-skl-6260u failed to collect. IGT log at Patchwork_4792/fi-skl-6260u/igt.log
> fi-skl-6700hq failed to collect. IGT log at Patchwork_4792/fi-skl-6700hq/igt.log
> fi-skl-6700k failed to collect. IGT log at Patchwork_4792/fi-skl-6700k/igt.log
> fi-skl-6770hq failed to collect. IGT log at Patchwork_4792/fi-skl-6770hq/igt.log
> fi-snb-2520m failed to collect. IGT log at Patchwork_4792/fi-snb-2520m/igt.log
> fi-snb-2600 failed to collect. IGT log at Patchwork_4792/fi-snb-2600/igt.log
> 
> 26f8446d6f098e0f05c0aaaa09f29f7f086decf8 drm-tip: 2017y-05m-23d-21h-
> 09m-34s UTC integration manifest
> 9306eb3 drm/i915: Set PWM divider to match desired frequency in vbt 45be0dd
> drm: Add definition for eDP backlight frequency
> 98ab393 drm/i915: Add option to support dynamic backlight via DPCD
> 51873e2 drm/i915: Allow choosing how to adjust brightness if both supported
> a782c81 drm/i915: Drop AUX backlight enable check for backlight control
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4792/
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 16+ messages in thread

* ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev8)
  2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
                   ` (5 preceding siblings ...)
  2017-05-23 22:51 ` ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev8) Patchwork
@ 2017-05-24  8:15 ` Patchwork
  6 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2017-05-24  8:15 UTC (permalink / raw)
  To: Puthikorn Voravootivat; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

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

Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-atomic:
                fail       -> PASS       (fi-snb-2600) fdo#100215

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

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:445s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:430s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:583s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:505s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:488s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:490s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:421s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:415s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:418s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:502s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:464s
fi-kbl-7500u     total:278  pass:255  dwarn:5   dfail:0   fail:0   skip:18  time:470s
fi-kbl-7560u     total:278  pass:263  dwarn:5   dfail:0   fail:0   skip:10  time:569s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:457s
fi-skl-6700hq    total:278  pass:239  dwarn:0   dfail:1   fail:17  skip:21  time:434s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:471s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:506s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:444s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:533s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time:405s

26f8446d6f098e0f05c0aaaa09f29f7f086decf8 drm-tip: 2017y-05m-23d-21h-09m-34s UTC integration manifest
b8fd48f drm/i915: Set PWM divider to match desired frequency in vbt
c09ffde drm: Add definition for eDP backlight frequency
7da6b4f drm/i915: Add option to support dynamic backlight via DPCD
0bd9b03 drm/i915: Allow choosing how to adjust brightness if both supported
3227a9e drm/i915: Drop AUX backlight enable check for backlight control

== Logs ==

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control
  2017-05-23 22:38 ` [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control Puthikorn Voravootivat
@ 2017-05-24  8:16   ` Jani Nikula
  2017-05-26 12:28     ` Jani Nikula
  0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2017-05-24  8:16 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan; +Cc: Puthikorn Voravootivat, dri-devel

On Tue, 23 May 2017, Puthikorn Voravootivat <puthik@chromium.org> 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>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 8 +++++---
>  1 file changed, 5 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 b87c5a381d6a..a0995c00fc84 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",
> @@ -165,10 +169,8 @@ 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) &&
> -	    !((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_PWM_PIN_CAP)) {
>  		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
>  		return true;
>  	}

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Intel-gfx] [PATCH v9 2/5] drm/i915: Allow choosing how to adjust brightness if both supported
  2017-05-23 22:38 ` [PATCH v9 2/5] drm/i915: Allow choosing how to adjust brightness if both supported Puthikorn Voravootivat
@ 2017-05-24  8:26   ` Daniel Vetter
  0 siblings, 0 replies; 16+ messages in thread
From: Daniel Vetter @ 2017-05-24  8:26 UTC (permalink / raw)
  To: Puthikorn Voravootivat; +Cc: dri-devel, intel-gfx, Dhinakaran Pandiyan

On Tue, May 23, 2017 at 03:38:02PM -0700, Puthikorn Voravootivat wrote:
> 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            |  8 +++++---
>  drivers/gpu/drm/i915/i915_params.h            |  2 +-
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 26 ++++++++++++++++++++++----
>  3 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index b6a7e363d076..13cf3f1572ab 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = {
>  	.huc_firmware_path = NULL,
>  	.enable_dp_mst = true,
>  	.inject_load_failure = 0,
> -	.enable_dpcd_backlight = false,
> +	.enable_dpcd_backlight = -1,
>  	.enable_gvt = false,
>  };
>  
> @@ -246,9 +246,11 @@ MODULE_PARM_DESC(enable_dp_mst,
>  module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400);
>  MODULE_PARM_DESC(inject_load_failure,
>  	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
> -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
> +module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);

This module parameter must be an _unsafe one, we cannot expect users to
set this stuff correctly. For debugging this is fine, for shipping it's
not fine at all.

For upstream this either needs to work as-is (using suitable
heuristics/whitelists/whatever), or we need to throw out the entire
support. Having dead code around is not an option imo.

Also, Jani told me that apparently the reason we had to disable this is
that it broke an SDP. We don't support SDP once the platform is past PV,
because they're a pain. So if that's the only reason, then please
re-enable the code again.

Once more: No module options that you expect to be used for configuring
the driver. Especially around backlights the situation we're in is already
a disaster, let's not make it worse. If i915 doesn't know how to drive the
backlight, then no one else will.

As-is, nack on this approach. We need:
1) _unsafe mod option
2) get this enabled by default
3) fix exceptions in the code, not by exposing even more tuning knobs

Thanks, Daniel

>  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 a0995c00fc84..16ba1924308d 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable)
>  	else
>  		reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE);
>  
> +	/* TODO: If the panel also support enabling backlight via BL_ENABLE pin,
> +	 * the backlight will be enabled again in _intel_edp_backlight_on()
> +	 */
>  	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER,
>  			       reg_val) != 1) {
>  		DRM_DEBUG_KMS("Failed to %s aux backlight\n",
> @@ -168,20 +171,35 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector)
>  	/* Check the  eDP Display control capabilities registers to determine if
>  	 * the panel can support backlight control over the aux channel
>  	 */
> -	if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
> -	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
> -	    !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) {
> +	if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +	    (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
>  		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
>  		return true;
>  	}
>  	return false;
>  }
>  
> +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[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) &&
> +	       (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.219.gdb65acc882-goog
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v9 4/5] drm: Add definition for eDP backlight frequency
  2017-05-23 22:38 ` [PATCH v9 4/5] drm: Add definition for eDP backlight frequency Puthikorn Voravootivat
@ 2017-05-24  8:40   ` Jani Nikula
  2017-05-26 12:30     ` Jani Nikula
  0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2017-05-24  8:40 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan; +Cc: Puthikorn Voravootivat, dri-devel

On Tue, 23 May 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> 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>
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  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..eaa307f6ae8c 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)
           ^^

Extra space crept in.

BR,
Jani.

>  
>  #define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
>  
>  #define DP_EDP_BACKLIGHT_FREQ_SET           0x728
> +# define DP_EDP_BACKLIGHT_FREQ_BASE_KHZ     27000
>  
>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control
  2017-05-24  8:16   ` Jani Nikula
@ 2017-05-26 12:28     ` Jani Nikula
  0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2017-05-26 12:28 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan
  Cc: Manasi Navare, Stephane Marchesin, Puthikorn Voravootivat,
	dri-devel

On Wed, 24 May 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 23 May 2017, Puthikorn Voravootivat <puthik@chromium.org> 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>
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

And pushed to drm-intel-next-queued, thanks for the patch.

BR,
Jani.

>
>> ---
>>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 8 +++++---
>>  1 file changed, 5 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 b87c5a381d6a..a0995c00fc84 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",
>> @@ -165,10 +169,8 @@ 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) &&
>> -	    !((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_PWM_PIN_CAP)) {
>>  		DRM_DEBUG_KMS("AUX Backlight Control Supported!\n");
>>  		return true;
>>  	}

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v9 4/5] drm: Add definition for eDP backlight frequency
  2017-05-24  8:40   ` Jani Nikula
@ 2017-05-26 12:30     ` Jani Nikula
  0 siblings, 0 replies; 16+ messages in thread
From: Jani Nikula @ 2017-05-26 12:30 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan
  Cc: Manasi Navare, Stephane Marchesin, Puthikorn Voravootivat,
	dri-devel

On Wed, 24 May 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 23 May 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
>> 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>
>> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> ---
>>  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..eaa307f6ae8c 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)
>            ^^
>
> Extra space crept in.

Fixed that while applying to drm-intel-next-queued, thanks for the
patch. Pushed via drm-intel with Dave's IRC ack to not require a
backmerge for the subsequent patches.

BR,
Jani.


>
> BR,
> Jani.
>
>>  
>>  #define DP_EDP_BACKLIGHT_CONTROL_STATUS     0x727
>>  
>>  #define DP_EDP_BACKLIGHT_FREQ_SET           0x728
>> +# define DP_EDP_BACKLIGHT_FREQ_BASE_KHZ     27000
>>  
>>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MSB   0x72a
>>  #define DP_EDP_BACKLIGHT_FREQ_CAP_MIN_MID   0x72b

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v9 5/5] drm/i915: Set PWM divider to match desired frequency in vbt
  2017-05-23 22:38 ` [PATCH v9 5/5] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
@ 2017-05-26 12:49   ` Jani Nikula
  2017-05-26 23:01     ` Puthikorn Voravootivat
  0 siblings, 1 reply; 16+ messages in thread
From: Jani Nikula @ 2017-05-26 12:49 UTC (permalink / raw)
  To: intel-gfx, Dhinakaran Pandiyan
  Cc: Manasi Navare, Stephane Marchesin, Puthikorn Voravootivat,
	dri-devel

On Tue, 23 May 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> Read desired PWM frequency from panel vbt and calculate the
> value for divider in DPCD address 0x724 and 0x728 to have
> as many bits as possible for PWM duty cyle for granularity of
> brightness adjustment while the frequency divisor is still
> within 25% of the desired value.

IIUC this patch doesn't have a dependency on the more contentious
patches 2/5 and 3/5. This should probably be merged before them.

I share DK's concern about doing a bunch of reads and writes and
calculations every time the backlight's enabled. But I guess that could
be optimized later.

I haven't had time to check the changed algorithm here, but in the mean
time one comment below.

BR,
Jani.


>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 82 +++++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index f1b7855a2d2a..b7cd44550127 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -116,10 +116,85 @@ 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, fxp_min, fxp_max, fxp_actual, f = 1;
> +	u8 pn, pn_min, pn_max;
> +
> +	/* Find desired value of (F x P)
> +	 * Note that, if F x P is out of supported range, the maximum value or
> +	 * minimum value will applied automatically. So no need to check that.
> +	 */
> +	freq = dev_priv->vbt.backlight.pwm_freq_hz;
> +	DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> +	if (!freq) {
> +		DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> +		return;
> +	}
> +
> +	fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq);
> +
> +	/* Use highest possible value of Pn for more granularity of brightness
> +	 * adjustment while satifying the conditions below.
> +	 * - Pn is in the range of Pn_min and Pn_max
> +	 * - F is in the range of 1 and 255
> +	 * - FxP is within 25% of desired value.
> +	 *   Note: 25% is arbitrary value and may need some tweak.
> +	 */
> +	if (drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min) != 1) {
> +		DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> +		return;
> +	}
> +	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;
> +
> +	fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> +	fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> +	if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> +		DRM_DEBUG_KMS("VBT defined backlight frequency out of range\n");
> +		return;
> +	}
> +
> +	for (pn = pn_max; pn >= pn_min; pn--) {
> +		f = clamp(DIV_ROUND_CLOSEST(fxp , 1 << pn), 1, 255);
> +		fxp_actual = f << pn;
> +		if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> +			break;
> +	}
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux,
> +			       DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> +		DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> +		return;
> +	}
> +	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, new_dpcd_buf, edp_backlight_mode;
> +	bool freq_cap;
>  
>  	if (drm_dp_dpcd_readb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) != 1) {
> @@ -152,6 +227,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) {
> @@ -159,6 +238,9 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		}
>  	}
>  
> +	if (freq_cap)
> +		intel_dp_aux_set_pwm_freq(connector);

What happens if there are any errors within that function, and it fails
to write DP_EDP_BACKLIGHT_FREQ_SET, but
DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE has been set?


> +
>  	set_aux_backlight_enable(intel_dp, true);
>  	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
>  }

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v9 5/5] drm/i915: Set PWM divider to match desired frequency in vbt
  2017-05-26 12:49   ` Jani Nikula
@ 2017-05-26 23:01     ` Puthikorn Voravootivat
  0 siblings, 0 replies; 16+ messages in thread
From: Puthikorn Voravootivat @ 2017-05-26 23:01 UTC (permalink / raw)
  To: Jani Nikula
  Cc: intel-gfx, dri-devel, Dhinakaran Pandiyan, Puthikorn Voravootivat


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

Good catch.
It will use default frequency in this case. But it is better to not
set DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP in this case.

I will add return value to intel_dp_aux_set_pwm_freq() and set
DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP based on that.

On Fri, May 26, 2017 at 5:49 AM, Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Tue, 23 May 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> > Read desired PWM frequency from panel vbt and calculate the
> > value for divider in DPCD address 0x724 and 0x728 to have
> > as many bits as possible for PWM duty cyle for granularity of
> > brightness adjustment while the frequency divisor is still
> > within 25% of the desired value.
>
> IIUC this patch doesn't have a dependency on the more contentious
> patches 2/5 and 3/5. This should probably be merged before them.
>
> I share DK's concern about doing a bunch of reads and writes and
> calculations every time the backlight's enabled. But I guess that could
> be optimized later.
>
> I haven't had time to check the changed algorithm here, but in the mean
> time one comment below.
>
> BR,
> Jani.
>
>
> >
> > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 82
> +++++++++++++++++++++++++++
> >  1 file changed, 82 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index f1b7855a2d2a..b7cd44550127 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -116,10 +116,85 @@ 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, fxp_min, fxp_max, fxp_actual, f = 1;
> > +     u8 pn, pn_min, pn_max;
> > +
> > +     /* Find desired value of (F x P)
> > +      * Note that, if F x P is out of supported range, the maximum
> value or
> > +      * minimum value will applied automatically. So no need to check
> that.
> > +      */
> > +     freq = dev_priv->vbt.backlight.pwm_freq_hz;
> > +     DRM_DEBUG_KMS("VBT defined backlight frequency %u Hz\n", freq);
> > +     if (!freq) {
> > +             DRM_DEBUG_KMS("Use panel default backlight frequency\n");
> > +             return;
> > +     }
> > +
> > +     fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ),
> freq);
> > +
> > +     /* Use highest possible value of Pn for more granularity of
> brightness
> > +      * adjustment while satifying the conditions below.
> > +      * - Pn is in the range of Pn_min and Pn_max
> > +      * - F is in the range of 1 and 255
> > +      * - FxP is within 25% of desired value.
> > +      *   Note: 25% is arbitrary value and may need some tweak.
> > +      */
> > +     if (drm_dp_dpcd_readb(&intel_dp->aux,
> > +                            DP_EDP_PWMGEN_BIT_COUNT_CAP_MIN, &pn_min)
> != 1) {
> > +             DRM_DEBUG_KMS("Failed to read pwmgen bit count cap min\n");
> > +             return;
> > +     }
> > +     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;
> > +
> > +     fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4);
> > +     fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4);
> > +     if (fxp_min < (1 << pn_min) || (255 << pn_max) < fxp_max) {
> > +             DRM_DEBUG_KMS("VBT defined backlight frequency out of
> range\n");
> > +             return;
> > +     }
> > +
> > +     for (pn = pn_max; pn >= pn_min; pn--) {
> > +             f = clamp(DIV_ROUND_CLOSEST(fxp , 1 << pn), 1, 255);
> > +             fxp_actual = f << pn;
> > +             if (fxp_min <= fxp_actual && fxp_actual <= fxp_max)
> > +                     break;
> > +     }
> > +
> > +     if (drm_dp_dpcd_writeb(&intel_dp->aux,
> > +                            DP_EDP_PWMGEN_BIT_COUNT, pn) < 0) {
> > +             DRM_DEBUG_KMS("Failed to write aux pwmgen bit count\n");
> > +             return;
> > +     }
> > +     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, new_dpcd_buf, edp_backlight_mode;
> > +     bool freq_cap;
> >
> >       if (drm_dp_dpcd_readb(&intel_dp->aux,
> >                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) !=
> 1) {
> > @@ -152,6 +227,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) {
> > @@ -159,6 +238,9 @@ static void intel_dp_aux_enable_backlight(struct
> intel_connector *connector)
> >               }
> >       }
> >
> > +     if (freq_cap)
> > +             intel_dp_aux_set_pwm_freq(connector);
>
> What happens if there are any errors within that function, and it fails
> to write DP_EDP_BACKLIGHT_FREQ_SET, but
> DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE has been set?
>
>
> > +
> >       set_aux_backlight_enable(intel_dp, true);
> >       intel_dp_aux_set_backlight(connector, connector->panel.backlight.
> level);
> >  }
>
> --
> Jani Nikula, Intel Open Source Technology Center
>

[-- Attachment #1.2: Type: text/html, Size: 8797 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-05-26 23:01 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-23 22:38 [PATCH v9 0/5] Enhancement to intel_dp_aux_backlight driver Puthikorn Voravootivat
2017-05-23 22:38 ` [PATCH v9 1/5] drm/i915: Drop AUX backlight enable check for backlight control Puthikorn Voravootivat
2017-05-24  8:16   ` Jani Nikula
2017-05-26 12:28     ` Jani Nikula
2017-05-23 22:38 ` [PATCH v9 2/5] drm/i915: Allow choosing how to adjust brightness if both supported Puthikorn Voravootivat
2017-05-24  8:26   ` [Intel-gfx] " Daniel Vetter
2017-05-23 22:38 ` [PATCH v9 3/5] drm/i915: Add option to support dynamic backlight via DPCD Puthikorn Voravootivat
2017-05-23 22:38 ` [PATCH v9 4/5] drm: Add definition for eDP backlight frequency Puthikorn Voravootivat
2017-05-24  8:40   ` Jani Nikula
2017-05-26 12:30     ` Jani Nikula
2017-05-23 22:38 ` [PATCH v9 5/5] drm/i915: Set PWM divider to match desired frequency in vbt Puthikorn Voravootivat
2017-05-26 12:49   ` Jani Nikula
2017-05-26 23:01     ` Puthikorn Voravootivat
2017-05-23 22:51 ` ✓ Fi.CI.BAT: success for Enhancement to intel_dp_aux_backlight driver (rev8) Patchwork
2017-05-24  5:34   ` Saarinen, Jani
2017-05-24  8:15 ` Patchwork

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).