On Thu, May 23, 2024 at 12:41:32PM +0300, Jani Nikula wrote:
> 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?
The audio side initialization sequence has changed since 87c1694533c9,
but yes the incorrect (rare) sequence could've happened the same way
already at that point. I'll change the Fixes: line.
> > 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.
Yes, agreed this could be done as a follow-up after this change.
> Regardless,
>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
Thanks.
>
> > intel_display_debugfs_register(i915);
> >
> > /*
>
> --
> Jani Nikula, Intel
