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?
--
Ville Syrjälä
Intel
