[PATCH 0/5] Enchancement to intel_dp_aux_backlight driver

[PATCH 0/5] Enchancement to intel_dp_aux_backlight driver

[Puthikorn Voravootivat]

This patch set contain 5 patches.
- First two patches fix bug in intel_dp_aux_backlight where DPCD
  register is incorrectly used.
- Next two patches add default config for the eDP panel control
  when using this driver.
- Last patch makes the driver restore last brightness level after
  turning display off and on.

Puthikorn Voravootivat (5):
  drm/i915: Fix condition check for backlight control via DPCD
  drm/i915: Correctly enable blacklight adjustment via DPCD
  drm/i915: Support dynamic backlight via DPCD register
  drm/i915: Use highest frequency divider for PWM
  drm/i915: Store brightness level in aux backlight driver

 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 75 +++++++++++++++++++++++----
 1 file changed, 65 insertions(+), 10 deletions(-)

-- 
2.12.0.246.ga2ecc84866-goog

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

[PATCH 1/5] drm/i915: Fix condition check for backlight control via DPCD

[Puthikorn Voravootivat]

Currently the intel_dp_aux_backlight driver requires eDP panel
to support these conditions to allow the backlight adjust via
dpcd register.
1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
2) DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
3) not DP_EDP_BACKLIGHT_PIN_ENABLE_CAP
4) not DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP

Condition 2,3,4 are incorect because of the following reasons.
For 2), "backlight can be enabled via AUX" is not "brightness
can be adjusted via AUX". For 3) and 4), this requires panel
to not support backlight adjustment via eDP BL_PWM_DIM pin,
but there are some panels that support both AUX and eDP pin.

This patch fixes the condition by checking for these instead.
1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
2) DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP

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

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 6532e226db29..abfb09c1269f 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",
@@ -142,10 +146,8 @@ 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[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))) {
+	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;
 	}
-- 
2.12.0.246.ga2ecc84866-goog

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

[PATCH 2/5] drm/i915: Correctly enable blacklight adjustment via DPCD

[Puthikorn Voravootivat]

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

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

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index abfb09c1269f..16cd1265d264 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -101,15 +101,32 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
 	uint8_t dpcd_buf = 0;
+	uint8_t edp_backlight_mode = 0;
 
 	set_aux_backlight_enable(intel_dp, true);
 
-	if ((drm_dp_dpcd_readb(&intel_dp->aux,
-			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
-	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
-	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
-				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));
+	if (!drm_dp_dpcd_readb(&intel_dp->aux,
+			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf)) {
+		return;
+	}
+
+	edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
+
+	switch (edp_backlight_mode) {
+	case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
+	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
+		dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
+		dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
+		break;
+
+	/* Do nothing in these cases */
+	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
+	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:
+	default:
+		break;
+	}
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.12.0.246.ga2ecc84866-goog

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

[PATCH 3/5] drm/i915: Support dynamic backlight via DPCD register

[Puthikorn Voravootivat]

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

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 16cd1265d264..643b604be2de 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -97,10 +97,24 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
 	}
 }
 
+/*
+ * Set minimum / maximum dynamic brightness percentage. This value is expressed
+ * as the percentage of normal brightness in 5% increments.
+ */
+static void intel_dp_aux_set_dynamic_backlight_percent(
+		struct intel_dp *intel_dp, u32 min, u32 max)
+{
+	drm_dp_dpcd_writeb(&intel_dp->aux,
+		DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET, min / 5);
+	drm_dp_dpcd_writeb(&intel_dp->aux,
+		DP_EDP_DBC_MAXIMUM_BRIGHTNESS_SET, max / 5);
+}
+
 static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
 	uint8_t dpcd_buf = 0;
+	uint8_t new_dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
 
 	set_aux_backlight_enable(intel_dp, true);
