On 2022-11-10 4:39 PM, Pierre-Louis Bossart wrote:
On 11/10/22 08:13, Cezary Rojewski wrote:
--- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -934,8 +934,11 @@ static int avs_component_pm_op(struct snd_soc_component *component, bool be, rtd = snd_pcm_substream_chip(data->substream); if (rtd->dai_link->no_pcm == be && !rtd->dai_link->ignore_suspend) { ret = op(dai, data);
if (ret < 0)
if (ret < 0) {data->substream->runtime->status->state =SNDRV_PCM_STATE_DISCONNECTED;should runtime->state be used instead of runtime->status->state?
A quick grep shows the former seems more popular in drivers, status->seems to be only used in pcm_native.c?
Another plumbing question is whether it's actually ok to change the state of the runtime in a driver, are you not going to have locking issues? Very few drivers change the internal state and I wonder how legit/safe this is.
Unless something new has been added/updated, there is no runtime->state field available. All the PCM code works with runtime->status->state.
component->resume() gets fired before any PCMs have a chance to be resumed. Userspace cannot access us _yet_. Similarly for suspend, component->suspend() is called once all the streams receive TRIGGER_SUSPEND and from then on, we're on device-pm level already.
Well, in regard to grep, very few drivers enter the recovery jungle. All of this is to help improve user experience when things go south. Unfortunately for us, restoring regmap is insufficient to get AudioDSP happy. Right now if things do go south, userspace still performs all of the PCM commands on the stream as nothing has happened - snd_soc_suspend/resume() return 0 in all cases.
Regards, Czarek