On Wed, Feb 19, 2025 at 07:27:00PM +0530, Nautiyal, Ankit K wrote:
>
> On 2/18/2025 12:26 AM, Ville Syrjälä wrote:
> > On Mon, Feb 17, 2025 at 08:34:39PM +0200, Ville Syrjälä wrote:
> >> On Fri, Feb 14, 2025 at 05:41:26PM +0530, Ankit Nautiyal wrote:
> >>> During modeset enable sequence, program the fixed timings,
> >>> and turn on the VRR Timing Generator (VRR TG) for platforms
> >>> that always use VRR TG.
> >>>
> >>> Later if vrr timings are required, vrr_enable() will switch
> >>> to the real VRR timings.
> >>>
> >>> With this we dont want to reset TRANS_CTL_VRR Enable bit while updating
> >>> the vrr transcoder timings.
> >>>
> >>> v2: Update intel_vrr_set_transcoder_timings for fixed_rr.
> >>> v3: Update intel_set_transcoder_timings_lrr for fixed_rr. (Ville)
> >>> Add TRANS_PUSH while enabling VRR for fixed_rr.
> >>>
> >>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> >>> ---
> >>> drivers/gpu/drm/i915/display/intel_display.c | 10 +++++--
> >>> drivers/gpu/drm/i915/display/intel_vrr.c | 29 ++++++++++++++++++--
> >>> drivers/gpu/drm/i915/display/intel_vrr.h | 3 ++
> >>> 3 files changed, 36 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> >>> index 66c5ad46bfea..c9d1c091b109 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> >>> @@ -1755,6 +1755,7 @@ static void hsw_set_frame_start_delay(const struct intel_crtc_state *crtc_state)
> >>>
> >>> static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_state)
> >>> {
> >>> + struct intel_display *display = to_intel_display(crtc_state);
> >>> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> >>> @@ -1770,9 +1771,12 @@ static void hsw_configure_cpu_transcoder(const struct intel_crtc_state *crtc_sta
> >>> }
> >>>
> >>> intel_set_transcoder_timings(crtc_state);
> >>> - if (HAS_VRR(dev_priv))
> >>> - intel_vrr_set_transcoder_timings(crtc_state);
> >>> -
> >>> + if (HAS_VRR(dev_priv)) {
> >>> + if (intel_vrr_always_use_vrr_tg(display))
> >>> + intel_vrr_enable_fixed_rr_timings(crtc_state);
> >>> + else
> >>> + intel_vrr_set_transcoder_timings(crtc_state);
> >> I think intel_vrr_set_transcoder_timings() should just program the
> >> fixed timings always. And we shouldn't do anything else here.
> >>
> >> And I think the VRR_CTL enable/disable for the always_use_vrr_tg==true
> >> case should be done at some more appropriate spot in the modeset
> >> sequence. We can add eg. intel_vrr_trancoder_{enable,disable}() for
> >> that part.
> >>
> >>> + }
> >>> if (cpu_transcoder != TRANSCODER_EDP)
> >>> intel_de_write(dev_priv, TRANS_MULT(dev_priv, cpu_transcoder),
> >>> crtc_state->pixel_multiplier - 1);
> >>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> >>> index e247055bc486..4151fcd0f978 100644
> >>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> >>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> >>> @@ -456,8 +456,12 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
> >>> crtc_state->vrr.vmin - 1);
> >>> intel_de_write(display, TRANS_VRR_VMAX(display, cpu_transcoder),
> >>> crtc_state->vrr.vmax - 1);
> >>> - intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> >>> - trans_vrr_ctl(crtc_state));
> >>> + if (intel_vrr_always_use_vrr_tg(display))
> >>> + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> >>> + VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
> >>> + else
> >>> + intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> >>> + trans_vrr_ctl(crtc_state));
> >> We should probably remove this VRR_CTL frobbing from here entirely,
> >> and just always do it from the intel_vrr_trancoder_{enable,disable}().
> >> And obviously for the !always_use_vrr_tg case we just skip setting
> >> the enable bit there.
> > Hmm. One thing that might screw us over here is the fastboot vs.
> > vblank_delay stuff. We might have to allow that to reconfigure
> > the guardband...
> >
> > In that case we need to reprogram VRR_CTL from
> > intel_set_transcoder_timings_lrr(), and be careful what
> > we do with the enable bit. So it'd perhaps have to be
> > something like:
> > vrr_ctl = trans_vrr_ctl();
> > if (always_use_vrr_tg && !needs_modeset)
> > vrr_ctl |= VRR_CTL_VRR_ENABLE;
>
> I understand while using fastset for platforms that use vrr TG, we dont
> want to reset the VRR_ENABLE while setting the guardband.
>
> We want to use the function intel_vrr_transcoder_enable to handle bits
> for VRR_CTL.
>
> Do you mean to pass need_modeset flag to intel_vrr_transcoder_enable()?
intel_vrr_transcoder_{enable,disable}() should just be
intel_vrr_transcoder_enable()
{
if (!always_use_vrr_tg)
return;
enable vrr tg
}
intel_vrr_transcoder_disable()
{
if (!always_use_vrr_tg)
return;
disable vrr tg
}
and they should only be called from the full modeset
path.
--
Ville Syrjälä
Intel
