[PATCH v1] drm/i915/guc: Always disable interrupt ahead of synchronize_irq

[Andi Shyti]

Hi Zhanjun,

On Thu, Jan 23, 2025 at 08:23:51AM -0800, Zhanjun Dong wrote:
> The purpose of synchronize_irq is to wait for any pending IRQ handlers for the
> interrupt to complete, if synchronize_irq called before interrupt disabled, an
> tiny timing window created, where no more pending IRQ, but interrupt not
> disabled yet. Meanwhile, if the interrupt event happened in this timing window,
> an unexpected IRQ handling will be triggered.

I get the meaning, but could you please rephrase it to a more
meaningful sentence, please?

> Fixed by always disable interrupt ahead of synchronize_irq.

Please, don't use the past simple form, use the imperative form:

"Always disable interrupt ahead..."

or better:

"Always disable interrupts before calling intel_synchronyze_irq()"

> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13454
> Fixes: 26705e20752a ("drm/i915: Support for GuC interrupts")
> Fixes: 54c52a841250 ("drm/i915/guc: Correctly handle GuC interrupts on Gen11")
> Fixes: 2ae096872a2c ("drm/i915/pxp: Implement PXP irq handler")
> Fixes: 3e7abf814193 ("drm/i915: Extract GT render power state management")

There is an issue here, each fixes is introduced in different
kernel versions and they can't be mixed all together as it would
be impossible to backport them to the stable brances.

E.g.:
Fixes: 3e7abf814193 ("drm/i915: Extract GT render power state management")
Cc: <stable at vger.kernel.org> # v5.5+

Fixes: 2ae096872a2c ("drm/i915/pxp: Implement PXP irq handler")
Cc: <stable at vger.kernel.org> # v5.16+

Fixes: 54c52a841250 ("drm/i915/guc: Correctly handle GuC interrupts on Gen11")
Cc: <stable at vger.kernel.org> # v5.3+

Fixes: 26705e20752a ("drm/i915: Support for GuC interrupts")
Cc: <stable at vger.kernel.org> # v4.10+

Could you please split this patch in the four different patches
that address the four different fixes?

> 

No blank lines in the tag section, please.

> Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
> 
> ---
> Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Andi Shyti <andi.shyti at intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_rps.c      | 3 +--
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c   | 4 ++--
>  drivers/gpu/drm/i915/pxp/intel_pxp_irq.c | 2 +-
>  3 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
> index fa304ea088e4..0fe7a8d7f460 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> @@ -244,8 +244,8 @@ static void rps_disable_interrupts(struct intel_rps *rps)
>  	gen6_gt_pm_disable_irq(gt, GEN6_PM_RPS_EVENTS);
>  	spin_unlock_irq(gt->irq_lock);
>  
> +	rps_reset_interrupts(rps);
>  	intel_synchronize_irq(gt->i915);
> -

Sebastian has already commented in his review, but please don't
remove this blank line.

Andi

>  	/*
>  	 * Now that we will not be generating any more work, flush any
>  	 * outstanding tasks. As we are called on the RPS idle path,