--- Begin Message ---
- To: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>, vkoul@xxxxxxxxxx
- Subject: Re: [PATCH V6 8/8] soundwire: amd: add pm_prepare callback and pm ops support
- From: "Mukunda,Vijendar" <vijendar.mukunda@xxxxxxx>
- Date: Wed, 8 Mar 2023 19:49:03 +0530
- Cc: alsa-devel@xxxxxxxxxxxxxxxx, Basavaraj.Hiregoudar@xxxxxxx, Sunil-kumar.Dommati@xxxxxxx, Mario.Limonciello@xxxxxxx, amadeuszx.slawinski@xxxxxxxxxxxxxxx, Mastan.Katragadda@xxxxxxx, Arungopal.kondaveeti@xxxxxxx, claudiu.beznea@xxxxxxxxxxxxx, Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>, Sanyog Kale <sanyog.r.kale@xxxxxxxxx>, open list <linux-kernel@xxxxxxxxxxxxxxx>
- In-reply-to: <85aba51e-1feb-5eb0-2e21-b714e217f9e4@linux.intel.com>
- References: <20230307133135.545952-1-Vijendar.Mukunda@amd.com> <20230307133135.545952-9-Vijendar.Mukunda@amd.com> <4330af6a-ce97-53ed-f675-6d3d0ac8f32f@linux.intel.com> <d5a75826-d762-27fc-5820-6826debdecd9@amd.com> <9399110b-bbba-f96e-85ef-a317e8f4d518@linux.intel.com> <4cbbff8a-c596-e9cc-a6cf-6f8b66607505@amd.com> <85aba51e-1feb-5eb0-2e21-b714e217f9e4@linux.intel.com>
- User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.1
On 08/03/23 19:28, Pierre-Louis Bossart wrote:
>
> On 3/7/23 22:32, Mukunda,Vijendar wrote:
>> On 08/03/23 02:38, Pierre-Louis Bossart wrote:
>>> On 3/7/23 14:25, Mukunda,Vijendar wrote:
>>>> On 07/03/23 20:58, Pierre-Louis Bossart wrote:
>>>>>> +static int amd_resume_child_device(struct device *dev, void *data)
>>>>>> +{
>>>>>> + struct sdw_slave *slave = dev_to_sdw_dev(dev);
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (!slave->probed) {
>>>>>> + dev_dbg(dev, "skipping device, no probed driver\n");
>>>>>> + return 0;
>>>>>> + }
>>>>>> + if (!slave->dev_num_sticky) {
>>>>>> + dev_dbg(dev, "skipping device, never detected on bus\n");
>>>>>> + return 0;
>>>>>> + }
>>>>>> + if (!pm_runtime_suspended(dev))
>>>>>> + return 0;
>>>>>> + ret = pm_request_resume(dev);
>>>>> I still don't get why the test above was needed. It's racy and brings
>>>>> limited benefits.
>>>> As explained below thread,
>>>>
>>>> https://lore.kernel.org/lkml/acd3a560-1218-9f1d-06ec-19e4d3d4e2c9@xxxxxxx
>>>>
>>>> Our scenario is multiple peripheral devices are connected
>>>> over the same link.
>>>>
>>>> In our implementation, device_for_each_child() function invokes
>>>> amd_resume_child_device callback for each child.
>>>> When any one of the child device is active, It will break the
>>>> iteration, which results in failure resuming all child devices.
>>> Can you clarify the 'it will break the iteration' statement?
>>>
>>> Are you saying pm_request_resume() will return a negative error code if
>>> the device is already active?
>>>
>>> We've used an unconditional pm_request_resume() in the Intel code for
>>> quite some time, including with multiple amplifiers per link, and have
>>> never observed the issue you report, so I'd like to get to the root
>>> cause pretty please. You took the Intel code and added a test for AMD
>>> platforms, and I'd really like to understand if the Intel code was wrong
>>> in the first place, or if the test is not needed. Something does not add
>>> up here.
>> AMP Codec (In aggregate mode) + Jack Codec connected over the same
>> link on our platform.
>> Consider below, scenario.
>> Active stream is running on AMP codec and Jack codec is already in runtime
>> suspend state.
>> If system level suspend is invoked, in prepare callback, we need to resume
>> both the codec devices.
>>
>> device_for_each_child() will invoke amd_resume_child_device() function callback
>> for each device which will try to resume the child device in this case.
>> By definition, device_for_each_child() Iterate over @parent's child devices,
>> and invokes the callback for each. We check the return of amd_resume_child_device()
>> each time.
>> If it returns anything other than 0, we break out and return that value.
>>
>> In current scenario, As AMP codec is not in runtime suspend state,
>> pm_request_resume() will return a value as 1. This will break the
>> sequence for resuming rest of the child devices(JACK codec in our case).
> Well, yes, now that makes sense, thanks for the details.
>
> I think the reason why we didn't see the problem with the Intel code is
> that both amplifiers are on the same dailink, so they are by
> construction either both suspended or both active. We never had
> different types of devices on the same link.
>
> I would however suggest this simpler alternative, where only negative
> return values are returned:
>
> ret = pm_request_resume(dev);
> if (ret < 0) {
> dev_err(dev, "pm_request_resume failed: %d\n", ret);
> return ret;
> }
> return 0;
>
> this would work just fine, no?
> No, As explained, pm_request_resume() return value is 1 for active device.
>> As mentioned in an earlier thread, there are two possible solutions.
>> 1. check pm runtime suspend state and return 0 if it is not suspended
>> 2. simply always return 0 for amd_resume_child_device() function callback.
>>
>> We opted first one as solution.
> My suggestion looks like your option 2. It's cleaner IMHO.
To use option 2, we need to respin the patch series.
Is it okay if we fix it as supplement patch?
--- End Message ---