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

[Lisovskiy, Stanislav]

On Tue, Mar 29, 2022 at 04:24:30PM +0300, Souza, Jose wrote:
> On Tue, 2022-03-29 at 16:10 +0300, Lisovskiy, Stanislav wrote:
> > On Tue, Mar 22, 2022 at 03:36:10PM +0200, Souza, Jose wrote:
> > > On Tue, 2022-03-22 at 09:49 +0200, Lisovskiy, Stanislav wrote:
> > > > On Mon, Mar 21, 2022 at 07:01:27PM +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)) {
> > > > > 
> > > > > 
> > > > > And please check if we can only apply this when two or more pipes are enabled.
> > > > > Otherwise this will impact power numbers in the case that is matters most.
> > > > 
> > > > That one I can check. Probably need someone at office to disconnect all the pipes, except eDP to see
> > > > if its still reproducible, however I think I've seen it already happening.
> > > 
> > > You can have some hack code in the functions that check if a port is connected and return false for all ports other than port A/eDP.
> > 
> > Yes, did that. i915_display_info now reports only eDP connected. Modified intel_dp_detect to return disconnected state
> > for all non-eDP connectors, also modified hotplug and digport work routines to bail out for non PORT_A ports
> > just in case.
> > So even with eDP connected only FIFO underruns still happen, just ran kms_plane_multiple and it appeared immediately.
> 
> If that only happens with kms_plane_multiple with a particular number of planes or more please limit the workaround to this number of planes then.
> Some folks from ChromeOS team already shown concern about the power impacts of this workaround.
> 
> Also this information might be helpful for HW folks to help identify the issue.

It is not bound to amount of planes at all, I tried reducing N_PLANES to 1 even in that 
test - with same result.
Regarding power consumption concerns, thats true, of course if we figure out some other
solution, without CDCLK increase, that one should be used then.
I'm now discussing the issue with HW team.

Otherwise we just need to decide, whats more important. I think frequent FIFO underruns
would be anyway more visible to the customer, than some energy consumption increase.

Stan

> 
> > 
> > Stan
> > 
> > > 
> > > 
> > > > 
> > > > Stan
> > > > 
> > > > > 
> > > > > > +		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) {
> > > > > > +				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",
> > > > > 
> > > 
>