[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
Mon Feb 3 14:19:33 UTC 2025
On 1/30/2025 4:38 PM, Nautiyal, Ankit K wrote:
>
> 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?
For this I have modified the intel_crtc_vrr_enabling/disabling in the
new version:
https://patchwork.freedesktop.org/patch/635126/?series=141152&rev=3
Regards,
Ankit
>
>
>>
>> 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-gfx
mailing list