| Message ID | 20250326-topic-ufs-use-threaded-irq-v2-2-7b3e8a5037e6@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ufs: core: cleanup and threaded irq handler | expand |
On 3/26/25 4:36 AM, Neil Armstrong wrote: > When MCQ & Interrupt Aggregation are supported, the interrupt > are directly handled in the "hard" interrupt routine to > keep IOPs high since queues handling is done in separate > per-queue interrupt routines. The above explanation suggests that I/O completions are handled by the modified interrupt handler. This is not necessarily the case. With MCQ, I/O completions are either handled by dedicated interrupts or by the legacy interrupt handler. > Reported bandwidth is not affected on various tests. This kind of patch can only affect command completion latency but not the bandwidth, isn't it? > +/** > + * ufshcd_intr - Main interrupt service routine > + * @irq: irq number > + * @__hba: pointer to adapter instance > + * > + * Return: > + * IRQ_HANDLED - If interrupt is valid > + * IRQ_WAKE_THREAD - If handling is moved to threaded handled > + * IRQ_NONE - If invalid interrupt > + */ > +static irqreturn_t ufshcd_intr(int irq, void *__hba) > +{ > + struct ufs_hba *hba = __hba; > + > + /* > + * Move interrupt handling to thread when MCQ is not supported > + * or when Interrupt Aggregation is not supported, leading to > + * potentially longer interrupt handling. > + */ > + if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba)) > + return IRQ_WAKE_THREAD; > + > + /* Directly handle interrupts since MCQ handlers does the hard job */ > + return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) & > + ufshcd_readl(hba, REG_INTERRUPT_ENABLE)); > +} Where has ufshcd_is_intr_aggr_allowed() been defined? I can't find this function. For the MCQ case, this patch removes the loop from around ufshcd_sl_intr() without explaining in the patch description why this change has been made. Please explain all changes in the patch description. Thanks, Bart.
Hi, On 27/03/2025 12:56, Bart Van Assche wrote: > On 3/26/25 4:36 AM, Neil Armstrong wrote: > > When MCQ & Interrupt Aggregation are supported, the interrupt > > are directly handled in the "hard" interrupt routine to > > keep IOPs high since queues handling is done in separate > > per-queue interrupt routines. > > The above explanation suggests that I/O completions are handled by the > modified interrupt handler. This is not necessarily the case. With MCQ, > I/O completions are either handled by dedicated interrupts or by the > legacy interrupt handler. Will update the sentence with that > >> Reported bandwidth is not affected on various tests. > > This kind of patch can only affect command completion latency but not > the bandwidth, isn't it? Yes, but on a fully loaded system, it will enhance bandwidth but with a greater latency, but without eating irq handling time for other routines. > >> +/** >> + * ufshcd_intr - Main interrupt service routine >> + * @irq: irq number >> + * @__hba: pointer to adapter instance >> + * >> + * Return: >> + * IRQ_HANDLED - If interrupt is valid >> + * IRQ_WAKE_THREAD - If handling is moved to threaded handled >> + * IRQ_NONE - If invalid interrupt >> + */ >> +static irqreturn_t ufshcd_intr(int irq, void *__hba) >> +{ >> + struct ufs_hba *hba = __hba; >> + >> + /* >> + * Move interrupt handling to thread when MCQ is not supported >> + * or when Interrupt Aggregation is not supported, leading to >> + * potentially longer interrupt handling. >> + */ >> + if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba)) >> + return IRQ_WAKE_THREAD; >> + >> + /* Directly handle interrupts since MCQ handlers does the hard job */ >> + return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) & >> + ufshcd_readl(hba, REG_INTERRUPT_ENABLE)); >> +} > > Where has ufshcd_is_intr_aggr_allowed() been defined? I can't find this > function. It's in include/ufs/ufshcd.h > > For the MCQ case, this patch removes the loop from around > ufshcd_sl_intr() without explaining in the patch description why this change has been made. Please explain all changes in the patch > description. Ack will update explaining this change. Thanks, Neil > > Thanks, > > Bart.
On 3/26/25 1:36 AM, Neil Armstrong wrote: > +static irqreturn_t ufshcd_intr(int irq, void *__hba) > +{ > + struct ufs_hba *hba = __hba; > + > + /* > + * Move interrupt handling to thread when MCQ is not supported > + * or when Interrupt Aggregation is not supported, leading to > + * potentially longer interrupt handling. > + */ > + if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba)) > + return IRQ_WAKE_THREAD; > + > + /* Directly handle interrupts since MCQ handlers does the hard job */ > + return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) & > + ufshcd_readl(hba, REG_INTERRUPT_ENABLE)); > +} Calling ufshcd_is_intr_aggr_allowed() from the above interrupt handler seems wrong to me. I think you want to check whether or not ESI has been disabled since only if ESI is disabled all I/O completions are handled by a single interrupt if MCQ is enabled. Thanks, Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 5e73ac1e00788f3d599f0b3eb6e2806df9b6f6c3..5de25fc978dd7c4c1ac3b9ccbca2ab3f13d6aa65 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -6971,7 +6971,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status) } /** - * ufshcd_intr - Main interrupt service routine + * ufshcd_threaded_intr - Threaded interrupt service routine * @irq: irq number * @__hba: pointer to adapter instance * @@ -6979,7 +6979,7 @@ static irqreturn_t ufshcd_sl_intr(struct ufs_hba *hba, u32 intr_status) * IRQ_HANDLED - If interrupt is valid * IRQ_NONE - If invalid interrupt */ -static irqreturn_t ufshcd_intr(int irq, void *__hba) +static irqreturn_t ufshcd_threaded_intr(int irq, void *__hba) { u32 last_intr_status, intr_status, enabled_intr_status = 0; irqreturn_t retval = IRQ_NONE; @@ -7018,6 +7018,33 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba) return retval; } +/** + * ufshcd_intr - Main interrupt service routine + * @irq: irq number + * @__hba: pointer to adapter instance + * + * Return: + * IRQ_HANDLED - If interrupt is valid + * IRQ_WAKE_THREAD - If handling is moved to threaded handled + * IRQ_NONE - If invalid interrupt + */ +static irqreturn_t ufshcd_intr(int irq, void *__hba) +{ + struct ufs_hba *hba = __hba; + + /* + * Move interrupt handling to thread when MCQ is not supported + * or when Interrupt Aggregation is not supported, leading to + * potentially longer interrupt handling. + */ + if (!is_mcq_supported(hba) || !ufshcd_is_intr_aggr_allowed(hba)) + return IRQ_WAKE_THREAD; + + /* Directly handle interrupts since MCQ handlers does the hard job */ + return ufshcd_sl_intr(hba, ufshcd_readl(hba, REG_INTERRUPT_STATUS) & + ufshcd_readl(hba, REG_INTERRUPT_ENABLE)); +} + static int ufshcd_clear_tm_cmd(struct ufs_hba *hba, int tag) { int err = 0; @@ -10576,7 +10603,8 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) ufshcd_readl(hba, REG_INTERRUPT_ENABLE); /* IRQ registration */ - err = devm_request_irq(dev, irq, ufshcd_intr, IRQF_SHARED, UFSHCD, hba); + err = devm_request_threaded_irq(dev, irq, ufshcd_intr, ufshcd_threaded_intr, + IRQF_ONESHOT | IRQF_SHARED, UFSHCD, hba); if (err) { dev_err(hba->dev, "request irq failed\n"); goto out_disable;
On systems with a large number request slots and unavailable MCQ, the current design of the interrupt handler can delay handling of other subsystems interrupts causing display artifacts, GPU stalls or system firmware requests timeouts. Since the interrupt routine can take quite some time, it's preferable to move it to a threaded handler and leave the hard interrupt handler save the status and disable the irq until processing is finished in the thread. When MCQ & Interrupt Aggregation are supported, the interrupt are directly handled in the "hard" interrupt routine to keep IOPs high since queues handling is done in separate per-queue interrupt routines. This fixes all encountered issued when running FIO tests on the Qualcomm SM8650 platform. Example of errors reported on a loaded system: [drm:dpu_encoder_frame_done_timeout:2706] [dpu error]enc32 frame done timeout msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1: hangcheck detected gpu lockup rb 2! msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1: completed fence: 74285 msm_dpu ae01000.display-controller: [drm:hangcheck_handler [msm]] *ERROR* 67.5.20.1: submitted fence: 74286 Error sending AMC RPMH requests (-110) Reported bandwidth is not affected on various tests. Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/ufs/core/ufshcd.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-)