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

[Jani Nikula <jani.nikula@linux.intel.com>]

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;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx