dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
@ 2025-04-24  6:56 Junxiao Chang
  2025-04-24  7:15 ` Sebastian Andrzej Siewior
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Junxiao Chang @ 2025-04-24  6:56 UTC (permalink / raw)
  To: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	David Airlie, Simona Vetter, Sebastian Andrzej Siewior,
	Clark Williams, Steven Rostedt, intel-gfx, dri-devel,
	linux-kernel, linux-rt-devel
  Cc: junxiao.chang

MEI GSC interrupt comes from i915. It has top half and bottom half.
Top half is called from i915 interrupt handler. It should be in
irq disabled context.

With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
top half might be in threaded IRQ context. In this case, local IRQ
should be disabled for MEI GSC interrupt top half.

This change fixes A380/A770 GPU boot hang issue with RT kernel.

Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gsc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index 1e925c75fb080..9c72117263f78 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -270,6 +270,9 @@ static void gsc_init_one(struct drm_i915_private *i915, struct intel_gsc *gsc,
 static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
 {
 	int ret;
+#ifdef CONFIG_PREEMPT_RT
+	int irq_disabled_flag;
+#endif
 
 	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
 		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
@@ -284,7 +287,18 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
 	if (gt->gsc.intf[intf_id].irq < 0)
 		return;
 
+#ifdef CONFIG_PREEMPT_RT
+	/* mei interrupt top half should run in irq disabled context */
+	irq_disabled_flag = irqs_disabled();
+	if (!irq_disabled_flag)
+		local_irq_disable();
+#endif
 	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
+#ifdef CONFIG_PREEMPT_RT
+	if (!irq_disabled_flag)
+		local_irq_enable();
+#endif
+
 	if (ret)
 		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-24  6:56 [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context Junxiao Chang
@ 2025-04-24  7:15 ` Sebastian Andrzej Siewior
  2025-04-24  7:30   ` Jani Nikula
  2025-04-25  6:04 ` Junxiao Chang
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2025-04-24  7:15 UTC (permalink / raw)
  To: Junxiao Chang
  Cc: Jani Nikula, Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin,
	David Airlie, Simona Vetter, Clark Williams, Steven Rostedt,
	intel-gfx, dri-devel, linux-kernel, linux-rt-devel

On 2025-04-24 14:56:08 [+0800], Junxiao Chang wrote:
> MEI GSC interrupt comes from i915. It has top half and bottom half.
> Top half is called from i915 interrupt handler. It should be in
> irq disabled context.
> 
> With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
> top half might be in threaded IRQ context. In this case, local IRQ
> should be disabled for MEI GSC interrupt top half.
> 
> This change fixes A380/A770 GPU boot hang issue with RT kernel.

This should have a Fixes when generic_handle_irq() was introduced. 

> Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gsc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
> index 1e925c75fb080..9c72117263f78 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -270,6 +270,9 @@ static void gsc_init_one(struct drm_i915_private *i915, struct intel_gsc *gsc,
>  static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>  {
>  	int ret;
> +#ifdef CONFIG_PREEMPT_RT
> +	int irq_disabled_flag;
> +#endif
>  
>  	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
>  		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
> @@ -284,7 +287,18 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>  	if (gt->gsc.intf[intf_id].irq < 0)
>  		return;
>  
> +#ifdef CONFIG_PREEMPT_RT
> +	/* mei interrupt top half should run in irq disabled context */
> +	irq_disabled_flag = irqs_disabled();
> +	if (!irq_disabled_flag)
> +		local_irq_disable();
> +#endif
>  	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);

What about generic_handle_irq_safe() instead the whole ifdef show?

> +#ifdef CONFIG_PREEMPT_RT
> +	if (!irq_disabled_flag)
> +		local_irq_enable();
> +#endif
> +
>  	if (ret)
>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>  }