@@ -110,15 +124,14 @@ 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:
-		dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
-		dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
-		drm_dp_dpcd_writeb(&intel_dp->aux,
-			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
+		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
+		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
 		break;
 
 	/* Do nothing in these cases */
@@ -127,6 +140,16 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	default:
 		break;
 	}
+
+	if (intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP) {
+		new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;
+		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
+	}
+
+	if (new_dpcd_buf != dpcd_buf) {
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
+	}
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.12.0.246.ga2ecc84866-goog

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

[PATCH 4/5] drm/i915: Use highest frequency divider for PWM

[Puthikorn Voravootivat]

TCON tend to have better brightness scaling with lower
PWM frequency. This patch set the divider to highest
value to lower the PWM frequency.

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 643b604be2de..32b426006a6a 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -116,6 +116,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 	uint8_t dpcd_buf = 0;
 	uint8_t new_dpcd_buf = 0;
 	uint8_t edp_backlight_mode = 0;
+	bool freq_cap = false;
 
 	set_aux_backlight_enable(intel_dp, true);
 
@@ -146,10 +147,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
 	}
 
+	/* Use highest frequency divider if supported */
+	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
+	if (freq_cap)
+		new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+
 	if (new_dpcd_buf != dpcd_buf) {
 		drm_dp_dpcd_writeb(&intel_dp->aux,
 			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
 	}
+
+	if (freq_cap) {
+		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
+			0xff);
+	}
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.12.0.246.ga2ecc84866-goog

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

[PATCH 5/5] drm/i915: Store brightness level in aux backlight driver

[Puthikorn Voravootivat]

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

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

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
index 32b426006a6a..fd7258715c88 100644
--- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -95,6 +95,7 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
 		DRM_DEBUG_KMS("Failed to write aux backlight level\n");
 		return;
 	}
