Re: [PATCH v3] soundwire: stream: Prepare ports in parallel to reduce stream start latency

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

>>> 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.

>>> +        /* 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.

> 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
>>
>