diff mbox series

drm/i915/adl_p: Increase CDCLK by 15% if PSR2 is used

Message ID 20220318085226.7348-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

Commit Message

Stanislav Lisovskiy March 18, 2022, 8:52 a.m. UTC
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.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_cdclk.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Souza, Jose March 18, 2022, 12:21 p.m. UTC | #1
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.

> 
> 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;
> +			}
> +		}
> +	}
> +
>  	if (min_cdclk > dev_priv->max_cdclk_freq) {
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",
Souza, Jose March 18, 2022, 12:27 p.m. UTC | #2
On Fri, 2022-03-18 at 05:22 -0700, José Roberto de Souza 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.
> 
> > 
> > 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.

Ah and if a remember correctly those underruns only happens in a scenario with 4 pipes enabled? or it also happens with 2 and 3 pipes?
Anyways would be better to narrow down the cases where the workaround is applied.
So for at least in a case with a single pipe enabled we can have the lowest cdclk as possible. 

> 
> > +				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;
> > +			}
> > +		}
> > +	}
> > +
> >  	if (min_cdclk > dev_priv->max_cdclk_freq) {
> >  		drm_dbg_kms(&dev_priv->drm,
> >  			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",
>
Stanislav Lisovskiy March 18, 2022, 2:19 p.m. UTC | #3
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",
>
Stanislav Lisovskiy March 18, 2022, 2:22 p.m. UTC | #4
On Fri, Mar 18, 2022 at 02:27:53PM +0200, Souza, Jose wrote:
> On Fri, 2022-03-18 at 05:22 -0700, José Roberto de Souza 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.
> > 
> > > 
> > > 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.
> 
> Ah and if a remember correctly those underruns only happens in a scenario with 4 pipes enabled? or it also happens with 2 and 3 pipes?
> Anyways would be better to narrow down the cases where the workaround is applied.
> So for at least in a case with a single pipe enabled we can have the lowest cdclk as possible. 

I was thinking the same initially, but this underrun is observed in lesser pipe cases, when PSR2 
is enabled.

Stan

> 
> > 
> > > +				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;
> > > +			}
> > > +		}
> > > +	}
> > > +
> > >  	if (min_cdclk > dev_priv->max_cdclk_freq) {
> > >  		drm_dbg_kms(&dev_priv->drm,
> > >  			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",
> > 
>
Souza, Jose March 18, 2022, 2:38 p.m. UTC | #5
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.			

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

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.

> 
> 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",
> >
Souza, Jose March 18, 2022, 2:40 p.m. UTC | #6
On Fri, 2022-03-18 at 16:22 +0200, Lisovskiy, Stanislav wrote:
> On Fri, Mar 18, 2022 at 02:27:53PM +0200, Souza, Jose wrote:
> > On Fri, 2022-03-18 at 05:22 -0700, José Roberto de Souza 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.
> > > 
> > > > 
> > > > 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.
> > 
> > Ah and if a remember correctly those underruns only happens in a scenario with 4 pipes enabled? or it also happens with 2 and 3 pipes?
> > Anyways would be better to narrow down the cases where the workaround is applied.
> > So for at least in a case with a single pipe enabled we can have the lowest cdclk as possible. 
> 
> I was thinking the same initially, but this underrun is observed in lesser pipe cases, when PSR2 
> is enabled.

Please check if at least the one pipe case really need this WA.

> 
> Stan
> 
> > 
> > > 
> > > > +				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;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +
> > > >  	if (min_cdclk > dev_priv->max_cdclk_freq) {
> > > >  		drm_dbg_kms(&dev_priv->drm,
> > > >  			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",
> > > 
> >
Stanislav Lisovskiy March 18, 2022, 2:45 p.m. UTC | #7
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",
> > > 
>
diff mbox series

Patch

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));
 	}
 
+	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 * 100, 85);
+				break;
+			}
+		}
+	}
+
 	if (min_cdclk > dev_priv->max_cdclk_freq) {
 		drm_dbg_kms(&dev_priv->drm,
 			    "required cdclk (%d kHz) exceeds max (%d kHz)\n",