On Tue, 2025-08-12 at 13:59 +0530, Nautiyal, Ankit K wrote:
>
> On 7/21/2025 4:44 PM, Jouni Högander wrote:
> > On Lunarlake and onwards it is possible to generate PSR "frame
> > change"
> > event using TRANS_PUSH mechanism. Implement function to enable this
> > and
> > take PSR into account in intel_vrr_send_push.
> >
> > v5: use intel_psr_use_trans_push for
> > intel_vrr_psr_frame_change_enable
> > v4:
> > - use rmw when enabling/disabling transcoder
> > - set TRANS_PUSH_EN conditionally in intel_vrr_send_push
> > - do not call intel_vrr_send_push from
> > intel_psr_trigger_frame_change
> > - do not enable using TRANS_PUSH mechanism for PSR "Frame
> > Change"
> > v3:
> > - use rmw when enabling/disabling
> > - keep LNL_TRANS_PUSH_PSR_PR_EN set always on LunarLake and
> > onwards
> > v2: use intel_vrr_trans_push_enabled_set_clear instead of rmw
> >
> > Signed-off-by: Jouni Högander <jouni.hogan...@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_crtc.c | 4 ++-
> > drivers/gpu/drm/i915/display/intel_psr.c | 11 +++++--
> > drivers/gpu/drm/i915/display/intel_vrr.c | 39 ++++++++++++++++--
> > -----
> > drivers/gpu/drm/i915/display/intel_vrr.h | 1 +
> > 4 files changed, 40 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index a187db6df2d3..5233970b95e5 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -723,7 +723,9 @@ void intel_pipe_update_end(struct
> > intel_atomic_state *state,
> > * which would cause the next frame to terminate already
> > at vmin
> > * vblank start instead of vmax vblank start.
> > */
> > - if (!state->base.legacy_cursor_update)
> > + if (!state->base.legacy_cursor_update ||
> > + (intel_psr_use_trans_push(new_crtc_state) &&
> > + !new_crtc_state->vrr.enable))
> > intel_vrr_send_push(NULL, new_crtc_state);
>
>
> Currently with PSR, vrr.enable will not be concurrent.
> Is this required for concurrent Panel Replay + VRR?
It's required for DP Panel Replay + VRR.
>
>
> >
> > local_irq_enable();
> > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index d7c19b053fcc..172b1070c7b7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1950,6 +1950,9 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> > intel_dmc_block_pkgc(display, intel_dp->psr.pipe,
> > true);
> >
> > intel_alpm_configure(intel_dp, crtc_state);
> > +
> > + if (intel_psr_use_trans_push(crtc_state))
> > + intel_vrr_psr_frame_change_enable(crtc_state);
>
>
> Not sure it it will be correct to set only this bit, when TRANS_PUSH
> is
> not enabled. May be we can use a flag and use that when are
> enabling/disabling TRANS_VRR_PUSH
Bspec 68934 specifically says:
"Neither Pushes nor VRR need to be enabled for this mechanism to work"
>
>
> > }
> >
> > static bool psr_interrupt_error_check(struct intel_dp *intel_dp)
> > @@ -2336,9 +2339,11 @@ void
> > intel_psr_trigger_frame_change_event(struct intel_dsb *dsb,
> > intel_pre_commit_crtc_state(state, crtc);
> > struct intel_display *display = to_intel_display(crtc);
> >
> > - if (crtc_state->has_psr)
> > - intel_de_write_dsb(display, dsb,
> > - CURSURFLIVE(display, crtc-
> > >pipe), 0);
> > + if (!crtc_state->has_psr || DISPLAY_VER(display) >= 20)
>
> I think it would be better to use a macro
> HAS_PUSH_WITH_PSR_FRAME_CHANGE
> or something similar.
>
>
> > + return;
> > +
> > + intel_de_write_dsb(display, dsb,
> > + CURSURFLIVE(display, crtc->pipe), 0);
> > }
> >
> > /**
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 3eed37f271b0..9db6127abeaa 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -10,6 +10,7 @@
> > #include "intel_display_regs.h"
> > #include "intel_display_types.h"
> > #include "intel_dp.h"
> > +#include "intel_psr.h"
> > #include "intel_vrr.h"
> > #include "intel_vrr_regs.h"
> >
> > @@ -505,16 +506,23 @@ void intel_vrr_send_push(struct intel_dsb
> > *dsb,
> > {
> > struct intel_display *display =
> > to_intel_display(crtc_state);
> > enum transcoder cpu_transcoder = crtc_state-
> > >cpu_transcoder;
> > + u32 trans_push;
> >
> > - if (!crtc_state->vrr.enable)
> > + if (!crtc_state->vrr.enable &&
> > !intel_psr_use_trans_push(crtc_state))
> > return;
> >
> > if (dsb)
> > intel_dsb_nonpost_start(dsb);
> >
> > - intel_de_write_dsb(display, dsb,
> > - TRANS_PUSH(display, cpu_transcoder),
> > - TRANS_PUSH_EN | TRANS_PUSH_SEND);
> > + trans_push = TRANS_PUSH_SEND;
> > +
> > + if (crtc_state->vrr.enable)
> > + trans_push |= TRANS_PUSH_EN;
> > + if (intel_psr_use_trans_push(crtc_state))
> > + trans_push |= LNL_TRANS_PUSH_PSR_PR_EN;
> > +
> > + intel_de_write_dsb(display, dsb, TRANS_PUSH(display,
> > cpu_transcoder),
> > + trans_push);
> >
> > if (dsb)
> > intel_dsb_nonpost_end(dsb);
> > @@ -609,9 +617,7 @@ void intel_vrr_enable(const struct
> > intel_crtc_state *crtc_state)
> > crtc_state->vrr.vmax - 1);
> > intel_de_write(display, TRANS_VRR_FLIPLINE(display,
> > cpu_transcoder),
> > crtc_state->vrr.flipline - 1);
> > -
> > - intel_de_write(display, TRANS_PUSH(display,
> > cpu_transcoder),
> > - TRANS_PUSH_EN);
> > + intel_de_rmw(display, TRANS_PUSH(display, cpu_transcoder),
> > 0, TRANS_PUSH_EN);
> >
> > if (!intel_vrr_always_use_vrr_tg(display)) {
> > intel_vrr_set_db_point_and_transmission_line(crtc_
> > state);
> > @@ -627,6 +633,15 @@ void intel_vrr_enable(const struct
> > intel_crtc_state *crtc_state)
> > }
> > }
> >
> > +void intel_vrr_psr_frame_change_enable(const struct
> > intel_crtc_state *crtc_state)
> > +{
> > + struct intel_display *display =
> > to_intel_display(crtc_state);
> > + enum transcoder cpu_transcoder = crtc_state-
> > >cpu_transcoder;
> > +
> > + intel_de_rmw(display, TRANS_VRR_CTL(display,
> > cpu_transcoder), 0,
> > + LNL_TRANS_PUSH_PSR_PR_EN);
>
>
> I think you mean TRANS_VR_PUSH here instead of TRAN_VRR_CTL.
>
> But as I mentioned earlier, instead of setting this bit, it would be
> better to check flag and set the bit in the existing place.
Ouch.
>
>
> Regards,
>
> Ankit
>
> > +}
> > +
> > void intel_vrr_disable(const struct intel_crtc_state
> > *old_crtc_state)
> > {
> > struct intel_display *display =
> > to_intel_display(old_crtc_state);
> > @@ -641,7 +656,8 @@ void intel_vrr_disable(const struct
> > intel_crtc_state *old_crtc_state)
> > intel_de_wait_for_clear(display,
> > TRANS_VRR_STATUS(display,
> > cpu_transcoder),
> > VRR_STATUS_VRR_EN_LIVE,
> > 1000);
> > - intel_de_write(display, TRANS_PUSH(display,
> > cpu_transcoder), 0);
> > + intel_de_rmw(display, TRANS_PUSH(display,
> > cpu_transcoder),
> > + TRANS_PUSH_EN, 0);
> > }
> >
> > intel_vrr_set_fixed_rr_timings(old_crtc_state);
> > @@ -664,8 +680,8 @@ void intel_vrr_transcoder_enable(const struct
> > intel_crtc_state *crtc_state)
> > return;
> > }
> >
> > - intel_de_write(display, TRANS_PUSH(display,
> > cpu_transcoder),
> > - TRANS_PUSH_EN);
> > + intel_de_rmw(display, TRANS_PUSH(display, cpu_transcoder),
> > + 0, TRANS_PUSH_EN);
> >
> > intel_vrr_set_db_point_and_transmission_line(crtc_state);
> >
> > @@ -688,7 +704,8 @@ void intel_vrr_transcoder_disable(const struct
> > intel_crtc_state *crtc_state)
> >
> > intel_de_wait_for_clear(display, TRANS_VRR_STATUS(display,
> > cpu_transcoder),
> > VRR_STATUS_VRR_EN_LIVE, 1000);
> > - intel_de_write(display, TRANS_PUSH(display,
> > cpu_transcoder), 0);
> > + intel_de_rmw(display, TRANS_PUSH(display, cpu_transcoder),
> > + TRANS_PUSH_EN, 0);
> > }
> >
> > bool intel_vrr_is_fixed_rr(const struct intel_crtc_state
> > *crtc_state)
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h
> > b/drivers/gpu/drm/i915/display/intel_vrr.h
> > index 38bf9996b883..6f815fd05c36 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.h
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
> > @@ -30,6 +30,7 @@ void intel_vrr_check_push_sent(struct intel_dsb
> > *dsb,
> > const struct intel_crtc_state
> > *crtc_state);
> > bool intel_vrr_is_push_sent(const struct intel_crtc_state
> > *crtc_state);
> > void intel_vrr_disable(const struct intel_crtc_state
> > *old_crtc_state);
> > +void intel_vrr_psr_frame_change_enable(const struct
> > intel_crtc_state *crtc_state);
> > void intel_vrr_get_config(struct intel_crtc_state *crtc_state);
> > int intel_vrr_vmax_vtotal(const struct intel_crtc_state
> > *crtc_state);
> > int intel_vrr_vmin_vtotal(const struct intel_crtc_state
> > *crtc_state);
