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

[Lisovskiy, Stanislav]

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 at 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.
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",
>