Jani Nikula <[email protected]> writes: > On Fri, 08 May 2026, Gustavo Sousa <[email protected]> wrote: >> Jani Nikula <[email protected]> writes: >> >>> On Thu, 07 May 2026, James Xiong <[email protected]> wrote: >>>> During driver probe, DMC firmware is loaded asynchronously via a >>>> workqueue. There is a race between parse_dmc_fw() setting the payload >>>> pointer (making has_dmc_id_fw() return true) and intel_dmc_load_program() >>>> writing the firmware to hardware registers. If the probe thread calls >>>> intel_dmc_enable_pipe() -> assert_dmc_loaded() in this window via >>>> intel_modeset_setup_hw_state(), it sees parsed payload but stale HW >>>> registers, triggering a ~20% intermittent WARNING on ADL-N warm boot. >>> >>> I wonder if intel_dmc_enable_pipe() should call intel_dmc_wait_fw_load() >>> itself? >> >> I also though about that, but, if you need to call >> intel_dmc_wait_fw_load() in a regular modeset flow, wouldn't that be a >> symptom of a bug? > > Perhaps you could theoretically end up in that situation if you have no > pipes enabled at probe, and the first modeset comes before the firmware > has finished loading? > > Similarly, why should we unconditionally wait for DMC firmware load here > if there are no pipes enabled at probe and we could just plunge on?
Hm... That's a good point: we would be adding an unnecessary wait in that scenario. As a second thought, I guess making the call in intel_dmc_enable_pipe() is a safe approach, and arguably makes more sense: it is that function that expects the DMC to be loaded anyway. > > But primarily I'm thinking about the maintenance of all the dependencies > here. We have so many of these subtle ordering dependencies, especially > at probe. Can we not add more? Speaking of which, I wonder if DMC being loaded concurrently with a modeset sequence could cause issues. I don't remember from the top of my head: do we have even try to prevent that? -- Gustavo Sousa > > I'm not adamant about either approach, just expressing my gut feelings. > > > BR, > Jani. > > > >> >> -- >> Gustavo Sousa >> >>> >>>> >>>> v2: Fix by calling intel_dmc_wait_fw_load() in >>>> intel_modeset_setup_hw_state() before iterating the CRTCs (Gustavo >>>> Sousa). >>>> >>>> Signed-off-by: James Xiong <[email protected]> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_modeset_setup.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c >>>> b/drivers/gpu/drm/i915/display/intel_modeset_setup.c >>>> index e88082c8caac..277e56848470 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c >>>> @@ -961,6 +961,8 @@ void intel_modeset_setup_hw_state(struct intel_display >>>> *display, >>>> * intel_sanitize_plane_mapping() may need to do vblank >>>> * waits, so we need vblank interrupts restored beforehand. >>>> */ >>>> + intel_dmc_wait_fw_load(display); >>>> + >>> >>> No matter what, the comment above now appears to refer to this >>> intel_dmc_wait_fw_load() call, which makes no sense at all. >>> >>> BR, >>> Jani. >>> >>>> for_each_intel_crtc(display->drm, crtc) { >>>> struct intel_crtc_state *crtc_state = >>>> to_intel_crtc_state(crtc->base.state); >>> >>> -- >>> Jani Nikula, Intel > > -- > Jani Nikula, Intel