+	connector->panel.backlight.level = level;
 }
 
 /*
@@ -161,6 +162,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
 		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
 			0xff);
 	}
+	intel_dp_aux_set_backlight(connector, connector->panel.backlight.level);
 }
 
 static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
-- 
2.12.0.246.ga2ecc84866-goog

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

✓ Fi.CI.BAT: success for Enchancement to intel_dp_aux_backlight driver

[Patchwork]

== Series Details ==

Series: Enchancement to intel_dp_aux_backlight driver
URL   : https://patchwork.freedesktop.org/series/20944/
State : success

== Summary ==

Series 20944v1 Enchancement to intel_dp_aux_backlight driver
https://patchwork.freedesktop.org/api/1.0/series/20944/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                incomplete -> PASS       (fi-skl-6770hq)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 465s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 612s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 511s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 604s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 512s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 516s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 440s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 439s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 504s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 491s
fi-kbl-7500u     total:278  pass:259  dwarn:1   dfail:0   fail:0   skip:18  time: 473s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 507s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 598s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 501s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 551s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 555s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 422s

79e440ca583b2678e540d616d87cc6f250368be9 drm-tip: 2017y-03m-08d-20h-49m-20s UTC integration manifest
c534b17 drm/i915: Store brightness level in aux backlight driver
13e7657 drm/i915: Use highest frequency divider for PWM
5cbac5c drm/i915: Support dynamic backlight via DPCD register
083f3b3 drm/i915: Correctly enable blacklight adjustment via DPCD
5dfe83a drm/i915: Fix condition check for backlight control via DPCD

== Logs ==

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

Re: [PATCH 1/5] drm/i915: Fix condition check for backlight control via DPCD

[Jani Nikula]

On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> Currently the intel_dp_aux_backlight driver requires eDP panel
> to support these conditions to allow the backlight adjust via
> dpcd register.
> 1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> 2) DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
> 3) not DP_EDP_BACKLIGHT_PIN_ENABLE_CAP
> 4) not DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP
>
> Condition 2,3,4 are incorect because of the following reasons.
> For 2), "backlight can be enabled via AUX" is not "brightness
> can be adjusted via AUX". For 3) and 4), this requires panel
> to not support backlight adjustment via eDP BL_PWM_DIM pin,
> but there are some panels that support both AUX and eDP pin.
>
> This patch fixes the condition by checking for these instead.
> 1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
> 2) DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP

I understand your point, but IIRC the goal with the current code was to
support DPCD backlight control *only* when PWM pin backlight control was
not possible. Your patch turns it around, and enables DPCD backlight
control whenever it's possible.

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

It's also true that the current code conflates backlight enable and
adjustment caps. But that's largely because we also currently do not
support any type of mixed mode. And that would require quite a bit more
code. With your patch, you could end up with DPCD backlight adjustment
with no backligt enable/disable capability.

So I acknowledge the limitations in the current code, but I'm also not
convinced this patch series would be sufficient.


BR,
Jani.

>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 6532e226db29..abfb09c1269f 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",
> @@ -142,10 +146,8 @@ 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[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))) {
> +	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;
>  	}

-- 
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 2/5] drm/i915: Correctly enable blacklight adjustment via DPCD

[Jani Nikula]

On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> intel_dp_aux_enable_backlight() assumed that the register
> BACKLIGHT_BRIGHTNESS_CONTROL_MODE can only has value 01
> (DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET) when initialize.
>
> This patch fixed that by handling all cases of that register.
>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 29 +++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index abfb09c1269f..16cd1265d264 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -101,15 +101,32 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
>  	uint8_t dpcd_buf = 0;
> +	uint8_t edp_backlight_mode = 0;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> -	if ((drm_dp_dpcd_readb(&intel_dp->aux,
> -			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf) == 1) &&
> -	    ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) ==
> -	     DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET))
> -		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER,
> -				   (dpcd_buf | DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD));

That's clearly bonkers, it ends up selecting
DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT by accident, not
DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD, because it doesn't mask the bits.

> +	if (!drm_dp_dpcd_readb(&intel_dp->aux,
> +			       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, &dpcd_buf)) {
> +		return;
> +	}
> +
> +	edp_backlight_mode = dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> +
> +	switch (edp_backlight_mode) {
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_PWM:
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET:
> +		dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> +		dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
> +		break;
> +
> +	/* Do nothing in these cases */
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD:
> +	case DP_EDP_BACKLIGHT_CONTROL_MODE_PRODUCT:

Based on the current requirements for using DPCD backlight control, the
only power-on default we should end up here with is
DP_EDP_BACKLIGHT_CONTROL_MODE_PRESET.

In particular, I don't think we should accept the PRODUCT mode without
ensuring we output PWM to the eDP pins as well. And currently DPCD and
PWM controls are mutually exclusive.

BR,
Jani.


> +	default:
> +		break;
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

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

Re: [PATCH 3/5] drm/i915: Support dynamic backlight via DPCD register

[Jani Nikula]

On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> 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.
>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 31 +++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 16cd1265d264..643b604be2de 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -97,10 +97,24 @@ intel_dp_aux_set_backlight(struct intel_connector *connector, u32 level)
>  	}
>  }
>  
> +/*
> + * Set minimum / maximum dynamic brightness percentage. This value is expressed
> + * as the percentage of normal brightness in 5% increments.
> + */
> +static void intel_dp_aux_set_dynamic_backlight_percent(
> +		struct intel_dp *intel_dp, u32 min, u32 max)

static void
intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,
					   u32 min, u32 max)

would be preferable indentation.

