On 1/25/2025 3:23 AM, Ville Syrjälä wrote:
On Fri, Jan 24, 2025 at 08:30:16PM +0530, Ankit Nautiyal wrote:
To work seamlessly between variable and fixed timings,
intel_vrr_{enable,disable}() should just flip between the fixed and
variable timings in vmin/flipline/vmax.
The idea is to just do this for all the platforms, regardless of whether
we also toggle the VRR_CTL enable bit there.
For platforms for which vrr timing generator is always set, VRR_CTL
enable bit does not need to toggle, so modify the vrr_{enable/disable}
for this.
Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
---
drivers/gpu/drm/i915/display/intel_display.c | 5 ++-
drivers/gpu/drm/i915/display/intel_vrr.c | 44 ++++++++++++--------
drivers/gpu/drm/i915/display/intel_vrr.h | 4 +-
3 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 7bdd41158a93..a0d6360f4cda 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1310,7 +1310,7 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
intel_psr_pre_plane_update(state, crtc);
if (intel_crtc_vrr_disabling(state, crtc)) {
- intel_vrr_disable(old_crtc_state);
+ intel_vrr_disable(old_crtc_state, false);
intel_crtc_update_active_timings(old_crtc_state, false);
}
@@ -1751,6 +1751,7 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
{
struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+ struct intel_display *display = to_intel_display(crtc_state);
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
if (crtc_state->has_pch_encoder) {
@@ -7161,7 +7162,7 @@ static void commit_pipe_post_planes(struct intel_atomic_state *state,
skl_detach_scalers(new_crtc_state);
if (intel_crtc_vrr_enabling(state, crtc))
- intel_vrr_enable(new_crtc_state);
+ intel_vrr_enable(new_crtc_state, false);
}
static void intel_enable_crtc(struct intel_atomic_state *state,
diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
index ccc40867c10a..10a9bcb8daae 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.c
+++ b/drivers/gpu/drm/i915/display/intel_vrr.c
@@ -496,7 +496,7 @@ bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state)
return intel_de_read(display, TRANS_PUSH(display, cpu_transcoder)) & TRANS_PUSH_SEND;
}
-void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
+void intel_vrr_enable(const struct intel_crtc_state *crtc_state, bool always_use_vrr_tg)
That new parameter shouldn't be needed. We should already know whether
we're always using the VRR timing generator or not.
{
struct intel_display *display = to_intel_display(crtc_state);
enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
@@ -507,21 +507,25 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
if (!intel_vrrtg_is_enabled(crtc_state))
return;
- if (intel_vrr_use_push(crtc_state))
- intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
- TRANS_PUSH_EN);
+ intel_vrr_set_transcoder_timings(crtc_state);
That guy probably does a few too many things for us.
Either we need to chop it up or not even use it.
We just want vmax/vmin/flipline updated here.
Yes right. The below code snippet seems more appropriate for this case.
So I'm thinking this should look more or less like this:
vrr_enable() {
write(VMAX, crtc_state->vrr.vmax - 1);
write(VMIN, crtc_state->vrr.vmin - 1);
write(FLIPLINE, crtc_state->vrr.flipline - 1);
if (!always_use_vrr_tg) {
enable PUSH
enable VRR_VTL
wait for VRR status
}
}
vrr_disable() {
if (!always_use_vrr_tg) {
disable VRR_VTL
wait for VRR status
disable PUSH
}
write(VMAX, intel_vrr_fixed_rr_vmax(crtc_state) - 1);
write(VMIN, intel_vrr_fixed_rr_vmin(crtc_state) - 1);
write(FLIPLINE, intel_vrr_fixed_rr_flipline(crtc_state) - 1);
}
And similarly during modeset enable sequence we should also
always program those fixed timings, then turn on the VRR TG at
an appropriate spot (if always using it), and let the later
vrr_enable() (if necessary) switch to the real VRR timings.
That way it works alsmost the same regardless of whether
whether we always use the VRR TG or not.
Alright, I got the idea. Will try this next.
There is one doubt about intel_vrr_enabling/disabling helper.
Earlier we were using is_{enabling/disabling}(vrr.enable) where
vrr.enable was tracking if variable timing is used. We would still be
tracking here the same thing right ?
Since I have removed vrr.enable and using vrr.mode, the macros:
is_{enabling/disabling}(feature, old_crtc_state, new_crtc_state) wont work.
Should I still keep vrr.enable? Or perhaps modify the conditions in
intel_vrr_enabling/disabling?
The fixed_rr stuff could be written like so (then they would work
for all platforms, if anyone feels like trying this mode of
operation on ICL/TGL as well):
intel_vrr_fixed_rr_vtotal()
{
if (DISPLAY_VER >= 13)
return crtc_vtotal;
else
return crtc_vtotal -
intel_vrr_real_vblank_delay();
}
intel_vrr_fixed_rr_vmax()
{
return intel_vrr_fixed_rr_vtotal();
}
intel_vrr_fixed_rr_vmin()
{
return intel_vrr_fixed_rr_vtotal() -
intel_vrr_flipline_offset();
}
intel_vrr_fixed_rr_flipline()
{
return intel_vrr_fixed_rr_vtotal();
}
This is clear, will use these in vrr_disable.
Regards,
Ankit
- if (crtc_state->vrr.mode == INTEL_VRRTG_MODE_CMRR) {
- intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
- VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE |
- trans_vrr_ctl(crtc_state));
- } else {
- intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
- VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
+ if (!always_use_vrr_tg) {
+ if (intel_vrr_use_push(crtc_state))
+ intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
+ TRANS_PUSH_EN);
+
+ if (crtc_state->vrr.mode == INTEL_VRRTG_MODE_CMRR) {
+ intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
+ VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE |
+ trans_vrr_ctl(crtc_state));
+ } else {
+ intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
+ VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
+ }
}
}
-void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
+void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state, bool always_use_vrr_tg)
{
struct intel_display *display = to_intel_display(old_crtc_state);
enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
@@ -532,12 +536,16 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
if (!intel_vrrtg_is_enabled(old_crtc_state))
return;
- intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
- trans_vrr_ctl(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);
+ if (!always_use_vrr_tg) {
+ intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
+ trans_vrr_ctl(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_vrr_set_transcoder_timings(old_crtc_state);
}
void intel_vrr_get_config(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 8d53aab3590d..da6a86cee0e8 100644
--- a/drivers/gpu/drm/i915/display/intel_vrr.h
+++ b/drivers/gpu/drm/i915/display/intel_vrr.h
@@ -22,11 +22,11 @@ void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
struct drm_connector_state *conn_state);
void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
-void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
+void intel_vrr_enable(const struct intel_crtc_state *crtc_state, bool always_use_vrr_tg);
void intel_vrr_send_push(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_disable(const struct intel_crtc_state *old_crtc_state, bool always_use_vrr_tg);
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);
--
2.45.2
