Skip to content

SKL+ ipc timeout fix series #845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
keyonjie wants to merge 7 commits into from
Closed

SKL+ ipc timeout fix series #845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0caf6ca
ASoC: SOF: add elapse work for spcm to fix trigger stop ipc timeout
keyonjie Apr 17, 2019
7e936ca
ASoC: SOF: hda-stream: use work for pcm period elapse
keyonjie Apr 17, 2019
05aec05
ASoC: SOF: ipc: change to use snd_sof_pcm_elapsed() for pcm elapse
keyonjie Apr 18, 2019
158fb72
ASoC: SOF: hda-ipc: fix ipc irq_status modified by stream interrupt i…
keyonjie Apr 17, 2019
0dce7e0
ASoC: SOF: hda-stream: add and strore hda stream number
keyonjie Apr 18, 2019
3c6e128
ASoC: SOF: hda-stream: refine interrupt handler to handle real stream…
keyonjie Apr 17, 2019
0513cdd
ASoC: SOF: cnl: add check for handling ipc new message from dsp
keyonjie Apr 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ static irqreturn_t cnl_ipc_irq_thread(int irq, void *context)
}

/* new message from DSP */
if (hipctdr & CNL_DSP_REG_HIPCTDR_BUSY) {
if (hipctdr & CNL_DSP_REG_HIPCTDR_BUSY &&
hipcctl & CNL_DSP_REG_HIPCCTL_BUSY) {

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, we've been there before. The hipcctl register controls the generation of the interrupt, so it's useless to test in the interrupt if the interrupt is enabled. NAK on this one.

@keyonjie keyonjie Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me remove this commit after some experiments here, as written in the commit message, this may be not needed.

hipctdd = snd_sof_dsp_read(sdev, HDA_DSP_BAR,
CNL_DSP_REG_HIPCTDD);
msg = hipctdr & CNL_DSP_REG_HIPCTDR_MSG_MASK;
Expand All @@ -101,6 +102,11 @@ static irqreturn_t cnl_ipc_irq_thread(int irq, void *context)
snd_sof_ipc_msgs_rx(sdev);
}

/* mask BUSY interrupt */
snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR,
HDA_DSP_REG_HIPCCTL,
HDA_DSP_REG_HIPCCTL_BUSY, 0);

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this unmasked?


/*
* clear busy interrupt to tell dsp controller this
* interrupt has been accepted, not trigger it again
Expand Down
10 changes: 6 additions & 4 deletions sound/soc/sof/intel/hda-ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,19 +234,21 @@ irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context)
{
struct snd_sof_dev *sdev = context;
int ret = IRQ_NONE;
u32 irq_status = 0;

spin_lock(&sdev->hw_lock);

/* store status */
sdev->irq_status = snd_sof_dsp_read(sdev, HDA_DSP_BAR,
HDA_DSP_REG_ADSPIS);
irq_status = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPIS);
dev_vdbg(sdev->dev, "irq handler: irq_status:0x%x\n", irq_status);

/* invalid message ? */
if (sdev->irq_status == 0xffffffff)
if (irq_status == 0xffffffff)
goto out;

/* IPC message ? */
if (sdev->irq_status & HDA_DSP_ADSPIS_IPC) {
if (irq_status & HDA_DSP_ADSPIS_IPC) {
sdev->irq_status = irq_status;
/* disable IPC interrupt */
snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR,
HDA_DSP_REG_ADSPIC,
Expand Down
18 changes: 15 additions & 3 deletions sound/soc/sof/intel/hda-stream.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,15 +433,23 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
irqreturn_t hda_dsp_stream_interrupt(int irq, void *context)

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix your commit message

ASoC: SOF: Intel: hda-stream: handle real stream interrupts only

The stream and IPC share the same interrupt. The stream interrupt handler mistakenly uses the ipc interrupt
and return IRQ_HANDLED, causing the ipc interrupt to be missed.

Make sure the stream interrupt handler only deals with stream-related interrupts.

@keyonjie keyonjie Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, thanks, let me take it.

{
struct hdac_bus *bus = context;
struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
u32 stream_mask;
u32 status;

dev_vdbg(bus->dev, "stream irq, INTSTS status: 0x%x\n",
snd_hdac_chip_readl(bus, INTSTS));

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary? If you add a log, then better add a log of the wrong interrupt being discarded

@keyonjie keyonjie Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hit and debug these issues several rounds, so I want to add a verbose log here for the developer, print out the whole register here is good enough for the developer, it has several bits, the developer need to refer to spec to understand it fully.


if (!pm_runtime_active(bus->dev))
return IRQ_NONE;

spin_lock(&bus->reg_lock);

status = snd_hdac_chip_readl(bus, INTSTS);
if (status == 0 || status == 0xffffffff) {
stream_mask = GENMASK(sof_hda->stream_total - 1, 0) | AZX_INT_CTRL_EN;

/* Not stream interrupt or register inaccessible, ignore it.*/
if (!(status & stream_mask) || status == 0xffffffff) {
spin_unlock(&bus->reg_lock);
return IRQ_NONE;
}
Expand All @@ -465,8 +473,8 @@ irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context)
{
struct hdac_bus *bus = context;
struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
struct hdac_stream *s;
u32 status = snd_hdac_chip_readl(bus, INTSTS);
struct hdac_stream *s;
u32 sd_status;

/* check streams */
Expand All @@ -486,7 +494,7 @@ irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context)

/* Inform ALSA only in case not do that with IPC */
if (sof_hda->no_ipc_position)
snd_pcm_period_elapsed(s->substream);
snd_sof_pcm_elapsed(s->substream);

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like all this nonsense of no_ipc_position. We are circling on the same problems on how to report the position...

@keyonjie keyonjie Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine, :) but that has no relationship with this PR, right?


}
}
Expand All @@ -500,6 +508,7 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
struct hdac_ext_stream *stream;

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix typos in commit message

