On Tue, 21 May 2024, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> intel_display_debugfs_register(i915);
>
> /*
--
Jani Nikula, Intel
