Message-ID: <d86e401a-205d-42a3-b90d-239d669c6ee1@opensource.cirrus.com>
Date: Wed, 10 Dec 2025 09:59:03 +0000
From: Richard Fitzgerald <rf@...nsource.cirrus.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@...ux.dev>, vkoul@...nel.org,
        yung-chuan.liao@...ux.intel.com
Cc: linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
        patches@...nsource.cirrus.com
Subject: Re: [PATCH v3] soundwire: stream: Prepare ports in parallel to reduce
 stream start latency

On 9/12/25 16:41, Pierre-Louis Bossart wrote:
> 
>>>> Changes in V2:
>>>> +    if (simple_ch_prep_sm)
>>>> +        return 0;
>>>> +
>>>> +    /*
>>>> +     * Check if already prepared. Avoid overhead of waiting for interrupt
>>>> +     * and port_ready completion if we don't need to.
>>>> +     */
> 
> 1.
> 
>>>> +    val = sdw_read_no_pm(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
>>>> +    if (val < 0) {
>>>> +        ret = val;
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    if (val & p_rt->ch_mask) {
>>>
>>> Can you explain why we don't use the ch_mask in the already-prepared case? I am missing something.
>>>
>> I'm not sure what you mean here. The if() immediately above your comment
>> uses ch_mask to check the already-prepared state.
> 
> I was referring to the 1. above, you read the prepare status without checking for ch_mask first.
>

What would be the purpose of checking ch_mask before the read?

>>>> +        /* Wait for completion on port ready */
>>>> +        port_ready = &s_rt->slave->port_ready[p_rt->num];
>>>> +        wait_for_completion_timeout(port_ready, msecs_to_jiffies(ch_prep_timeout));
>>>
>>> I understand the code is the same as before but would there be any merit in checking the timeout before starting a read? If the device is already in the weeds, doing another read adds even more time before reporting an error.
>>>
>> Do you mean save the system time when the DPN_PREPARE was written to
>> that peripheral and then check here whether the timeout period has
>> already elapsed?
> 
> I meant testing the return value of wait_for_completion_timeout(). If you already timed out at this point with a return value of zero, there's no point in checking the status any more, the system is in the weeds.
> 

Wait completion will _always_ timeout because this code is holding the
bus lock, which blocks the ALERT handler from running and signalling
the completion. The wait_for_completion_timeout() is effectively
msleep(msecs_to_jiffies(ch_prep_timeout));
So we have to read the register afterwards to see whether the peripheral
actually prepared.

I've left the useless wait_for_completion_timeout() in the code so this
commit is only changing what it says it is changing, and nothing else.

What to do about the deadlocked wait_for_completion_timeout() is a
separate problem.

>> If that's what you mean, I don't see much advantage in that. If the
>> hardware is working correctly, this will be detected by the read above
>> that checks if the peripheral has already prepared. If it has we skip
>> the wait_for_completion_timeout().
>>
>> If the peripheral is "in the weeds", so that its prepare time has
>> already passed and it still isn't ready, we're no longer in a state
>> where we care about minimizing audio startup time because the hardware
>> is now broken. So it's probably not worth complicating the code to
>> take a few milliseconds off that case.
> 
> I agree it's no longer about minimizing the start time but rather providing an error faster, without waiting for a second timeout on read.
>   
>>>> +        val = sdw_read_no_pm(s_rt->slave, SDW_DPN_PREPARESTATUS(p_rt->num));
>>>> +        if ((val < 0) || (val & p_rt->ch_mask)) {
>>>> +            ret = (val < 0) ? val : -ETIMEDOUT;
>>>> +            goto err;
>>>> +        }
>>>> +    }
>>> T
>>>
>>
> 

Powered by blists - more mailing lists