[PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Nirmoy Das]

Flush xe ordered_wq in case of ufence timeout which is observed
on LNL and that points to the recent scheduling issue with E-cores.

This is similar to the recent fix:
commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
response timeout") and should be removed once there is E core
scheduling fix.

v2: Add platform check(Himal)
    s/__flush_workqueue/flush_workqueue(Jani)

Cc: Badal Nilawar <badal.nilawar@intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: <stable@vger.kernel.org> # v6.11+
Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
Suggested-by: Matthew Brost <matthew.brost@intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
index f5deb81eba01..78a0ad3c78fe 100644
--- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
+++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
@@ -13,6 +13,7 @@
 #include "xe_device.h"
 #include "xe_gt.h"
 #include "xe_macros.h"
+#include "compat-i915-headers/i915_drv.h"
 #include "xe_exec_queue.h"
 
 static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
@@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
 		}
 
 		if (!timeout) {
+			if (IS_LUNARLAKE(xe)) {
+				/*
+				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
+				 * worker in case of g2h response timeout")
+				 *
+				 * TODO: Drop this change once workqueue scheduling delay issue is
+				 * fixed on LNL Hybrid CPU.
+				 */
+				flush_workqueue(xe->ordered_wq);
+				err = do_compare(addr, args->value, args->mask, args->op);
+				if (err <= 0)
+					break;
+			}
 			err = -ETIME;
 			break;
 		}
-- 
2.46.0

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Nirmoy Das]

On 10/24/2024 5:18 PM, Nirmoy Das wrote:
> Flush xe ordered_wq in case of ufence timeout which is observed
> on LNL and that points to the recent scheduling issue with E-cores.
>
> This is similar to the recent fix:
> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
> response timeout") and should be removed once there is E core
> scheduling fix.
>
> v2: Add platform check(Himal)
>     s/__flush_workqueue/flush_workqueue(Jani)
>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: <stable@vger.kernel.org> # v6.11+
> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> index f5deb81eba01..78a0ad3c78fe 100644
> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> @@ -13,6 +13,7 @@
>  #include "xe_device.h"
>  #include "xe_gt.h"
>  #include "xe_macros.h"
> +#include "compat-i915-headers/i915_drv.h"

Sorry sent too soon. This is bit out of place. I will sort it and resend after sometime to accumulate reviews.


>  #include "xe_exec_queue.h"
>  
>  static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
> @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>  		}
>  
>  		if (!timeout) {
> +			if (IS_LUNARLAKE(xe)) {
> +				/*
> +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
> +				 * worker in case of g2h response timeout")
> +				 *
> +				 * TODO: Drop this change once workqueue scheduling delay issue is
> +				 * fixed on LNL Hybrid CPU.
> +				 */
> +				flush_workqueue(xe->ordered_wq);
> +				err = do_compare(addr, args->value, args->mask, args->op);
> +				if (err <= 0)
> +					break;
> +			}
>  			err = -ETIME;
>  			break;
>  		}

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Jani Nikula]

On Thu, 24 Oct 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
> Flush xe ordered_wq in case of ufence timeout which is observed
> on LNL and that points to the recent scheduling issue with E-cores.
>
> This is similar to the recent fix:
> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
> response timeout") and should be removed once there is E core
> scheduling fix.
>
> v2: Add platform check(Himal)
>     s/__flush_workqueue/flush_workqueue(Jani)
>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: <stable@vger.kernel.org> # v6.11+
> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> index f5deb81eba01..78a0ad3c78fe 100644
> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> @@ -13,6 +13,7 @@
>  #include "xe_device.h"
>  #include "xe_gt.h"
>  #include "xe_macros.h"
> +#include "compat-i915-headers/i915_drv.h"

Sorry, you just can't use this in xe core. At all. Not even a little
bit. It's purely for i915 display compat code.

If you need it for the LNL platform check, you need to use:

	xe->info.platform == XE_LUNARLAKE

