| Message ID | 20250306131100.3989503-16-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use VRR timing generator for fixed refresh rate modes | expand |
On Thu, Mar 06, 2025 at 06:40:54PM +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(). > v6: > -Disable VRR just before intel_ddi_disable_transcoder_func(). (Ville) > -Correct the sequence of configuring PUSH and VRR Enable/Disable. (Ville) > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@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 | 63 ++++++++++++++++----- > drivers/gpu/drm/i915/display/intel_vrr.h | 2 + > 4 files changed, 60 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 676c1826f15c..6d89a87b3419 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" > > @@ -3248,6 +3249,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) { > @@ -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.c > index 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.c > index f523a48e6186..d7580b6e4e37 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; > - } > - > if (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,54 @@ 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; > + > + if (!intel_vrr_always_use_vrr_tg(display)) { > + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), > + trans_vrr_ctl(crtc_state)); IMO we should write 0 here too. > + 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); Does this do anything? I would have thought the VRR live status would clear already earlier when we disable the transcoder? > + intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0); > +} > + > 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..c4ee8a758e19 100644 > --- a/drivers/gpu/drm/i915/display/intel_vrr.h > +++ b/drivers/gpu/drm/i915/display/intel_vrr.h > @@ -35,5 +35,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
On 3/6/2025 10:16 PM, Ville Syrjälä wrote: > On Thu, Mar 06, 2025 at 06:40:54PM +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(). >> v6: >> -Disable VRR just before intel_ddi_disable_transcoder_func(). (Ville) >> -Correct the sequence of configuring PUSH and VRR Enable/Disable. (Ville) >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@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 | 63 ++++++++++++++++----- >> drivers/gpu/drm/i915/display/intel_vrr.h | 2 + >> 4 files changed, 60 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c >> index 676c1826f15c..6d89a87b3419 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" >> >> @@ -3248,6 +3249,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) { >> @@ -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.c >> index 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.c >> index f523a48e6186..d7580b6e4e37 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; >> - } >> - >> if (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,54 @@ 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; >> + >> + if (!intel_vrr_always_use_vrr_tg(display)) { >> + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), >> + trans_vrr_ctl(crtc_state)); > IMO we should write 0 here too. There is one problem. If we do not write trans_vrr_ctl but crtc_state has flipline, vmin, vmax all set to some value, then we get mismatch in vrr_get_config. Currently we are reading out vrr.flipline/vmax/vmin only if VRR_CTL has VRR_CTL_FLIP_LINE_EN as set. Also, I think we need to write trans_vrr_ctl in intel_vrr_set_transcoder_timings(). Currently that is removed. Mismatch is also seen where the platform has only eDP connected and we just do fastset. In that case intel_vrr_transcoder_enable(). doesnt get call and we get a mismatch. > >> + 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); > Does this do anything? I would have thought the VRR live status would > clear already earlier when we disable the transcoder? Hmm, in Bspec:68925 it seems its mentioned to wait for VRR live status and then clear the VRR PUSH_ENABLE. But not sure if it applies if we are disable the transcoder. Regards, Ankit > >> + intel_de_write(display, TRANS_PUSH(display, cpu_transcoder), 0); >> +} >> + >> 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..c4ee8a758e19 100644 >> --- a/drivers/gpu/drm/i915/display/intel_vrr.h >> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h >> @@ -35,5 +35,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
On Fri, Mar 07, 2025 at 05:33:46PM +0530, Nautiyal, Ankit K wrote: > > On 3/6/2025 10:16 PM, Ville Syrjälä wrote: > > On Thu, Mar 06, 2025 at 06:40:54PM +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(). > >> v6: > >> -Disable VRR just before intel_ddi_disable_transcoder_func(). (Ville) > >> -Correct the sequence of configuring PUSH and VRR Enable/Disable. (Ville) > >> > >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@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 | 63 ++++++++++++++++----- > >> drivers/gpu/drm/i915/display/intel_vrr.h | 2 + > >> 4 files changed, 60 insertions(+), 14 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > >> index 676c1826f15c..6d89a87b3419 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" > >> > >> @@ -3248,6 +3249,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) { > >> @@ -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.c > >> index 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.c > >> index f523a48e6186..d7580b6e4e37 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; > >> - } > >> - > >> if (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,54 @@ 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; > >> + > >> + if (!intel_vrr_always_use_vrr_tg(display)) { > >> + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), > >> + trans_vrr_ctl(crtc_state)); > > IMO we should write 0 here too. > > There is one problem. If we do not write trans_vrr_ctl but crtc_state > has flipline, vmin, vmax all set to some value, then we get mismatch in > vrr_get_config. This is intel_vrr_transcoder_disable(). Nothing is expected to be configured for disabled transcoders.
On 3/7/2025 6:20 PM, Ville Syrjälä wrote: > On Fri, Mar 07, 2025 at 05:33:46PM +0530, Nautiyal, Ankit K wrote: >> On 3/6/2025 10:16 PM, Ville Syrjälä wrote: >>> On Thu, Mar 06, 2025 at 06:40:54PM +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(). >>>> v6: >>>> -Disable VRR just before intel_ddi_disable_transcoder_func(). (Ville) >>>> -Correct the sequence of configuring PUSH and VRR Enable/Disable. (Ville) >>>> >>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@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 | 63 ++++++++++++++++----- >>>> drivers/gpu/drm/i915/display/intel_vrr.h | 2 + >>>> 4 files changed, 60 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c >>>> index 676c1826f15c..6d89a87b3419 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" >>>> >>>> @@ -3248,6 +3249,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) { >>>> @@ -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.c >>>> index 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.c >>>> index f523a48e6186..d7580b6e4e37 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; >>>> - } >>>> - >>>> if (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,54 @@ 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; >>>> + >>>> + if (!intel_vrr_always_use_vrr_tg(display)) { >>>> + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), >>>> + trans_vrr_ctl(crtc_state)); >>> IMO we should write 0 here too. >> There is one problem. If we do not write trans_vrr_ctl but crtc_state >> has flipline, vmin, vmax all set to some value, then we get mismatch in >> vrr_get_config. > This is intel_vrr_transcoder_disable(). Nothing is expected to be > configured for disabled transcoders. Okay will set trans_vrr_ctl to 0. Will retain the wait for clearing of VRR Live status bit for now, but I am open to have it removed for disable transcoder case. Regards, Ankit >
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 676c1826f15c..6d89a87b3419 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" @@ -3248,6 +3249,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) { @@ -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.c index 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.c index f523a48e6186..d7580b6e4e37 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; - } - if (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,54 @@ 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; + + 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); + + 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); +} + 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..c4ee8a758e19 100644 --- a/drivers/gpu/drm/i915/display/intel_vrr.h +++ b/drivers/gpu/drm/i915/display/intel_vrr.h @@ -35,5 +35,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__ */
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(). v6: -Disable VRR just before intel_ddi_disable_transcoder_func(). (Ville) -Correct the sequence of configuring PUSH and VRR Enable/Disable. (Ville) Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@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 | 63 ++++++++++++++++----- drivers/gpu/drm/i915/display/intel_vrr.h | 2 + 4 files changed, 60 insertions(+), 14 deletions(-)