| Message ID | 20170523223805.46372-3-puthik@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index b6a7e363d076..13cf3f1572ab 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -63,7 +63,7 @@ struct i915_params i915 __read_mostly = { .huc_firmware_path = NULL, .enable_dp_mst = true, .inject_load_failure = 0, - .enable_dpcd_backlight = false, + .enable_dpcd_backlight = -1, .enable_gvt = false, }; @@ -246,9 +246,11 @@ MODULE_PARM_DESC(enable_dp_mst, module_param_named_unsafe(inject_load_failure, i915.inject_load_failure, uint, 0400); MODULE_PARM_DESC(inject_load_failure, "Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)"); -module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600); +module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600); MODULE_PARM_DESC(enable_dpcd_backlight, - "Enable support for DPCD backlight control (default:false)"); + "Enable support for DPCD backlight control " + "(-1:disable (default), 0:Use PWM pin if both supported, " + "1:Use DPCD if both supported"); module_param_named(enable_gvt, i915.enable_gvt, bool, 0400); MODULE_PARM_DESC(enable_gvt, diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h index 34148cc8637c..ac02efce6e22 100644 --- a/drivers/gpu/drm/i915/i915_params.h +++ b/drivers/gpu/drm/i915/i915_params.h @@ -66,7 +66,7 @@ func(bool, verbose_state_checks); \ func(bool, nuclear_pageflip); \ func(bool, enable_dp_mst); \ - func(bool, enable_dpcd_backlight); \ + func(int, enable_dpcd_backlight); \ func(bool, enable_gvt) #define MEMBER(T, member) T member diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index a0995c00fc84..16ba1924308d 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -43,6 +43,9 @@ static void set_aux_backlight_enable(struct intel_dp *intel_dp, bool enable) else reg_val &= ~(DP_EDP_BACKLIGHT_ENABLE); + /* TODO: If the panel also support enabling backlight via BL_ENABLE pin, + * the backlight will be enabled again in _intel_edp_backlight_on() + */ if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_EDP_DISPLAY_CONTROL_REGISTER, reg_val) != 1) { DRM_DEBUG_KMS("Failed to %s aux backlight\n", @@ -168,20 +171,35 @@ intel_dp_aux_display_control_capable(struct intel_connector *connector) /* Check the eDP Display control capabilities registers to determine if * the panel can support backlight control over the aux channel */ - if (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP && - (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP) && - !(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) { + if ((intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) && + (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) { DRM_DEBUG_KMS("AUX Backlight Control Supported!\n"); return true; } return false; } +static bool +intel_dp_pwm_pin_display_control_capable(struct intel_connector *connector) +{ + struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); + + /* Check the eDP Display control capabilities registers to determine if + * the panel can support backlight control via BL_PWM_DIM eDP pin + */ + return (intel_dp->edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) && + (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP); +} + int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector) { struct intel_panel *panel = &intel_connector->panel; - if (!i915.enable_dpcd_backlight) + if (i915.enable_dpcd_backlight == -1) + return -ENODEV; + + if (i915.enable_dpcd_backlight == 0 && + intel_dp_pwm_pin_display_control_capable(intel_connector)) return -ENODEV; if (!intel_dp_aux_display_control_capable(intel_connector))
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(-)