Although platform checks in xe code are generally discouraged.

BR,
Jani.



>  #include "xe_exec_queue.h"
>  
>  static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
> @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>  		}
>  
>  		if (!timeout) {
> +			if (IS_LUNARLAKE(xe)) {
> +				/*
> +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
> +				 * worker in case of g2h response timeout")
> +				 *
> +				 * TODO: Drop this change once workqueue scheduling delay issue is
> +				 * fixed on LNL Hybrid CPU.
> +				 */
> +				flush_workqueue(xe->ordered_wq);
> +				err = do_compare(addr, args->value, args->mask, args->op);
> +				if (err <= 0)
> +					break;
> +			}
>  			err = -ETIME;
>  			break;
>  		}

-- 
Jani Nikula, Intel

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[John Harrison]

On 10/24/2024 08:18, Nirmoy Das wrote:
> Flush xe ordered_wq in case of ufence timeout which is observed
> on LNL and that points to the recent scheduling issue with E-cores.
>
> This is similar to the recent fix:
> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
> response timeout") and should be removed once there is E core
> scheduling fix.
>
> v2: Add platform check(Himal)
>      s/__flush_workqueue/flush_workqueue(Jani)
>
> Cc: Badal Nilawar <badal.nilawar@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: <stable@vger.kernel.org> # v6.11+
> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> index f5deb81eba01..78a0ad3c78fe 100644
> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> @@ -13,6 +13,7 @@
>   #include "xe_device.h"
>   #include "xe_gt.h"
>   #include "xe_macros.h"
> +#include "compat-i915-headers/i915_drv.h"
>   #include "xe_exec_queue.h"
>   
>   static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
> @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>   		}
>   
>   		if (!timeout) {
> +			if (IS_LUNARLAKE(xe)) {
> +				/*
> +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
> +				 * worker in case of g2h response timeout")
> +				 *
> +				 * TODO: Drop this change once workqueue scheduling delay issue is
> +				 * fixed on LNL Hybrid CPU.
> +				 */
> +				flush_workqueue(xe->ordered_wq);
If we are having multiple instances of this workaround, can we wrap them 
up in as 'LNL_FLUSH_WORKQUEUE(q)' or some such? Put the IS_LNL check 
inside the macro and make it pretty obvious exactly where all the 
instances are by having a single macro name to search for.

John.

> +				err = do_compare(addr, args->value, args->mask, args->op);
> +				if (err <= 0)
> +					break;
> +			}
>   			err = -ETIME;
>   			break;
>   		}

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Matthew Brost]

On Thu, Oct 24, 2024 at 10:14:21AM -0700, John Harrison wrote:
> On 10/24/2024 08:18, Nirmoy Das wrote:
> > Flush xe ordered_wq in case of ufence timeout which is observed
> > on LNL and that points to the recent scheduling issue with E-cores.
> > 
> > This is similar to the recent fix:
> > commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
> > response timeout") and should be removed once there is E core
> > scheduling fix.
> > 
> > v2: Add platform check(Himal)
> >      s/__flush_workqueue/flush_workqueue(Jani)
> > 
> > Cc: Badal Nilawar <badal.nilawar@intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > Cc: John Harrison <John.C.Harrison@Intel.com>
> > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: <stable@vger.kernel.org> # v6.11+
> > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
> > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > index f5deb81eba01..78a0ad3c78fe 100644
> > --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > @@ -13,6 +13,7 @@
> >   #include "xe_device.h"
> >   #include "xe_gt.h"
> >   #include "xe_macros.h"
> > +#include "compat-i915-headers/i915_drv.h"
> >   #include "xe_exec_queue.h"
> >   static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
> > @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
> >   		}
> >   		if (!timeout) {
> > +			if (IS_LUNARLAKE(xe)) {
> > +				/*
> > +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
> > +				 * worker in case of g2h response timeout")
> > +				 *
> > +				 * TODO: Drop this change once workqueue scheduling delay issue is
> > +				 * fixed on LNL Hybrid CPU.
> > +				 */
> > +				flush_workqueue(xe->ordered_wq);
> If we are having multiple instances of this workaround, can we wrap them up
> in as 'LNL_FLUSH_WORKQUEUE(q)' or some such? Put the IS_LNL check inside the
> macro and make it pretty obvious exactly where all the instances are by
> having a single macro name to search for.
> 

+1, I think Lucas is suggesting something similar to this on the chat to
make sure we don't lose track of removing these W/A when this gets
fixed.

Matt

> John.
> 
> > +				err = do_compare(addr, args->value, args->mask, args->op);
> > +				if (err <= 0)
> > +					break;
> > +			}
> >   			err = -ETIME;
> >   			break;
> >   		}
> 

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Nirmoy Das]

