[PATCH 31/35] drm/i915/vrr: Always set vrr vmax/vmin/flipline in vrr_{enable/disable}
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Thu Jan 30 11:08:08 UTC 2025
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 at intel.com>
>> ---
>> 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
More information about the Intel-xe
mailing list