Re: [PATCH v4 2/2] drm/i915/display: Sync state to BIOS for seamless handoff

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 01, 2026 at 08:44:32PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 01, 2026 at 06:19:00PM +0200, Maarten Lankhorst wrote:
> > Hey,
> > 
> > Den 2026-03-27 kl. 11:11, skrev Ville Syrjälä:
> > > On Thu, Mar 26, 2026 at 04:10:13PM -0700, Juasheem Sultan wrote:
> > >> Hi,
> > >>
> > >> Thanks for looking at this.
> > >>
> > >> You're saying instead of manually adopting the state that I should focus on
> > >> modifying the comparisons that we do to determine if we can perform a
> > >> fastset?
> > > 
> > > We don't want any fuzzy fastset hacks anywhere. I intentionally killed
> > > all that stuff because it was making it impossible to trust that the
> > > software state actually represents what the hardware is doing.
> > > 
> > > Someone needs to figure out what exactly is the difference between
> > > the states between the GOP and the driver, and then figure out where
> > > that difference is coming from.
> > 
> > In some cases the firmware may have different considerations for setting a mode,
> > not necesssarily worse than what the display driver is using, just different.
> > 
> > In this case userspace requests a mode for example 1920x1080, but it doesn't
> > particularly care what pll's are used or anything nor has it a way to specify
> > it at all.
> > 
> > Until there is a major change like disabling the screen, what are the downsides
> > taking over the inherited mode from the firmware, and postponing changing the
> > display mode?
> 
> Because all the hacks we had for this of the form:
> 1. fully compute a new state
> 2. randomly overwrite bits and pieces of that with the 
>    old state
> 3. somehow hope that whatever got overwritten isn't incompatible
>    with all the stuff that wasn't overwritten
> 
> It was a completely unmaintainable mess that could result in all
> kinds of weird bugs.
> 
> And to be clear, we *do* take the state the firmware was using, and
> ideally we don't recompute it at all. It's only because we lack certain
> readout support/etc that we sometimes punt to the full computation and
> therefore we may end up with a fastset mismatch (see the
> .initial_fastset_check() stuff). The correct way to solve that is to
> add the missing readout support, not add more hacks.

I suppose I should say that only really covers in the initial_commit().

I do admit that we may have a bit of a problem with the initial
userspace commit (when crtc_state->inherited is cleared). In the past
that was required at least to fix audio, but audio shouldn't really need
that anymore since I added fastset support for it.

What could be attempted is to move the audio state computation into
.compute_config_late(), and if userspace didn't flag a modeset on the
first commit, then we'd skip the full .compute_config() and just do
the _late() part. Afterwards the normal audio fastset should kick in.
That's assuming audio isn't a dependency of some other stuff in
.compute_config().

And of course we'd have to think whether we have some other features
besides audio that users actually want to work, but we only
enable/disable them in response to the full .compute_config()...

