| Message ID | 20170418234824.157355-2-puthik@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <puthik@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)"); 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; >
On Wed, 2017-05-03 at 00:54 +0000, Pandiyan, Dhinakaran wrote: > 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 <puthik@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)"); > > 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 > Ignore that, time for coffee. > > > } > > > > 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 > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@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. > > Signed-off-by: Puthikorn Voravootivat <puthik@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. 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. 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;
On Wed, May 3, 2017 at 2:11 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Tue, 18 Apr 2017, Puthikorn Voravootivat <puthik@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 <puthik@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 >
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)"); 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; } 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;
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 <puthik@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(-)