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 04:38:27PM +0200, Souza, Jose wrote:
> On Fri, 2022-03-18 at 16:19 +0200, Lisovskiy, Stanislav wrote:
> > 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.
> 
> So please add this information to commit message and code comment.			

Yes.

> 
> > 
> > > 
> > > > 
> > > > 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;
> > 

So 588 here is the number of which 500 constitutes 85 %, i.e those "88,.." are 15 % of 588,
but not of 500.

Not huge difference though, but needs to be fixed.

> > 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.
> 
> It will if you are iterating with for_each_intel_encoder_with_psr() and there is more than one encoder with PSR2 enabled.
> Switching to crtc_state->has_psr2 and then increasing the min_cdclk of the pipe with PSR2 enabled, should fix it.

But we call "break" there, after we meet first encoder with psr2_enabled, we exit the for cycle.

Stan

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