Re: [alsa-devel] [PATCH v3 2/2] soundwire: qcom: add support for SoundWire controller

[Pierre-Louis Bossart]

On 10/30/19 9:56 AM, Srinivas Kandagatla wrote:
> On 21/10/2019 05:44, Vinod Koul wrote:
> > On 11-10-19, 16:44, Srinivas Kandagatla wrote:
> >
> > > +static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id)
> > > +{
> > > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = dev_id;
> > > +ÂÂÂ u32 sts, value;
> > > +ÂÂÂ unsigned long flags;
> > > +
> > > +ÂÂÂ ctrl->reg_read(ctrl, SWRM_INTERRUPT_STATUS, &sts);
> > > +
> > > +ÂÂÂ if (sts & SWRM_INTERRUPT_STATUS_CMD_ERROR) {
> > > +ÂÂÂÂÂÂÂ ctrl->reg_read(ctrl, SWRM_CMD_FIFO_STATUS, &value);
> > > +ÂÂÂÂÂÂÂ dev_err_ratelimited(ctrl->dev,
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "CMD error, fifo status 0x%x\n",
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ value);
> > > +ÂÂÂÂÂÂÂ ctrl->reg_write(ctrl, SWRM_CMD_FIFO_CMD, 0x1);
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ if ((sts & SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED) ||
> > > +ÂÂÂÂÂÂÂ sts & SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS)
> > > +ÂÂÂÂÂÂÂ schedule_work(&ctrl->slave_work);
> >
> > we are in irq thread, so why not do the work here rather than schedule
> > it?
>
> The reason is that, sdw_handle_slave_status() we will read device id 
> registers, which are fifo based in this controller and triggers an 
> interrupt for each read.
> So all the such reads will timeout waiting for interrupt if we do not do 
> it in a separate thread.

Yes, it's similar for Intel. we don't read device ID in the handler or 
reads would time out. And in the latest patches we also use a work queue 
for the slave status handling (due to MSI handling issues).
Even if this timeout problem did not exists, updates to the slave status 
will typically result in additional read/writes, which are going to be 
throttled by the command bandwidth (frame rate), so this status update 
should really not be done in a handler. This has to be done in a thread 
or work queue.

> > > +static int qcom_swrm_compute_params(struct sdw_bus *bus)
> > > +{
> > > +ÂÂÂ struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus);
> > > +ÂÂÂ struct sdw_master_runtime *m_rt;
> > > +ÂÂÂ struct sdw_slave_runtime *s_rt;
> > > +ÂÂÂ struct sdw_port_runtime *p_rt;
> > > +ÂÂÂ struct qcom_swrm_port_config *pcfg;
> > > +ÂÂÂ int i = 0;
> > > +
> > > +ÂÂÂ list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
> > > +ÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &m_rt->port_list, port_node) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ pcfg = &ctrl->pconfig[p_rt->num - 1];
> > > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.port_num = p_rt->num;
> > > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.sample_interval = pcfg->si + 1;
> > > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset1 = pcfg->off1;
> > > +ÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset2 = pcfg->off2;
> > > +ÂÂÂÂÂÂÂ }
> > > +
> > > +ÂÂÂÂÂÂÂ list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcfg = &ctrl->pconfig[i];
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.port_num = p_rt->num;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.sample_interval =
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pcfg->si + 1;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset1 = pcfg->off1;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ p_rt->transport_params.offset2 = pcfg->off2;
> > > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ i++;
> > > +ÂÂÂÂÂÂÂÂÂÂÂ }
> >
> > Can you explain this one, am not sure I understood this. This fn is
> > supposed to compute and fill up the params, all I can see is filling up!
> Bandwidth parameters are currently coming from board specific Device 
> Tree, which are programmed here.

'compute' does not mean 'dynamic on-demand bandwidth allocation', it's 
perfectly legal to use fixed tables as done here.

> > > +static const struct snd_soc_dai_ops qcom_swrm_pdm_dai_ops = {
> > > +ÂÂÂ .hw_params = qcom_swrm_hw_params,
> > > +ÂÂÂ .prepare = qcom_swrm_prepare,
> > > +ÂÂÂ .hw_free = qcom_swrm_hw_free,
> > > +ÂÂÂ .startup = qcom_swrm_startup,
> > > +ÂÂÂ .shutdown = qcom_swrm_shutdown,
> > > +ÂÂÂÂÂÂÂ .set_sdw_stream = qcom_swrm_set_sdw_stream,
> >
> > why does indent look off to me!
> Yep, Fixed in next version.
>
> --srini
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@xxxxxxxxxxxxxxxx
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel