On Wed, May 3, 2017 at 2:11 AM, Jani Nikula <jani.nik...@linux.intel.com> wrote:
> On Tue, 18 Apr 2017, Puthikorn Voravootivat <put...@chromium.org> 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. > > Currently this is by design. But I think we agreed previously that the > driver also has incorrect capability checks for the current design. The > first priority would be to fix those checks. And the patch order in the > series should reflect that. > > > 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. > > "also" in the commit message is a strong clue it should be a separate > patch. What you describe is potentially a fix that should precede this > patch. > I will split this into 3 patches. 1. Fix cap check 2. Drop AUX backlight enable requirement 3. Allow choosing to use PWM pin or AUX if both supported > > > > Signed-off-by: Puthikorn Voravootivat <put...@chromium.org> > > --- > > 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)"); > > See my comments below. I think you need to rethink the options. > > > > > 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; > > This is not right. If we're going to use the PWM pin for backlight > control, it's what's in intel_panel.c, and > intel_dp_aux_init_backlight_funcs() should return -ENODEV. We only > support one or the other. > Fixed in next patch set. > > I think you probably need to have a hard look at Table 10-1 "Summary of > Backlight Control Modes Using DPCD Registers" in eDP 1.4b, and tell us > what modes you really want to support and how. For example, the product > mode or any DPCD/PWM mixed modes aren't very easily achieved with the > current design. > What we actually need is that we have panel that - does not support display backlight enable via AUX - support display backlight adjustment via AUX - support display backlight enable via eDP BL_ENABLE pin - support display backlight adjustment via eDP PWM pin but that pin is not connected - support display backlight frequency adjustment via AUX and we want to - enable backlight via eDP BL_ENABLE pin - adjust backlight brightness via AUX - adjust backlight frequency via AUX > > BR, > Jani. > > > > + 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; > > } > > > > 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; > > -- > Jani Nikula, Intel Open Source Technology Center >
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx