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

[Takashi Iwai]

On Mon, 09 Aug 2021 06:02:21 +0200,
Vinod Koul wrote:
> 
> On 06-08-21, 11:17, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 8/6/21 8:37 AM, Vinod Koul wrote:
> > > On 02-08-21, 10:46, Pierre-Louis Bossart wrote:
> > >>
> > >>>>>>>> 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.
> > >>
> > >> The 'nonatomic' concept is only used for an FE dailink which expose a
> > >> PCM device:
> > >>
> > >> soc-pcm.c:	pcm->nonatomic = rtd->dai_link->nonatomic;
> > >>
> > >> Setting a BE dailink as 'nonatomic' would not accomplish much since BEs
> > >> use the 'no_pcm' option.
> > > 
> > > are no_pcm & nonatomic supposed to be not used together? So if FE is
> > > nonatomic would BE trigger be atomic or nonatomic?
> > 
> > I don't follow the multiple negations, so let me retry:
> > 
> > For Intel machine drivers, all BE dailinks use
> > .no_pcm = 1 (explicit setting)
> > .nonatomic = 0 (implicit).
> 
> that was my question, how is it implicit?
> Should be explicitly set, right?
> 
> > 
> > All FE dailinks use
> > .no_pcm = 0 (implicit)
> > .nonatomic = 1 (explicit setting)
> > 
> > >> So the question is: is there any issue with sending an IPC in a DAI
> > >> trigger callback?
> > > 
> > > Sorry looks like we diverged, orignal question was can we do heavy tasks
> > > in trigger, the answer is no, unless one uses nonatomic flag which was
> > > added so that people can do that work with DSPs like sending IPCs..
> > > Maybe we should add heavy slimbus/soundwire handling to it too...?
> > 
> > I don't think the answer is as clear as you describe it Vinod.
> > 
> > The .nonatomic field is at the BE dailink level.
> > 
> > Unless I am missing something, I don't see anything that lets me set a
> > .nonatomic property at the *DAI* level.
> 
> I would say that was a miss in original design, it should have been set
> at dai level or at least allowed to propagate from dai level setting.
> 
> Now we are allowed to set it at dai_link but it is governed by dai
> behaviour (DSP based DAI etc...)

Actually, there was one big piece I overlooked.  The whole DPCM BE
operation is *always* tied with FE's.  That is, the nonatomic flag is
completely ignored for BE, but just follows what FE sets up.

And that's the very confusing point when reviewing the code.  You
cannot know whether it's written for non-atomic context or not.  This
means that it's also error-prone; the code that assumes the operation
in a certain mode might mismatch with the bound FE.

So, ideally, both FE and BE should set the proper nonatomic flags, and
have a consistency check with WARN_ON() at the run time.


thanks,

Takashi