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

[plbossart]

We have a Global Interrupt Enable (GIE) flag, which can be used to
mask all possible interrupts instead of disabling each possible source
of interrupts.

Since we have a shared interrupt, the proposal is to use a single
mask, merge all handlers together and test the sources of interrupts
without any racy behavior.

Credits to Bard Liao for most of the ideas, I just experimented with
the GIE flag.

Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

[plbossart]

SOFCI TEST

[ranj063]

looks good to me

[plbossart]

@fredoh9 @xiulipan is CI offline today?

[bardliao]

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.

[bardliao]

Thanks @plbossart LGTM. I will test it on my side.

[plbossart]

tests with SoundWire show the IPC issues are gone, so merging to make sure everyone can test further.

[kv2019i]

@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?

[plbossart]

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.

[bardliao]

@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.

[plbossart]

@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

[bardliao]

@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

[lyakh]

@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.

[plbossart]

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;
}