RE: [PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger callback

From: "Liao, Bard" <bard.liao@intel.com>
To: Vinod Koul <vkoul@kernel.org>, Takashi Iwai <tiwai@suse.de>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"broonie@kernel.org" <broonie@kernel.org>,
	Bard Liao <yung-chuan.liao@linux.intel.com>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Subject: RE: [PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger callback
Date: Mon, 2 Aug 2021 07:29:42 +0000	[thread overview]
Message-ID: <DM6PR11MB4074EF8726AA5ED297871225FFEF9@DM6PR11MB4074.namprd11.prod.outlook.com> (raw)
In-Reply-To: <YQeQxj7Ejh14jIoc@matsya>

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, August 2, 2021 2:29 PM
> To: Takashi Iwai <tiwai@suse.de>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Bard Liao
> <yung-chuan.liao@linux.intel.com>; broonie@kernel.org; alsa-devel@alsa-
> project.org; Liao, Bard <bard.liao@intel.com>; Ranjani Sridharan
> <ranjani.sridharan@linux.intel.com>
> Subject: Re: [PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger
> callback
> 
> On 02-08-21, 07:49, Takashi Iwai wrote:
> > On Mon, 02 Aug 2021 06:35:16 +0200,
> > Vinod Koul wrote:
> > >
> > > On 27-07-21, 09:12, Pierre-Louis Bossart wrote:
> > > > Thanks Takashi for the review.
> > > >
> > > >
> > > > >> This patch provides both a simplification of the suspend flows
> > > > >> and a better balanced operation during suspend/resume transition.
> > > > >>
> > > > >> The exiting code relies on a convoluted way of dealing with
> > > > >> suspend signals. Since there is no .suspend DAI callback, we
> > > > >> used the component .suspend and marked all the component DAI
> > > > >> dmas as 'suspended'. The information was used in the .prepare
> > > > >> stage to differentiate resume operations from xrun handling,
> > > > >> and only reinitialize SHIM registers and DMA in the former case.
> > > > >>
> > > > >> While this solution has been working reliably for about 2
> > > > >> years, there is a much better solution consisting in trapping
> > > > >> the TRIGGER_SUSPEND in the .trigger DAI ops. The DMA is still
> > > > >> marked in the same way for the .prepare op to run, but in
> > > > >> addition the callbacks sent to DSP firmware are now balanced.
> > > > >>
> > > > >> Normal operation:
> > > > >> hw_params -> intel_params_stream
> > > > >> hw_free   -> intel_free_stream
> > > > >>
> > > > >> suspend    -> intel_free_stream
> > > > >> prepare    -> intel_params_stream
> > > > >>
> > > > >> This balanced operation was not required with existing SOF
> > > > >> firmware relying on static pipelines instantiated at every
> > > > >> boot. With the on-going transition to dynamic pipelines, it's
> > > > >> however a requirement to keep the use count for the DAI widget
> > > > >> balanced across all transitions.
> > > > >
> > > > > The trigger callback is handled in the stream lock atomically,
> > > > > and are you sure that you want to operate a possibly heavy task there?
> > > >
> > > > It's a good objection that we didn't think of.
> > >
> > > Doesn't Intel use non atomic trigger to send IPCs which anyway
> > > involve code which can sleep..?
> >
> > sof_sdw.c doesn't seem setting it?
> 
> Yes I think init_dai_link() should set it. Maybe Pierre/Bard would know why.

init_dai_link() is to assign dai link elements only. No IPC is needed.

> 
> --
> ~Vinod