> 
> > 
> > Having a flicker-free boot is a desired feature to have, and something intel
> > handles a lot better than the other drivers.
> > 
> > Kind regards,
> > ~Maarten Lankhorst
> > 
> > >>
> > >> -Juasheem
> > >>
> > >> On Wed, Mar 18, 2026 at 5:00 AM Maarten Lankhorst <dev@lankhorst.se> wrote:
> > >>
> > >>> Hey,
> > >>>
> > >>> Den 2026-03-17 kl. 23:09, skrev Juasheem Sultan:
> > >>>> Align DP timings and C10 PLL state with BIOS values if within a 0.5%
> > >>>> clock threshold. This prevents minor mismatches from triggering a full
> > >>>> modeset during the first atomic commit, ensuring a flicker-free handoff.
> > >>>>
> > >>>> Signed-off-by: Juasheem Sultan <jdsultan@google.com>
> > >>>> ---
> > >>>>  drivers/gpu/drm/i915/display/intel_display.c | 67 ++++++++++++++++++++
> > >>>>  1 file changed, 67 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > >>> b/drivers/gpu/drm/i915/display/intel_display.c
> > >>>> index c4246481fc2f..22e5e931f134 100644
> > >>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
> > >>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > >>>> @@ -6397,6 +6397,71 @@ static int
> > >>> intel_atomic_check_config_and_link(struct intel_atomic_state *state)
> > >>>>
> > >>>>       return ret;
> > >>>>  }
> > >>>> +
> > >>>> +// Helper function to sanitize pll state
> > >>>> +static void intel_sanitize_pll_state(struct intel_crtc_state
> > >>> *old_crtc_state,
> > >>>> +             struct intel_crtc_state *new_crtc_state)
> > >>>> +{
> > >>>> +     int j;
> > >>>> +
> > >>>> +     for (j = 4; j < 9; j++) {
> > >>>> +             if (new_crtc_state->dpll_hw_state.cx0pll.c10.pll[j] !=
> > >>>> +
> > >>>  old_crtc_state->dpll_hw_state.cx0pll.c10.pll[j]) {
> > >>>> +                     new_crtc_state->dpll_hw_state.cx0pll.c10.pll[j] =
> > >>>> +
> > >>>  old_crtc_state->dpll_hw_state.cx0pll.c10.pll[j];
> > >>>> +             }
> > >>>> +     }
> > >>>> +}
> > >>>> +
> > >>>> +/*
> > >>>> + * intel_dp_sanitize_seamless_boot - Snap driver state to BIOS state
> > >>> for seamless handoff.
> > >>>> + * @state: the atomic state to sanitize
> > >>>> + *
> > >>>> + * This function compares the driver's calculated new_state with the
> > >>> inherited BIOS state
> > >>>> + * (old_state). If they are within a small threshold (e.g., 0.5% for
> > >>> clock), it "snaps"
> > >>>> + * the new_state to match the BIOS state exactly. This prevents minor
> > >>> state mismatches
> > >>>> + * that would otherwise force a full modeset (and a screen flicker)
> > >>> during the initial
> > >>>> + * kernel handoff.
> > >>>> + */
> > >>>> +static void intel_dp_sanitize_seamless_boot(struct intel_atomic_state
> > >>> *state)
> > >>>> +{
> > >>>> +     struct intel_display *display = to_intel_display(state);
> > >>>> +     struct intel_crtc_state *new_crtc_state, *old_crtc_state;
> > >>>> +     struct intel_crtc *crtc;
> > >>>> +     struct intel_encoder *encoder;
> > >>>> +     int i;
> > >>>> +
> > >>>> +     for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > >>> new_crtc_state, i) {
> > >>>> +             /*
> > >>>> +              * We must check old_crtc_state->inherited because
> > >>> new_crtc_state->inherited
> > >>>> +              * is cleared at the start of intel_atomic_check for
> > >>> userspace commits.
> > >>>> +              */
> > >>>> +             if (!old_crtc_state->inherited ||
> > >>> !new_crtc_state->hw.active)
> > >>>> +                     continue;
> > >>>> +
> > >>>> +             if (intel_crtc_has_dp_encoder(new_crtc_state)) {
> > >>>> +                     int old_clock =
> > >>> old_crtc_state->hw.adjusted_mode.crtc_clock;
> > >>>> +                     int new_clock =
> > >>> new_crtc_state->hw.adjusted_mode.crtc_clock;
> > >>>> +                     int threshold = old_clock / 200; /* 0.5% */
> > >>>> +
> > >>>> +                     if (abs(new_clock - old_clock) <= threshold) {
> > >>>> +                             new_crtc_state->hw.pipe_mode.crtc_clock =
> > >>> old_clock;
> > >>>> +
> > >>>  new_crtc_state->hw.adjusted_mode.crtc_clock = old_clock;
> > >>>> +                             new_crtc_state->pixel_rate =
> > >>> old_crtc_state->pixel_rate;
> > >>>> +                             new_crtc_state->dp_m_n =
> > >>> old_crtc_state->dp_m_n;
> > >>>> +                     }
> > >>>> +             }
> > >>>> +
> > >>>> +             for_each_intel_encoder_mask(display->drm, encoder,
> > >>>> +                             new_crtc_state->uapi.encoder_mask) {
> > >>>> +                     if (intel_encoder_is_c10phy(encoder)) {
> > >>>> +                             if
> > >>> (!new_crtc_state->dpll_hw_state.cx0pll.ssc_enabled)
> > >>>> +
> > >>>  intel_sanitize_pll_state(old_crtc_state, new_crtc_state);
> > >>>> +                     }
> > >>>> +             }
> > >>>> +     }
> > >>>> +}
> > >>>> +
> > >>>>  /**
> > >>>>   * intel_atomic_check - validate state object
> > >>>>   * @dev: drm device
> > >>>> @@ -6447,6 +6512,8 @@ int intel_atomic_check(struct drm_device *dev,
> > >>>>       if (ret)
> > >>>>               goto fail;
> > >>>>
> > >>>> +     intel_dp_sanitize_seamless_boot(state);
> > >>>> +
> > >>>>       for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> > >>>>               if (!intel_crtc_needs_modeset(new_crtc_state))
> > >>>>                       continue;
> > >>>
> > >>> This might fix boot state, but in a way that complicates the code
> > >>> considerably.
> > >>>
> > >>> Have you considered updating intel_pipe_config_compare instead?
> > >>>
> > >>> Kind regards,
> > >>> ~Maarten Lankhorst
> > >>>
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel