Re: [Intel-xe] [PATCH v2 05/10] drm/xe: Notify userspace about GSC HW errors.

[Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>]

On 19/10/23 11:06, Ghimiray, Himal Prasad wrote:
> Hi Brian,
>
>> -----Original Message-----
>> From: Welty, Brian <brian.welty@intel.com>
>> Sent: 19 October 2023 06:22
>> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>; intel-
>> xe@lists.freedesktop.org
>> Subject: Re: [Intel-xe] [PATCH v2 05/10] drm/xe: Notify userspace about GSC
>> HW errors.
>>
>>
>>
>> On 10/17/2023 9:00 PM, Himal Prasad Ghimiray wrote:
>>> Send uevent incase of nonfatal errors reported by gsc.
>>>
>>> v2
>>> - No need to provide tile info in uevent because error is accepted
>>> only in tile0. (Aravind)
>> This is user API.  Don't we need to demonstrate user-space consumer of this
>> before merge?  Do we at least have IGT?  Also see question below.
> https://one-api.gitlab-pages.devtools.intel.com/level_zero/sysman/api.html#_CPPv441ZES_EVENT_TYPE_FLAG_DEVICE_RESET_REQUIRED is the consumer for this uevent.
> The hardware error injection via IGT is not possible. It explicitly needs python SV and ITP.
>>> Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_device_types.h |  3 +++
>>>   drivers/gpu/drm/xe/xe_hw_error.c     | 16 ++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_hw_error.h     |  3 ++-
>>>   drivers/gpu/drm/xe/xe_irq.c          |  4 ++++
>>>   include/uapi/drm/xe_drm.h            |  8 ++++++++
>>>   5 files changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
>>> b/drivers/gpu/drm/xe/xe_device_types.h
>>> index 2998ee517f0d..d2ee5549d20c 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -196,6 +196,9 @@ struct xe_tile {
>>>   	struct tile_hw_errors {
>>>   		struct xarray hw_error;
>>>   	} errors;
>>> +
>>> +	/** @gsc_hw_err_work: worker for uevent to report GSC HW errors
>> */
>>> +	struct work_struct gsc_hw_err_work;
>>>   };
>>>
>>>   /**
>>> diff --git a/drivers/gpu/drm/xe/xe_hw_error.c
>>> b/drivers/gpu/drm/xe/xe_hw_error.c
>>> index 1e94ee72a34f..9ac817c1dd03 100644
>>> --- a/drivers/gpu/drm/xe/xe_hw_error.c
>>> +++ b/drivers/gpu/drm/xe/xe_hw_error.c
>>> @@ -3,6 +3,8 @@
>>>    * Copyright © 2023 Intel Corporation
>>>    */
>>>
>>> +#include <drm/xe_drm.h>
>>> +
>>>   #include "xe_hw_error.h"
>>>
>>>   #include "regs/xe_regs.h"
>>> @@ -387,6 +389,19 @@ xe_gt_hw_error_handler(struct xe_gt *gt, const
>> enum hardware_error hw_err)
>>>   		xe_gt_hw_error_log_vector_reg(gt, hw_err);
>>>   }
>>>
>>> +void xe_gsc_hw_error_work(struct work_struct *work) {
>>> +	struct xe_tile *tile = container_of(work, typeof(*tile),
>> gsc_hw_err_work);
>>> +	char *csc_hw_error_event[3];
>>> +
>>> +	csc_hw_error_event[0] = XE_GSC_HW_HEALTH_UEVENT "=1";
>>> +	csc_hw_error_event[1] = "RESET_REQUIRED=1";
>>> +	csc_hw_error_event[2] = NULL;
>>> +
>>> +	kobject_uevent_env(&tile->xe->drm.primary->kdev->kobj,
>> KOBJ_CHANGE,
>>> +			   csc_hw_error_event);
>>> +}
>>> +
>> ...
>>> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
>>> index d48d8e3c898c..06f6fce1531d 100644
>>> --- a/include/uapi/drm/xe_drm.h
>>> +++ b/include/uapi/drm/xe_drm.h
>>> @@ -16,6 +16,14 @@ extern "C" {
>>>    * subject to backwards-compatibility constraints.
>>>    */
>>>
>>> +/**
>>> + * DOC: uevent generated by xe on it's tile node.
>> What does 'tile node' mean?  If I'm not mistaken, isn't
>> tile->xe->drm.primary->kdev->kobj just the device node?
> Miss from my end. It should be device node.
>>> + *
>>> + * XE_GSC_HW_HEALTH_UEVENT - Event is generated when GSC reports
>> HW
>>> + * errors. The value supplied with the event is always
>> "RESET_REQUIRED=1".
>>> + */
>>> +#define XE_GSC_HW_HEALTH_UEVENT "DEVICE_STATUS"
>> So both XE_RESET_FAILED_UEVENT and XE_GSC_HW_HEALTH_UEVENT are
>> defined as "DEVICE_STATUS".  Should be unique or isn't this going to be
>> confusing to the user reading these events?
> The values provided are different and uevents are on different nodes. 
> XE_RESET_FAILED_UEVENT is at pci node of xe device whereas XE_GSC_HW_HEALTH_UEVENT
> Is at device node.
the reset that you expect here to recover the device is a PCIe level reset same as in
XE_RESET_FAILED_UEVENTso why should this be with drm device node.

also the uevent doesn't seem to  convey the cause.

with XE_GSC_HW_HEALTH_UEVENT userspace will see:

DEVICE_STATUS=1

RESET_REQUIRED=1

TILE_ID=0

while for XE_RESET_FAILED_UEVENT:

DEVICE_STATUS=NEEDS_RESET

RESET_REQUIRED=1

TILE_ID=0

GT_ID=0

I would rather say we define one UEVENT UAPI

XE_RESET_REQUIRED "RESET_REQUIRED"

add event specific details like:

CSC_HW_ERROR = 1

or

GT_RESET_FAILED = 1

or like RESET_REASON="CSC_HW_ERROR/GT_RESET_FAILED"

and add additional details like tile and GT.

Thanks,
Aravind.
>
> BR
> Himal 
>> -Brian
>>
>>
>>> +
>>>   /**
>>>    * DOC: uevent generated by xe on it's pci node.
>>>    *