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

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

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.

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