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

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

On Fri, Mar 18, 2022 at 02:21:10PM +0200, Souza, Jose wrote:
> On Fri, 2022-03-18 at 10:52 +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.
> 
> It this workaround meant to be permanent? If yes we should file a HSD and get hardware folks feedback.

Nope, I hope we figure out some more elegant solution at some point.

> 
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_cdclk.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > index 8888fda8b701..095b79950788 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> > @@ -2325,6 +2325,19 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> >  					dev_priv->max_cdclk_freq));
> >  	}
> >  
> 
> Please add some comment in the code about this workaround.
> 
> 
> > +	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) {
> 
> You should check the has_psr2 in the crtc_state, PSR2 could be disabled when this state is committed.
> 
> > +				min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 85);
> 
> This is not increasing by 15%.
> 
> min_cdclk = 500
> 500 * 100 = 50000
> 50000 / 85 = 588.235294118
> 
> While 15% of 500 is 75.
> 
> Also if there is two CRTCs with PSR2 enabled you will bump min_cdclk twice.
> 
> > +				break;

No, we won't bump up it twice, because we initialize min_cdclk here from pixel rate initially
and only then apply all those dirty hacks and optimizations. There is similar code above as
well.
For each crtc we call this function but as starting point always its pixel rate is used,
then the max() of those would be the actual new cdclk.

As for 15%, good point took this from expression above in that func, but indeed this is 
no a 15%.

Stan

> > +			}
> > +		}
> > +	}
> > +
> >  	if (min_cdclk > dev_priv->max_cdclk_freq) {
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",
>