Sebastian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-24  7:15 ` Sebastian Andrzej Siewior
@ 2025-04-24  7:30   ` Jani Nikula
  2025-04-24 10:53     ` Chang, Junxiao
  0 siblings, 1 reply; 13+ messages in thread
From: Jani Nikula @ 2025-04-24  7:30 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior, Junxiao Chang
  Cc: Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
	Simona Vetter, Clark Williams, Steven Rostedt, intel-gfx,
	dri-devel, linux-kernel, linux-rt-devel

On Thu, 24 Apr 2025, Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> On 2025-04-24 14:56:08 [+0800], Junxiao Chang wrote:
>> MEI GSC interrupt comes from i915. It has top half and bottom half.
>> Top half is called from i915 interrupt handler. It should be in
>> irq disabled context.
>> 
>> With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
>> top half might be in threaded IRQ context. In this case, local IRQ
>> should be disabled for MEI GSC interrupt top half.
>> 
>> This change fixes A380/A770 GPU boot hang issue with RT kernel.
>
> This should have a Fixes when generic_handle_irq() was introduced. 
>
>> Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/intel_gsc.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
>> index 1e925c75fb080..9c72117263f78 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
>> @@ -270,6 +270,9 @@ static void gsc_init_one(struct drm_i915_private *i915, struct intel_gsc *gsc,
>>  static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>>  {
>>  	int ret;
>> +#ifdef CONFIG_PREEMPT_RT
>> +	int irq_disabled_flag;
>> +#endif
>>  
>>  	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
>>  		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
>> @@ -284,7 +287,18 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>>  	if (gt->gsc.intf[intf_id].irq < 0)
>>  		return;
>>  
>> +#ifdef CONFIG_PREEMPT_RT
>> +	/* mei interrupt top half should run in irq disabled context */
>> +	irq_disabled_flag = irqs_disabled();
>> +	if (!irq_disabled_flag)
>> +		local_irq_disable();
>> +#endif
>>  	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
>
> What about generic_handle_irq_safe() instead the whole ifdef show?

Anything without the ifdefs would be welcome.

BR,
Jani.

>
>> +#ifdef CONFIG_PREEMPT_RT
>> +	if (!irq_disabled_flag)
>> +		local_irq_enable();
>> +#endif
>> +
>>  	if (ret)
>>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>>  }
>
> Sebastian

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-24  7:30   ` Jani Nikula
@ 2025-04-24 10:53     ` Chang, Junxiao
  2025-04-24 11:07       ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 13+ messages in thread
From: Chang, Junxiao @ 2025-04-24 10:53 UTC (permalink / raw)
  To: Jani Nikula, Sebastian Andrzej Siewior
  Cc: Joonas Lahtinen, Vivi, Rodrigo, Tvrtko Ursulin, David Airlie,
	Simona Vetter, Clark Williams, Steven Rostedt,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, linux-rt-devel@lists.linux.dev

On Thu, 24 Apr 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>On Thu, 24 Apr 2025, Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>wrote:
>> On 2025-04-24 14:56:08 [+0800], Junxiao Chang wrote:
>>> MEI GSC interrupt comes from i915. It has top half and bottom half.
>>> Top half is called from i915 interrupt handler. It should be in irq
>>> disabled context.
>>>
>>> With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI
>>> GSC top half might be in threaded IRQ context. In this case, local
>>> IRQ should be disabled for MEI GSC interrupt top half.
>>>
>>> This change fixes A380/A770 GPU boot hang issue with RT kernel.
>>
>> This should have a Fixes when generic_handle_irq() was introduced.

If PREEMPT_RT is disabled, original driver works fine. I prefer to not
add "Fixes:"?

>>
>>> Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/gt/intel_gsc.c | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c
>>> b/drivers/gpu/drm/i915/gt/intel_gsc.c
>>> index 1e925c75fb080..9c72117263f78 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
>>> @@ -270,6 +270,9 @@ static void gsc_init_one(struct drm_i915_private
>>> *i915, struct intel_gsc *gsc,  static void gsc_irq_handler(struct
>>> intel_gt *gt, unsigned int intf_id)  {
>>>  	int ret;
>>> +#ifdef CONFIG_PREEMPT_RT
>>> +	int irq_disabled_flag;
>>> +#endif
>>>
>>>  	if (intf_id >= INTEL_GSC_NUM_INTERFACES) {
>>>  		gt_warn_once(gt, "GSC irq: intf_id %d is out of range", intf_id);
>>> @@ -284,7 +287,18 @@ static void gsc_irq_handler(struct intel_gt *gt,
>unsigned int intf_id)
>>>  	if (gt->gsc.intf[intf_id].irq < 0)
>>>  		return;
>>>
>>> +#ifdef CONFIG_PREEMPT_RT
>>> +	/* mei interrupt top half should run in irq disabled context */
>>> +	irq_disabled_flag = irqs_disabled();
>>> +	if (!irq_disabled_flag)
>>> +		local_irq_disable();
>>> +#endif
>>>  	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
>>
>> What about generic_handle_irq_safe() instead the whole ifdef show?
>
>Anything without the ifdefs would be welcome.

Sebastain's suggestion looks very good. I just tried it, it works well with
both RT and non RT, it doesn't need ifdefs. I will do more testing.

>BR,
>Jani.
>
>>
>>> +#ifdef CONFIG_PREEMPT_RT
>>> +	if (!irq_disabled_flag)
>>> +		local_irq_enable();
>>> +#endif
>>> +
>>>  	if (ret)
>>>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);  }
>>
>> Sebastian
>
>--
>Jani Nikula, Intel
Thanks!
Junxiao

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: RE: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-24 10:53     ` Chang, Junxiao
@ 2025-04-24 11:07       ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2025-04-24 11:07 UTC (permalink / raw)
  To: Chang, Junxiao
  Cc: Jani Nikula, Joonas Lahtinen, Vivi, Rodrigo, Tvrtko Ursulin,
	David Airlie, Simona Vetter, Clark Williams, Steven Rostedt,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, linux-rt-devel@lists.linux.dev

