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; }

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

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

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);

}