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

[Jani Nikula <jani.nikula@linux.intel.com>]

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?

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?

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