Re: [PATCH 00/23] Use VRR timing generator for fixed refresh rate modes

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

On 1/22/2025 6:38 PM, Ville Syrjälä wrote:
> On Wed, Jan 22, 2025 at 04:08:07PM +0530, Nautiyal, Ankit K wrote:
>> On 12/13/2024 11:01 PM, Ville Syrjälä wrote:
>>> On Mon, Nov 11, 2024 at 02:41:58PM +0530, Ankit Nautiyal wrote:
>>>> Even though the VRR timing generator (TG) is primarily used for
>>>> variable refresh rates, it can be used for fixed refresh rates as
>>>> well. For a fixed refresh rate the Flip Line and Vmax must be equal
>>>> (TRANS_VRR_FLIPLINE = TRANS_VRR_VMAX). Beyond that, there are some
>>>> dependencies between the VRR timings and the legacy timing generator
>>>> registers.
>>>>
>>>> This series is an attempt to use VRR TG for fixed refresh rate.
>>>> For platforms XE2LPD+, always go with VRR timing generator for both fixed and
>>>> variable refresh rate cases.
>>> Some ideas I had while thinking about this:
>>>
>>> - perhaps the most fundemental decision we have to make is how do
>>>     we compute the vrr state in each case. My first idea was to
>>>     stick to the current way of always computing it as if vrr is enabled,
>>>     but that complicates the state checker needlessly, so I'm actually
>>>     leaning towards always computing the vrr state based on actual
>>>     uapi.vrr_enable knob. So when that knob is disabled we always compute
>>>     vmin=flipline=vmax. We're going to anyway have to repgrogram all those
>>>     registers when toggle VRR anyway.
>>>
>>> - intel_vrr_{enable,disable}() should just flip between the fixed and
>>>     variable timings in vmin/flipline/vmax. I think we should just do this
>>>     for all the platforms, regarless of whether we also toggle the VRR_CTL
>>>     enable bit there. This minimizes the differences between the two
>>>     approaches fairly well, and it should also work well with the previous
>>>     idea of computing the vrr state based on uapi.vrr_enable. I'm still
>>>     a bit uneasy wrt. repgramming the timings on the fly since none of
>>>     the registers are double buffered. So not entirely sure if we need
>>>     to spend some brain cells on coming up with some kind of safe order
>>>     of writing the registers or not.
>>>
>>> - guardbad/pipeline full probably can't be reprogrammed on the fly,
>>>     so we need to make sure it satisfies both the fixed and variable
>>>     timings. I think we should probably just always set vmin=crtc_vtotal
>>>     instead of the using the current refresh rate based approach. That
>>>     way we never need to change anything to do with the guardband.
>>>
>>> - I was initially thinking we chould jsue the PUSH_EN bit to
>>>     differentiate between variable and fixed timings, but at least tgl
>>>     and adl refuse to play ball and the display just blanks out if you
>>>     attempt to use the VRR timing generator without enabling push.
>>>     So probably we'll just need make the distinction based on
>>>     flipline==vmax.
>>>
>>> - cmrr I've not really though about. It'll still involve frobbing the
>>>     VRR_CTL and whatnot (which I think might be double buffered unlike
>>>     all the other VRR registers). So not sure how to make sure the changes
>>>     beween the modes are done safely. I think easiest to just never use
>>>     cmrr for now, and we can figure it out later.
>>>
>>> So maybe something along the lines of:
>>> 1) do the vmin==crtc_vtotal change
>>> 2) neuter cmrr
>>> 2) change the state computation and
>>>      add vmin/vmax/flipline reprogramming to vrr_{enable,disable}()
>>>      (need to make sure we can still do the fastset vrr toggle)
>> I am getting some confusion around the vrr_enable and vrr_disable
>> functions, with respect to platforms prior to MTL.
>>
>> For prior platforms TGL-ADL, where we do not want to have fixed timings,
>> what happens to them when uapi.vrr_enable is not set.
>>
>> Do we intend to use the same logic as was earlier where we compute vrr
>> timings and write them anyway without enabling VRR?
> I'm thinking we could use basically the same logic
> for all platforms. Something like this:
>
> compute_config()
> {
> 	if (can do vrr && vrr.uapi.enabled)
> 		compute vrr timings into crtc_state.vrr
> 	else
> 		compute fixed timings into crtc_state.vrr
> }
>
> vrr_enable()
> {
> 	write VMAX/VMIN/FLIPINE with vrr timings from crtc_state.vrr
>
> 	if (!always_use_vrr_tg) {
> 		enable PUSH
> 		enable VRR_CTL
> 		// probably wait for vrr live status==enabled here
> 	}
> }
>
> vrr_disable()
> {
> 	if (!always_use_vrr_tg) {
> 		disable VRR_CTL
> 		// wait for vrr live status==disabled here
> 		disable PUSH
> 	}
>
> 	write VMAX/VMIN/FLIPINE with fixed timings
> }
>
> That way the behaviour is consistent between all the
> platforms (apart from the push/vrr enable bits), the state
> checker shouldn't need any changes AFAICS, and it's trivial
> to change which platforms use the always_use_vrr_tg approach
> (should we need to do so).
>
> Or do you see any problems with that approach?

Thanks for clearing this. I think we can work with the above approach.

I am trying to work with something based on above approach, but 
currently I am seeing issues with PSR2 with VRR fixed_rr.

Switching between VRR and Fixed RR is fine though.

  Perhaps I am missing something around PSR.

Will continue the testing and will post the patches in couple of days.

Thanks again for quick response.

Regards,
Ankit

>