On 2020-12-11 at 16:13:56 +0200, Jani Nikula wrote:
> On Fri, 04 Dec 2020, Anshuman Gupta <anshuman.gupta@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
