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

Thu Oct 19 06:36:00 UTC 2023

> -----Original Message-----
> From: Aravind Iddamsetty <aravind.iddamsetty at linux.intel.com>
> Sent: 19 October 2023 11:32
> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>; Welty,
> Brian <brian.welty at intel.com>; intel-xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH v2 05/10] drm/xe: Notify userspace about GSC
> HW errors.
> 
> 
> On 19/10/23 11:06, Ghimiray, Himal Prasad wrote:
> > Hi Brian,
> >
> >> -----Original Message-----
> >> From: Welty, Brian <brian.welty at intel.com>
> >> Sent: 19 October 2023 06:22
> >> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>; intel-
> >> xe at 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 at intel.com>
> >>> Signed-off-by: Himal Prasad Ghimiray
> >>> <himal.prasad.ghimiray at 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

This makes sense. Will modify in next patch. 

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