Re: [PATCH] drm/i915: Fix audio component initialization

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

On Tue, 21 May 2024, Imre Deak <imre.deak@intel.com> wrote:
> After registering the audio component in i915_audio_component_init()
> the audio driver may call i915_audio_component_get_power() via the
> component ops. This could program AUD_FREQ_CNTRL with an uninitialized
> value if the latter function is called before display.audio.freq_cntrl
> gets initialized. The get_power() function also does a modeset which in
> the above case happens too early before the initialization step and
> triggers the
>
> "Reject display access from task"
>
> error message added by the Fixes: commit below.
>
> Fix the above issue by registering the audio component only after the
> initialization step.
>
> Fixes: bd738d859e71 ("drm/i915: Prevent modesets during driver init/shutdown")

I think the race condition exists before that commit, actually.

Already commit 87c1694533c9 ("drm/i915: save AUD_FREQ_CNTRL state at
audio domain suspend") adds freq_cntrl init after component register,
and the order should be different, right?

> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10291
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_audio.c    | 32 ++++++++++++-------
>  drivers/gpu/drm/i915/display/intel_audio.h    |  1 +
>  .../drm/i915/display/intel_display_driver.c   |  2 ++
>  3 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index adde87900557f..4c031e97f9a55 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -1267,17 +1267,6 @@ static const struct component_ops i915_audio_component_bind_ops = {
>  static void i915_audio_component_init(struct drm_i915_private *i915)
>  {
>  	u32 aud_freq, aud_freq_init;
> -	int ret;
> -
> -	ret = component_add_typed(i915->drm.dev,
> -				  &i915_audio_component_bind_ops,
> -				  I915_COMPONENT_AUDIO);
> -	if (ret < 0) {
> -		drm_err(&i915->drm,
> -			"failed to add audio component (%d)\n", ret);
> -		/* continue with reduced functionality */
> -		return;
> -	}
>  
>  	if (DISPLAY_VER(i915) >= 9) {
>  		aud_freq_init = intel_de_read(i915, AUD_FREQ_CNTRL);
> @@ -1300,6 +1289,21 @@ static void i915_audio_component_init(struct drm_i915_private *i915)
>  
>  	/* init with current cdclk */
>  	intel_audio_cdclk_change_post(i915);
> +}
> +
> +static void i915_audio_component_register(struct drm_i915_private *i915)
> +{
> +	int ret;
> +
> +	ret = component_add_typed(i915->drm.dev,
> +				  &i915_audio_component_bind_ops,
> +				  I915_COMPONENT_AUDIO);
> +	if (ret < 0) {
> +		drm_err(&i915->drm,
> +			"failed to add audio component (%d)\n", ret);
> +		/* continue with reduced functionality */
> +		return;
> +	}
>  
>  	i915->display.audio.component_registered = true;
>  }
> @@ -1332,6 +1336,12 @@ void intel_audio_init(struct drm_i915_private *i915)
>  		i915_audio_component_init(i915);
>  }
>  
> +void intel_audio_register(struct drm_i915_private *i915)
> +{
> +	if (!i915->display.audio.lpe.platdev)
> +		i915_audio_component_register(i915);
> +}
> +
>  /**
>   * intel_audio_deinit() - deinitialize the audio driver
>   * @i915: the i915 drm device private data
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.h b/drivers/gpu/drm/i915/display/intel_audio.h
> index 9327954b801e5..576c061d72a45 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.h
> +++ b/drivers/gpu/drm/i915/display/intel_audio.h
> @@ -28,6 +28,7 @@ void intel_audio_codec_get_config(struct intel_encoder *encoder,
>  void intel_audio_cdclk_change_pre(struct drm_i915_private *dev_priv);
>  void intel_audio_cdclk_change_post(struct drm_i915_private *dev_priv);
>  void intel_audio_init(struct drm_i915_private *dev_priv);
> +void intel_audio_register(struct drm_i915_private *i915);
>  void intel_audio_deinit(struct drm_i915_private *dev_priv);
>  void intel_audio_sdp_split_update(const struct intel_crtc_state *crtc_state);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 89bd032ed995e..794b4af380558 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -540,6 +540,8 @@ void intel_display_driver_register(struct drm_i915_private *i915)
>  
>  	intel_display_driver_enable_user_access(i915);
>  
> +	intel_audio_register(i915);
> +

It's a bit silly that intel_display_driver_register() now calls both
intel_audio_init() and intel_audio_register(). We should probably move
the init earlier. The register part shouldn't really be doing any
hardware initialization stuff, just expose the software interfaces to
the world.

Regardless,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>  	intel_display_debugfs_register(i915);
>  
>  	/*

-- 
Jani Nikula, Intel