On Mon, 10 Jun 2024, "Kandpal, Suraj" <suraj.kandpal@xxxxxxxxx> wrote:
>> Subject: Re: [PATCH 2/2] drm/i915/psr: Implment WA to help reach PC10
>>
>> On Thu, 06 Jun 2024, Suraj Kandpal <suraj.kandpal@xxxxxxxxx> wrote:
>> > To reach PC10 when PKG_C_LATENCY is configure we must do the following
>> > things
>> > 1) Enter PSR1 only when delayed_vblank < 6 lines and DC5 can be
>> > entered
>> > 2) Allow PSR2 deep sleep when DC5 can be entered
>> > 3) DC5 can be entered when all transocoder have either PSR1, PSR2 or
>> > eDP 1.5 PR ALPM enabled and VBI is disabled and flips and pushes are
>> > not happening.
>> >
>> > WA: 16023497226
>> > Signed-off-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/display/intel_psr.c | 75
>> > +++++++++++++++++++++++-
>> > 1 file changed, 73 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
>> > b/drivers/gpu/drm/i915/display/intel_psr.c
>> > index 6fc88f6c6b26..b22745c019df 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
>> > @@ -811,12 +811,81 @@ static u8 psr_compute_idle_frames(struct
>> intel_dp *intel_dp)
>> > return idle_frames;
>> > }
>> >
>> > +static bool intel_psr_check_delayed_vblank_limit(struct drm_i915_private
>> *i915,
>> > + enum transcoder
>> cpu_transcoder) {
>> > + return intel_de_read(i915,
>> > +TRANS_SET_CONTEXT_LATENCY(cpu_transcoder)) >= 6;
>>
> Hi Jani,
> Thanks for the reviews
>
>> Please don't use the hardware to preserve the state for you. It will get really
>> complicated to maintain.
>>
>
> Yes wanted to calculate the delayed vblank using the following way
> Adjusted_mode->vblank_start - adjusted_mode->vblank_end
> But I'll need crtc_state for that and I don't see a way of deriving it
> Specially when this function is called from intel_psr_work
> One way could be to have this wa check function be called from
> Intel_psr_enable_locked and save the corresponding Booleans in
> Intel_psr or make in drm_i915_private
> structure and access that when intel_psr_activate is called from
> Intel_psr_resume and intel_psr_work.
> Do you think that could be feasible ?
You'll be able to figure out a lot of cases up front at compute config
time, and disable PSR beforehand.
You'll know LNL_PKG_C_LATENCY (we seem to always configure it). You'll
know TRANS_SET_CONTEXT_LATENCY. You'll know whether all transcoders have
PSR enabled.
I think you'll need to split the conditions, and disable PSR as early as
possible when it should not be enabled. Then at actual enabling time,
you'll know the conditions that already have to hold, and you can check
fewer things.
This workaround is a bummer because it's permanent. It also means we
need to do this properly. Can't just poke at random stuff, because it'll
be painful forever.
BR,
Jani.
>
>> > +}
>> > +
>> > +static bool intel_psr_is_dpkgc_configured(struct drm_i915_private
>> > +*i915) {
>> > + return intel_de_read(i915, LNL_PKG_C_LATENCY) == U32_MAX;
>>
>> Ditto.
>>
>
> Similar question as above only place that I can manage a state to see if it is configured or not
> would be in drm_i915_private.
>
>> > +}
>> > +
>> > +static bool intel_psr_is_dc5_entry_possible(struct drm_i915_private
>> > +*i915) {
>> > + struct intel_crtc *intel_crtc;
>> > + bool ret = true;
>> > +
>> > + for_each_intel_crtc(&i915->drm, intel_crtc) {
>> > + struct intel_encoder *encoder;
>> > + struct drm_crtc *crtc = &intel_crtc->base;
>> > + enum pipe pipe = intel_crtc->pipe;
>> > +
>> > + if (!crtc->active)
>> > + continue;
>> > +
>> > + if (!(i915->display.irq.de_irq_mask[pipe] &
>> GEN8_PIPE_VBLANK))
>>
>> You have no business looking directly at that. It's for display irq code *only*.
>>
>
> Is there another way I can ensure if the vblank interrupt for the particular pipe is disabled?
>
>> > + ret = false;
>> > +
>> > + for_each_encoder_on_crtc(&i915->drm, crtc, encoder) {
>> > + struct intel_dp *intel_dp = enc_to_intel_dp(_encoder);
>> > + struct intel_psr *psr = &intel_dp->psr;
>> > +
>> > + if (!psr->enabled)
>> > + ret = false;
>> > + }
>> > + }
>> > +
>> > + return ret;
>> > +}
>> > +
>> > +static bool wa_16023497226_check(struct intel_dp *intel_dp, bool
>> > +psr1) {
>> > + struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>> > + enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>> > +
>> > + if (DISPLAY_VER(i915) != 20)
>> > + return true;
>> > +
>> > + if (is_dpkg_c_configured(i915)) {
>> > + if (psr1 &&
>> > + (intel_psr_check_delayed_vblank_limit(i915,
>> cpu_transcoder) ||
>> > + intel_psr_is_dc5_entry_possible(i915)))
>> > + return true;
>> > + else if (!psr1 && is_dc5_entry_possible(i915))
>> > + return true;
>> > + else
>> > + return false;
>> > + }
>> > +
>> > + return true;
>> > +}
>> > +
>> > static bool hsw_activate_psr1(struct intel_dp *intel_dp) {
>> > struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> > enum transcoder cpu_transcoder = intel_dp->psr.transcoder;
>> > u32 max_sleep_time = 0x1f;
>> > - u32 val = EDP_PSR_ENABLE;
>> > + u32 val = 0;
>> > +
>> > + /* WA: 16023497226*/
>> > + if (wa_16023497226_check(intel_dp, true)) {
>> > + val = EDP_PSR_ENABLE;
>> > + } else {
>> > + drm_dbg_kms(&dev_priv->drm, "PSR1 was not activated\n");
>>
>> Please add reason.
>>
>> > + return false;
>> > + }
>>
>> Switch the condition around and use early return.
>>
>
> Sure will do.
>
> Regards,
> Suraj Kandpal
>> >
>> > val |= EDP_PSR_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
>> >
>> > @@ -910,7 +979,9 @@ static void hsw_activate_psr2(struct intel_dp
>> *intel_dp)
>> > u32 val = EDP_PSR2_ENABLE;
>> > u32 psr_val = 0;
>> >
>> > - val |= EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
>> > + /* WA: 16023497226*/
>> > + if (wa_16023497226_check(intel_dp, false))
>> > + val |=
>> EDP_PSR2_IDLE_FRAMES(psr_compute_idle_frames(intel_dp));
>> >
>> > if (DISPLAY_VER(dev_priv) < 14 && !IS_ALDERLAKE_P(dev_priv))
>> > val |= EDP_SU_TRACK_ENABLE;
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
