* [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
@ 2024-05-01 11:17 Simon Trimmer
2024-05-02 7:34 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Simon Trimmer @ 2024-05-01 11:17 UTC (permalink / raw
To: tiwai; +Cc: linux-sound, alsa-devel, linux-kernel, patches, Simon Trimmer
It is possible that during system boot when there multiple devices
attempting simultaneous initialization on a slow control bus the
download of firmware and tuning data may take a user perceivable amount
of time.
Adopt a pattern used in the ASoC driver and perform this activity in a
background thread so that interactive performance is not impaired.
Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
---
sound/pci/hda/cs35l56_hda.c | 89 ++++++++++++++++++++++++++++++-------
sound/pci/hda/cs35l56_hda.h | 4 ++
2 files changed, 76 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c
index 1a3f84599cb5..042c2407c007 100644
--- a/sound/pci/hda/cs35l56_hda.c
+++ b/sound/pci/hda/cs35l56_hda.c
@@ -13,6 +13,7 @@
#include <linux/pm_runtime.h>
#include <linux/regmap.h>
#include <linux/slab.h>
+#include <linux/workqueue.h>
#include <sound/core.h>
#include <sound/cs-amp-lib.h>
#include <sound/hda_codec.h>
@@ -50,11 +51,19 @@ static const struct reg_sequence cs35l56_hda_dai_config[] = {
};
+static void cs35l56_hda_wait_dsp_ready(struct cs35l56_hda *cs35l56)
+{
+ /* Wait for patching to complete */
+ flush_work(&cs35l56->dsp_work);
+}
+
static void cs35l56_hda_play(struct cs35l56_hda *cs35l56)
{
unsigned int val;
int ret;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
pm_runtime_get_sync(cs35l56->base.dev);
ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_PLAY);
if (ret == 0) {
@@ -180,6 +189,8 @@ static int cs35l56_hda_mixer_get(struct snd_kcontrol *kcontrol,
unsigned int reg_val;
int i;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
regmap_read(cs35l56->base.regmap, kcontrol->private_value, ®_val);
reg_val &= CS35L56_ASP_TXn_SRC_MASK;
@@ -203,6 +214,8 @@ static int cs35l56_hda_mixer_put(struct snd_kcontrol *kcontrol,
if (item >= CS35L56_NUM_INPUT_SRC)
return -EINVAL;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
regmap_update_bits_check(cs35l56->base.regmap, kcontrol->private_value,
CS35L56_INPUT_MASK, cs35l56_tx_input_values[item],
&changed);
@@ -227,6 +240,8 @@ static int cs35l56_hda_posture_get(struct snd_kcontrol *kcontrol,
unsigned int pos;
int ret;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_read(cs35l56->base.regmap, CS35L56_MAIN_POSTURE_NUMBER, &pos);
if (ret)
return ret;
@@ -248,6 +263,8 @@ static int cs35l56_hda_posture_put(struct snd_kcontrol *kcontrol,
(pos > CS35L56_MAIN_POSTURE_MAX))
return -EINVAL;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_update_bits_check(cs35l56->base.regmap,
CS35L56_MAIN_POSTURE_NUMBER,
CS35L56_MAIN_POSTURE_MASK,
@@ -291,6 +308,8 @@ static int cs35l56_hda_vol_get(struct snd_kcontrol *kcontrol,
int vol;
int ret;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_read(cs35l56->base.regmap, CS35L56_MAIN_RENDER_USER_VOLUME, &raw_vol);
if (ret)
@@ -323,6 +342,8 @@ static int cs35l56_hda_vol_put(struct snd_kcontrol *kcontrol,
raw_vol = (vol + CS35L56_MAIN_RENDER_USER_VOLUME_MIN) <<
CS35L56_MAIN_RENDER_USER_VOLUME_SHIFT;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_update_bits_check(cs35l56->base.regmap,
CS35L56_MAIN_RENDER_USER_VOLUME,
CS35L56_MAIN_RENDER_USER_VOLUME_MASK,
@@ -539,8 +560,9 @@ static void cs35l56_hda_release_firmware_files(const struct firmware *wmfw_firmw
kfree(coeff_filename);
}
-static void cs35l56_hda_add_dsp_controls(struct cs35l56_hda *cs35l56)
+static void cs35l56_hda_create_dsp_controls_work(struct work_struct *work)
{
+ struct cs35l56_hda *cs35l56 = container_of(work, struct cs35l56_hda, control_work);
struct hda_cs_dsp_ctl_info info;
info.device_name = cs35l56->amp_name;
@@ -566,7 +588,7 @@ static void cs35l56_hda_apply_calibration(struct cs35l56_hda *cs35l56)
dev_info(cs35l56->base.dev, "Calibration applied\n");
}
-static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
+static void cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
{
const struct firmware *coeff_firmware = NULL;
const struct firmware *wmfw_firmware = NULL;
@@ -574,15 +596,27 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
char *wmfw_filename = NULL;
unsigned int preloaded_fw_ver;
bool firmware_missing;
- int ret = 0;
+ bool add_dsp_controls_required = false;
+ int ret;
- /* Prepare for a new DSP power-up */
+ /*
+ * Prepare for a new DSP power-up. If the DSP has had firmware
+ * downloaded previously then it needs to be powered down so that it
+ * can be updated and if hadn't been patched before then the controls
+ * will need to be added once firmware download succeeds.
+ */
if (cs35l56->base.fw_patched)
cs_dsp_power_down(&cs35l56->cs_dsp);
+ else
+ add_dsp_controls_required = true;
cs35l56->base.fw_patched = false;
- pm_runtime_get_sync(cs35l56->base.dev);
+ ret = pm_runtime_resume_and_get(cs35l56->base.dev);
+ if (ret < 0) {
+ dev_err(cs35l56->base.dev, "Failed to resume and get %d\n", ret);
+ return;
+ }
/*
* The firmware can only be upgraded if it is currently running
@@ -606,7 +640,6 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
*/
if (!coeff_firmware && firmware_missing) {
dev_err(cs35l56->base.dev, ".bin file required but not found\n");
- ret = -ENOENT;
goto err_fw_release;
}
@@ -657,6 +690,15 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
CS35L56_FIRMWARE_MISSING);
cs35l56->base.fw_patched = true;
+ /*
+ * Adding controls is deferred to prevent a lock inversion - ALSA takes
+ * the controls_rwsem when adding a control, the get() / put()
+ * functions of a control are called holding controls_rwsem and those
+ * that depend on running firmware wait for dsp_work() to complete.
+ */
+ if (add_dsp_controls_required)
+ queue_work(cs35l56->dsp_wq, &cs35l56->control_work);
+
ret = cs_dsp_run(&cs35l56->cs_dsp);
if (ret)
dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
@@ -676,15 +718,19 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
coeff_firmware, coeff_filename);
err_pm_put:
pm_runtime_put(cs35l56->base.dev);
+}
- return ret;
+static void cs35l56_hda_dsp_work(struct work_struct *work)
+{
+ struct cs35l56_hda *cs35l56 = container_of(work, struct cs35l56_hda, dsp_work);
+
+ cs35l56_hda_fw_load(cs35l56);
}
static int cs35l56_hda_bind(struct device *dev, struct device *master, void *master_data)
{
struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev);
struct hda_component *comps = master_data;
- int ret;
if (!comps || cs35l56->index < 0 || cs35l56->index >= HDA_MAX_COMPONENTS)
return -EINVAL;
@@ -698,12 +744,9 @@ static int cs35l56_hda_bind(struct device *dev, struct device *master, void *mas
strscpy(comps->name, dev_name(dev), sizeof(comps->name));
comps->playback_hook = cs35l56_hda_playback_hook;
- ret = cs35l56_hda_fw_load(cs35l56);
- if (ret)
- return ret;
+ queue_work(cs35l56->dsp_wq, &cs35l56->dsp_work);
cs35l56_hda_create_controls(cs35l56);
- cs35l56_hda_add_dsp_controls(cs35l56);
#if IS_ENABLED(CONFIG_SND_DEBUG)
cs35l56->debugfs_root = debugfs_create_dir(dev_name(cs35l56->base.dev), sound_debugfs_root);
@@ -720,6 +763,9 @@ static void cs35l56_hda_unbind(struct device *dev, struct device *master, void *
struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev);
struct hda_component *comps = master_data;
+ cancel_work_sync(&cs35l56->dsp_work);
+ cancel_work_sync(&cs35l56->control_work);
+
cs35l56_hda_remove_controls(cs35l56);
#if IS_ENABLED(CONFIG_SND_DEBUG)
@@ -747,6 +793,10 @@ static int cs35l56_hda_system_suspend(struct device *dev)
{
struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev);
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
+ flush_work(&cs35l56->control_work);
+
if (cs35l56->playing)
cs35l56_hda_pause(cs35l56);
@@ -842,11 +892,8 @@ static int cs35l56_hda_system_resume(struct device *dev)
ret = cs35l56_is_fw_reload_needed(&cs35l56->base);
dev_dbg(cs35l56->base.dev, "fw_reload_needed: %d\n", ret);
- if (ret > 0) {
- ret = cs35l56_hda_fw_load(cs35l56);
- if (ret)
- return ret;
- }
+ if (ret > 0)
+ queue_work(cs35l56->dsp_wq, &cs35l56->dsp_work);
if (cs35l56->playing)
cs35l56_hda_play(cs35l56);
@@ -964,6 +1011,14 @@ int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int hid, int id)
mutex_init(&cs35l56->base.irq_lock);
dev_set_drvdata(cs35l56->base.dev, cs35l56);
+ cs35l56->dsp_wq = create_singlethread_workqueue("cs35l56-dsp");
+ if (!cs35l56->dsp_wq) {
+ ret = -ENOMEM;
+ goto err;
+ }
+ INIT_WORK(&cs35l56->dsp_work, cs35l56_hda_dsp_work);
+ INIT_WORK(&cs35l56->control_work, cs35l56_hda_create_dsp_controls_work);
+
ret = cs35l56_hda_read_acpi(cs35l56, hid, id);
if (ret)
goto err;
diff --git a/sound/pci/hda/cs35l56_hda.h b/sound/pci/hda/cs35l56_hda.h
index 464e4aa63cd1..d207e4e0d167 100644
--- a/sound/pci/hda/cs35l56_hda.h
+++ b/sound/pci/hda/cs35l56_hda.h
@@ -14,6 +14,7 @@
#include <linux/firmware/cirrus/cs_dsp.h>
#include <linux/firmware/cirrus/wmfw.h>
#include <linux/regulator/consumer.h>
+#include <linux/workqueue.h>
#include <sound/cs35l56.h>
struct dentry;
@@ -21,6 +22,9 @@ struct dentry;
struct cs35l56_hda {
struct cs35l56_base base;
struct hda_codec *codec;
+ struct work_struct dsp_work;
+ struct work_struct control_work;
+ struct workqueue_struct *dsp_wq;
int index;
const char *system_name;
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
2024-05-01 11:17 [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background Simon Trimmer
@ 2024-05-02 7:34 ` Takashi Iwai
2024-05-02 9:21 ` Richard Fitzgerald
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2024-05-02 7:34 UTC (permalink / raw
To: Simon Trimmer; +Cc: tiwai, linux-sound, alsa-devel, linux-kernel, patches
On Wed, 01 May 2024 13:17:55 +0200,
Simon Trimmer wrote:
> @@ -964,6 +1011,14 @@ int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int hid, int id)
> mutex_init(&cs35l56->base.irq_lock);
> dev_set_drvdata(cs35l56->base.dev, cs35l56);
>
> + cs35l56->dsp_wq = create_singlethread_workqueue("cs35l56-dsp");
> + if (!cs35l56->dsp_wq) {
> + ret = -ENOMEM;
> + goto err;
> + }
Do we really need a dedicated workqueue? In most usages, simple
schedule_work*() works fine and is recommended.
thanks,
Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
2024-05-02 7:34 ` Takashi Iwai
@ 2024-05-02 9:21 ` Richard Fitzgerald
2024-05-02 9:52 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Richard Fitzgerald @ 2024-05-02 9:21 UTC (permalink / raw
To: Takashi Iwai, Simon Trimmer
Cc: tiwai, linux-sound, alsa-devel, linux-kernel, patches
On 02/05/2024 08:34, Takashi Iwai wrote:
> On Wed, 01 May 2024 13:17:55 +0200,
> Simon Trimmer wrote:
>> @@ -964,6 +1011,14 @@ int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int hid, int id)
>> mutex_init(&cs35l56->base.irq_lock);
>> dev_set_drvdata(cs35l56->base.dev, cs35l56);
>>
>> + cs35l56->dsp_wq = create_singlethread_workqueue("cs35l56-dsp");
>> + if (!cs35l56->dsp_wq) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>
> Do we really need a dedicated workqueue? In most usages, simple
> schedule_work*() works fine and is recommended.
>
On a slow I2C bus with 4 amps this work could take over 2 seconds.
That seems too long to be blocking a global system queue. We use a
dedicated queue in the ASoC driver.
Also if we queue work on an ordered (single-threaded) system queue the
firmware won't be downloaded to multiple amps in parallel, so we don't
get the best use of the available bus bandwidth.
>
> thanks,
>
> Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
2024-05-02 9:21 ` Richard Fitzgerald
@ 2024-05-02 9:52 ` Takashi Iwai
2024-05-02 10:17 ` Simon Trimmer
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2024-05-02 9:52 UTC (permalink / raw
To: Richard Fitzgerald
Cc: Simon Trimmer, tiwai, linux-sound, alsa-devel, linux-kernel,
patches
On Thu, 02 May 2024 11:21:36 +0200,
Richard Fitzgerald wrote:
>
> On 02/05/2024 08:34, Takashi Iwai wrote:
> > On Wed, 01 May 2024 13:17:55 +0200,
> > Simon Trimmer wrote:
> >> @@ -964,6 +1011,14 @@ int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int hid, int id)
> >> mutex_init(&cs35l56->base.irq_lock);
> >> dev_set_drvdata(cs35l56->base.dev, cs35l56);
> >> + cs35l56->dsp_wq =
> >> create_singlethread_workqueue("cs35l56-dsp");
> >> + if (!cs35l56->dsp_wq) {
> >> + ret = -ENOMEM;
> >> + goto err;
> >> + }
> >
> > Do we really need a dedicated workqueue? In most usages, simple
> > schedule_work*() works fine and is recommended.
> >
>
> On a slow I2C bus with 4 amps this work could take over 2 seconds.
> That seems too long to be blocking a global system queue. We use a
> dedicated queue in the ASoC driver.
>
> Also if we queue work on an ordered (single-threaded) system queue the
> firmware won't be downloaded to multiple amps in parallel, so we don't
> get the best use of the available bus bandwidth.
OK, that sounds like a sensible argument.
But the patch has no call of a queue destructor. Won't it leak
resources?
thanks,
Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
2024-05-02 9:52 ` Takashi Iwai
@ 2024-05-02 10:17 ` Simon Trimmer
2024-05-02 10:25 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: Simon Trimmer @ 2024-05-02 10:17 UTC (permalink / raw
To: 'Takashi Iwai', 'Richard Fitzgerald'
Cc: tiwai, linux-sound, alsa-devel, linux-kernel, patches
> -----Original Message-----
> From: Takashi Iwai <tiwai@suse.de>
> Sent: Thursday, May 2, 2024 10:53 AM
> To: Richard Fitzgerald <rf@opensource.cirrus.com>
> Cc: Simon Trimmer <simont@opensource.cirrus.com>; tiwai@suse.com; linux-
> sound@vger.kernel.org; alsa-devel@alsa-project.org; linux-
> kernel@vger.kernel.org; patches@opensource.cirrus.com
> Subject: Re: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the
> background
>
> On Thu, 02 May 2024 11:21:36 +0200,
> Richard Fitzgerald wrote:
> >
> > On 02/05/2024 08:34, Takashi Iwai wrote:
> > > On Wed, 01 May 2024 13:17:55 +0200,
> > > Simon Trimmer wrote:
> > >> @@ -964,6 +1011,14 @@ int cs35l56_hda_common_probe(struct
> cs35l56_hda *cs35l56, int hid, int id)
> > >> mutex_init(&cs35l56->base.irq_lock);
> > >> dev_set_drvdata(cs35l56->base.dev, cs35l56);
> > >> + cs35l56->dsp_wq =
> > >> create_singlethread_workqueue("cs35l56-dsp");
> > >> + if (!cs35l56->dsp_wq) {
> > >> + ret = -ENOMEM;
> > >> + goto err;
> > >> + }
> > >
> > > Do we really need a dedicated workqueue? In most usages, simple
> > > schedule_work*() works fine and is recommended.
> > >
> >
> > On a slow I2C bus with 4 amps this work could take over 2 seconds.
> > That seems too long to be blocking a global system queue. We use a
> > dedicated queue in the ASoC driver.
> >
> > Also if we queue work on an ordered (single-threaded) system queue the
> > firmware won't be downloaded to multiple amps in parallel, so we don't
> > get the best use of the available bus bandwidth.
>
> OK, that sounds like a sensible argument.
>
> But the patch has no call of a queue destructor. Won't it leak
> resources?
Oops that's a good spot - I missed that and will send a v2
Cheers,
-Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
2024-05-02 10:17 ` Simon Trimmer
@ 2024-05-02 10:25 ` Takashi Iwai
0 siblings, 0 replies; 8+ messages in thread
From: Takashi Iwai @ 2024-05-02 10:25 UTC (permalink / raw
To: Simon Trimmer
Cc: 'Richard Fitzgerald', tiwai, linux-sound, alsa-devel,
linux-kernel, patches
On Thu, 02 May 2024 12:17:48 +0200,
Simon Trimmer wrote:
>
> > -----Original Message-----
> > From: Takashi Iwai <tiwai@suse.de>
> > Sent: Thursday, May 2, 2024 10:53 AM
> > To: Richard Fitzgerald <rf@opensource.cirrus.com>
> > Cc: Simon Trimmer <simont@opensource.cirrus.com>; tiwai@suse.com; linux-
> > sound@vger.kernel.org; alsa-devel@alsa-project.org; linux-
> > kernel@vger.kernel.org; patches@opensource.cirrus.com
> > Subject: Re: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the
> > background
> >
> > On Thu, 02 May 2024 11:21:36 +0200,
> > Richard Fitzgerald wrote:
> > >
> > > On 02/05/2024 08:34, Takashi Iwai wrote:
> > > > On Wed, 01 May 2024 13:17:55 +0200,
> > > > Simon Trimmer wrote:
> > > >> @@ -964,6 +1011,14 @@ int cs35l56_hda_common_probe(struct
> > cs35l56_hda *cs35l56, int hid, int id)
> > > >> mutex_init(&cs35l56->base.irq_lock);
> > > >> dev_set_drvdata(cs35l56->base.dev, cs35l56);
> > > >> + cs35l56->dsp_wq =
> > > >> create_singlethread_workqueue("cs35l56-dsp");
> > > >> + if (!cs35l56->dsp_wq) {
> > > >> + ret = -ENOMEM;
> > > >> + goto err;
> > > >> + }
> > > >
> > > > Do we really need a dedicated workqueue? In most usages, simple
> > > > schedule_work*() works fine and is recommended.
> > > >
> > >
> > > On a slow I2C bus with 4 amps this work could take over 2 seconds.
> > > That seems too long to be blocking a global system queue. We use a
> > > dedicated queue in the ASoC driver.
> > >
> > > Also if we queue work on an ordered (single-threaded) system queue the
> > > firmware won't be downloaded to multiple amps in parallel, so we don't
> > > get the best use of the available bus bandwidth.
> >
> > OK, that sounds like a sensible argument.
> >
> > But the patch has no call of a queue destructor. Won't it leak
> > resources?
>
> Oops that's a good spot - I missed that and will send a v2
If you submit a newer version, it'd be appreciated to explain about
the workqueue usage in the patch description, too.
thanks,
Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
@ 2024-06-18 12:53 Simon Trimmer
2024-06-18 12:59 ` Simon Trimmer
0 siblings, 1 reply; 8+ messages in thread
From: Simon Trimmer @ 2024-06-18 12:53 UTC (permalink / raw
To: tiwai; +Cc: linux-sound, alsa-devel, linux-kernel, patches, Simon Trimmer
It is possible that during system boot when there multiple devices
attempting simultaneous initialization on a slow control bus the
download of firmware and tuning data may take a user perceivable amount
of time (a slow I2C bus with 4 amps this work could take over 2
seconds).
Adopt a pattern used in the ASoC driver and perform this activity in a
background thread so that interactive performance is not impaired. The
system_long_wq is a parallel workqueue and driver instances will perform
their firmware downloads in parallel to make best use of available bus
bandwidth.
Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
---
sound/pci/hda/cs35l56_hda.c | 90 ++++++++++++++++++++++++++++++-------
sound/pci/hda/cs35l56_hda.h | 3 ++
2 files changed, 76 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/cs35l56_hda.c b/sound/pci/hda/cs35l56_hda.c
index cc4aa90db1d1..96d3f13c5abf 100644
--- a/sound/pci/hda/cs35l56_hda.c
+++ b/sound/pci/hda/cs35l56_hda.c
@@ -50,11 +50,19 @@ static const struct reg_sequence cs35l56_hda_dai_config[] = {
};
+static void cs35l56_hda_wait_dsp_ready(struct cs35l56_hda *cs35l56)
+{
+ /* Wait for patching to complete */
+ flush_work(&cs35l56->dsp_work);
+}
+
static void cs35l56_hda_play(struct cs35l56_hda *cs35l56)
{
unsigned int val;
int ret;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
pm_runtime_get_sync(cs35l56->base.dev);
ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_PLAY);
if (ret == 0) {
@@ -180,6 +188,8 @@ static int cs35l56_hda_mixer_get(struct snd_kcontrol *kcontrol,
unsigned int reg_val;
int i;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
regmap_read(cs35l56->base.regmap, kcontrol->private_value, ®_val);
reg_val &= CS35L56_ASP_TXn_SRC_MASK;
@@ -203,6 +213,8 @@ static int cs35l56_hda_mixer_put(struct snd_kcontrol *kcontrol,
if (item >= CS35L56_NUM_INPUT_SRC)
return -EINVAL;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
regmap_update_bits_check(cs35l56->base.regmap, kcontrol->private_value,
CS35L56_INPUT_MASK, cs35l56_tx_input_values[item],
&changed);
@@ -227,6 +239,8 @@ static int cs35l56_hda_posture_get(struct snd_kcontrol *kcontrol,
unsigned int pos;
int ret;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_read(cs35l56->base.regmap, CS35L56_MAIN_POSTURE_NUMBER, &pos);
if (ret)
return ret;
@@ -248,6 +262,8 @@ static int cs35l56_hda_posture_put(struct snd_kcontrol *kcontrol,
(pos > CS35L56_MAIN_POSTURE_MAX))
return -EINVAL;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_update_bits_check(cs35l56->base.regmap,
CS35L56_MAIN_POSTURE_NUMBER,
CS35L56_MAIN_POSTURE_MASK,
@@ -291,6 +307,8 @@ static int cs35l56_hda_vol_get(struct snd_kcontrol *kcontrol,
int vol;
int ret;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_read(cs35l56->base.regmap, CS35L56_MAIN_RENDER_USER_VOLUME, &raw_vol);
if (ret)
@@ -323,6 +341,8 @@ static int cs35l56_hda_vol_put(struct snd_kcontrol *kcontrol,
raw_vol = (vol + CS35L56_MAIN_RENDER_USER_VOLUME_MIN) <<
CS35L56_MAIN_RENDER_USER_VOLUME_SHIFT;
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+
ret = regmap_update_bits_check(cs35l56->base.regmap,
CS35L56_MAIN_RENDER_USER_VOLUME,
CS35L56_MAIN_RENDER_USER_VOLUME_MASK,
@@ -539,8 +559,9 @@ static void cs35l56_hda_release_firmware_files(const struct firmware *wmfw_firmw
kfree(coeff_filename);
}
-static void cs35l56_hda_add_dsp_controls(struct cs35l56_hda *cs35l56)
+static void cs35l56_hda_create_dsp_controls_work(struct work_struct *work)
{
+ struct cs35l56_hda *cs35l56 = container_of(work, struct cs35l56_hda, control_work);
struct hda_cs_dsp_ctl_info info;
info.device_name = cs35l56->amp_name;
@@ -566,7 +587,7 @@ static void cs35l56_hda_apply_calibration(struct cs35l56_hda *cs35l56)
dev_info(cs35l56->base.dev, "Calibration applied\n");
}
-static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
+static void cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
{
const struct firmware *coeff_firmware = NULL;
const struct firmware *wmfw_firmware = NULL;
@@ -574,15 +595,34 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
char *wmfw_filename = NULL;
unsigned int preloaded_fw_ver;
bool firmware_missing;
- int ret = 0;
+ bool add_dsp_controls_required = false;
+ int ret;
+
+ /*
+ * control_work must be flushed before proceeding, but we can't do that
+ * here as it would create a deadlock on controls_rwsem so it must be
+ * performed before queuing dsp_work.
+ */
+ WARN_ON_ONCE(work_busy(&cs35l56->control_work));
- /* Prepare for a new DSP power-up */
+ /*
+ * Prepare for a new DSP power-up. If the DSP has had firmware
+ * downloaded previously then it needs to be powered down so that it
+ * can be updated and if hadn't been patched before then the controls
+ * will need to be added once firmware download succeeds.
+ */
if (cs35l56->base.fw_patched)
cs_dsp_power_down(&cs35l56->cs_dsp);
+ else
+ add_dsp_controls_required = true;
cs35l56->base.fw_patched = false;
- pm_runtime_get_sync(cs35l56->base.dev);
+ ret = pm_runtime_resume_and_get(cs35l56->base.dev);
+ if (ret < 0) {
+ dev_err(cs35l56->base.dev, "Failed to resume and get %d\n", ret);
+ return;
+ }
/*
* The firmware can only be upgraded if it is currently running
@@ -606,7 +646,6 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
*/
if (!coeff_firmware && firmware_missing) {
dev_err(cs35l56->base.dev, ".bin file required but not found\n");
- ret = -ENOENT;
goto err_fw_release;
}
@@ -659,6 +698,15 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
CS35L56_FIRMWARE_MISSING);
cs35l56->base.fw_patched = true;
+ /*
+ * Adding controls is deferred to prevent a lock inversion - ALSA takes
+ * the controls_rwsem when adding a control, the get() / put()
+ * functions of a control are called holding controls_rwsem and those
+ * that depend on running firmware wait for dsp_work() to complete.
+ */
+ if (add_dsp_controls_required)
+ queue_work(system_long_wq, &cs35l56->control_work);
+
ret = cs_dsp_run(&cs35l56->cs_dsp);
if (ret)
dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
@@ -678,8 +726,13 @@ static int cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
coeff_firmware, coeff_filename);
err_pm_put:
pm_runtime_put(cs35l56->base.dev);
+}
- return ret;
+static void cs35l56_hda_dsp_work(struct work_struct *work)
+{
+ struct cs35l56_hda *cs35l56 = container_of(work, struct cs35l56_hda, dsp_work);
+
+ cs35l56_hda_fw_load(cs35l56);
}
static int cs35l56_hda_bind(struct device *dev, struct device *master, void *master_data)
@@ -687,7 +740,6 @@ static int cs35l56_hda_bind(struct device *dev, struct device *master, void *mas
struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev);
struct hda_component_parent *parent = master_data;
struct hda_component *comp;
- int ret;
comp = hda_component_from_index(parent, cs35l56->index);
if (!comp)
@@ -701,12 +753,10 @@ static int cs35l56_hda_bind(struct device *dev, struct device *master, void *mas
strscpy(comp->name, dev_name(dev), sizeof(comp->name));
comp->playback_hook = cs35l56_hda_playback_hook;
- ret = cs35l56_hda_fw_load(cs35l56);
- if (ret)
- return ret;
+ flush_work(&cs35l56->control_work);
+ queue_work(system_long_wq, &cs35l56->dsp_work);
cs35l56_hda_create_controls(cs35l56);
- cs35l56_hda_add_dsp_controls(cs35l56);
#if IS_ENABLED(CONFIG_SND_DEBUG)
cs35l56->debugfs_root = debugfs_create_dir(dev_name(cs35l56->base.dev), sound_debugfs_root);
@@ -724,6 +774,9 @@ static void cs35l56_hda_unbind(struct device *dev, struct device *master, void *
struct hda_component_parent *parent = master_data;
struct hda_component *comp;
+ cancel_work_sync(&cs35l56->dsp_work);
+ cancel_work_sync(&cs35l56->control_work);
+
cs35l56_hda_remove_controls(cs35l56);
#if IS_ENABLED(CONFIG_SND_DEBUG)
@@ -752,6 +805,9 @@ static int cs35l56_hda_system_suspend(struct device *dev)
{
struct cs35l56_hda *cs35l56 = dev_get_drvdata(dev);
+ cs35l56_hda_wait_dsp_ready(cs35l56);
+ flush_work(&cs35l56->control_work);
+
if (cs35l56->playing)
cs35l56_hda_pause(cs35l56);
@@ -850,11 +906,8 @@ static int cs35l56_hda_system_resume(struct device *dev)
ret = cs35l56_is_fw_reload_needed(&cs35l56->base);
dev_dbg(cs35l56->base.dev, "fw_reload_needed: %d\n", ret);
- if (ret > 0) {
- ret = cs35l56_hda_fw_load(cs35l56);
- if (ret)
- return ret;
- }
+ if (ret > 0)
+ queue_work(system_long_wq, &cs35l56->dsp_work);
if (cs35l56->playing)
cs35l56_hda_play(cs35l56);
@@ -972,6 +1025,9 @@ int cs35l56_hda_common_probe(struct cs35l56_hda *cs35l56, int hid, int id)
mutex_init(&cs35l56->base.irq_lock);
dev_set_drvdata(cs35l56->base.dev, cs35l56);
+ INIT_WORK(&cs35l56->dsp_work, cs35l56_hda_dsp_work);
+ INIT_WORK(&cs35l56->control_work, cs35l56_hda_create_dsp_controls_work);
+
ret = cs35l56_hda_read_acpi(cs35l56, hid, id);
if (ret)
goto err;
diff --git a/sound/pci/hda/cs35l56_hda.h b/sound/pci/hda/cs35l56_hda.h
index 464e4aa63cd1..c40d159507c2 100644
--- a/sound/pci/hda/cs35l56_hda.h
+++ b/sound/pci/hda/cs35l56_hda.h
@@ -14,6 +14,7 @@
#include <linux/firmware/cirrus/cs_dsp.h>
#include <linux/firmware/cirrus/wmfw.h>
#include <linux/regulator/consumer.h>
+#include <linux/workqueue.h>
#include <sound/cs35l56.h>
struct dentry;
@@ -21,6 +22,8 @@ struct dentry;
struct cs35l56_hda {
struct cs35l56_base base;
struct hda_codec *codec;
+ struct work_struct dsp_work;
+ struct work_struct control_work;
int index;
const char *system_name;
--
2.34.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
2024-06-18 12:53 Simon Trimmer
@ 2024-06-18 12:59 ` Simon Trimmer
0 siblings, 0 replies; 8+ messages in thread
From: Simon Trimmer @ 2024-06-18 12:59 UTC (permalink / raw
To: tiwai; +Cc: linux-sound, alsa-devel, linux-kernel, patches
On Tue, Jun 18, 2024 at 1:53 PM Simon Trimmer
> Subject: [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background
>
> It is possible that during system boot when there multiple devices
> attempting simultaneous initialization on a slow control bus the
> download of firmware and tuning data may take a user perceivable amount
> of time (a slow I2C bus with 4 amps this work could take over 2
> seconds).
>
> Adopt a pattern used in the ASoC driver and perform this activity in a
> background thread so that interactive performance is not impaired. The
> system_long_wq is a parallel workqueue and driver instances will perform
> their firmware downloads in parallel to make best use of available bus
> bandwidth.
>
> Signed-off-by: Simon Trimmer <simont@opensource.cirrus.com>
Sorry Takashi this should have had v2 in the Subject - when fixing up v1 we
found all the things that triggered the series of hda_component changes
so it fell off my mental queue as something I'd published before.
I'll do a re-send with that corrected for Lore.
-Simon
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-06-18 13:07 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-01 11:17 [PATCH] ALSA: hda: cs35l56: Perform firmware download in the background Simon Trimmer
2024-05-02 7:34 ` Takashi Iwai
2024-05-02 9:21 ` Richard Fitzgerald
2024-05-02 9:52 ` Takashi Iwai
2024-05-02 10:17 ` Simon Trimmer
2024-05-02 10:25 ` Takashi Iwai
-- strict thread matches above, loose matches on Subject: below --
2024-06-18 12:53 Simon Trimmer
2024-06-18 12:59 ` Simon Trimmer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).