On 10/24/2024 6:32 PM, Jani Nikula wrote:
> On Thu, 24 Oct 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>> Flush xe ordered_wq in case of ufence timeout which is observed
>> on LNL and that points to the recent scheduling issue with E-cores.
>>
>> This is similar to the recent fix:
>> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
>> response timeout") and should be removed once there is E core
>> scheduling fix.
>>
>> v2: Add platform check(Himal)
>>     s/__flush_workqueue/flush_workqueue(Jani)
>>
>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: <stable@vger.kernel.org> # v6.11+
>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>> index f5deb81eba01..78a0ad3c78fe 100644
>> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
>> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>> @@ -13,6 +13,7 @@
>>  #include "xe_device.h"
>>  #include "xe_gt.h"
>>  #include "xe_macros.h"
>> +#include "compat-i915-headers/i915_drv.h"
> Sorry, you just can't use this in xe core. At all. Not even a little
> bit. It's purely for i915 display compat code.
>
> If you need it for the LNL platform check, you need to use:
>
> 	xe->info.platform == XE_LUNARLAKE


Will do that. That macro looked odd but I didn't know a better way.

>
> Although platform checks in xe code are generally discouraged.


This issue unfortunately depending on platform instead of graphics IP.


Thanks,

Nirmoy

>
> BR,
> Jani.
>
>
>
>>  #include "xe_exec_queue.h"
>>  
>>  static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
>> @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>>  		}
>>  
>>  		if (!timeout) {
>> +			if (IS_LUNARLAKE(xe)) {
>> +				/*
>> +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
>> +				 * worker in case of g2h response timeout")
>> +				 *
>> +				 * TODO: Drop this change once workqueue scheduling delay issue is
>> +				 * fixed on LNL Hybrid CPU.
>> +				 */
>> +				flush_workqueue(xe->ordered_wq);
>> +				err = do_compare(addr, args->value, args->mask, args->op);
>> +				if (err <= 0)
>> +					break;
>> +			}
>>  			err = -ETIME;
>>  			break;
>>  		}

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Matthew Brost]

On Fri, Oct 25, 2024 at 06:06:47PM +0200, Nirmoy Das wrote:
>    On 10/24/2024 7:22 PM, Matthew Brost wrote:
> 
> On Thu, Oct 24, 2024 at 10:14:21AM -0700, John Harrison wrote:
> 
> On 10/24/2024 08:18, Nirmoy Das wrote:
> 
> Flush xe ordered_wq in case of ufence timeout which is observed
> on LNL and that points to the recent scheduling issue with E-cores.
> 
> This is similar to the recent fix:
> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
> response timeout") and should be removed once there is E core
> scheduling fix.
> 
> v2: Add platform check(Himal)
>      s/__flush_workqueue/flush_workqueue(Jani)
> 
> Cc: Badal Nilawar [1]<badal.nilawar@intel.com>
> Cc: Jani Nikula [2]<jani.nikula@intel.com>
> Cc: Matthew Auld [3]<matthew.auld@intel.com>
> Cc: John Harrison [4]<John.C.Harrison@Intel.com>
> Cc: Himal Prasad Ghimiray [5]<himal.prasad.ghimiray@intel.com>
> Cc: Lucas De Marchi [6]<lucas.demarchi@intel.com>
> Cc: [7]<stable@vger.kernel.org> # v6.11+
> Link: [8]https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
> Suggested-by: Matthew Brost [9]<matthew.brost@intel.com>
> Signed-off-by: Nirmoy Das [10]<nirmoy.das@intel.com>
> Reviewed-by: Matthew Brost [11]<matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wai
> t_user_fence.c
> index f5deb81eba01..78a0ad3c78fe 100644
> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> @@ -13,6 +13,7 @@
>   #include "xe_device.h"
>   #include "xe_gt.h"
>   #include "xe_macros.h"
> +#include "compat-i915-headers/i915_drv.h"
>   #include "xe_exec_queue.h"
>   static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
> @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *
> data,
>                 }
>                 if (!timeout) {
> +                       if (IS_LUNARLAKE(xe)) {
> +                               /*
> +                                * This is analogous to e51527233804 ("drm/xe/gu
> c/ct: Flush g2h
> +                                * worker in case of g2h response timeout")
> +                                *
> +                                * TODO: Drop this change once workqueue schedul
> ing delay issue is
> +                                * fixed on LNL Hybrid CPU.
> +                                */
> +                               flush_workqueue(xe->ordered_wq);
> 
> If we are having multiple instances of this workaround, can we wrap them up
> in as 'LNL_FLUSH_WORKQUEUE(q)' or some such? Put the IS_LNL check inside the
> macro and make it pretty obvious exactly where all the instances are by
> having a single macro name to search for.
> 
> 
> +1, I think Lucas is suggesting something similar to this on the chat to
> make sure we don't lose track of removing these W/A when this gets
> fixed.
> 
> Matt
> 
>    Sounds good. I will add LNL_FLUSH_WORKQUEUE() and use that for all the
>    places we need this WA.
> 

You will need 2 macros...

- LNL_FLUSH_WORKQUEUE() which accepts xe_device, workqueue_struct
- LNL_FLUSH_WORK() which accepts xe_device, work_struct

Matt

>    Regards,
> 
>    Nirmoy
> 
> 
> 
> John.
> 
> 
> +                               err = do_compare(addr, args->value, args->mask,
> args->op);
> +                               if (err <= 0)
> +                                       break;
> +                       }
>                         err = -ETIME;
>                         break;
>                 }
> 
> References
> 
>    1. mailto:badal.nilawar@intel.com
>    2. mailto:jani.nikula@intel.com
>    3. mailto:matthew.auld@intel.com
>    4. mailto:John.C.Harrison@Intel.com
>    5. mailto:himal.prasad.ghimiray@intel.com
>    6. mailto:lucas.demarchi@intel.com
>    7. mailto:stable@vger.kernel.org
>    8. https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
>    9. mailto:matthew.brost@intel.com
>   10. mailto:nirmoy.das@intel.com
>   11. mailto:matthew.brost@intel.com

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[John Harrison]

