On 2020-11-24 at 18:44:06 +0200, Imre Deak wrote: > On Tue, Nov 24, 2020 at 03:28:47PM +0530, Anshuman Gupta wrote: > > Platforms with South Display Engine on PCH, doesn't > > require to get/put the AUX power domain in order to > > access PPS register because PPS registers are always on > > with South display on PCH. > > > > Cc: Imre Deak <imre.deak@xxxxxxxxx> > > Cc: <stable@xxxxxxxxxxxxxxx> > > Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx> > > Could you describe the issue the patch is fixing? This fixes the display glitches causes by race between brightness update thread and flip thread. while brightness is being updated it reads pp_ctrl reg to check whether backlight is enabled and get/put the AUX power domain, this enables and disable DC Off power well(DC3CO) back and forth. IMO there are two work item for above race needed to be addressed. 1. Don't get AUX power for PPS register access (this patch addressed this). 2. skl_program_plane() should wait for DC3CO exit delay to avoid any race with DC3CO disable sequence. (WIP) > > For accessing PPS registers the AUX power well may not be needed, but > I'm not sure if this also applies to PPS functionality in general. For > instance forcing VDD is required for AUX functionality. AFAIU edp_panel_vdd_on explicitly get AUX power in order to force the VDD. > > In any case we do need a power reference for any register access, so I > don't think not getting any power reference for PPS is ok. IMO if PPS register lies in PCH(South Display), it is not correct to take any power domain which are associated with north display power wells. This patch is inspired from the comment in pps_lock, quoting that "See intel_power_sequencer_reset() why we need a power domain reference here." intel_power_sequencer_reset is not being called for platforms with split PCH, stating that PPS registers are always on. https://patchwork.freedesktop.org/patch/259077/ ((v4: (James Ausmus))) Could you please provide your opinion to use intel_runtime_pm_get() before accessing PPS register in order to get a wakeref. Thanks, Anshuman Gupta. > > --Imre > > > --- > > drivers/gpu/drm/i915/display/intel_dp.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 3896d08c4177..84a2c49e154c 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -872,8 +872,9 @@ pps_lock(struct intel_dp *intel_dp) > > * See intel_power_sequencer_reset() why we need > > * a power domain reference here. > > */ > > - wakeref = intel_display_power_get(dev_priv, > > - intel_aux_power_domain(dp_to_dig_port(intel_dp))); > > + if (!HAS_PCH_SPLIT(dev_priv)) > > + wakeref = intel_display_power_get(dev_priv, > > + intel_aux_power_domain(dp_to_dig_port(intel_dp))); > > > > mutex_lock(&dev_priv->pps_mutex); > > > > @@ -886,9 +887,11 @@ 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); > > - intel_display_power_put(dev_priv, > > - intel_aux_power_domain(dp_to_dig_port(intel_dp)), > > - wakeref); > > + > > + if (!HAS_PCH_SPLIT(dev_priv)) > > + intel_display_power_put(dev_priv, > > + intel_aux_power_domain(dp_to_dig_port(intel_dp)), > > + wakeref); > > return 0; > > } > > > > -- > > 2.26.2 > >
