________________________________________ From: Puthikorn Voravootivat [puthik@google.com] on behalf of Puthikorn Voravootivat [puthik@chromium.org] Sent: Tuesday, May 16, 2017 5:33 PM To: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran Cc: dri-devel@lists.freedesktop.org; Jani Nikula; Navare, Manasi D; Stephane Marchesin; Puthikorn Voravootivat Subject: [PATCH v8 1/5] drm/i915: Drop AUX backlight enable check for backlight control

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 | 1 - 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index b87c5a381d6a..d32c06583e0b 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -165,7 +165,6 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector) * the panel can support backlight control over the aux channel */ if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP && - (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) && !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) || (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) { -- 2.13.0.303.g4ebf302169-goog

^ still has the problem I mentioned last time. How about this?

diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index b87c5a3..7072bcf 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 Wed, 2017-05-17 at 14:04 -0700, Puthikorn Voravootivat wrote:

On Wed, May 17, 2017 at 1:09 PM, Pandiyan, Dhinakaran dhinakaran.pandiyan@intel.com wrote:

    ________________________________________
    From: Puthikorn Voravootivat [puthik@google.com] on behalf of
    Puthikorn Voravootivat [puthik@chromium.org]
    Sent: Tuesday, May 16, 2017 5:33 PM
    To: intel-gfx@lists.freedesktop.org; Pandiyan, Dhinakaran
    Cc: dri-devel@lists.freedesktop.org; Jani Nikula; Navare,
    Manasi D; Stephane Marchesin; Puthikorn Voravootivat
    Subject: [PATCH v8 1/5] drm/i915: Drop AUX backlight enable
    check for backlight control


    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 | 1 -
    1 file changed, 1 deletion(-)

    diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
    b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
    index b87c5a381d6a..d32c06583e0b 100644
    --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
    +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
    @@ -165,7 +165,6 @@
    intel_dp_aux_display_control_capable(struct intel_connector
    *connector)
    * the panel can support backlight control over the aux channel
    */
    if (intel_dp->edp_dpcd[1] &
    DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP &&
    - (intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
    (intel_dp->edp_dpcd[2] &
    DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) &&
    !((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP) ||
    (intel_dp->edp_dpcd[2] &
    DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))) {
    --
    2.13.0.303.g4ebf302169-goog



    ^ still has the problem I mentioned last time. How about
    this? 


    diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
    b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
    index b87c5a3..7072bcf 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;
            }

This works too. I probably misunderstood your comment as I move the code to next patch in the set.

Please feel free to question any of the comments if you do not agree.

-DK

---

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

Hi Dhinakaran,

Quick question

So what is the update about adding new option in i915_params? Is this patch good to go after fixing the 2 points you mentioned?

Thanks

On Wed, May 17, 2017 at 1:33 PM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote:

On Tue, 2017-05-16 at 17:34 -0700, Puthikorn Voravootivat wrote:

...

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 | 40

+++++++++++++++++++++++----

...

3 files changed, 41 insertions(+), 7 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..c0eeb8fc2013 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -100,10 +100,27 @@ intel_dp_aux_set_backlight(struct intel_connector

*connector, u32 level)

...

  }

}

+/*

• • Set minimum / maximum dynamic brightness percentage. This value is

expressed

...

• • as the percentage of normal brightness in 5% increments.
• */

+static void +intel_dp_aux_set_dynamic_backlight_percent(struct intel_dp *intel_dp,

•                                    u32 min, u32 max)
  

+{

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

};

...

• if (drm_dp_dpcd_write(&intel_dp->aux,
  

DP_EDP_DBC_MINIMUM_BRIGHTNESS_SET,

...

•                   dbc, sizeof(dbc) < 0)) {
  

Incorrect parentheses placement and return value check.

...

•         DRM_DEBUG_KMS("Failed to write aux DBC brightness
  

level\n");

...

• }
  

+}

static void intel_dp_aux_enable_backlight(struct intel_connector

*connector)

...

{ struct intel_dp *intel_dp = enc_to_intel_dp(&connector->

encoder->base);

...

  uint8_t dpcd_buf = 0;

• uint8_t new_dpcd_buf = 0;
  

nit: unnecessary initialization.

...

  uint8_t edp_backlight_mode = 0;

  if (drm_dp_dpcd_readb(&intel_dp->aux,

@@ -113,18 +130,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 +147,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);

...

}

On Tue, 2017-05-16 at 17:34 -0700, Puthikorn Voravootivat 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.

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 c0eeb8fc2013..a01cbf3db1c2 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -116,12 +116,87 @@ 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 = 0; uint8_t new_dpcd_buf = 0; uint8_t edp_backlight_mode = 0;

• bool freq_cap;

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

@@ -154,6 +229,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) {

@@ -161,6 +240,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);

}

I guess, we could find a way to do away with the +-25% allowance in computing the freq. divider, reduce DPCD reads and avoid repeating freq. divider computations. But, I think this is good for now.