<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Fri, Jun 2, 2017 at 10:56 AM, Pandiyan, Dhinakaran <<a href="mailto:dhinakaran.pandiyan@intel.com" target="_blank" class="cremed">dhinakaran.pandiyan@intel.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 2017-06-02 at 17:42 +0000, Pandiyan, Dhinakaran wrote: Somehow the CC's got removed in my previous reply, adding them back. See one additional comment below. <div><div class="h5"> > On Fri, 2017-05-26 at 18:42 -0700, Puthikorn Voravootivat wrote: > > Add heuristic to decide that AUX or PWM pin should use for > > backlight brightness adjustment and modify i915 param description > > to have auto, force disable, and force enable. > > > > 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 > > > > Signed-off-by: Puthikorn Voravootivat <<a href="mailto:puthik@chromium.org" class="cremed">puthik@chromium.org</a>> > > --- > > drivers/gpu/drm/i915/i915_params.c | 7 +-- > > drivers/gpu/drm/i915/i915_params.h | 2 +- > > drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 64 +++++++++++++++++++++++++-- > > 3 files changed, 66 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > > index b6a7e363d076..3758ae1f11b4 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,10 @@ 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_unsafe(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:auto (default), 0:force disable, 1:force enabled if 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); \ </div></div>Please move this above the bools, see comment in code that's a few lines above. </blockquote><div>Will do</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -DK <div><div class="h5"> > > 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..c89aae804659 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() > > + */ > > Unrelated hunk, please remove. This should have been included in one of > the previous patches. </div></div></blockquote><div>Removed</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > > > 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,15 +171,66 @@ 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; > > } > > > > +/* > > + * 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[1] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) > > Isn't BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP in edp_dpcd[2] ? > </div></div></blockquote><div>Thank you for catching this.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > > > + return true; > > + > > + /* Panel supports regional backlight brightness adjustment */ > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3, > > Spec says this register is new to eDP 1.4. I don't see a check for > version. > </div></div></blockquote><div>There is no need to check this. In not supported eDP panel, this register will read all 0s. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"> > > > + &reg_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; > > I don't know if more than 8 bits is relatively better than the > resolution PWM pin allows. I guess, we could fix that later by comparing > the number of brightness levels. > > > + > > + /* 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; > > @@ -187,6 +241,10 @@ 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; > </div></div>> _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org" class="cremed">Intel-gfx@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank" class="cremed">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </blockquote></div> </div></div>