On Tue, Jul 18, 2017 at 11:56:23AM -0700, Puthikorn Voravootivat wrote:
> May be the older panel might not work well with this feature.
> David/Jani, what do you think about adding check that the panel is eDP
> 1.4 or later in the heuristic?
While this patch might seem correct on the surface (with the patch
it now correctly prevents non-eDP 1.4 registers from being read on
sinks that aren't eDP 1.4 compliant), it still isn't correct
as per my understanding of the eDP 1.4 spec.
Chapter 10.1 of the eDP 1.4 spec clearly states that backlight through
AUX is an eDP v1.4 (and higher) feature.
The first conditional in this function will return true
if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
If so, the backlight will attempt to use AUX backlight control
even on eDP sinks with a lower revision than v1.4.
So, per my reading of the eDP v1.4 spec, the conditional should be
the very first thing in this function (my eDP v1.3 device works
fine if the eDP version checks is at the beginning of the function,
but fails if it's after the PWM pin cap check).
Kind regards, David
> diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> index b25cd88fc1c5..e63f2296fd34 100644
> --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> @@ -292,7 +292,7 @@ intel_dp_aux_display_control_capable(struct
> intel_connector *connector)
> * 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.
> + * that the panel is eDP 1.4 or later and support any of the feature list here.
> * - Regional backlight brightness adjustment
> * - Backlight PWM frequency set
> * - More than 8 bits resolution of brightness level
> @@ -310,6 +310,10 @@ intel_dp_aux_display_control_heuristic(struct
> intel_connector *connector)
> if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP))
> return true;
>
> + /* Enable this for eDP 1.4 panel or later. */
> + if (intel_dp->edp_dpcd[0] < DP_EDP_14)
> + return false;
> +
> /* Panel supports regional backlight brightness adjustment */
> if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3,
> ®_val) != 1) {
>
> On Mon, Jul 10, 2017 at 7:23 AM, David Weinehall
> <david.weinehall@xxxxxxxxxxxxxxx> wrote:
> > This reverts commit 560a758d39c616f83ac25ff6e0816a49ebe6401c.
> >
> > This patch has been identified to introduce a backlight regression
> > on at least two different platforms; either the heuristic is broken
> > (if so the patch needs to be reworked) or the in-kernel DPCD backlight
> > support is broken (if so it's premature to enable DPCD backlight
> > even if the platforms support it).
> >
> > Signed-off-by: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_params.c | 7 ++-
> > drivers/gpu/drm/i915/i915_params.h | 1 -
> > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 61 ++-------------------------
> > 3 files changed, 6 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index 88b9d3e6713a..bec5ade371b0 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 = -1,
> > + .enable_dpcd_backlight = false,
> > .enable_gvt = false,
> > .enable_dbc = true,
> > };
> > @@ -247,10 +247,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_unsafe(enable_dpcd_backlight, i915.enable_dpcd_backlight, int, 0600);
> > +module_param_named(enable_dpcd_backlight, i915.enable_dpcd_backlight, bool, 0600);
> > MODULE_PARM_DESC(enable_dpcd_backlight,
> > - "Enable support for DPCD backlight control "
> > - "(-1:auto (default), 0:force disable, 1:force enabled if supported");
> > + "Enable support for DPCD backlight control (default:false)");
> >
> > 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 057e203e6bda..6a88c76664db 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -53,7 +53,6 @@
> > func(int, edp_vswing); \
> > func(int, reset); \
> > func(unsigned int, inject_load_failure); \
> > - func(int, enable_dpcd_backlight); \
> > /* leave bools at the end to not create holes */ \
> > func(bool, alpha_support); \
> > func(bool, enable_cmd_parser); \
> > diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > index b25cd88fc1c5..f32dec02e540 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
> > @@ -277,66 +277,15 @@ 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)) {
> > + 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)) {
> > 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[2] & 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;
> > @@ -347,10 +296,6 @@ 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;
> > --
> > 2.13.2
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
