RE: [PATCH 1/1] drm/i915/dmc: fix assert_dmc_loaded WARN during async firmware load

[Gustavo Sousa <gustavo.sousa@intel.com>]

"Saarinen, Jani" <jani.saarinen@intel.com> writes:

> Hi, 
>
>> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Gustavo
>> Sousa
>> Sent: Thursday, 7 May 2026 17.40
>> To: Xiong, James <james.xiong@intel.com>; intel-xe@lists.freedesktop.org
>> Cc: Xiong, James <james.xiong@intel.com>
>> Subject: Re: [PATCH 1/1] drm/i915/dmc: fix assert_dmc_loaded WARN during
>> async firmware load
>> 
>> James Xiong <james.xiong@intel.com> writes:
>> 
>> > 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, it sees
>> > parsed payload but stale HW registers, triggering a ~20% intermittent
>> > WARNING on ADL-N warm boot.
>> >
>> > Fix this by adding a 'loaded' flag to struct intel_dmc, set with
>> > WRITE_ONCE() after intel_dmc_load_program() completes. Check it with
>> > READ_ONCE() in intel_dmc_enable_pipe() and intel_dmc_disable_pipe() as
>> > an additional guard before has_dmc_id_fw().
>> 
>> Making intel_dmc_enable_pipe() bail if the DMC is not yet loaded doesn't
>> look like the correct solution here.
>
> Should this be also sent to intel-gfx to get tested on i915 systems?

Agreed. ADL is supported on i915.

--
Gustavo Sousa

>> 
>> Does the warning come from intel_modeset_setup_hw_state()'s call stack?
>> If so, I wonder if adding a call to intel_dmc_wait_fw_load() before iterating
>> the crtcs would be a saner approach.
>> 
>> --
>> Gustavo Sousa
>> 
>> >
>> > Signed-off-by: James Xiong <james.xiong@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_dmc.c | 16 ++++++++++++++--
>> >  1 file changed, 14 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c
>> > b/drivers/gpu/drm/i915/display/intel_dmc.c
>> > index 0df4f42ba3e3..c51103515820 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_dmc.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>> > @@ -79,6 +79,7 @@ struct intel_dmc {
>> >  		u32 *payload;
>> >  		bool present;
>> >  	} dmc_info[DMC_FW_MAX];
>> > +	bool loaded;
>> >  };
>> >
>> >  /* Note: This may be NULL. */
>> > @@ -417,6 +418,13 @@ bool intel_dmc_has_payload(struct intel_display
>> *display)
>> >  	return has_dmc_id_fw(display, DMC_FW_MAIN);  }
>> >
>> > +static bool intel_dmc_loaded(struct intel_display *display) {
>> > +	struct intel_dmc *dmc = display_to_dmc(display);
>> > +
>> > +	return dmc && READ_ONCE(dmc->loaded); }
>> > +
>> >  static void initialize_stepping_info(struct intel_display *display,
>> > struct stepping_info *si)  {
>> >  	const char *step_name = DISPLAY_RUNTIME_INFO(display)-
>> >step_name;
>> > @@ -786,7 +794,8 @@ void intel_dmc_enable_pipe(const struct
>> intel_crtc_state *crtc_state)
>> >  	enum pipe pipe = crtc->pipe;
>> >  	enum intel_dmc_id dmc_id = PIPE_TO_DMC_ID(pipe);
>> >
>> > -	if (!is_valid_dmc_id(dmc_id) || !has_dmc_id_fw(display, dmc_id))
>> > +	if (!is_valid_dmc_id(dmc_id) || !intel_dmc_loaded(display) ||
>> > +	    !has_dmc_id_fw(display, dmc_id))
>> >  		return;
>> >
>> >  	if (!can_enable_pipedmc(crtc_state)) { @@ -821,7 +830,8 @@ void
>> > intel_dmc_disable_pipe(const struct intel_crtc_state *crtc_state)
>> >  	enum pipe pipe = crtc->pipe;
>> >  	enum intel_dmc_id dmc_id = PIPE_TO_DMC_ID(pipe);
>> >
>> > -	if (!is_valid_dmc_id(dmc_id) || !has_dmc_id_fw(display, dmc_id))
>> > +	if (!is_valid_dmc_id(dmc_id) || !intel_dmc_loaded(display) ||
>> > +	    !has_dmc_id_fw(display, dmc_id))
>> >  		return;
>> >
>> >  	if (DISPLAY_VER(display) >= 14)
>> > @@ -942,6 +952,8 @@ void intel_dmc_load_program(struct intel_display
>> *display)
>> >  	gen9_set_dc_state_debugmask(display);
>> >
>> >  	pipedmc_clock_gating_wa(display, false);
>> > +
>> > +	WRITE_ONCE(display_to_dmc(display)->loaded, true);
>> >  }
>> >
>> >  /**
>> > --
>> > 2.34.1