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

[Richard Fitzgerald]

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