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

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