On 10/25/2024 09:03, Nirmoy Das wrote:
> On 10/24/2024 6:32 PM, Jani Nikula wrote:
>> On Thu, 24 Oct 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>>> Flush xe ordered_wq in case of ufence timeout which is observed
>>> on LNL and that points to the recent scheduling issue with E-cores.
>>>
>>> This is similar to the recent fix:
>>> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
>>> response timeout") and should be removed once there is E core
>>> scheduling fix.
>>>
>>> v2: Add platform check(Himal)
>>>      s/__flush_workqueue/flush_workqueue(Jani)
>>>
>>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: <stable@vger.kernel.org> # v6.11+
>>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
>>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>> index f5deb81eba01..78a0ad3c78fe 100644
>>> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>> @@ -13,6 +13,7 @@
>>>   #include "xe_device.h"
>>>   #include "xe_gt.h"
>>>   #include "xe_macros.h"
>>> +#include "compat-i915-headers/i915_drv.h"
>> Sorry, you just can't use this in xe core. At all. Not even a little
>> bit. It's purely for i915 display compat code.
>>
>> If you need it for the LNL platform check, you need to use:
>>
>> 	xe->info.platform == XE_LUNARLAKE
>
> Will do that. That macro looked odd but I didn't know a better way.
>
>> Although platform checks in xe code are generally discouraged.
>
> This issue unfortunately depending on platform instead of graphics IP.
But isn't this issue dependent upon the CPU platform not the graphics 
platform? As in, a DG2 card plugged in to a LNL host will also have this 
issue. So testing any graphics related value is technically incorrect.

John.

>
>
> Thanks,
>
> Nirmoy
>
>> BR,
>> Jani.
>>
>>
>>
>>>   #include "xe_exec_queue.h"
>>>   
>>>   static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
>>> @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>>>   		}
>>>   
>>>   		if (!timeout) {
>>> +			if (IS_LUNARLAKE(xe)) {
>>> +				/*
>>> +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
>>> +				 * worker in case of g2h response timeout")
>>> +				 *
>>> +				 * TODO: Drop this change once workqueue scheduling delay issue is
>>> +				 * fixed on LNL Hybrid CPU.
>>> +				 */
>>> +				flush_workqueue(xe->ordered_wq);
>>> +				err = do_compare(addr, args->value, args->mask, args->op);
>>> +				if (err <= 0)
>>> +					break;
>>> +			}
>>>   			err = -ETIME;
>>>   			break;
>>>   		}

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Matthew Brost]

On Fri, Oct 25, 2024 at 11:27:55AM -0700, John Harrison wrote:
> On 10/25/2024 09:03, Nirmoy Das wrote:
> > On 10/24/2024 6:32 PM, Jani Nikula wrote:
> > > On Thu, 24 Oct 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
> > > > Flush xe ordered_wq in case of ufence timeout which is observed
> > > > on LNL and that points to the recent scheduling issue with E-cores.
> > > > 
> > > > This is similar to the recent fix:
> > > > commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
> > > > response timeout") and should be removed once there is E core
> > > > scheduling fix.
> > > > 
> > > > v2: Add platform check(Himal)
> > > >      s/__flush_workqueue/flush_workqueue(Jani)
> > > > 
> > > > Cc: Badal Nilawar <badal.nilawar@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > > Cc: John Harrison <John.C.Harrison@Intel.com>
> > > > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Cc: <stable@vger.kernel.org> # v6.11+
> > > > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
> > > > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
> > > >   1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > > > index f5deb81eba01..78a0ad3c78fe 100644
> > > > --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > > > +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
> > > > @@ -13,6 +13,7 @@
> > > >   #include "xe_device.h"
> > > >   #include "xe_gt.h"
> > > >   #include "xe_macros.h"
> > > > +#include "compat-i915-headers/i915_drv.h"
> > > Sorry, you just can't use this in xe core. At all. Not even a little
> > > bit. It's purely for i915 display compat code.
> > > 
> > > If you need it for the LNL platform check, you need to use:
> > > 
> > > 	xe->info.platform == XE_LUNARLAKE
> > 
> > Will do that. That macro looked odd but I didn't know a better way.
> > 
> > > Although platform checks in xe code are generally discouraged.
> > 
> > This issue unfortunately depending on platform instead of graphics IP.
> But isn't this issue dependent upon the CPU platform not the graphics
> platform? As in, a DG2 card plugged in to a LNL host will also have this
> issue. So testing any graphics related value is technically incorrect.
> 

This is a good point, maybe for now we blindly do this regardless of
platform. It is basically harmless to do this after a timeout... Also a
warning message if we can detect this fixed the timeout for CI purposes.

Matt

> John.
> 
> > 
> > 
> > Thanks,
> > 
> > Nirmoy
> > 
> > > BR,
> > > Jani.
> > > 
> > > 
> > > 
> > > >   #include "xe_exec_queue.h"
> > > >   static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
> > > > @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
> > > >   		}
> > > >   		if (!timeout) {
> > > > +			if (IS_LUNARLAKE(xe)) {
> > > > +				/*
> > > > +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
> > > > +				 * worker in case of g2h response timeout")
> > > > +				 *
> > > > +				 * TODO: Drop this change once workqueue scheduling delay issue is
> > > > +				 * fixed on LNL Hybrid CPU.
> > > > +				 */
> > > > +				flush_workqueue(xe->ordered_wq);
> > > > +				err = do_compare(addr, args->value, args->mask, args->op);
> > > > +				if (err <= 0)
> > > > +					break;
> > > > +			}
> > > >   			err = -ETIME;
> > > >   			break;
> > > >   		}
> 

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Nirmoy Das]

On 10/25/2024 8:34 PM, Matthew Brost wrote:
> On Fri, Oct 25, 2024 at 11:27:55AM -0700, John Harrison wrote:
>> On 10/25/2024 09:03, Nirmoy Das wrote:
>>> On 10/24/2024 6:32 PM, Jani Nikula wrote:
>>>> On Thu, 24 Oct 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>>>>> Flush xe ordered_wq in case of ufence timeout which is observed
>>>>> on LNL and that points to the recent scheduling issue with E-cores.
>>>>>
>>>>> This is similar to the recent fix:
>>>>> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
>>>>> response timeout") and should be removed once there is E core
>>>>> scheduling fix.
>>>>>
>>>>> v2: Add platform check(Himal)
>>>>>      s/__flush_workqueue/flush_workqueue(Jani)
>>>>>
>>>>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>> Cc: <stable@vger.kernel.org> # v6.11+
>>>>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
>>>>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>>>>>   1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>> index f5deb81eba01..78a0ad3c78fe 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>> @@ -13,6 +13,7 @@
>>>>>   #include "xe_device.h"
>>>>>   #include "xe_gt.h"
>>>>>   #include "xe_macros.h"
>>>>> +#include "compat-i915-headers/i915_drv.h"
>>>> Sorry, you just can't use this in xe core. At all. Not even a little
>>>> bit. It's purely for i915 display compat code.
>>>>
>>>> If you need it for the LNL platform check, you need to use:
>>>>
>>>> 	xe->info.platform == XE_LUNARLAKE
>>> Will do that. That macro looked odd but I didn't know a better way.
>>>
>>>> Although platform checks in xe code are generally discouraged.
>>> This issue unfortunately depending on platform instead of graphics IP.
>> But isn't this issue dependent upon the CPU platform not the graphics
>> platform? As in, a DG2 card plugged in to a LNL host will also have this
>> issue. So testing any graphics related value is technically incorrect.


Haven't thought about. LNL only has x8 PCIe lanes shared between NVME and other IOs but thunderbolt based eGPU should be easily doable.

I think I could do "if (boot_cpu_data.x86_vfm == INTEL_LUNARLAKE_M)" instead.

>>
> This is a good point, maybe for now we blindly do this regardless of
> platform. It is basically harmless to do this after a timeout... Also a
> warning message if we can detect this fixed the timeout for CI purposes.

I am open to this as well. Please let me know which one should be a better solution here.


Regards,

Nirmoy