> +{

	u8 dbc[] = { DIV_ROUND_CLOSEST(min, 5), DIV_ROUND_CLOSEST(max, 5)};

	drm_dp_dpcd_write(&intel_dp->aux, DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,
			  dbc, sizeof(dbc));

> +	drm_dp_dpcd_writeb(&intel_dp->aux,
> +		DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET, min / 5);
> +	drm_dp_dpcd_writeb(&intel_dp->aux,
> +		DP_EDP_DBC_MAXIMUM_BRIGHTNESS_SET, max / 5);
> +}
> +
>  static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  {
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base);
>  	uint8_t dpcd_buf = 0;
> +	uint8_t new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
>  
>  	set_aux_backlight_enable(intel_dp, true);
> @@ -110,15 +124,14 @@ 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:
> -		dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> -		dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
> -		drm_dp_dpcd_writeb(&intel_dp->aux,
> -			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, dpcd_buf);
> +		new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
> +		new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
>  		break;
>  
>  	/* Do nothing in these cases */
> @@ -127,6 +140,16 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  	default:
>  		break;
>  	}
> +
> +	if (intel_dp->edp_dpcd[2] & DP_EDP_DYNAMIC_BACKLIGHT_CAP) {
> +		new_dpcd_buf |= DP_EDP_DYNAMIC_BACKLIGHT_ENABLE;
> +		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
> +	}
> +
> +	if (new_dpcd_buf != dpcd_buf) {
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

-- 
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 4/5] drm/i915: Use highest frequency divider for PWM

[Jani Nikula]

On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
> TCON tend to have better brightness scaling with lower
> PWM frequency. This patch set the divider to highest
> value to lower the PWM frequency.

Just setting 0xff to DP_EDP_BACKLIGHT_FREQ_SET is way too arbitrary and
panel dependent. Going to a too low frequency may lead to flicker, and
you have no idea how low you're going because you ignore
DP_EDP_PWMGEN_BIT_COUNT completely. I think it's possible to land in
audible frequencies too, depending on the TCON hardware. (*)

I think the way to go is to check the VBT for OEM specified backlight
frequency, tuned for the specific hardware, and do the math to set the
registers right. This is what we use (well, fall back to) for the PWM
pin frequency setting in intel_panel.c.

BR,
Jani.


(*) Admittedly there's a certain charm in the idea of getting a bug
report, "I can hear my backlight". ;)


>
> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index 643b604be2de..32b426006a6a 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -116,6 +116,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  	uint8_t dpcd_buf = 0;
>  	uint8_t new_dpcd_buf = 0;
>  	uint8_t edp_backlight_mode = 0;
> +	bool freq_cap = false;
>  
>  	set_aux_backlight_enable(intel_dp, true);
>  
> @@ -146,10 +147,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>  		intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
>  	}
>  
> +	/* Use highest frequency divider if supported */
> +	freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
> +	if (freq_cap)
> +		new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
> +
>  	if (new_dpcd_buf != dpcd_buf) {
>  		drm_dp_dpcd_writeb(&intel_dp->aux,
>  			DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
>  	}
> +
> +	if (freq_cap) {
> +		drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
> +			0xff);
> +	}
>  }
>  
>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)

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

Re: [PATCH 1/5] drm/i915: Fix condition check for backlight control via DPCD

[Puthikorn Voravootivat]

I think that there won't be a good way to determine which way of
adjusting backlight
is preferred when the panel support both PWM pin and DPCD.

How about extending the current i915_params.enable_dpcd_backlight to this.
{ 0:disable(default) 1: prefer PWM pin, 2: prefer DPCD }

If it is 1), use the existing condition and if it is 2) use my new condition.

This would solve the limitation of the current code and avoid the
mixed mode complexity
since user need to purposely set the param to 2 to prefer DPCD.

Thanks

