Re: [Intel-xe] [PATCH 06/11] drm/xe: Notify userspace about GSC HW errors.

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

On 11/10/23 12:55, Ghimiray, Himal Prasad wrote:
>
>> -----Original Message-----
>> From: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> Sent: 11 October 2023 12:53
>> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>; intel-
>> xe@lists.freedesktop.org
>> Subject: Re: [Intel-xe] [PATCH 06/11] drm/xe: Notify userspace about GSC
>> HW errors.
>>
>>
>> On 27/09/23 17:16, Himal Prasad Ghimiray wrote:
>>> Send uevent incase of nonfatal errors reported by gsc.
>>>
>>> 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     | 20 ++++++++++++++++++++
>>>  drivers/gpu/drm/xe/xe_hw_error.h     |  3 ++-
>>>  drivers/gpu/drm/xe/xe_irq.c          |  4 ++++
>>>  include/uapi/drm/xe_drm.h            |  9 +++++++++
>>>  5 files changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
>>> b/drivers/gpu/drm/xe/xe_device_types.h
>>> index 6aa4f4801d81..ff476a167be4 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -179,6 +179,9 @@ struct xe_tile {
>>>  	struct tile_hw_errors {
>>>  		unsigned long count[XE_TILE_HW_ERROR_MAX];
>>>  	} 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 eb76b8e6a338..76ae12df013c 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"
>>> @@ -366,6 +368,22 @@ xe_gt_hw_error_handler(struct xe_gt *gt, const
>> enum hardware_error hw_err)
>>>  		xe_gt_hw_error_status_reg_handler(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[4];
>>> +
>>> +	csc_hw_error_event[0] = XE_GSC_HW_HEALTH_UEVENT "=1";
>>> +	csc_hw_error_event[1] = "RESET_REQUIRED=1";
>>> +	csc_hw_error_event[2] = kasprintf(GFP_KERNEL, "TILE_ID=%d", tile-
>>> id);
>>> +	csc_hw_error_event[3] = NULL;
>>> +
>>> +	kobject_uevent_env(&tile->xe->drm.primary->kdev->kobj,
>> KOBJ_CHANGE,
>>> +			   csc_hw_error_event);
>>> +
>>> +	kfree(csc_hw_error_event[2]);
>>> +}
>>> +
>>>  static void
>>>  xe_gsc_hw_error_handler(struct xe_tile *tile, const enum
>>> hardware_error hw_err)  { @@ -423,6 +441,8 @@
>>> xe_gsc_hw_error_handler(struct xe_tile *tile, const enum hardware_error
>> hw_err)
>>>  			drm_err_ratelimited(&tile_to_xe(tile)->drm,
>>>  					    HW_ERR "GSC detected %s %s
>> error, bit[%d] is set\n",
>>>  					    errmsg, hw_err_str, errbit);
>>> +
>>> +			schedule_work(&tile->gsc_hw_err_work);
>>>  		}
>>>  		tile->errors.count[indx]++;
>>>  	}
>>> diff --git a/drivers/gpu/drm/xe/xe_hw_error.h
>>> b/drivers/gpu/drm/xe/xe_hw_error.h
>>> index 155722a0af4c..ee7705b3343b 100644
>>> --- a/drivers/gpu/drm/xe/xe_hw_error.h
>>> +++ b/drivers/gpu/drm/xe/xe_hw_error.h
>>> @@ -7,6 +7,7 @@
>>>
>>>  #include <linux/stddef.h>
>>>  #include <linux/types.h>
>>> +#include <linux/workqueue.h>
>>>
>>>  /* Error categories reported by hardware */  enum hardware_error { @@
>>> -121,5 +122,5 @@ struct xe_tile;
>>>
>>>  void xe_hw_error_irq_handler(struct xe_tile *tile, const u32
>>> master_ctl);  void xe_process_hw_errors(struct xe_device *xe);
>>> -
>>> +void xe_gsc_hw_error_work(struct work_struct *work);
>>>  #endif
>>> diff --git a/drivers/gpu/drm/xe/xe_irq.c b/drivers/gpu/drm/xe/xe_irq.c
>>> index 06c9b43e2c71..285c657cc789 100644
>>> --- a/drivers/gpu/drm/xe/xe_irq.c
>>> +++ b/drivers/gpu/drm/xe/xe_irq.c
>>> @@ -586,6 +586,10 @@ int xe_irq_install(struct xe_device *xe)
>>>  	irq_handler_t irq_handler;
>>>  	int err, irq;
>>>
>>> +	struct xe_tile *tile = xe_device_get_root_tile(xe);
>>> +
>>> +	INIT_WORK(&tile->gsc_hw_err_work, xe_gsc_hw_error_work);
>>> +
>>>  	irq_handler = xe_irq_handler(xe);
>>>  	if (!irq_handler) {
>>>  		drm_err(&xe->drm, "No supported interrupt handler"); diff --
>> git
>>> a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h index
>>> d48d8e3c898c..c45833defcc7 100644
>>> --- a/include/uapi/drm/xe_drm.h
>>> +++ b/include/uapi/drm/xe_drm.h
>>> @@ -16,6 +16,15 @@ extern "C" {
>>>   * subject to backwards-compatibility constraints.
>>>   */
>>>
>>> +/**
>>> + * DOC: uevent generated by xe on it's tile 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".
>>> + * Additional information supplied is tile id on which error is reported.
>> what is the relevance of tile id if it always reported on tile 0 only.
> Hmm. Ya right. Any other information we would like to send ?
> Instead of DEVICE_STATUS is it ok to send GSC_HW_STATUS ?
I think RESET_REQUIRED is sufficient but may be you have to add more details
to UAPI DOC why RESET is needed.

Thanks,

Aravind.
>> Thanks,
>>
>> Aravind
>>> + */
>>> +#define XE_GSC_HW_HEALTH_UEVENT "DEVICE_STATUS"
>>> +
>>>  /**
>>>   * DOC: uevent generated by xe on it's pci node.
>>>   *