>
> Matt
>
>> John.
>>
>>>
>>> Thanks,
>>>
>>> Nirmoy
>>>
>>>> BR,
>>>> Jani.
>>>>
>>>>
>>>>
>>>>>   #include "xe_exec_queue.h"
>>>>>   static int do_compare(u64 addr, u64 value, u64 mask, u16 op)
>>>>> @@ -155,6 +156,19 @@ int xe_wait_user_fence_ioctl(struct drm_device *dev, void *data,
>>>>>   		}
>>>>>   		if (!timeout) {
>>>>> +			if (IS_LUNARLAKE(xe)) {
>>>>> +				/*
>>>>> +				 * This is analogous to e51527233804 ("drm/xe/guc/ct: Flush g2h
>>>>> +				 * worker in case of g2h response timeout")
>>>>> +				 *
>>>>> +				 * TODO: Drop this change once workqueue scheduling delay issue is
>>>>> +				 * fixed on LNL Hybrid CPU.
>>>>> +				 */
>>>>> +				flush_workqueue(xe->ordered_wq);
>>>>> +				err = do_compare(addr, args->value, args->mask, args->op);
>>>>> +				if (err <= 0)
>>>>> +					break;
>>>>> +			}
>>>>>   			err = -ETIME;
>>>>>   			break;
>>>>>   		}

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Lucas De Marchi]

On Fri, Oct 25, 2024 at 09:33:39PM +0200, Nirmoy Das wrote:
>
>On 10/25/2024 8:34 PM, Matthew Brost wrote:
>> On Fri, Oct 25, 2024 at 11:27:55AM -0700, John Harrison wrote:
>>> On 10/25/2024 09:03, Nirmoy Das wrote:
>>>> On 10/24/2024 6:32 PM, Jani Nikula wrote:
>>>>> On Thu, 24 Oct 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>>>>>> Flush xe ordered_wq in case of ufence timeout which is observed
>>>>>> on LNL and that points to the recent scheduling issue with E-cores.
>>>>>>
>>>>>> This is similar to the recent fix:
>>>>>> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
>>>>>> response timeout") and should be removed once there is E core
>>>>>> scheduling fix.
>>>>>>
>>>>>> v2: Add platform check(Himal)
>>>>>>      s/__flush_workqueue/flush_workqueue(Jani)
>>>>>>
>>>>>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>>>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>>> Cc: <stable@vger.kernel.org> # v6.11+
>>>>>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
>>>>>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>>>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>>>>>>   1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>>> index f5deb81eba01..78a0ad3c78fe 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>>> @@ -13,6 +13,7 @@
>>>>>>   #include "xe_device.h"
>>>>>>   #include "xe_gt.h"
>>>>>>   #include "xe_macros.h"
>>>>>> +#include "compat-i915-headers/i915_drv.h"
>>>>> Sorry, you just can't use this in xe core. At all. Not even a little
>>>>> bit. It's purely for i915 display compat code.
>>>>>
>>>>> If you need it for the LNL platform check, you need to use:
>>>>>
>>>>> 	xe->info.platform == XE_LUNARLAKE
>>>> Will do that. That macro looked odd but I didn't know a better way.
>>>>
>>>>> Although platform checks in xe code are generally discouraged.
>>>> This issue unfortunately depending on platform instead of graphics IP.
>>> But isn't this issue dependent upon the CPU platform not the graphics
>>> platform? As in, a DG2 card plugged in to a LNL host will also have this
>>> issue. So testing any graphics related value is technically incorrect.
>
>
>Haven't thought about. LNL only has x8 PCIe lanes shared between NVME and other IOs but thunderbolt based eGPU should be easily doable.
>
>I think I could do "if (boot_cpu_data.x86_vfm == INTEL_LUNARLAKE_M)" instead.
>
>>>
>> This is a good point, maybe for now we blindly do this regardless of
>> platform. It is basically harmless to do this after a timeout... Also a
>> warning message if we can detect this fixed the timeout for CI purposes.
>
>I am open to this as well. Please let me know which one should be a better solution here.

if it's a cheap thing without side-effects, go for the version without
the platform check and document it in commit message / source comment

Lucas De Marchi

