Re: [Intel-gfx] [PATCH 1/1] drm/i915/dp: optimize pps_lock wherever required

[Anshuman Gupta <anshuman.gupta@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx