Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221031064918.14178-1-tiwai@suse.de>
In-Reply-To: <20221031064918.14178-1-tiwai@suse.de>
From:   Alex Deucher <alexdeucher@gmail.com>
Date:   Tue, 1 Nov 2022 10:29:40 -0400
Message-ID: <CADnq5_NVo8wXXBx+iMMFAvuxr77t96fXF8Krw1_fPUOsOtJMzQ@mail.gmail.com>
Subject: Re: [PATCH] drm/radeon: Use a local mutex for bind/unbind protection
To:     Takashi Iwai <tiwai@suse.de>
Cc:     Alex Deucher <alexander.deucher@amd.com>,
        dri-devel@lists.freedesktop.org,
        Thomas Zimmermann <tzimmermann@suse.de>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Oct 31, 2022 at 2:49 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> We used drm_modeset_lock_all() for protecting the audio component
> bind/unbind operations against the notification, but it seems leading
> to kernel WARNING and Oops.  Moreover, the use of
> drm_modeset_lock_all() is rather an overkill only for this
> protection.
>
> This patch introduces a new mutex that protects just the bind/unbind
> and the notify calls and replaces the drm_modeset_lock with it.
>
> Fixes: 34d84636e5e0 ("drm/radeon: Add HD-audio component notifier support (v4)")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1569
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

Applied.  I will squash this in with 34d84636e5e0.

Alex

> ---
>  drivers/gpu/drm/radeon/radeon.h        |  1 +
>  drivers/gpu/drm/radeon/radeon_audio.c  | 21 ++++++++++++---------
>  drivers/gpu/drm/radeon/radeon_device.c |  1 +
>  3 files changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index d82424525f5a..2e7161acd443 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -1799,6 +1799,7 @@ struct r600_audio {
>         struct radeon_audio_basic_funcs *funcs;
>         struct drm_audio_component *component;
>         bool component_registered;
> +       struct mutex component_mutex;
>  };
>
>  /*
> diff --git a/drivers/gpu/drm/radeon/radeon_audio.c b/drivers/gpu/drm/radeon/radeon_audio.c
> index 4ceb90556127..71b67d4efe08 100644
> --- a/drivers/gpu/drm/radeon/radeon_audio.c
> +++ b/drivers/gpu/drm/radeon/radeon_audio.c
> @@ -181,8 +181,7 @@ static struct radeon_audio_funcs dce6_dp_funcs = {
>         .dpms = evergreen_dp_enable,
>  };
>
> -static void radeon_audio_component_notify(struct drm_audio_component *acomp,
> -                                         int port);
> +static void radeon_audio_component_notify(struct radeon_device *rdev, int port);
>
>  static void radeon_audio_enable(struct radeon_device *rdev,
>                                 struct r600_audio_pin *pin, u8 enable_mask)
> @@ -212,7 +211,7 @@ static void radeon_audio_enable(struct radeon_device *rdev,
>         if (rdev->audio.funcs->enable)
>                 rdev->audio.funcs->enable(rdev, pin, enable_mask);
>
> -       radeon_audio_component_notify(rdev->audio.component, pin->id);
> +       radeon_audio_component_notify(rdev, pin->id);
>  }
>
>  static void radeon_audio_interface_init(struct radeon_device *rdev)
> @@ -731,12 +730,16 @@ unsigned int radeon_audio_decode_dfs_div(unsigned int div)
>  /*
>   * Audio component support
>   */
> -static void radeon_audio_component_notify(struct drm_audio_component *acomp,
> -                                         int port)
> +static void radeon_audio_component_notify(struct radeon_device *rdev, int port)
>  {
> +       struct drm_audio_component *acomp;
> +
> +       mutex_lock(&rdev->audio.component_mutex);
> +       acomp = rdev->audio.component;
>         if (acomp && acomp->audio_ops && acomp->audio_ops->pin_eld_notify)
>                 acomp->audio_ops->pin_eld_notify(acomp->audio_ops->audio_ptr,
>                                                  port, -1);
> +       mutex_unlock(&rdev->audio.component_mutex);
>  }
>
>  static int radeon_audio_component_get_eld(struct device *kdev, int port,
> @@ -787,11 +790,11 @@ static int radeon_audio_component_bind(struct device *kdev,
>         if (WARN_ON(!device_link_add(hda_kdev, kdev, DL_FLAG_STATELESS)))
>                 return -ENOMEM;
>
> -       drm_modeset_lock_all(dev);
> +       mutex_lock(&rdev->audio.component_mutex);
>         acomp->ops = &radeon_audio_component_ops;
>         acomp->dev = kdev;
>         rdev->audio.component = acomp;
> -       drm_modeset_unlock_all(dev);
> +       mutex_unlock(&rdev->audio.component_mutex);
>
>         return 0;
>  }
> @@ -805,11 +808,11 @@ static void radeon_audio_component_unbind(struct device *kdev,
>
>         device_link_remove(hda_kdev, kdev);
>
> -       drm_modeset_lock_all(dev);
> +       mutex_lock(&rdev->audio.component_mutex);
>         rdev->audio.component = NULL;
>         acomp->ops = NULL;
>         acomp->dev = NULL;
> -       drm_modeset_unlock_all(dev);
> +       mutex_unlock(&rdev->audio.component_mutex);
>  }
>
>  static const struct component_ops radeon_audio_component_bind_ops = {
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c
> index 17bfbbe906c8..2e13ce2b65d3 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1312,6 +1312,7 @@ int radeon_device_init(struct radeon_device *rdev,
>         mutex_init(&rdev->pm.mutex);
>         mutex_init(&rdev->gpu_clock_mutex);
>         mutex_init(&rdev->srbm_mutex);
> +       mutex_init(&rdev->audio.component_mutex);
>         init_rwsem(&rdev->pm.mclk_lock);
>         init_rwsem(&rdev->exclusive_lock);
>         init_waitqueue_head(&rdev->irq.vblank_queue);
> --
> 2.35.3
>