| Message ID | 20170527014209.186331-2-puthik@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote: > Add heuristic to decide that AUX or PWM pin should use for > backlight brightness adjustment and modify i915 param description > to have auto, force disable, and force enable. > > The heuristic to determine that using AUX pin is better than using > PWM pin is that the panel support any of the feature list here. > - Regional backlight brightness adjustment > - Backlight PWM frequency set > - More than 8 bits resolution of brightness level > - Backlight enablement via AUX and not by BL_ENABLE pin > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > --- > drivers/gpu/drm/i915/i915_params.c | 7 +-- > drivers/gpu/drm/i915/i915_params.h | 2 +- > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++-- > 3 files changed, 66 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index b6a7e363d076..3758ae1f11b4 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,10 @@ 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_unsafe(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:auto (default), 0:force disable, 1:force enabled if 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..c89aae804659 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() > + */ Unrelated hunk, please remove. This should have been included in one of the previous patches. > 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,15 +171,66 @@ 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; > } > > +/* > + * Heuristic function whether we should use AUX for backlight adjustment or not. > + * > + * We should use AUX for backlight brightness adjustment if panel doesn't this > + * via PWM pin or using AUX is better than using PWM pin. > + * > + * The heuristic to determine that using AUX pin is better than using PWM pin is > + * that the panel support any of the feature list here. > + * - Regional backlight brightness adjustment > + * - Backlight PWM frequency set > + * - More than 8 bits resolution of brightness level > + * - Backlight enablement via AUX and not by BL_ENABLE pin > + * > + * If all above are not true, assume that using PWM pin is better. > + */ > +static bool > +intel_dp_aux_display_control_heuristic(struct intel_connector *connector) > +{ > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > + uint8_t reg_val; > + > + /* Panel doesn't support adjusting backlight brightness via PWN pin */ > + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ? > + return true; > + > + /* Panel supports regional backlight brightness adjustment */ > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3, Spec says this register is new to eDP 1.4. I don't see a check for version. > + ®_val) != 1) { > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n", > + DP_EDP_GENERAL_CAP_3); > + return false; > + } > + if (reg_val > 0) > + return true; > + > + /* Panel supports backlight PWM frequency set */ > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) > + return true; > + > + /* Panel supports more than 8 bits resolution of brightness level */ > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > + return true; I don't know if more than 8 bits is relatively better than the resolution PWM pin allows. I guess, we could fix that later by comparing the number of brightness levels. > + > + /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */ > + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && > + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)) > + return true; > + > + return false; > + > +} > + > int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) > { > struct intel_panel *panel = &intel_connector->panel; > @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) > if (!intel_dp_aux_display_control_capable(intel_connector)) > return -ENODEV; > > + if (i915.enable_dpcd_backlight == -1 && > + !intel_dp_aux_display_control_heuristic(intel_connector)) > + return -ENODEV; > + > panel->backlight.setup = intel_dp_aux_setup_backlight; > panel->backlight.enable = intel_dp_aux_enable_backlight; > panel->backlight.disable = intel_dp_aux_disable_backlight;
On Fri, 2017-06-02 at 17:42 +0000, Pandiyan, Dhinakaran wrote: Somehow the CC's got removed in my previous reply, adding them back. See one additional comment below. > On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote: > > Add heuristic to decide that AUX or PWM pin should use for > > backlight brightness adjustment and modify i915 param description > > to have auto, force disable, and force enable. > > > > The heuristic to determine that using AUX pin is better than using > > PWM pin is that the panel support any of the feature list here. > > - Regional backlight brightness adjustment > > - Backlight PWM frequency set > > - More than 8 bits resolution of brightness level > > - Backlight enablement via AUX and not by BL_ENABLE pin > > > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > > --- > > drivers/gpu/drm/i915/i915_params.c | 7 +-- > > drivers/gpu/drm/i915/i915_params.h | 2 +- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++-- > > 3 files changed, 66 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > > index b6a7e363d076..3758ae1f11b4 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,10 @@ 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_unsafe(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:auto (default), 0:force disable, 1:force enabled if 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); \ Please move this above the bools, see comment in code that's a few lines above. -DK > > 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..c89aae804659 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() > > + */ > > Unrelated hunk, please remove. This should have been included in one of > the previous patches. > > > 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,15 +171,66 @@ 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; > > } > > > > +/* > > + * Heuristic function whether we should use AUX for backlight adjustment or not. > > + * > > + * We should use AUX for backlight brightness adjustment if panel doesn't this > > + * via PWM pin or using AUX is better than using PWM pin. > > + * > > + * The heuristic to determine that using AUX pin is better than using PWM pin is > > + * that the panel support any of the feature list here. > > + * - Regional backlight brightness adjustment > > + * - Backlight PWM frequency set > > + * - More than 8 bits resolution of brightness level > > + * - Backlight enablement via AUX and not by BL_ENABLE pin > > + * > > + * If all above are not true, assume that using PWM pin is better. > > + */ > > +static bool > > +intel_dp_aux_display_control_heuristic(struct intel_connector *connector) > > +{ > > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > > + uint8_t reg_val; > > + > > + /* Panel doesn't support adjusting backlight brightness via PWN pin */ > > + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) > > Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ? > > > > + return true; > > + > > + /* Panel supports regional backlight brightness adjustment */ > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3, > > Spec says this register is new to eDP 1.4. I don't see a check for > version. > > > > + ®_val) != 1) { > > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n", > > + DP_EDP_GENERAL_CAP_3); > > + return false; > > + } > > + if (reg_val > 0) > > + return true; > > + > > + /* Panel supports backlight PWM frequency set */ > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) > > + return true; > > + > > + /* Panel supports more than 8 bits resolution of brightness level */ > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) > > + return true; > > I don't know if more than 8 bits is relatively better than the > resolution PWM pin allows. I guess, we could fix that later by comparing > the number of brightness levels. > > > + > > + /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */ > > + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && > > + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)) > > + return true; > > + > > + return false; > > + > > +} > > + > > int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) > > { > > struct intel_panel *panel = &intel_connector->panel; > > @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) > > if (!intel_dp_aux_display_control_capable(intel_connector)) > > return -ENODEV; > > > > + if (i915.enable_dpcd_backlight == -1 && > > + !intel_dp_aux_display_control_heuristic(intel_connector)) > > + return -ENODEV; > > + > > panel->backlight.setup = intel_dp_aux_setup_backlight; > > panel->backlight.enable = intel_dp_aux_enable_backlight; > > panel->backlight.disable = intel_dp_aux_disable_backlight; > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Jun 2, 2017 at 10:56 AM, Pandiyan, Dhinakaran < dhinakaran.pandiyan@intel.com> wrote: > On Fri, 2017-06-02 at 17:42 +0000, Pandiyan, Dhinakaran wrote: > > Somehow the CC's got removed in my previous reply, adding them back. See > one additional comment below. > > > > On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote: > > > Add heuristic to decide that AUX or PWM pin should use for > > > backlight brightness adjustment and modify i915 param description > > > to have auto, force disable, and force enable. > > > > > > The heuristic to determine that using AUX pin is better than using > > > PWM pin is that the panel support any of the feature list here. > > > - Regional backlight brightness adjustment > > > - Backlight PWM frequency set > > > - More than 8 bits resolution of brightness level > > > - Backlight enablement via AUX and not by BL_ENABLE pin > > > > > > Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> > > > --- > > > drivers/gpu/drm/i915/i915_params.c | 7 +-- > > > drivers/gpu/drm/i915/i915_params.h | 2 +- > > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 > +++++++++++++++++++++++++-- > > > 3 files changed, 66 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > > > index b6a7e363d076..3758ae1f11b4 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,10 @@ 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_unsafe(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:auto (default), 0:force disable, 1:force enabled if > 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); \ > > > Please move this above the bools, see comment in code that's a few lines > above. > > Will do > -DK > > > > 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..c89aae804659 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() > > > + */ > > > > Unrelated hunk, please remove. This should have been included in one of > > the previous patches. > Removed > > > > > 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,15 +171,66 @@ 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; > > > } > > > > > > +/* > > > + * Heuristic function whether we should use AUX for backlight > adjustment or not. > > > + * > > > + * We should use AUX for backlight brightness adjustment if panel > doesn't this > > > + * via PWM pin or using AUX is better than using PWM pin. > > > + * > > > + * The heuristic to determine that using AUX pin is better than using > PWM pin is > > > + * that the panel support any of the feature list here. > > > + * - Regional backlight brightness adjustment > > > + * - Backlight PWM frequency set > > > + * - More than 8 bits resolution of brightness level > > > + * - Backlight enablement via AUX and not by BL_ENABLE pin > > > + * > > > + * If all above are not true, assume that using PWM pin is better. > > > + */ > > > +static bool > > > +intel_dp_aux_display_control_heuristic(struct intel_connector > *connector) > > > +{ > > > + struct intel_dp *intel_dp = enc_to_intel_dp(&connector-> > encoder->base); > > > + uint8_t reg_val; > > > + > > > + /* Panel doesn't support adjusting backlight brightness via PWN > pin */ > > > + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_ > PWM_PIN_CAP)) > > > > Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ? > > > Thank you for catching this. > > > > > + return true; > > > + > > > + /* Panel supports regional backlight brightness adjustment */ > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3, > > > > Spec says this register is new to eDP 1.4. I don't see a check for > > version. > > > There is no need to check this. In not supported eDP panel, this register will read all 0s. > > > > > + ®_val) != 1) { > > > + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n", > > > + DP_EDP_GENERAL_CAP_3); > > > + return false; > > > + } > > > + if (reg_val > 0) > > > + return true; > > > + > > > + /* Panel supports backlight PWM frequency set */ > > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) > > > + return true; > > > + > > > + /* Panel supports more than 8 bits resolution of brightness level > */ > > > + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_ > BYTE_COUNT) > > > + return true; > > > > I don't know if more than 8 bits is relatively better than the > > resolution PWM pin allows. I guess, we could fix that later by comparing > > the number of brightness levels. > > > > > + > > > + /* Panel supports enabling backlight via AUX but not by BL_ENABLE > pin */ > > > + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && > > > + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)) > > > + return true; > > > + > > > + return false; > > > + > > > +} > > > + > > > int intel_dp_aux_init_backlight_funcs(struct intel_connector > *intel_connector) > > > { > > > struct intel_panel *panel = &intel_connector->panel; > > > @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct > intel_connector *intel_connector) > > > if (!intel_dp_aux_display_control_capable(intel_connector)) > > > return -ENODEV; > > > > > > + if (i915.enable_dpcd_backlight == -1 && > > > + !intel_dp_aux_display_control_heuristic(intel_connector)) > > > + return -ENODEV; > > > + > > > panel->backlight.setup = intel_dp_aux_setup_backlight; > > > panel->backlight.enable = intel_dp_aux_enable_backlight; > > > panel->backlight.disable = intel_dp_aux_disable_backlight; > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b6a7e363d076..3758ae1f11b4 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,10 @@ 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_unsafe(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:auto (default), 0:force disable, 1:force enabled if 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..c89aae804659 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,15 +171,66 @@ 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; } +/* + * Heuristic function whether we should use AUX for backlight adjustment or not. + * + * We should use AUX for backlight brightness adjustment if panel doesn't this + * via PWM pin or using AUX is better than using PWM pin. + * + * The heuristic to determine that using AUX pin is better than using PWM pin is + * that the panel support any of the feature list here. + * - Regional backlight brightness adjustment + * - Backlight PWM frequency set + * - More than 8 bits resolution of brightness level + * - Backlight enablement via AUX and not by BL_ENABLE pin + * + * If all above are not true, assume that using PWM pin is better. + */ +static bool +intel_dp_aux_display_control_heuristic(struct intel_connector *connector) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); + uint8_t reg_val; + + /* Panel doesn't support adjusting backlight brightness via PWN pin */ + if (!(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) + return true; + + /* Panel supports regional backlight brightness adjustment */ + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3, + ®_val) != 1) { + DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n", + DP_EDP_GENERAL_CAP_3); + return false; + } + if (reg_val > 0) + return true; + + /* Panel supports backlight PWM frequency set */ + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) + return true; + + /* Panel supports more than 8 bits resolution of brightness level */ + if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_BYTE_COUNT) + return true; + + /* Panel supports enabling backlight via AUX but not by BL_ENABLE pin */ + if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) && + !(intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_PIN_ENABLE_CAP)) + return true; + + return false; + +} + int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) { struct intel_panel *panel = &intel_connector->panel; @@ -187,6 +241,10 @@ int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) if (!intel_dp_aux_display_control_capable(intel_connector)) return -ENODEV; + if (i915.enable_dpcd_backlight == -1 && + !intel_dp_aux_display_control_heuristic(intel_connector)) + return -ENODEV; + panel->backlight.setup = intel_dp_aux_setup_backlight; panel->backlight.enable = intel_dp_aux_enable_backlight; panel->backlight.disable = intel_dp_aux_disable_backlight;
Add heuristic to decide that AUX or PWM pin should use for backlight brightness adjustment and modify i915 param description to have auto, force disable, and force enable. The heuristic to determine that using AUX pin is better than using PWM pin is that the panel support any of the feature list here. - Regional backlight brightness adjustment - Backlight PWM frequency set - More than 8 bits resolution of brightness level - Backlight enablement via AUX and not by BL_ENABLE pin Signed-off-by: Puthikorn Voravootivat <puthik@chromium.org> --- drivers/gpu/drm/i915/i915_params.c | 7 +-- drivers/gpu/drm/i915/i915_params.h | 2 +- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 7 deletions(-)