On 2025-04-24 10:53:31 [+0000], Chang, Junxiao wrote:
> >> This should have a Fixes when generic_handle_irq() was introduced.
> 
> If PREEMPT_RT is disabled, original driver works fine. I prefer to not
> add "Fixes:"?

PREEMPT_RT is mainline. It deserves the same fixes as other parts of the
kernel.

Sebastian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-24  6:56 [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context Junxiao Chang
  2025-04-24  7:15 ` Sebastian Andrzej Siewior
@ 2025-04-25  6:04 ` Junxiao Chang
  2025-04-25  8:00   ` Jani Nikula
  2025-04-25  8:27   ` Sebastian Andrzej Siewior
  2025-04-25 12:04 ` Junxiao Chang
  2025-04-25 15:11 ` Junxiao Chang
  3 siblings, 2 replies; 13+ messages in thread
From: Junxiao Chang @ 2025-04-25  6:04 UTC (permalink / raw)
  To: tomas.winkler, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Tvrtko Ursulin, David Airlie, Simona Vetter,
	Sebastian Andrzej Siewior, Clark Williams, Steven Rostedt,
	Daniele Ceraolo Spurio, Vitaly Lubart, Alexander Usyskin,
	intel-gfx, dri-devel, linux-kernel, linux-rt-devel
  Cc: junxiao.chang, furong.zhou

MEI GSC interrupt comes from i915. It has top half and bottom half.
Top half is called from i915 interrupt handler. It should be in
irq disabled context.

With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
top half might be in threaded IRQ context. generic_handle_irq_safe API
could be called from either IRQ or process context, it disables local
IRQ then calls MEI GSC interrupt top half.

This change fixes A380/A770 GPU boot hang issue with RT kernel.

Fixes: 1e3dc1d8622b ("drm/i915/gsc: add gsc as a mei auxiliary device")
Tested-by: Furong Zhou <furong.zhou@intel.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gsc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index 1e925c75fb080..a099d885508ac 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -284,7 +284,9 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
 	if (gt->gsc.intf[intf_id].irq < 0)
 		return;
 
-	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
+	/* It can be called in both irq context and in thread context */
+	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);
+
 	if (ret)
 		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-25  6:04 ` Junxiao Chang
@ 2025-04-25  8:00   ` Jani Nikula
  2025-04-25  8:33     ` Chang, Junxiao
  2025-04-25  8:27   ` Sebastian Andrzej Siewior
  1 sibling, 1 reply; 13+ messages in thread
