Sorry for the wait. This is not a complete review, just some quick comments for now.
On Tue, 2017-04-18 at 16:48 -0700, Puthikorn Voravootivat wrote: > Currently the intel_dp_aux_backlight driver requires eDP panel > to not also support backlight adjustment via PWM pin to use > this driver. > > This force the eDP panel that support both ways of backlight > adjustment to do it via PWM pin. > > This patch adds the new prefer DPCD mode in the i915_param > to make it enable to prefer DPCD over the PWM via kernel param. > > 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 <[email protected]> > --- > drivers/gpu/drm/i915/i915_params.c | 6 ++--- > drivers/gpu/drm/i915/i915_params.h | 2 +- > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 33 > +++++++++++++++++++-------- > 3 files changed, 27 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_params.c > b/drivers/gpu/drm/i915/i915_params.c > index b6a7e363d076..960393dd5edf 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 = 0, > .enable_gvt = false, > }; > > @@ -246,9 +246,9 @@ 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 (0:disable (default), > 1:prefer PWM pin, 2: prefer DPCD)"); I looked at other int parameters, -1=default, 0=disable(prefer PWM in your case), 1=enable(prefer DPCD) seems like the more common way to do this. > > 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..bf6e2c60f697 100644 > --- a/drivers/gpu/drm/i915/i915_params.h > +++ b/drivers/gpu/drm/i915/i915_params.h > @@ -51,6 +51,7 @@ > func(int, use_mmio_flip); \ > func(int, mmio_debug); \ > func(int, edp_vswing); \ > + func(int, enable_dpcd_backlight); \ > func(unsigned int, inject_load_failure); \ > /* leave bools at the end to not create holes */ \ > func(bool, alpha_support); \ > @@ -66,7 +67,6 @@ > func(bool, verbose_state_checks); \ > func(bool, nuclear_pageflip); \ > func(bool, enable_dp_mst); \ > - func(bool, 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 6532e226db29..42f73d9a3ccf 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, > ®_val) < 0) { > DRM_DEBUG_KMS("Failed to read DPCD register 0x%x\n", > @@ -138,27 +142,36 @@ static bool > intel_dp_aux_display_control_capable(struct intel_connector *connector) > { > struct intel_dp *intel_dp = enc_to_intel_dp(&connector->encoder->base); > + bool supported; > > /* 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))) { > - DRM_DEBUG_KMS("AUX Backlight Control Supported!\n"); > - return true; > + switch (i915.enable_dpcd_backlight) { > + case 1: /* prefer PWM pin */ > + supported = (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); > + break; > + case 2: /* prefer DPCD */ > + supported = (intel_dp->edp_dpcd[1] & > DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP) && > + (intel_dp->edp_dpcd[2] & > DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP); > + break; > + default: > + supported = false; > } > - return false; > + > + if (supported) > + DRM_DEBUG_KMS("AUX Backlight Control Supported!\n"); > + > + return supported; If i915.enable_dpcd_backlight = 0, aren't you returning an uninitialized variable here? -DK > } > > int intel_dp_aux_init_backlight_funcs(struct intel_connector > *intel_connector) > { > struct intel_panel *panel = &intel_connector->panel; > > - if (!i915.enable_dpcd_backlight) > - return -ENODEV; > - > if (!intel_dp_aux_display_control_capable(intel_connector)) > return -ENODEV; > _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
