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

[Nirmoy Das <nirmoy.das@intel.com>]

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