| Message ID | 20201204081845.26528-2-anshuman.gupta@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display glitches fixes | expand |
On Fri, 04 Dec 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote: > Reading backlight status from PPS register doesn't require > AUX power on the platform which has South Display Engine on PCH. > It invokes a unnecessary power well enable/disable noise. > optimize it wherever is possible. Three aspects here: 1. What's the root cause for the glitches, really? AFAICT this is still an open question, judging from the discussion in previous versions. 2. See why we end up here in the first place for brightness updates. It's a long story (*), but maybe the fix isn't to optimize this path, but to avoid calling this function for regular brightness updates to begin with? 3. The implementation here seems like a hack, to be honest. Considering the points above, it really has a bad vibe of papering over something else. BR, Jani. (*) It was a Chrome OS requirement originally to be able to quickly switch off backlight through the backlight sysfs interface, without switching off the display through the KMS API. For whatever reason. We can't just set the PWM to 0, because that may an invalid thing to do on some boards out there. (On some device it ended up pulling other lanes on the eDP connector to 0 V, but I digress.) So the hack is we have a way to switch the eDP power sequencer backlight bit off/on, as a substate of enabled backlight, through using the backlight sysfs to set the brightness to 0 or using bl_power. > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 47 +++++++++++++++++++++++-- > 1 file changed, 45 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 2d4d5e95af84..7e18e4ff50f4 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -892,6 +892,47 @@ pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wakeref) > return 0; > } > > +/* > + * Platform with PCH based SDE doesn't require to enable AUX power > + * for simple PPS register access like whether backlight is enabled. > + * use pch_pps_lock()/pch_pps_unlock() wherever we don't require > + * aux power to avoid unnecessary power well enable/disable back > + * and forth. > + */ > +static intel_wakeref_t > +pch_pps_lock(struct intel_dp *intel_dp) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + intel_wakeref_t wakeref; > + > + if (!HAS_PCH_SPLIT(dev_priv)) > + wakeref = intel_display_power_get(dev_priv, > + intel_aux_power_domain(dp_to_dig_port(intel_dp))); > + else > + wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); > + > + mutex_lock(&dev_priv->pps_mutex); > + > + return wakeref; > +} > + > +static intel_wakeref_t > +pch_pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wakeref) > +{ > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > + > + mutex_unlock(&dev_priv->pps_mutex); > + > + if (!HAS_PCH_SPLIT(dev_priv)) > + intel_display_power_put(dev_priv, > + intel_aux_power_domain(dp_to_dig_port(intel_dp)), > + wakeref); > + else > + intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref); > + > + return 0; > +} > + > #define with_pps_lock(dp, wf) \ > for ((wf) = pps_lock(dp); (wf); (wf) = pps_unlock((dp), (wf))) > > @@ -3453,8 +3494,10 @@ static void intel_edp_backlight_power(struct intel_connector *connector, > bool is_enabled; > > is_enabled = false; > - with_pps_lock(intel_dp, wakeref) > - is_enabled = ilk_get_pp_control(intel_dp) & EDP_BLC_ENABLE; > + wakeref = pch_pps_lock(intel_dp); > + is_enabled = ilk_get_pp_control(intel_dp) & EDP_BLC_ENABLE; > + pch_pps_unlock(intel_dp, wakeref); > + > if (is_enabled == enable) > return;
On 2020-12-11 at 16:13:56 +0200, Jani Nikula wrote: > On Fri, 04 Dec 2020, Anshuman Gupta <anshuman.gupta@intel.com> wrote: > > Reading backlight status from PPS register doesn't require > > AUX power on the platform which has South Display Engine on PCH. > > It invokes a unnecessary power well enable/disable noise. > > optimize it wherever is possible. > > Three aspects here: Thanks Jani for comments. > > 1. What's the root cause for the glitches, really? AFAICT this is still > an open question, judging from the discussion in previous versions. Yes it is still open, but it can be concluded from the experiments (*) that this issue is not due to race in driver between flips update and brightness update. > > 2. See why we end up here in the first place for brightness > updates. It's a long story (*), but maybe the fix isn't to optimize this > path, but to avoid calling this function for regular brightness updates > to begin with? Agree with you, may be this is the correct time to pursue for a correct fix. > > 3. The implementation here seems like a hack, to be honest. Considering > the points above, it really has a bad vibe of papering over something > else. Could you please provide your inputs to improve this patch so chrome-os can use this patch for their consumption. Meanwhile in parallel we can work to fix this brightness interface. (*) Experiments: 1. Get/Put POWER_DOMAIN_MODESET power domain always in atomic_commit_tail (suggested by Ville). Not helping to fix the glitch. 2. 200us delay in starts of atomic_commit_tail to serialze the flips against DC3CO exit delay(200us). Not helping to fix the glitch. > > BR, > Jani. > > > > (*) > It was a Chrome OS requirement originally to be able to quickly switch > off backlight through the backlight sysfs interface, without switching > off the display through the KMS API. For whatever reason. We can't just > set the PWM to 0, because that may an invalid thing to do on some boards > out there. (On some device it ended up pulling other lanes on the eDP > connector to 0 V, but I digress.) For my curiosity i am interested to know, how did other linux distribution like ubuntu handled the brightness update by dedicated brightness key before this original requirement from chrome-os? Thanks, Anshuman Gupta. > > So the hack is we have a way to switch the eDP power sequencer backlight > bit off/on, as a substate of enabled backlight, through using the > backlight sysfs to set the brightness to 0 or using bl_power. > > > > > Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 47 +++++++++++++++++++++++-- > > 1 file changed, 45 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 2d4d5e95af84..7e18e4ff50f4 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -892,6 +892,47 @@ pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wakeref) > > return 0; > > } > > > > +/* > > + * Platform with PCH based SDE doesn't require to enable AUX power > > + * for simple PPS register access like whether backlight is enabled. > > + * use pch_pps_lock()/pch_pps_unlock() wherever we don't require > > + * aux power to avoid unnecessary power well enable/disable back > > + * and forth. > > + */ > > +static intel_wakeref_t > > +pch_pps_lock(struct intel_dp *intel_dp) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + intel_wakeref_t wakeref; > > + > > + if (!HAS_PCH_SPLIT(dev_priv)) > > + wakeref = intel_display_power_get(dev_priv, > > + intel_aux_power_domain(dp_to_dig_port(intel_dp))); > > + else > > + wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); > > + > > + mutex_lock(&dev_priv->pps_mutex); > > + > > + return wakeref; > > +} > > + > > +static intel_wakeref_t > > +pch_pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wakeref) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + > > + mutex_unlock(&dev_priv->pps_mutex); > > + > > + if (!HAS_PCH_SPLIT(dev_priv)) > > + intel_display_power_put(dev_priv, > > + intel_aux_power_domain(dp_to_dig_port(intel_dp)), > > + wakeref); > > + else > > + intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref); > > + > > + return 0; > > +} > > + > > #define with_pps_lock(dp, wf) \ > > for ((wf) = pps_lock(dp); (wf); (wf) = pps_unlock((dp), (wf))) > > > > @@ -3453,8 +3494,10 @@ static void intel_edp_backlight_power(struct intel_connector *connector, > > bool is_enabled; > > > > is_enabled = false; > > - with_pps_lock(intel_dp, wakeref) > > - is_enabled = ilk_get_pp_control(intel_dp) & EDP_BLC_ENABLE; > > + wakeref = pch_pps_lock(intel_dp); > > + is_enabled = ilk_get_pp_control(intel_dp) & EDP_BLC_ENABLE; > > + pch_pps_unlock(intel_dp, wakeref); > > + > > if (is_enabled == enable) > > return; > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 2d4d5e95af84..7e18e4ff50f4 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -892,6 +892,47 @@ pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wakeref) return 0; } +/* + * Platform with PCH based SDE doesn't require to enable AUX power + * for simple PPS register access like whether backlight is enabled. + * use pch_pps_lock()/pch_pps_unlock() wherever we don't require + * aux power to avoid unnecessary power well enable/disable back + * and forth. + */ +static intel_wakeref_t +pch_pps_lock(struct intel_dp *intel_dp) +{ + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + intel_wakeref_t wakeref; + + if (!HAS_PCH_SPLIT(dev_priv)) + wakeref = intel_display_power_get(dev_priv, + intel_aux_power_domain(dp_to_dig_port(intel_dp))); + else + wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm); + + mutex_lock(&dev_priv->pps_mutex); + + return wakeref; +} + +static intel_wakeref_t +pch_pps_unlock(struct intel_dp *intel_dp, intel_wakeref_t wakeref) +{ + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); + + mutex_unlock(&dev_priv->pps_mutex); + + if (!HAS_PCH_SPLIT(dev_priv)) + intel_display_power_put(dev_priv, + intel_aux_power_domain(dp_to_dig_port(intel_dp)), + wakeref); + else + intel_runtime_pm_put(&dev_priv->runtime_pm, wakeref); + + return 0; +} + #define with_pps_lock(dp, wf) \ for ((wf) = pps_lock(dp); (wf); (wf) = pps_unlock((dp), (wf))) @@ -3453,8 +3494,10 @@ static void intel_edp_backlight_power(struct intel_connector *connector, bool is_enabled; is_enabled = false; - with_pps_lock(intel_dp, wakeref) - is_enabled = ilk_get_pp_control(intel_dp) & EDP_BLC_ENABLE; + wakeref = pch_pps_lock(intel_dp); + is_enabled = ilk_get_pp_control(intel_dp) & EDP_BLC_ENABLE; + pch_pps_unlock(intel_dp, wakeref); + if (is_enabled == enable) return;
Reading backlight status from PPS register doesn't require AUX power on the platform which has South Display Engine on PCH. It invokes a unnecessary power well enable/disable noise. optimize it wherever is possible. Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 47 +++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-)