Skip to content

[RFC] ASoC: SOF: merge ipc and stream irq #1483

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

Merged
plbossart merged 1 commit into from
Nov 21, 2019
Merged

[RFC] ASoC: SOF: merge ipc and stream irq #1483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion sound/soc/sof/intel/apl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ const struct snd_sof_dsp_ops sof_apl_ops = {
.block_write = sof_block_write,

/* doorbell */
.irq_handler = hda_dsp_ipc_irq_handler,
.irq_thread = hda_dsp_ipc_irq_thread,

/* ipc */
Expand Down
1 change: 0 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 @@ -232,7 +232,6 @@ const struct snd_sof_dsp_ops sof_cnl_ops = {
.block_write = sof_block_write,

/* doorbell */
.irq_handler = hda_dsp_ipc_irq_handler,
.irq_thread = cnl_ipc_irq_thread,

/* ipc */
Expand Down
14 changes: 7 additions & 7 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 @@ -237,14 +237,14 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
return IRQ_HANDLED;
}

/* is this IRQ for ADSP ? - we only care about IPC here */
irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context)
/* Check if it is an IPC and disable IPC interrupt if yes */
bool check_ipc(struct snd_sof_dev *sdev)
{
struct snd_sof_dev *sdev = context;
int ret = IRQ_NONE;
bool ret = false;
u32 irq_status;

spin_lock(&sdev->hw_lock);
/* The function can be called at irq thread, so use spin_lock_irq */
spin_lock_irq(&sdev->hw_lock);

@plbossart plbossart Nov 14, 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.

question:
I can never remember the rules for spin_locks.

Do we need to protect with the _irq suffix only here, or also in the thread?

@paulstelian97 paulstelian97 Nov 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe it has to be protected with _irq in the thread and not necessarily in the IRQ handler (you'd use _irq if you're not 100% certain the code runs only in IRQ handlers). IIRC, that is.

@bardliao bardliao Nov 15, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @paulstelian97 for the explanation . @plbossart hda_dsp_ipc_irq_handler() will be called in the irq thread that's why I add the _irq suffix here. But actually I don't know if we do need the spin_lock or not. @RanderWang discussed it with me about it, but we didn't have a conclusion.

@ranj063 ranj063 Nov 19, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao I'm not sure about this. I dont think we need to use spin_lock_irq(). Back when we're still upstreaming the SOF driver, IIRC the comment upstream was that spin_lock_irq() should only be in the hard interrupt handler.

@bardliao bardliao Nov 20, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. I will double check it.

@bardliao bardliao Nov 20, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ranj063 I can see spin_lock() is called twice without spin_unlock() if I test it with spin_lock() instead of spin_lock_irq(). I think the reason is that the irq did not be disabled when we use spin_lock() and the irq handler could be triggered when the irq thread is still running. Also, spin_lock_irq() is using in hda_dsp_stream_threaded_handler() which is a irq thread actually. So I believe that we should use spin_lock_irq()

@kv2019i kv2019i Nov 20, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao @ranj063 You definitely need to use the "_irq()" variant here. These is_xxx_irq() functions are called both from hard-irq and softirq context, so plain spin_lock() will not work.


/* store status */
irq_status = snd_sof_dsp_read(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPIS);
Expand All @@ -260,11 +260,11 @@ irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context)
snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR,
HDA_DSP_REG_ADSPIC,
HDA_DSP_ADSPIC_IPC, 0);
ret = IRQ_WAKE_THREAD;
ret = true;
}

out:
spin_unlock(&sdev->hw_lock);
spin_unlock_irq(&sdev->hw_lock);
return ret;
}

Expand Down
16 changes: 9 additions & 7 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 @@ -550,22 +550,23 @@ int hda_dsp_stream_hw_free(struct snd_sof_dev *sdev,
return 0;
}

irqreturn_t hda_dsp_stream_interrupt(int irq, void *context)
bool is_stream_irq(struct snd_sof_dev *sdev)
{
struct hdac_bus *bus = context;
int ret = IRQ_WAKE_THREAD;
struct hdac_bus *bus = sof_to_bus(sdev);
bool ret = true;

@ranj063 ranj063 Nov 21, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao @plbossart I find this odd the default return value is true. The logic is correct but typically we always assume its false and return true when conditions are favorable.

@plbossart plbossart Nov 21, 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.

the initial code assumed an IRQ_WAKE_THREAD as default, so it's acceptable to me.

u32 status;

spin_lock(&bus->reg_lock);
/* The function can be called at irq thread, so use spin_lock_irq */
spin_lock_irq(&bus->reg_lock);

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

/* Register inaccessible, ignore it.*/
if (status == 0xffffffff)
ret = IRQ_NONE;
ret = false;

spin_unlock(&bus->reg_lock);
spin_unlock_irq(&bus->reg_lock);

@kv2019i kv2019i Nov 19, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a bit asymmetric in that is_ipc() function disabled IPC interrupts in this check, while this function does not disable the interrupt source? Why is this?

@bardliao bardliao Nov 20, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good question. I think we should disable it as well, but we can do it in another PR.

@RanderWang RanderWang Nov 20, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

yes, please check the comments for request_threaded_irq

If yes it needs to disable the interrupt on the device and return IRQ_WAKE_THREAD which will wake up the handler thread and run @thread_fn. This split handler design is necessary to support shared interrupts.

@kv2019i kv2019i Nov 20, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ack @bardliao and @RanderWang This is matching existing code, so I'm ok to change in another PR. I do have a comment on naming the is_ipc_irq() functions (see my other comment).


return ret;
}
Expand Down Expand Up @@ -603,7 +604,8 @@ static bool hda_dsp_stream_check(struct hdac_bus *bus, u32 status)

irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context)
{
struct hdac_bus *bus = context;
struct snd_sof_dev *sdev = context;
struct hdac_bus *bus = sof_to_bus(sdev);
#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
u32 rirb_status;
#endif
Expand Down
48 changes: 27 additions & 21 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,28 @@ static const struct sof_intel_dsp_desc
return chip_info;
}

irqreturn_t hda_dsp_interrupt_handler(int irq, void *context)
{
struct snd_sof_dev *sdev = context;

if (check_ipc(sdev) || is_stream_irq(sdev))
return IRQ_WAKE_THREAD;

return IRQ_NONE;
}

irqreturn_t hda_dsp_interrupt_thread(int irq, void *context)
{
struct snd_sof_dev *sdev = context;

if (check_ipc(sdev))
sof_ops(sdev)->irq_thread(irq, sdev);
if (is_stream_irq(sdev))
hda_dsp_stream_threaded_handler(irq, sdev);

return IRQ_HANDLED;
}

@ranj063 ranj063 Nov 21, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao this looks bad. You call check_ipc() in the handler, and then wake up the thread and then check_ipc() again and do the same thing over?

@bardliao bardliao Nov 21, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ranj063 Yes, that's odd, but there are more than one interrupt could be trigger and one may be triggered after the handler is checked. So we need to recheck it on the thread. Actually, I think we can have a flag that store the interrupt status which is checked on the handler. So we won't need to double check it on the thread again. Please see hda_dsp_irq_thread

@ranj063 ranj063 Nov 21, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why not just return IRQ_WAKE_THREAD in hda_dsp_interrupt_handler() and then handle all the checks in the thread?

@bardliao bardliao Nov 21, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Why not just return IRQ_WAKE_THREAD in hda_dsp_interrupt_handler() and then handle all the checks in the thread?

@ranj063 We can only do it when there is not a shared irq. i.e. No one else could trigger the irq. Probably, we can do that after ipc/stream/sdw irq are merged. And we don't need the handler if it return IRQ_WAKE_THREAD always.


int hda_dsp_probe(struct snd_sof_dev *sdev)
{
struct pci_dev *pci = to_pci_dev(sdev->dev);
Expand Down Expand Up @@ -506,9 +528,7 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
*/
if (hda_use_msi && pci_alloc_irq_vectors(pci, 1, 1, PCI_IRQ_MSI) > 0) {
dev_info(sdev->dev, "use msi interrupt mode\n");
hdev->irq = pci_irq_vector(pci, 0);
/* ipc irq number is the same of hda irq */
sdev->ipc_irq = hdev->irq;
sdev->ipc_irq = pci_irq_vector(pci, 0);

@ranj063 ranj063 Nov 19, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao if this is a common irq, we should consider renaming it to maybe just irq?

@bardliao bardliao Nov 20, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I also think about it. However, for those platforms without HDA, the irq is still for ipc only. That's why I leave it as it is for now.

@ranj063 ranj063 Nov 21, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao even in that case it makes sense to rename it to something else. Maybe in another PR

/* initialised to "false" by kzalloc() */
sdev->msi_enabled = true;
}
Expand All @@ -519,28 +539,17 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)
* in IO-APIC mode, hda->irq and ipc_irq are using the same
* irq number of pci->irq
*/
hdev->irq = pci->irq;
sdev->ipc_irq = pci->irq;
}

dev_dbg(sdev->dev, "using HDA IRQ %d\n", hdev->irq);
ret = request_threaded_irq(hdev->irq, hda_dsp_stream_interrupt,
hda_dsp_stream_threaded_handler,
IRQF_SHARED, "AudioHDA", bus);
if (ret < 0) {
dev_err(sdev->dev, "error: failed to register HDA IRQ %d\n",
hdev->irq);
goto free_irq_vector;
}

dev_dbg(sdev->dev, "using IPC IRQ %d\n", sdev->ipc_irq);
ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_ipc_irq_handler,
sof_ops(sdev)->irq_thread, IRQF_SHARED,
"AudioDSP", sdev);
ret = request_threaded_irq(sdev->ipc_irq, hda_dsp_interrupt_handler,
hda_dsp_interrupt_thread,
IRQF_SHARED, "AudioDSP", sdev);

@kv2019i kv2019i Nov 19, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao Shouldn't this patch remove hda_dsp_ipc_irq_handler as well?

@bardliao bardliao Nov 20, 2019

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

hda_dsp_ipc_irq_handler is assigned to cnl and apl's irq_handler ops. But no one uses it. So, yes, we can remove hda_dsp_ipc_irq_handler as well.

if (ret < 0) {
dev_err(sdev->dev, "error: failed to register IPC IRQ %d\n",
sdev->ipc_irq);
goto free_hda_irq;
goto free_irq_vector;
}

pci_set_master(pci);
Expand Down Expand Up @@ -571,8 +580,6 @@ int hda_dsp_probe(struct snd_sof_dev *sdev)

free_ipc_irq:
free_irq(sdev->ipc_irq, sdev);
free_hda_irq:
free_irq(hdev->irq, bus);
free_irq_vector:
if (sdev->msi_enabled)
pci_free_irq_vectors(pci);
Expand Down Expand Up @@ -618,7 +625,6 @@ int hda_dsp_remove(struct snd_sof_dev *sdev)
SOF_HDA_PPCTL_GPROCEN, 0);

free_irq(sdev->ipc_irq, sdev);
free_irq(hda->irq, bus);
if (sdev->msi_enabled)
pci_free_irq_vectors(pci);

Expand Down
6 changes: 2 additions & 4 deletions sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,6 @@ struct sof_intel_hda_dev {
/* the maximum number of streams (playback + capture) supported */
u32 stream_max;

int irq;

/* PM related */
bool l1_support_changed;/* during suspend, is L1SEN changed or not */

Expand Down Expand Up @@ -511,11 +509,12 @@ int hda_dsp_stream_hw_params(struct snd_sof_dev *sdev,
struct snd_pcm_hw_params *params);
int hda_dsp_stream_trigger(struct snd_sof_dev *sdev,
struct hdac_ext_stream *stream, int cmd);
irqreturn_t hda_dsp_stream_interrupt(int irq, void *context);
irqreturn_t hda_dsp_stream_threaded_handler(int irq, void *context);
int hda_dsp_stream_setup_bdl(struct snd_sof_dev *sdev,
struct snd_dma_buffer *dmab,
struct hdac_stream *stream);
bool check_ipc(struct snd_sof_dev *sdev);
bool is_stream_irq(struct snd_sof_dev *sdev);

@kv2019i kv2019i Nov 21, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@bardliao I'd have uniform names to this (even if stream irq function is changed in a later patch). E.g. something like:
check_ipc_irq()
check_stream_irq()

@plbossart plbossart Nov 21, 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.

agree, I'll add an update to fix the naming but we should merge this today since it'll give us one working day to fix things before the week-end.


struct hdac_ext_stream *
hda_dsp_stream_get(struct snd_sof_dev *sdev, int direction);
Expand All @@ -540,7 +539,6 @@ void hda_dsp_ipc_get_reply(struct snd_sof_dev *sdev);
int hda_dsp_ipc_get_mailbox_offset(struct snd_sof_dev *sdev);
int hda_dsp_ipc_get_window_offset(struct snd_sof_dev *sdev, u32 id);

irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context);
irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context);
int hda_dsp_ipc_cmd_done(struct snd_sof_dev *sdev, int dir);

Expand Down