Re: [PATCH v5 05/14] ASoC: SOF: Add PCM operations support

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

>>>>>> +	/* set runtime config */
>>>>>> +	runtime->hw.info = SNDRV_PCM_INFO_MMAP |
>>>>>> +			  SNDRV_PCM_INFO_MMAP_VALID |
>>>>>> +			  SNDRV_PCM_INFO_INTERLEAVED |
>>>>>> +			  SNDRV_PCM_INFO_PAUSE |
>>>>>> +			  SNDRV_PCM_INFO_RESUME |
>>>>>> +			  SNDRV_PCM_INFO_NO_PERIOD_WAKEUP;
>>>>>
>>>>> Does SOF support the full resume?  That is, the stream gets resumed
>>>>> at
>>>>> the exact position that was suspended.  Most devices can't, hence
>>>>> they
>>>>> don't set *_INFO_RESUME flag and let user-space restarting.
>>>> Hi Takashi,
>>>>
>>>> Thanks for your comment. The SOF driver definitely cannot guarantee to
>>>> resume from the exact same position due to the fact that triggers for
>>>> the host dma and link dma are not synchronized and also IPC scheduling
>>>> will affect this. So we should remove INOF_RESUME from hw.info.
>>>>
>>>> I do one follow up question for you. When a stream resumes/restarts
>>>> after suspend, we have the need for restoring the hw_params in the SOF
>>>> driver prior to handling the START trigger. I noticed that the
>>>> legacy/skylake driver does not seem to do this though. Do you have any
>>>> insights on what we might be missing in the SOF driver?
>>>
>>> So, SOF mandates the re-setup of the hw params in BE at resume?
>>> Then the right place to do is in the prepare callback.  Without
>>> SNDRV_PCM_INFO_RESUME flag, the PCM core won't trigger RESUME, but
>>> returns -ENOSYS.  Then user-space will do prepare, and start with the
>>> normal trigger.
>>
>> Takashi, some of us at Intel are a bit nervous about returning an
>> error to force the application to jump to its error-handling code
>> (typically only done for xruns and not always rock-solid) and as a
>> side effect get a prepare ioctl. I checked that e.g. CRAS does support
>> this mode but I wonder how many application developers know about
>> this. I personally didn't.
> 
> Essentially all applications are supposed to do so.  There are only
> few drivers that support really the full resume.  Some drivers set the
> flag, but many of them must be incorrect, especially ASoC ones.
> 
>> Moreover, it seems that the default behavior is rather to support
>> RESUME, even if the hardware can't precisely restart from the same
>> position, so we are concerned about exposing new errors when SOF is
>> selected instead of legacy drivers (Baytrail, Atom-SST and
>> Skylake-SST).
>>
>> In short, is this really your recommendation to remove this
>> INFO_RESUME flag? And if yes, should we demote all existing Intel
>> drivers and remove this flag as well?
> 
> Yes, better to start with less -- especially if this allows us to
> simplify the code a lot.

ok, thanks for confirming. we'll remove the INFO_RESUME flag in SOF and 
follow-up with the removal on all other Intel drivers. Thanks for 
enlightening us on this.
-Pierre