On Thu, Mar 9, 2017 at 1:50 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
>> Currently the intel_dp_aux_backlight driver requires eDP panel
>> to support these conditions to allow the backlight adjust via
>> dpcd register.
>> 1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
>> 2) DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
>> 3) not DP_EDP_BACKLIGHT_PIN_ENABLE_CAP
>> 4) not DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP
>>
>> Condition 2,3,4 are incorect because of the following reasons.
>> For 2), "backlight can be enabled via AUX" is not "brightness
>> can be adjusted via AUX". For 3) and 4), this requires panel
>> to not support backlight adjustment via eDP BL_PWM_DIM pin,
>> but there are some panels that support both AUX and eDP pin.
>>
>> This patch fixes the condition by checking for these instead.
>> 1) DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP
>> 2) DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP
>
> I understand your point, but IIRC the goal with the current code was to
> support DPCD backlight control *only* when PWM pin backlight control was
> not possible. Your patch turns it around, and enables DPCD backlight
> control whenever it's possible.
>
>> This patch also add a check to DP_EDP_BACKLIGHT_AUX_ENABLE_CAP
>> in set_aux_backlight_enable() since the backlight enablement
>> can be done via BL_ENABLE eDP connector pin in the case that
>> it does not support doing that via AUX.
>
> It's also true that the current code conflates backlight enable and
> adjustment caps. But that's largely because we also currently do not
> support any type of mixed mode. And that would require quite a bit more
> code. With your patch, you could end up with DPCD backlight adjustment
> with no backligt enable/disable capability.
>
> So I acknowledge the limitations in the current code, but I'm also not
> convinced this patch series would be sufficient.
>
>
> BR,
> Jani.
>
>>
>> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>> ---
>>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> index 6532e226db29..abfb09c1269f 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",
>> @@ -142,10 +146,8 @@ 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[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))) {
>> +     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;
>>       }
>
> --
> 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 4/5] drm/i915: Use highest frequency divider for PWM

[Puthikorn Voravootivat]

Agree that your suggestion is better. I will drop this patch in the
next version of the set.

Thanks

On Thu, Mar 9, 2017 at 2:40 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Wed, 08 Mar 2017, Puthikorn Voravootivat <puthik@chromium.org> wrote:
>> TCON tend to have better brightness scaling with lower
>> PWM frequency. This patch set the divider to highest
>> value to lower the PWM frequency.
>
> Just setting 0xff to DP_EDP_BACKLIGHT_FREQ_SET is way too arbitrary and
> panel dependent. Going to a too low frequency may lead to flicker, and
> you have no idea how low you're going because you ignore
> DP_EDP_PWMGEN_BIT_COUNT completely. I think it's possible to land in
> audible frequencies too, depending on the TCON hardware. (*)
>
> I think the way to go is to check the VBT for OEM specified backlight
> frequency, tuned for the specific hardware, and do the math to set the
> registers right. This is what we use (well, fall back to) for the PWM
> pin frequency setting in intel_panel.c.
>
> BR,
> Jani.
>
>
> (*) Admittedly there's a certain charm in the idea of getting a bug
> report, "I can hear my backlight". ;)
>
>
>>
>> Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org>
>> ---
>>  drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> index 643b604be2de..32b426006a6a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
>> @@ -116,6 +116,7 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>>       uint8_t dpcd_buf = 0;
>>       uint8_t new_dpcd_buf = 0;
>>       uint8_t edp_backlight_mode = 0;
>> +     bool freq_cap = false;
>>
>>       set_aux_backlight_enable(intel_dp, true);
>>
>> @@ -146,10 +147,20 @@ static void intel_dp_aux_enable_backlight(struct intel_connector *connector)
>>               intel_dp_aux_set_dynamic_backlight_percent(intel_dp, 0, 100);
>>       }
>>
>> +     /* Use highest frequency divider if supported */
>> +     freq_cap = intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP;
>> +     if (freq_cap)
>> +             new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
>> +
>>       if (new_dpcd_buf != dpcd_buf) {
>>               drm_dp_dpcd_writeb(&intel_dp->aux,
>>                       DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
>>       }
>> +
>> +     if (freq_cap) {
>> +             drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_BACKLIGHT_FREQ_SET,
>> +                     0xff);
>> +     }
>>  }
>>
>>  static void intel_dp_aux_disable_backlight(struct intel_connector *connector)
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx