Skip to content

[TEST][ [RFC] ASoC: SOF: Intel: hda: use global interrupt enable/disable #1547

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 26, 2019
Merged

[TEST][ [RFC] ASoC: SOF: Intel: hda: use global interrupt enable/disable #1547

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
4 changes: 0 additions & 4 deletions sound/soc/sof/intel/cnl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,6 @@ static irqreturn_t cnl_ipc_irq_thread(int irq, void *context)
"nothing to do in IPC IRQ thread\n");
}

/* re-enable IPC interrupt */
snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPIC,
HDA_DSP_ADSPIC_IPC, HDA_DSP_ADSPIC_IPC);

return IRQ_HANDLED;
}

Expand Down
11 changes: 2 additions & 9 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 @@ -230,14 +230,10 @@ irqreturn_t hda_dsp_ipc_irq_thread(int irq, void *context)
"nothing to do in IPC IRQ thread\n");
}

/* re-enable IPC interrupt */
snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPIC,
HDA_DSP_ADSPIC_IPC, HDA_DSP_ADSPIC_IPC);

return IRQ_HANDLED;
}

/* Check if it is an IPC and disable IPC interrupt if yes */
/* Check if an IPC IRQ occurred */
bool hda_dsp_check_ipc_irq(struct snd_sof_dev *sdev)
{
bool ret = false;
Expand All @@ -256,10 +252,7 @@ bool hda_dsp_check_ipc_irq(struct snd_sof_dev *sdev)

/* IPC message ? */
if (irq_status & HDA_DSP_ADSPIS_IPC) {
/* disable IPC interrupt */
snd_sof_dsp_update_bits_unlocked(sdev, HDA_DSP_BAR,
HDA_DSP_REG_ADSPIC,
HDA_DSP_ADSPIC_IPC, 0);
sdev->irq_event |= SOF_HDA_IRQ_IPC;
ret = true;
}

Expand Down
10 changes: 6 additions & 4 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 @@ -553,7 +553,7 @@ int hda_dsp_stream_hw_free(struct snd_sof_dev *sdev,
bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
{
struct hdac_bus *bus = sof_to_bus(sdev);
bool ret = true;
bool ret = false;
u32 status;

/* The function can be called at irq thread, so use spin_lock_irq */
Expand All @@ -562,9 +562,11 @@ bool hda_dsp_check_stream_irq(struct snd_sof_dev *sdev)
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 = false;
/* if Register inaccessible, ignore it.*/
if (status != 0xffffffff) {
sdev->irq_event |= SOF_HDA_IRQ_STREAM;
ret = true;
}

spin_unlock_irq(&bus->reg_lock);

Expand Down
31 changes: 26 additions & 5 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,9 +406,20 @@ static irqreturn_t hda_dsp_interrupt_handler(int irq, void *context)
{
struct snd_sof_dev *sdev = context;

if (hda_dsp_check_ipc_irq(sdev) ||
hda_dsp_check_stream_irq(sdev))
/* clear flags for interrupt sources */
sdev->irq_event = 0;

if (hda_dsp_check_stream_irq(sdev) ||
hda_dsp_check_ipc_irq(sdev)) {
Copy link
Collaborator

@bardliao bardliao Nov 26, 2019

Choose a reason for hiding this comment

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

Just note that hda_dsp_check_ipc_irq() will not be called if hda_dsp_check_stream_irq() return true. However hda_dsp_check_ipc_irq() will be called in hda_dsp_interrupt_thread() in that case. So, indeed, we don't need to check remaining event in the handler when there is already one event is confirmed.


/* disable GIE interrupt */
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
SOF_HDA_INTCTL,
SOF_HDA_INT_GLOBAL_EN,
0);

return IRQ_WAKE_THREAD;
}

return IRQ_NONE;
}
Expand All @@ -417,11 +428,21 @@ static irqreturn_t hda_dsp_interrupt_thread(int irq, void *context)
{
struct snd_sof_dev *sdev = context;

if (hda_dsp_check_ipc_irq(sdev))
sof_ops(sdev)->irq_thread(irq, sdev);
if (hda_dsp_check_stream_irq(sdev))
/* deal with streams and controller first */
if (sdev->irq_event & SOF_HDA_IRQ_STREAM ||
hda_dsp_check_stream_irq(sdev))
Copy link
Collaborator

@kv2019i kv2019i Nov 26, 2019

Choose a reason for hiding this comment

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

@plbossart @bardliao I was late to review as this is already merged, but isn't the "irq_event" flag superfluous here when we anyways call the check_foobar_irq() function in the thread?

Copy link
Member Author

@plbossart plbossart Nov 26, 2019

Choose a reason for hiding this comment

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

it's a minor optimization, if we know the source needs to be dealt with we don't recheck in the thread. we can remove this if no one thinks it's useful, I didn't share the code upstream yet to allow for additional fixes/improvements.

The other point I am not sure about is if we need to use snd_sof_dsp_update_bits() or snd_sof_dsp_update_bits_unlocked() for the GIE changes. it does look like I am using the same hw_lock twice.

Copy link
Collaborator

@bardliao bardliao Nov 27, 2019

Choose a reason for hiding this comment

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

@kv2019i @plbossart Can we check Global Interrupt Status (GIS) in the handler if we don't use irq_event? Also, we may not need to lock sdev->hw_lock in hda_dsp_check_ipc_irq() any more. I will test it today.

Copy link
Member Author

@plbossart plbossart Nov 27, 2019

Choose a reason for hiding this comment

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

@kv2019i @plbossart Can we check Global Interrupt Status (GIS) in the handler if we don't use irq_event? Also, we may not need to lock sdev->hw_lock in hda_dsp_check_ipc_irq() any more. I will test it today.

sorry, can you elaborate, I didn't get your question @bardliao

Copy link
Collaborator

@bardliao bardliao Nov 27, 2019

Choose a reason for hiding this comment

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

@kv2019i @plbossart Can we check Global Interrupt Status (GIS) in the handler if we don't use irq_event? Also, we may not need to lock sdev->hw_lock in hda_dsp_check_ipc_irq() any more. I will test it today.

sorry, can you elaborate, I didn't get your question @bardliao

@plbossart Please check #1554

hda_dsp_stream_threaded_handler(irq, sdev);

if (sdev->irq_event & SOF_HDA_IRQ_IPC ||
hda_dsp_check_ipc_irq(sdev))
sof_ops(sdev)->irq_thread(irq, sdev);

Copy link
Collaborator

@lyakh lyakh Jan 9, 2020

Choose a reason for hiding this comment

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

@plbossart Sorry for missing this PR originally. Don't we have a race here? I was reviewing #1491 so I went to refresh my memory and re-read #1042 and there are explanations there, why in the IRQ thread the hard interrupt has to be enabled before processing it. If an edge-triggered interrupt hits after the thread has read the status for the last time and before the interrupt is re-enabled, we lose it.

Copy link
Member Author

@plbossart plbossart Jan 9, 2020

Choose a reason for hiding this comment

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

the interrupts are disabled before the thread reads the status, and re-enabled after the threads deal with all status, so not sure what race conditions you are seeing.
This code is also completely obsolete, please refer to the current tree

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

	/*
	 * Get global interrupt status. It includes all hardware interrupt
	 * sources in the Intel HD Audio controller.
	 */
	if (snd_sof_dsp_read(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTSTS) &
	    SOF_HDA_INTSTS_GIS) {

		/* disable GIE interrupt */
		snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
					SOF_HDA_INTCTL,
					SOF_HDA_INT_GLOBAL_EN,
					0);

		return IRQ_WAKE_THREAD;
	}

	return IRQ_NONE;
}

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

	/* deal with streams and controller first */
	if (hda_dsp_check_stream_irq(sdev))
		hda_dsp_stream_threaded_handler(irq, sdev);

	if (hda_dsp_check_ipc_irq(sdev))
		sof_ops(sdev)->irq_thread(irq, sdev);

	/* enable GIE interrupt */
	snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
				SOF_HDA_INTCTL,
				SOF_HDA_INT_GLOBAL_EN,
				SOF_HDA_INT_GLOBAL_EN);

	return IRQ_HANDLED;
}

/* enable GIE interrupt */
snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR,
SOF_HDA_INTCTL,
SOF_HDA_INT_GLOBAL_EN,
SOF_HDA_INT_GLOBAL_EN);

return IRQ_HANDLED;
}

Expand Down
5 changes: 5 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 @@ -389,6 +389,11 @@ struct sof_intel_dsp_bdl {
#define SOF_HDA_PLAYBACK 0
#define SOF_HDA_CAPTURE 1

/* flags to memorize IPC source (not hardware-defined) */
#define SOF_HDA_IRQ_IPC BIT(0)
#define SOF_HDA_IRQ_STREAM BIT(1)
#define SOF_HDA_IRQ_SDW BIT(2)

/* represents DSP HDA controller frontend - i.e. host facing control */
struct sof_intel_hda_dev {

Expand Down
1 change: 1 addition & 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 @@ -387,6 +387,7 @@ struct snd_sof_dev {
u32 dtrace_draining;

bool msi_enabled;
u32 irq_event;

void *private; /* core does not touch this */
};
Expand Down