| Message ID | 20220321104904.12425-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/adl_p: Increase CDCLK by 15% if PSR2 is used | expand |
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. > + 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",
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)) { 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. > + 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",
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. 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", >
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@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. 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", >
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. > 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", > >
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", > > > >
On Tue, 2022-03-22 at 15:30 +0200, Lisovskiy, Stanislav wrote: > 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. When the state is in commit phase psr2_enabled it will eventually be assigned but at intel_crtc_compute_min_cdclk() you need to check has_psr2. > 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", > > > > > >
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@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. > > 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", > >
Hi, On 3/21/22 06:49, 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) { > + 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", Not sure if this will get thru as I'm not subscribed to this list but I tested the patch below on my X1 Yoga 7 (Intel ADL-P with Step C0 silicon) and it didn't fix some screen tearing issues I'm seeing there that are PSR2 related I also tried increasing the timeout to *300 to see if that made any difference and it didn't. Let me know if there's anything else I can try out - I have a couple of ADL-P machines I can test against and both are seeing screen tearing Thanks Mark
On Tue, Mar 22, 2022 at 09:55:35AM -0400, Mark Pearson wrote: > Hi, > > On 3/21/22 06:49, 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) { > > + 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", > > Not sure if this will get thru as I'm not subscribed to this list but I > tested the patch below on my X1 Yoga 7 (Intel ADL-P with Step C0 > silicon) and it didn't fix some screen tearing issues I'm seeing there > that are PSR2 related > > I also tried increasing the timeout to *300 to see if that made any > difference and it didn't. > > Let me know if there's anything else I can try out - I have a couple of > ADL-P machines I can test against and both are seeing screen tearing Are you getting this screen tearing because of the FIFO underruns? Those should be visible in dmesg. This patch can fix only part of underrun issues caused by PSR2. If your screen tearing caused by PSR2, but there are no underruns that patch won't help for sure. Stan > > Thanks > Mark
Thanks Stanislav, On 3/22/22 10:18, Lisovskiy, Stanislav wrote: > On Tue, Mar 22, 2022 at 09:55:35AM -0400, Mark Pearson wrote: >> Hi, >> >> On 3/21/22 06:49, 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) { >>> + 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", >> >> Not sure if this will get thru as I'm not subscribed to this list but I >> tested the patch below on my X1 Yoga 7 (Intel ADL-P with Step C0 >> silicon) and it didn't fix some screen tearing issues I'm seeing there >> that are PSR2 related >> >> I also tried increasing the timeout to *300 to see if that made any >> difference and it didn't. >> >> Let me know if there's anything else I can try out - I have a couple of >> ADL-P machines I can test against and both are seeing screen tearing > > Are you getting this screen tearing because of the FIFO underruns? > Those should be visible in dmesg. This patch can fix only part of underrun > issues caused by PSR2. > If your screen tearing caused by PSR2, but there are no underruns that > patch won't help for sure. > Ah - OK, no FIFO underruns that I've noticed in the logs but I was hoping the two were connected. I'll keep an eye out for those in the meantime. I guess I'll just wave a flag and say I'm seeing PSR2 related tearing issues. If I disable PSR2 selective fetch it goes away (i915.enable_psr2_sel_fetch=0) - but as that's a different issue to this patch thread I don't want to drag the conversation too far sideways. Thanks! Mark
Hi Mark See comment below. On Tue, 2022-03-22 at 10:23 -0400, Mark Pearson wrote: > Thanks Stanislav, > > On 3/22/22 10:18, Lisovskiy, Stanislav wrote: > > On Tue, Mar 22, 2022 at 09:55:35AM -0400, Mark Pearson wrote: > > > Hi, > > > > > > On 3/21/22 06:49, 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) { > > > > + 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", > > > > > > Not sure if this will get thru as I'm not subscribed to this list but I > > > tested the patch below on my X1 Yoga 7 (Intel ADL-P with Step C0 > > > silicon) and it didn't fix some screen tearing issues I'm seeing there > > > that are PSR2 related > > > > > > I also tried increasing the timeout to *300 to see if that made any > > > difference and it didn't. > > > > > > Let me know if there's anything else I can try out - I have a couple of > > > ADL-P machines I can test against and both are seeing screen tearing > > > > Are you getting this screen tearing because of the FIFO underruns? > > Those should be visible in dmesg. This patch can fix only part of underrun > > issues caused by PSR2. > > If your screen tearing caused by PSR2, but there are no underruns that > > patch won't help for sure. > > > Ah - OK, no FIFO underruns that I've noticed in the logs but I was > hoping the two were connected. I'll keep an eye out for those in the > meantime. > > I guess I'll just wave a flag and say I'm seeing PSR2 related tearing > issues. If I disable PSR2 selective fetch it goes away > (i915.enable_psr2_sel_fetch=0) - but as that's a different issue to this > patch thread I don't want to drag the conversation too far sideways. Do you have a bug for your issue? But before you file it please search in our bug tracker if there is another similar issue. With your description so far it looks like you are affected by this https://gitlab.freedesktop.org/drm/intel/-/issues/5077 . It was already fixed in drm-tip but it will take a while to land on Linus master and the on distros kernel. > > Thanks! > Mark
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@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. 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", > > > >
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@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. > > 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", > > > > > >
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@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", > > > > > > > > >
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) { + 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",
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(+)