Re: [Intel-gfx] [PATCH] drm/i915/adl_p: Increase CDCLK by 15% if PSR2 is used

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Tue, Mar 22, 2022 at 03:16:41PM +0200, Souza, Jose wrote:
> On Tue, 2022-03-22 at 09:48 +0200, Lisovskiy, Stanislav wrote:
> > On Mon, Mar 21, 2022 at 06:58:39PM +0200, Souza, Jose wrote:
> > > On Mon, 2022-03-21 at 12:49 +0200, Stanislav Lisovskiy wrote:
> > > > We are currently getting FIFO underruns, in particular
> > > > when PSR2 is enabled. There seem to be no existing workaround
> > > > or patches, which can fix that issue(were expecting some recent
> > > > selective fetch update and DBuf bw/SAGV fixes to help,
> > > > which unfortunately didn't).
> > > > Current idea is that it looks like for some reason the
> > > > DBuf prefill time isn't enough once we exit PSR2, despite its
> > > > theoretically correct.
> > > > So bump it up a bit by 15%(minimum experimental amount required
> > > > to get it working), if PSR2 is enabled.
> > > > For PSR1 there is no need in this hack, so we limit it only
> > > > to PSR2 and Alderlake.
> > > > 
> > > > v2: - Added comment(Jose Souza)
> > > >     - Fixed 15% calculation(Jose Souza)
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_cdclk.c | 26 ++++++++++++++++++++++
> > > >  1 file changed, 26 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > index 8888fda8b701..92d57869983a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > > > @@ -2325,6 +2325,32 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > > >  					dev_priv->max_cdclk_freq));
> > > >  	}
> > > >  
> > > > +
> > > > +	/*
> > > > +	 * HACK.  We are getting FIFO underruns, in particular
> > > > +	 * when PSR2 is enabled. There seem to be no existing workaround
> > > > +	 * or patches as of now.
> > > > +	 * Current idea is that it looks like for some reason the
> > > > +	 * DBuf prefill time isn't enough once we exit PSR2, despite its
> > > > +	 * theoretically correct.
> > > > +	 * So bump it up a bit by 15%(minimum experimental amount required
> > > > +	 * to get it working), if PSR2 is enabled.
> > > > +	 * For PSR1 there is no need in this hack, so we limit it only
> > > > +	 * to PSR2 and Alderlake.
> > > > +	 */
> > > > +	if (IS_ALDERLAKE_P(dev_priv)) {
> > > > +		struct intel_encoder *encoder;
> > > > +
> > > > +		for_each_intel_encoder_with_psr(&dev_priv->drm, encoder) {
> > > > +			struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > > +
> > > > +			if (intel_dp->psr.psr2_enabled) {
> > > 
> > > Again, you can't use this, PSR could end up disabled when this atomic commit it applied.
> > > Please use intel_crtc_state.has_psr2.
> > 
> > Yes, but if PSR2 is disabled - we don't need this hack, we can live with lower
> > CDCLK then, thus saving power. And once PSR2 is enabled we'll have to switch it
> > on. I intentionally didn't want to enable it always, if PSR2 is supported in principle - we care only if its indeed enabled.
> 
> The problem is that this code don't handle this cases.
> intel_dp->psr.psr2_enabled has the current PSR2 state, while crtc_state have the future(as soon as this state is applied) PSR2 state.
> You should avoid access global state as much as possible during the atomic check phase.
> 
> In a case like, PSR2 disabled going to to a state with PSR2 enabled will cause this workaround to not be applied.

Ah ok, so that intel_dp->psr.psr2_enabled isn't part of committed state, actually yes, that explains - 
I use only dev_priv to get it, but not atomic state.

So has_psr2 indicates the state to be committed? Actually I saw it being assigned to psr2_enabled in
some places, but wasn't sure.
Then need to use that one. The name is a bit confusing then.

Stan

> 
> > Also CDCLK is the last thing, which is being calculated, thats how architecture
> > is designed, so we can rely on all the states here, if you mean that.
> > 
> > Even if this would be not working(not aware why but still), would anyway prefer
> > to find someway to enable this only, when PSR2 is indeed enabled, otherwise
> > we would be kind of wasting power..
> > 
> > 
> > Stan
> > 
> > > 
> > > 
> > > > +				min_cdclk = DIV_ROUND_UP(min_cdclk * 115, 100);
> > > > +				break;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +
> > > >  	if (min_cdclk > dev_priv->max_cdclk_freq) {
> > > >  		drm_dbg_kms(&dev_priv->drm,
> > > >  			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",
> > > 
>