@keyonjie keyonjie Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry don't get this.

@plbossart plbossart Apr 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add and strore hda stream number

s/strore/store

struct hdac_stream *hstream;
struct pci_dev *pci = to_pci_dev(sdev->dev);
struct sof_intel_hda_dev *sof_hda = bus_to_sof_hda(bus);
int sd_offset;
int i, num_playback, num_capture, num_total, ret;
u32 gcap;
Expand Down Expand Up @@ -657,6 +666,9 @@ int hda_dsp_stream_init(struct snd_sof_dev *sdev)
list_add_tail(&hstream->list, &bus->stream_list);
}

/* store the total playback + capture stream number */
sof_hda->stream_total = num_total;

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? You don't use this information for anything in this patch set?

@keyonjie keyonjie Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is used in the consequent stream interrupt refining one, to configure the correct bit mask for interrupt.

@plbossart plbossart Apr 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somehow your commits are not in the right order in GitHub? It makes the reviews really hard...


return 0;
}

Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,9 @@ struct sof_intel_hda_dev {
/* if position update IPC needed */
u32 no_ipc_position;

/* the total playback + capture stream number */
u32 stream_total;

int irq;

/* DMIC device */
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/ipc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,7 @@ static void ipc_period_elapsed(struct snd_sof_dev *sdev, u32 msg_id)

/* only inform ALSA for period_wakeup mode */
if (!stream->substream->runtime->no_period_wakeup)
snd_pcm_period_elapsed(stream->substream);
snd_sof_pcm_elapsed(stream->substream);
}

/* DSP notifies host of an XRUN within FW */
Expand Down
39 changes: 39 additions & 0 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,39 @@ static int sof_pcm_dsp_params(struct snd_sof_pcm *spcm, struct snd_pcm_substream
return ret;
}

/*

@plbossart plbossart Apr 18, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update your commit message, it doesn't describe the problem nor why this patch helps.
Some of the English is incorrect, I don't know what 'we do elapse' means, nor what the 'PCM elapse' is.
The explanations are so convoluted it's hard to figure things out.
The additional question is why this belongs in the SOF core?

@keyonjie keyonjie Apr 19, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, let me refine the commit, as you know, I always need to improve my English. :)

This belongs to SOF PCM core, not only for DPIB mode, it is similar to sof/ipc.c, SKL- or any other platforms which using SOF IPC mechanism should use it, you can see it is used in the last commit "ASoC: SOF: ipc: change to use snd_sof_pcm_elapsed() for pcm elapse" for byt/bdw...

* sof pcm period elapse work
*/
static void sof_elapsed_work(struct work_struct *work)
{
struct snd_sof_pcm_stream *sps =
container_of(work, struct snd_sof_pcm_stream, elapse_work);

snd_pcm_period_elapsed(sps->substream);
}

/*
* sof pcm period elapse, this might be called at irq context.
*/
void snd_sof_pcm_elapsed(struct snd_pcm_substream *substream)
{
struct snd_soc_pcm_runtime *rtd = substream->private_data;
struct snd_soc_component *component =
snd_soc_rtdcom_lookup(rtd, DRV_NAME);
struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
struct snd_sof_pcm *spcm;

spcm = snd_sof_find_spcm_dai(sdev, rtd);
if (!spcm) {
dev_err(sdev->dev,
"error: period elapsed for unknown stream!\n");
return;
}

schedule_work(&spcm->stream[substream->stream].elapse_work);
}
EXPORT_SYMBOL(snd_sof_pcm_elapsed);

/* this may get called several times by oss emulation */
static int sof_pcm_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
Expand Down Expand Up @@ -169,6 +202,9 @@ static int sof_pcm_hw_params(struct snd_pcm_substream *substream,
/* save pcm hw_params */
memcpy(&spcm->params[substream->stream], params, sizeof(*params));

INIT_WORK(&spcm->stream[substream->stream].elapse_work,
sof_elapsed_work);

return ret;
}

Expand Down Expand Up @@ -203,6 +239,9 @@ static int sof_pcm_hw_free(struct snd_pcm_substream *substream)
sizeof(stream), &reply, sizeof(reply));

snd_pcm_lib_free_pages(substream);

cancel_work_sync(&spcm->stream[substream->stream].elapse_work);

return ret;
}

Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ struct snd_sof_pcm_stream {
struct snd_dma_buffer page_table;
struct sof_ipc_stream_posn posn;
struct snd_pcm_substream *substream;
struct work_struct elapse_work;
};

/* ALSA SOF PCM device */
Expand Down Expand Up @@ -495,6 +496,7 @@ struct snd_sof_pcm *snd_sof_find_spcm_comp(struct snd_sof_dev *sdev,
int *direction);
struct snd_sof_pcm *snd_sof_find_spcm_pcm_id(struct snd_sof_dev *sdev,
unsigned int pcm_id);
void snd_sof_pcm_elapsed(struct snd_pcm_substream *substream);

/*
* Stream IPC
Expand Down