On 3/20/2025 11:09 AM, Nautiyal, Ankit K wrote:
On 3/19/2025 8:06 PM, Ville Syrjälä wrote:On Tue, Mar 18, 2025 at 01:05:34PM +0530, Ankit Nautiyal wrote:During modeset enable sequence, program the fixed timings, and turn on theVRR Timing Generator (VRR TG) for platforms that always use VRR TG. For this intel_vrr_set_transcoder now always programs fixed timings. Later if vrr timings are required, vrr_enable() will switch to the real VRR timings.For platforms that will always use VRR TG, the VRR_CTL Enable bit is setand reset in the transcoder enable/disable path. v2: Update intel_vrr_set_transcoder_timings for fixed_rr. v3: Update intel_set_transcoder_timings_lrr for fixed_rr. (Ville) v4: Have separate functions to enable/disable VRR CTL v5: -For platforms that do not always have VRRTG on, do write bits other than enable bit and also use write the TRANS_VRR_PUSH register. (Ville) -Avoid writing trans_ctl_vrr if !vrr_possible(). v6: -Disable VRR just before intel_ddi_disable_transcoder_func(). (Ville)-Correct the sequence of configuring PUSH and VRR Enable/Disable. (Ville)v7: Reset trans_vrr_ctl to 0 unconditionally in intel_vrr_transcoder_disable(). (Ville) Signed-off-by: Ankit Nautiyal <ankit.k.nauti...@intel.com> Reviewed-by: Ville Syrjälä <ville.syrj...@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 5 ++ drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 ++drivers/gpu/drm/i915/display/intel_vrr.c | 57 ++++++++++++++++-----drivers/gpu/drm/i915/display/intel_vrr.h | 2 + 4 files changed, 54 insertions(+), 14 deletions(-)diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.cindex f38c998935b9..44f4465c27e2 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -78,6 +78,7 @@ #include "intel_tc.h" #include "intel_vdsc.h" #include "intel_vdsc_regs.h" +#include "intel_vrr.h" #include "skl_scaler.h" #include "skl_universal_plane.h"@@ -3249,6 +3250,8 @@ static void intel_ddi_post_disable_hdmi_or_sst(struct intel_atomic_state *state,drm_dp_dpcd_poll_act_handled(&intel_dp->aux, 0); } + intel_vrr_transcoder_disable(old_crtc_state); + intel_ddi_disable_transcoder_func(old_crtc_state);for_each_pipe_crtc_modeset_disable(display, pipe_crtc, old_crtc_state, i) { @@ -3522,6 +3525,8 @@ static void intel_ddi_enable(struct intel_atomic_state *state,intel_ddi_enable_transcoder_func(encoder, crtc_state); + intel_vrr_transcoder_enable(crtc_state); + /* Enable/Disable DP2.0 SDP split config before transcoder */ intel_audio_sdp_split_update(crtc_state);diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.cindex bd47cf127b4c..d2988b9a6e7b 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c@@ -1065,6 +1065,8 @@ static void mst_stream_post_disable(struct intel_atomic_state *state,drm_dp_remove_payload_part2(&intel_dp->mst.mgr, new_mst_state, old_payload, new_payload); + intel_vrr_transcoder_disable(old_crtc_state); + intel_ddi_disable_transcoder_func(old_crtc_state);for_each_pipe_crtc_modeset_disable(display, pipe_crtc, old_crtc_state, i) { @@ -1326,6 +1328,8 @@ static void mst_stream_enable(struct intel_atomic_state *state,intel_ddi_enable_transcoder_func(encoder, pipe_config); + intel_vrr_transcoder_enable(pipe_config); + intel_ddi_clear_act_sent(encoder, pipe_config); intel_de_rmw(display, TRANS_DDI_FUNC_CTL(display, trans), 0,diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.cindex 66afa66c66af..d4912199c3ac 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.c +++ b/drivers/gpu/drm/i915/display/intel_vrr.c@@ -460,12 +460,6 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)intel_de_rmw(display, CHICKEN_TRANS(display, cpu_transcoder), 0, PIPE_VBLANK_WITH_DELAY); - if (!intel_vrr_possible(crtc_state)) { - intel_de_write(display, - TRANS_VRR_CTL(display, cpu_transcoder), 0); - return; - } -Seems to me that removing this is what causes the state checker woes in BAT. I think we can leave this in, as our own computed state shouldalways have a valid flipline and it's only the GOP that doesn't have one.Yeah makes sense. I was wondering what was missing, thanks for spotting this.I have sent revised patch in reply to this patch. With this last patch will not be required, I'll remove the last patch.
I guess we still need the last patch without that for case where platforms do not always have VRR TG enabled.
For such platforms if full modeset happens, the trans_vrr_ctl gets filled, but if it doesn't happen trans_vrr_ctl is not filled, but VRR timings are all filled.
Since we depend on FLIPLINE_EN in trans_vrr to be set before reading VRR.flipline/vmin/vmax, we get a mismatch.
E.g: https://intel-gfx-ci.01.org/tree/drm-tip/Trybot_146621v1/bat-rplp-1/igt@i915_module_l...@load.html
Regards, Ankit
Regards, Ankitif (crtc_state->cmrr.enable) {intel_de_write(display, TRANS_CMRR_M_HI(display, cpu_transcoder),upper_32_bits(crtc_state->cmrr.cmrr_m));@@ -477,14 +471,7 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)lower_32_bits(crtc_state->cmrr.cmrr_n)); } - intel_de_write(display, TRANS_VRR_VMIN(display, cpu_transcoder), - crtc_state->vrr.vmin - 1); - intel_de_write(display, TRANS_VRR_VMAX(display, cpu_transcoder), - crtc_state->vrr.vmax - 1); - intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), - trans_vrr_ctl(crtc_state));- intel_de_write(display, TRANS_VRR_FLIPLINE(display, cpu_transcoder),- crtc_state->vrr.flipline - 1); + intel_vrr_set_fixed_rr_timings(crtc_state); if (HAS_AS_SDP(display)) intel_de_write(display,@@ -618,6 +605,48 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)intel_vrr_set_fixed_rr_timings(old_crtc_state); }+void intel_vrr_transcoder_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; + + if (!HAS_VRR(display)) + return; + + if (!intel_vrr_possible(crtc_state)) + return; + + if (!intel_vrr_always_use_vrr_tg(display)) {+ intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),+ trans_vrr_ctl(crtc_state)); + return; + } + + intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), + TRANS_PUSH_EN); + + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), + VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state)); +} ++void intel_vrr_transcoder_disable(const struct intel_crtc_state *crtc_state)+{ + struct intel_display *display = to_intel_display(crtc_state); + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; + + if (!HAS_VRR(display)) + return; + + if (!intel_vrr_possible(crtc_state)) + return; ++ intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), 0);++ 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); +} + bool intel_vrr_is_fixed_rr(const struct intel_crtc_state *crtc_state) { return crtc_state->vrr.flipline &&diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.hindex 65d2b0eead51..859f1dc8a6d7 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.h +++ b/drivers/gpu/drm/i915/display/intel_vrr.h@@ -36,5 +36,7 @@ int intel_vrr_vmax_vblank_start(const struct intel_crtc_state *crtc_state); int intel_vrr_vmin_vblank_start(const struct intel_crtc_state *crtc_state); int intel_vrr_vblank_delay(const struct intel_crtc_state *crtc_state); bool intel_vrr_is_fixed_rr(const struct intel_crtc_state *crtc_state); +void intel_vrr_transcoder_enable(const struct intel_crtc_state *crtc_state); +void intel_vrr_transcoder_disable(const struct intel_crtc_state *crtc_state);#endif /* __INTEL_VRR_H__ */ -- 2.45.2