From: Jani Nikula @ 2025-04-25  8:00 UTC (permalink / raw)
  To: Junxiao Chang, tomas.winkler, Joonas Lahtinen, Rodrigo Vivi,
	Tvrtko Ursulin, David Airlie, Simona Vetter,
	Sebastian Andrzej Siewior, Clark Williams, Steven Rostedt,
	Daniele Ceraolo Spurio, Vitaly Lubart, Alexander Usyskin,
	intel-gfx, dri-devel, linux-kernel, linux-rt-devel
  Cc: junxiao.chang, furong.zhou

On Fri, 25 Apr 2025, Junxiao Chang <junxiao.chang@intel.com> wrote:
> MEI GSC interrupt comes from i915. It has top half and bottom half.
> Top half is called from i915 interrupt handler. It should be in
> irq disabled context.
>
> With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
> top half might be in threaded IRQ context. generic_handle_irq_safe API
> could be called from either IRQ or process context, it disables local
> IRQ then calls MEI GSC interrupt top half.
>
> This change fixes A380/A770 GPU boot hang issue with RT kernel.
>
> Fixes: 1e3dc1d8622b ("drm/i915/gsc: add gsc as a mei auxiliary device")
> Tested-by: Furong Zhou <furong.zhou@intel.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gsc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
> index 1e925c75fb080..a099d885508ac 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -284,7 +284,9 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>  	if (gt->gsc.intf[intf_id].irq < 0)
>  		return;
>  
> -	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> +	/* It can be called in both irq context and in thread context */

What is "It" in this case?

> +	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);
> +
>  	if (ret)
>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>  }

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-25  6:04 ` Junxiao Chang
  2025-04-25  8:00   ` Jani Nikula
@ 2025-04-25  8:27   ` Sebastian Andrzej Siewior
  1 sibling, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2025-04-25  8:27 UTC (permalink / raw)
  To: Junxiao Chang
  Cc: tomas.winkler, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Tvrtko Ursulin, David Airlie, Simona Vetter, Clark Williams,
	Steven Rostedt, Daniele Ceraolo Spurio, Vitaly Lubart,
	Alexander Usyskin, intel-gfx, dri-devel, linux-kernel,
	linux-rt-devel, furong.zhou

On 2025-04-25 14:04:54 [+0800], Junxiao Chang wrote:
> MEI GSC interrupt comes from i915. It has top half and bottom half.
> Top half is called from i915 interrupt handler. It should be in
> irq disabled context.
> 
> With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
> top half might be in threaded IRQ context. generic_handle_irq_safe API
> could be called from either IRQ or process context, it disables local
> IRQ then calls MEI GSC interrupt top half.
> 
> This change fixes A380/A770 GPU boot hang issue with RT kernel.
> 
> Fixes: 1e3dc1d8622b ("drm/i915/gsc: add gsc as a mei auxiliary device")
> Tested-by: Furong Zhou <furong.zhou@intel.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gsc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
> index 1e925c75fb080..a099d885508ac 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -284,7 +284,9 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>  	if (gt->gsc.intf[intf_id].irq < 0)
>  		return;
>  
> -	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> +	/* It can be called in both irq context and in thread context */

I don't know why this deserves a comment. However, generic_handle_irq()
is used from the IRQ chip, everything that signals the interrupt. This,
if it comes from an interrupt handler itself, should use the _safe()
variant. It used to be a issue also with threaded interrupts on !RT but
for other reasons this is no longer the case.

> +	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);
> +
>  	if (ret)
>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>  }

Sebastian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* RE: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-25  8:00   ` Jani Nikula
@ 2025-04-25  8:33     ` Chang, Junxiao
  0 siblings, 0 replies; 13+ messages in thread
From: Chang, Junxiao @ 2025-04-25  8:33 UTC (permalink / raw)
  To: Jani Nikula, tomas.winkler@intel.com, Joonas Lahtinen,
	Vivi, Rodrigo, Tvrtko Ursulin, David Airlie, Simona Vetter,
	Sebastian Andrzej Siewior, Clark Williams, Steven Rostedt,
	Ceraolo Spurio, Daniele, Vitaly Lubart, Usyskin, Alexander,
	intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, linux-rt-devel@lists.linux.dev
  Cc: Zhou, Furong

On Fri, 25 Apr 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>On Fri, 25 Apr 2025, Junxiao Chang <junxiao.chang@intel.com> wrote:
>> MEI GSC interrupt comes from i915. It has top half and bottom half.
>>
>> -	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
>> +	/* It can be called in both irq context and in thread context */
>
>What is "It" in this case?
"It" means GSC interrupt handler, which is called via below API generic_handle_irq_safe. Sebastain has a comment on it as well that it doesn't deserves a comment. I could delete this comment.
>
>> +	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);
>> +
>>  	if (ret)
>>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);  }
>
>--
>Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-24  6:56 [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context Junxiao Chang
  2025-04-24  7:15 ` Sebastian Andrzej Siewior
  2025-04-25  6:04 ` Junxiao Chang
@ 2025-04-25 12:04 ` Junxiao Chang
  2025-04-25 12:38   ` Sebastian Andrzej Siewior
  2025-04-25 15:11 ` Junxiao Chang
  3 siblings, 1 reply; 13+ messages in thread