Re: [PATCH v2] drm/xe/ufence: Flush xe ordered_wq in case of ufence timeout

[Nirmoy Das]

On 10/25/2024 9:56 PM, Lucas De Marchi wrote:
> On Fri, Oct 25, 2024 at 09:33:39PM +0200, Nirmoy Das wrote:
>>
>> On 10/25/2024 8:34 PM, Matthew Brost wrote:
>>> On Fri, Oct 25, 2024 at 11:27:55AM -0700, John Harrison wrote:
>>>> On 10/25/2024 09:03, Nirmoy Das wrote:
>>>>> On 10/24/2024 6:32 PM, Jani Nikula wrote:
>>>>>> On Thu, 24 Oct 2024, Nirmoy Das <nirmoy.das@intel.com> wrote:
>>>>>>> Flush xe ordered_wq in case of ufence timeout which is observed
>>>>>>> on LNL and that points to the recent scheduling issue with E-cores.
>>>>>>>
>>>>>>> This is similar to the recent fix:
>>>>>>> commit e51527233804 ("drm/xe/guc/ct: Flush g2h worker in case of g2h
>>>>>>> response timeout") and should be removed once there is E core
>>>>>>> scheduling fix.
>>>>>>>
>>>>>>> v2: Add platform check(Himal)
>>>>>>>      s/__flush_workqueue/flush_workqueue(Jani)
>>>>>>>
>>>>>>> Cc: Badal Nilawar <badal.nilawar@intel.com>
>>>>>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>>>>>> Cc: Matthew Auld <matthew.auld@intel.com>
>>>>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>>>>> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>>>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>>>> Cc: <stable@vger.kernel.org> # v6.11+
>>>>>>> Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2754
>>>>>>> Suggested-by: Matthew Brost <matthew.brost@intel.com>
>>>>>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>>>>>>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/xe/xe_wait_user_fence.c | 14 ++++++++++++++
>>>>>>>   1 file changed, 14 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_wait_user_fence.c b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>>>> index f5deb81eba01..78a0ad3c78fe 100644
>>>>>>> --- a/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>>>> +++ b/drivers/gpu/drm/xe/xe_wait_user_fence.c
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>   #include "xe_device.h"
>>>>>>>   #include "xe_gt.h"
>>>>>>>   #include "xe_macros.h"
>>>>>>> +#include "compat-i915-headers/i915_drv.h"
>>>>>> Sorry, you just can't use this in xe core. At all. Not even a little
>>>>>> bit. It's purely for i915 display compat code.
>>>>>>
>>>>>> If you need it for the LNL platform check, you need to use:
>>>>>>
>>>>>>     xe->info.platform == XE_LUNARLAKE
>>>>> Will do that. That macro looked odd but I didn't know a better way.
>>>>>
>>>>>> Although platform checks in xe code are generally discouraged.
>>>>> This issue unfortunately depending on platform instead of graphics IP.
>>>> But isn't this issue dependent upon the CPU platform not the graphics
>>>> platform? As in, a DG2 card plugged in to a LNL host will also have this
>>>> issue. So testing any graphics related value is technically incorrect.
>>
>>
>> Haven't thought about. LNL only has x8 PCIe lanes shared between NVME and other IOs but thunderbolt based eGPU should be easily doable.
>>
>> I think I could do "if (boot_cpu_data.x86_vfm == INTEL_LUNARLAKE_M)" instead.
>>
>>>>
>>> This is a good point, maybe for now we blindly do this regardless of
>>> platform. It is basically harmless to do this after a timeout... Also a
>>> warning message if we can detect this fixed the timeout for CI purposes.
>>
>> I am open to this as well. Please let me know which one should be a better solution here.
>
> if it's a cheap thing without side-effects, go for the version without
> the platform check and document it in commit message / source comment


That would be the previous rev. I will add the missing stable Cc and resend.



Thanks,

Nirmoy

>
> Lucas De Marchi