Re: [PATCH 31/35] drm/i915/vrr: Always set vrr vmax/vmin/flipline in vrr_{enable/disable}

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

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@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