Re: [PATCH v2] drm/i915/dmc: fix assert_dmc_loaded WARN during async firmware load

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

Jani Nikula <jani.nikula@linux.intel.com> writes:

> On Fri, 08 May 2026, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
>> Jani Nikula <jani.nikula@linux.intel.com> writes:
>>
>>> On Thu, 07 May 2026, James Xiong <james.xiong@intel.com> 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 <james.xiong@intel.com>
>>>> ---
>>>>  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