From: Junxiao Chang @ 2025-04-25 12:04 UTC (permalink / raw)
  To: tomas.winkler, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Tvrtko Ursulin, David Airlie, Simona Vetter,
	Sebastian Andrzej Siewior, Clark Williams, Steven Rostedt,
	Daniele Ceraolo Spurio, Vitaly Lubart, Alexander Usyskin,
	intel-gfx, dri-devel, linux-kernel, linux-rt-devel
  Cc: junxiao.chang, furong.zhou

MEI GSC interrupt comes from i915. It has top half and bottom half.
Top half is called from i915 interrupt handler. It should be in
irq disabled context.

With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
top half might be in threaded IRQ context. generic_handle_irq_safe API
could be called from either IRQ or process context, it disables local
IRQ then calls MEI GSC interrupt top half.

This change fixes A380/A770 GPU boot hang issue with RT kernel.

Fixes: 1e3dc1d8622b ("drm/i915/gsc: add gsc as a mei auxiliary device")
Tested-by: Furong Zhou <furong.zhou@intel.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gsc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index 1e925c75fb080..ad63abf1236a8 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -284,7 +284,8 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
 	if (gt->gsc.intf[intf_id].irq < 0)
 		return;
 
-	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
+	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);
+
 	if (ret)
 		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-25 12:04 ` Junxiao Chang
@ 2025-04-25 12:38   ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 13+ messages in thread
From: Sebastian Andrzej Siewior @ 2025-04-25 12:38 UTC (permalink / raw)
  To: Junxiao Chang
  Cc: tomas.winkler, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Tvrtko Ursulin, David Airlie, Simona Vetter, Clark Williams,
	Steven Rostedt, Daniele Ceraolo Spurio, Vitaly Lubart,
	Alexander Usyskin, intel-gfx, dri-devel, linux-kernel,
	linux-rt-devel, furong.zhou

On 2025-04-25 20:04:43 [+0800], Junxiao Chang wrote:
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -284,7 +284,8 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>  	if (gt->gsc.intf[intf_id].irq < 0)
>  		return;
>  
> -	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> +	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);
> +

that extra line looks odd, other than that
Acked-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

>  	if (ret)
>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>  }

Sebastian

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-24  6:56 [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context Junxiao Chang
                   ` (2 preceding siblings ...)
  2025-04-25 12:04 ` Junxiao Chang
@ 2025-04-25 15:11 ` Junxiao Chang
  2025-06-30 18:22   ` Rodrigo Vivi
  3 siblings, 1 reply; 13+ messages in thread
From: Junxiao Chang @ 2025-04-25 15:11 UTC (permalink / raw)
  To: tomas.winkler, Jani Nikula, Joonas Lahtinen, Rodrigo Vivi,
	Tvrtko Ursulin, David Airlie, Simona Vetter,
	Sebastian Andrzej Siewior, Clark Williams, Steven Rostedt,
	Daniele Ceraolo Spurio, Vitaly Lubart, Alexander Usyskin,
	intel-gfx, dri-devel, linux-kernel, linux-rt-devel
  Cc: junxiao.chang, furong.zhou

MEI GSC interrupt comes from i915. It has top half and bottom half.
Top half is called from i915 interrupt handler. It should be in
irq disabled context.

With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
top half might be in threaded IRQ context. generic_handle_irq_safe API
could be called from either IRQ or process context, it disables local
IRQ then calls MEI GSC interrupt top half.

This change fixes A380/A770 GPU boot hang issue with RT kernel.

Fixes: 1e3dc1d8622b ("drm/i915/gsc: add gsc as a mei auxiliary device")
Tested-by: Furong Zhou <furong.zhou@intel.com>
Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gsc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
index 1e925c75fb080..c43febc862dc3 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -284,7 +284,7 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
 	if (gt->gsc.intf[intf_id].irq < 0)
 		return;
 
-	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
+	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);
 	if (ret)
 		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context
  2025-04-25 15:11 ` Junxiao Chang
