On 3/5/2025 6:23 PM, Ville Syrjälä wrote:
On Wed, Mar 05, 2025 at 02:11:21PM +0530, Nautiyal, Ankit K wrote:On 3/5/2025 12:20 AM, Ville Syrjälä wrote:On Tue, Mar 04, 2025 at 01:49:41PM +0530, Ankit Nautiyal wrote:During modeset enable sequence, program the fixed timings, and turn on the VRR 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 set and 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(). Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> --- 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 | 59 ++++++++++++++++----- drivers/gpu/drm/i915/display/intel_vrr.h | 3 ++ 4 files changed, 57 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 20fc258a4d6d..6f083c28c455 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"@@ -3273,6 +3274,8 @@ static void intel_ddi_post_disable(struct intel_atomic_state *state,const struct intel_crtc_state *old_crtc_state, const struct drm_connector_state *old_conn_state) { + intel_vrr_transcoder_disable(old_crtc_state); +This isn't symmetric with the enable. If we do the enable just after intel_ddi_enable_transcoder_func() then I would like to see the disable done just before intel_ddi_disable_transcoder_func().Yes you are right. But as per bspec it seems enable and disable sequences are different. For enable the sequence is: TRANS_DDI_FUNC_CTL -> VRR_CTL ->TRANS_CONF But as per bspec: 49190, and 68849for Disable it is: VRR_CTL -> TRANS_CONF -> TRANS_DDI_FUNC_CTLI don't think that part applies to PTL anymore. In fact looks like there's no VRR disable step listed anymore listed. But I think we should have one, and the symmetric spot to enable makes most sense to me.
Good point. The step is removed for PTL, but can still see in LNL. Makes sense to have it symmetrical to enable case. I will make the suggested change.
Though I am following the spec, I am getting issues in the disabling part. Specifically WARN : pipe_off wait timed out for some platforms as flagged by the CI BAT failures.Hmm. I think the hardware should have swithed over to the legacy timing generator at that point since the transcoder was still active. So not sure what's causing the timeout. Did you have the TRANS_VTOTAL=0 patch included in those tests?
Oh Yes I had included that patch. That is one of the thing that might be the cause. I can get rid of that.
On PTL it wasnt giving an issue on local testing (with few tests, not at all as comprehensive as in CI)
Thanks & Regards, Ankit
I can try the change you mentioned to see if it helps.if (!intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST)) intel_ddi_post_disable_hdmi_or_sst(state, encoder, old_crtc_state, old_conn_state); @@ -3521,6 +3524,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..7dbc9b3bdbe4 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -1049,6 +1049,8 @@ static void mst_stream_post_disable(struct intel_atomic_state *state, intel_crtc_vblank_off(old_pipe_crtc_state); }+ intel_vrr_transcoder_disable(old_crtc_state);+Same here.intel_disable_transcoder(old_crtc_state);drm_dp_remove_payload_part1(&intel_dp->mst.mgr, new_mst_state, new_payload);@@ -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.c index c1387d3f60b2..97040ab9ed86 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.c +++ b/drivers/gpu/drm/i915/display/intel_vrr.c @@ -456,12 +456,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; - } - if (crtc_state->cmrr.enable) { intel_de_write(display, TRANS_CMRR_M_HI(display, cpu_transcoder), upper_32_bits(crtc_state->cmrr.cmrr_m)); @@ -473,14 +467,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, @@ -614,6 +601,50 @@ 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_VRR_CTL(display, cpu_transcoder), + VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state)); + + intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), + TRANS_PUSH_EN);IIRC push should generally be set before the VRR_CTL enable. Perhaps doesn't matter here, since the transcoder is still not enabled, but would be nice to be consistent with intel_vrr_enable().You are right will change this.+} + +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; + + 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_VRR_CTL(display, cpu_transcoder), 0);Should probably clear the push enable for good measure too.Will clear the push enable, while disabling. PS: I had tried both with and without clearing the PUSH enable after disabling VRR, but the "pipe_off wait timed out" persisted.+} + static 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 514822577e8a..c81f98f83b58 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.h +++ b/drivers/gpu/drm/i915/display/intel_vrr.h @@ -12,6 +12,7 @@ struct drm_connector_state; struct intel_atomic_state; struct intel_connector; struct intel_crtc_state; +struct intel_display;Leftover from some other change?Sorry about this , it was indeed leftover from other patch. Regards, Ankitstruct intel_dsb;bool intel_vrr_is_capable(struct intel_connector *connector);@@ -35,5 +36,7 @@ int intel_vrr_vmin_vtotal(const struct intel_crtc_state *crtc_state); 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); +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