@ 2025-06-30 18:22   ` Rodrigo Vivi
  0 siblings, 0 replies; 13+ messages in thread
From: Rodrigo Vivi @ 2025-06-30 18:22 UTC (permalink / raw)
  To: Junxiao Chang
  Cc: tomas.winkler, Jani Nikula, Joonas Lahtinen, Tvrtko Ursulin,
	David Airlie, Simona Vetter, Sebastian Andrzej Siewior,
	Clark Williams, Steven Rostedt, Daniele Ceraolo Spurio,
	Vitaly Lubart, Alexander Usyskin, intel-gfx, dri-devel,
	linux-kernel, linux-rt-devel, furong.zhou

On Fri, Apr 25, 2025 at 11:11:07PM +0800, Junxiao Chang wrote:
> MEI GSC interrupt comes from i915. It has top half and bottom half.
> Top half is called from i915 interrupt handler. It should be in
> irq disabled context.
> 
> With RT kernel, by default i915 IRQ handler is in threaded IRQ. MEI GSC
> top half might be in threaded IRQ context. generic_handle_irq_safe API
> could be called from either IRQ or process context, it disables local
> IRQ then calls MEI GSC interrupt top half.
> 
> This change fixes A380/A770 GPU boot hang issue with RT kernel.
> 
> Fixes: 1e3dc1d8622b ("drm/i915/gsc: add gsc as a mei auxiliary device")
> Tested-by: Furong Zhou <furong.zhou@intel.com>
> Suggested-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Junxiao Chang <junxiao.chang@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gsc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c b/drivers/gpu/drm/i915/gt/intel_gsc.c
> index 1e925c75fb080..c43febc862dc3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gsc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
> @@ -284,7 +284,7 @@ static void gsc_irq_handler(struct intel_gt *gt, unsigned int intf_id)
>  	if (gt->gsc.intf[intf_id].irq < 0)
>  		return;
>  
> -	ret = generic_handle_irq(gt->gsc.intf[intf_id].irq);
> +	ret = generic_handle_irq_safe(gt->gsc.intf[intf_id].irq);


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
and pushed to drm-intel-gt-next

>  	if (ret)
>  		gt_err_ratelimited(gt, "error handling GSC irq: %d\n", ret);
>  }
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2025-06-30 18:23 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-24  6:56 [PATCH] drm/i915/gsc: mei interrupt top half should be in irq disabled context Junxiao Chang
2025-04-24  7:15 ` Sebastian Andrzej Siewior
2025-04-24  7:30   ` Jani Nikula
2025-04-24 10:53     ` Chang, Junxiao
2025-04-24 11:07       ` Sebastian Andrzej Siewior
2025-04-25  6:04 ` Junxiao Chang
2025-04-25  8:00   ` Jani Nikula
2025-04-25  8:33     ` Chang, Junxiao
2025-04-25  8:27   ` Sebastian Andrzej Siewior
2025-04-25 12:04 ` Junxiao Chang
2025-04-25 12:38   ` Sebastian Andrzej Siewior
2025-04-25 15:11 ` Junxiao Chang
2025-06-30 18:22   ` Rodrigo Vivi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).