RE: [PATCH 3/5] drm/amdgpu: conduct bad gpu check during bootup/reset

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[AMD Public Use]

Thanks for your review, Tao.
Please check my comments after yours.

Regards,
Guchun

-----Original Message-----
From: Zhou1, Tao <Tao.Zhou1@xxxxxxx> 
Sent: Thursday, July 23, 2020 10:51 AM
To: Chen, Guchun <Guchun.Chen@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Li, Dennis <Dennis.Li@xxxxxxx>; Yang, Stanley <Stanley.Yang@xxxxxxx>; Clements, John <John.Clements@xxxxxxx>
Subject: RE: [PATCH 3/5] drm/amdgpu: conduct bad gpu check during bootup/reset

[AMD Official Use Only - Internal Distribution Only]

> -----Original Message-----
> From: Chen, Guchun <Guchun.Chen@xxxxxxx>
> Sent: Wednesday, July 22, 2020 11:14 AM
> To: amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Deucher, Alexander 
> <Alexander.Deucher@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>; 
> Li, Dennis <Dennis.Li@xxxxxxx>; Yang, Stanley <Stanley.Yang@xxxxxxx>; 
> Zhou1, Tao <Tao.Zhou1@xxxxxxx>; Clements, John <John.Clements@xxxxxxx>
> Cc: Chen, Guchun <Guchun.Chen@xxxxxxx>
> Subject: [PATCH 3/5] drm/amdgpu: conduct bad gpu check during 
> bootup/reset
>
> During boot up, when init eeprom, RAS will check if the BAD GPU tag is 
> saved or not. And will break boot up and notify user to RMA it.
>
> At the meanwhile, when saved bad page count to eeprom exceeds 
> threshold, one ras recovery will be triggered immediately, and bad gpu 
> check will be executed and reset will fail as well to remind user.
>
> User could set bad_page_threshold = 0 as module parameter when probing 
> driver to disable the bad feature check.
>
> Signed-off-by: Guchun Chen <guchun.chen@xxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 21 ++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c       | 25 +++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h       | 16 +++-
>  .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c    | 87 ++++++++++++++++++-
>  .../gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h    |  6 +-
>  5 files changed, 142 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 2662cd7c8685..d529bf7917f5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2065,7 +2065,9 @@ static int amdgpu_device_ip_init(struct 
> amdgpu_device *adev)
>   * Note: theoretically, this should be called before all vram allocations
>   * to protect retired page from abusing
>   */

[Tao] The comment above should be also updated
[Guchun]yeah, will update it later.

> -amdgpu_ras_recovery_init(adev);
> +r = amdgpu_ras_recovery_init(adev);
> +if (r)
> +goto init_failed;

[Tao] Are you sure "r != 0" equals to "bad gpu"? Other errors of recovery_init should not block gpu init.
[Guchun]Good point. This should be addressed carefully.
>
>  if (adev->gmc.xgmi.num_physical_nodes > 1)  
> amdgpu_xgmi_add_device(adev); @@ -4133,8 +4135,20 @@ static int 
> amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
>
>  amdgpu_fbdev_set_suspend(tmp_adev, 0);
>
> -/* must succeed. */
> -amdgpu_ras_resume(tmp_adev);
> +/*
> + * The CPU is BAD once faulty pages by ECC has
> + * reached the threshold, and ras recovery is
> + * scheduled. So add one check here to prevent
> + * recovery if it's one BAD GPU, and remind
> + * user to RMA this GPU.
> + */
> +if (!amdgpu_ras_check_bad_gpu(tmp_adev)) {
> +/* must succeed. */
> +amdgpu_ras_resume(tmp_adev);
> +} else {
> +r = -EINVAL;
> +goto out;
> +}
>
>  /* Update PSP FW topology after reset */  if (hive && tmp_adev-
> >gmc.xgmi.num_physical_nodes > 1) @@ -4142,7 +4156,6 @@ static int
> amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,  }  }
>
> -
>  out:
>  if (!r) {
>  amdgpu_irq_gpu_reset_resume_helper(tmp_adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index e3d67d85c55f..818d4154e4a3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -62,8 +62,6 @@ const char *ras_block_string[] = {  #define ras_err_str(i)
> (ras_error_string[ffs(i)])  #define ras_block_str(i) (ras_block_string[i])
>
> -#define AMDGPU_RAS_FLAG_INIT_BY_VBIOS1
> -#define AMDGPU_RAS_FLAG_INIT_NEED_RESET2
>  #define RAS_DEFAULT_FLAGS (AMDGPU_RAS_FLAG_INIT_BY_VBIOS)
>
>  /* inject address is 52 bits */
> @@ -1817,6 +1815,7 @@ int amdgpu_ras_recovery_init(struct amdgpu_device
> *adev)
>  struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
>  struct ras_err_handler_data **data;
>  uint32_t max_eeprom_records_len = 0;
> +bool bad_gpu = false;
>  int ret;
>
>  if (con)
> @@ -1838,9 +1837,14 @@ int amdgpu_ras_recovery_init(struct amdgpu_device
> *adev)
>  max_eeprom_records_len =
> amdgpu_ras_eeprom_get_record_max_length();
>  amdgpu_ras_validate_threshold(adev, max_eeprom_records_len);
>
> -ret = amdgpu_ras_eeprom_init(&con->eeprom_control);
> -if (ret)
> +ret = amdgpu_ras_eeprom_init(&con->eeprom_control, &bad_gpu);
> +/*
> + * we only fail this calling and halt booting when bad_gpu is true.
> + */
> +if (bad_gpu) {
> +ret = -EINVAL;
>  goto free;
> +}
>
>  if (con->eeprom_control.num_recs) {
>  ret = amdgpu_ras_load_bad_pages(adev); @@ -2189,3
> +2193,16 @@ bool amdgpu_ras_need_emergency_restart(struct
> amdgpu_device *adev)
>
>  return false;
>  }
> +
> +bool amdgpu_ras_check_bad_gpu(struct amdgpu_device *adev) {
> +struct amdgpu_ras *con = amdgpu_ras_get_context(adev);
> +bool bad_gpu = false;
> +
> +if (con && (con->bad_page_cnt_threshold != 0xFFFFFFFF))
> +amdgpu_ras_eeprom_check_bad_gpu(&con->eeprom_control,
> +&bad_gpu);
> +
> +/* We are only interested in variable bad_gpu. */
> +return bad_gpu;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> index 4672649a9293..d7a363b37feb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.h
> @@ -31,6 +31,10 @@
>  #include "ta_ras_if.h"
>  #include "amdgpu_ras_eeprom.h"
>
> +#define AMDGPU_RAS_FLAG_INIT_BY_VBIOS(0x1 << 0)
> +#define AMDGPU_RAS_FLAG_INIT_NEED_RESET(0x1 << 1)
> +#define AMDGPU_RAS_FLAG_SKIP_BAD_PAGE_RESV(0x1 << 2)
> +
>  enum amdgpu_ras_block {
>  AMDGPU_RAS_BLOCK__UMC = 0,
>  AMDGPU_RAS_BLOCK__SDMA,
> @@ -493,6 +497,8 @@ void amdgpu_ras_suspend(struct amdgpu_device
> *adev);  unsigned long amdgpu_ras_query_error_count(struct amdgpu_device
> *adev,
>  bool is_ce);
>
> +bool amdgpu_ras_check_bad_gpu(struct amdgpu_device *adev);
> +
>  /* error handling functions */
>  int amdgpu_ras_add_bad_pages(struct amdgpu_device *adev,
>  struct eeprom_table_record *bps, int pages); @@ -503,10
> +509,14 @@ static inline int amdgpu_ras_reset_gpu(struct amdgpu_device
> *adev)  {
>  struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
>
> -/* save bad page to eeprom before gpu reset,
> - * i2c may be unstable in gpu reset
> +/*
> + * Save bad page to eeprom before gpu reset, i2c may be unstable
> + * in gpu reset.
> + *
> + * Also, exclude the case when ras recovery issuer is
> + * eerprom page write.
>   */
> -if (in_task())
> +if (!(ras->flags & AMDGPU_RAS_FLAG_SKIP_BAD_PAGE_RESV) &&
> in_task())
>  amdgpu_ras_reserve_bad_pages(adev);
>
>  if (atomic_cmpxchg(&ras->in_recovery, 0, 1) == 0) diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> index a2c982b1eac6..96b63c026bad 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.c

[Tao] It's better to split it into two patches, one is for ras and another one is for eeprom
[Guchun]I tried this, however, even if for ras change, eeprom change is needed as well. So I merged them both into one patch.

> @@ -46,6 +46,9 @@
>  #define EEPROM_TABLE_HDR_VAL 0x414d4452  #define EEPROM_TABLE_VER
> 0x00010000
>
> +/* Bad GPU tag ‘BADG’ */
> +#define EEPROM_TABLE_HDR_BAD 0x42414447
> +
>  /* Assume 2 Mbit size */
>  #define EEPROM_SIZE_BYTES 256000
>  #define EEPROM_PAGE__SIZE_BYTES 256
> @@ -238,12 +241,14 @@ int amdgpu_ras_eeprom_reset_table(struct
> amdgpu_ras_eeprom_control *control)
>
>  }
>
> -int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control)
> +int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control,
> +bool *bad_gpu)
>  {
>  int ret = 0;
>  struct amdgpu_device *adev = to_amdgpu_device(control);
>  unsigned char buff[EEPROM_ADDRESS_SIZE +
> EEPROM_TABLE_HEADER_SIZE] = { 0 };
>  struct amdgpu_ras_eeprom_table_header *hdr = &control->tbl_hdr;
> +struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
>  struct i2c_msg msg = {
>  .addr= 0,
>  .flags= I2C_M_RD,
> @@ -251,6 +256,8 @@ int amdgpu_ras_eeprom_init(struct
> amdgpu_ras_eeprom_control *control)
>  .buf= buff,
>  };
>
> +*bad_gpu = false;
> +
>  /* Verify i2c adapter is initialized */
>  if (!adev->pm.smu_i2c.algo)
>  return -ENOENT;
> @@ -279,6 +286,11 @@ int amdgpu_ras_eeprom_init(struct
> amdgpu_ras_eeprom_control *control)
>  DRM_DEBUG_DRIVER("Found existing EEPROM table with %d
> records",
>   control->num_recs);
>
> +} else if ((ras->bad_page_cnt_threshold != 0xFFFFFFFF) && (
> +hdr->header == EEPROM_TABLE_HDR_BAD)) {
> +*bad_gpu = true;
> +DRM_ERROR("Detect BAD GPU TAG in eeprom table and "
> +"GPU shall be RMAed.\n");
>  } else {
>  DRM_INFO("Creating new EEPROM table");
>
> @@ -375,6 +387,44 @@ static uint32_t
> __correct_eeprom_dest_address(uint32_t curr_address)
>  return curr_address;
>  }
>
> +int amdgpu_ras_eeprom_check_bad_gpu(struct amdgpu_ras_eeprom_control
> *control,
> +bool *bad_gpu)
> +{
> +struct amdgpu_device *adev = to_amdgpu_device(control);
> +unsigned char buff[EEPROM_ADDRESS_SIZE +
> +EEPROM_TABLE_HEADER_SIZE] = { 0 };
> +struct amdgpu_ras_eeprom_table_header *hdr = &control->tbl_hdr;
> +struct i2c_msg msg = {
> +.addr = control->i2c_address,
> +.flags = I2C_M_RD,
> +.len = EEPROM_ADDRESS_SIZE +
> EEPROM_TABLE_HEADER_SIZE,
> +.buf = buff,
> +};
> +int ret;
> +
> +*bad_gpu = false;
> +
> +/* read EEPROM table header */
> +mutex_lock(&control->tbl_mutex);
> +ret = i2c_transfer(&adev->pm.smu_i2c, &msg, 1);
> +if (ret < 1) {
> +dev_err(adev->dev, "Failed to read EEPROM table header.\n");
> +goto err;

[Tao] One tab is missed
[Guchun]Correct this later.

> +}
> +
> +__decode_table_header_from_buff(hdr, &buff[2]);
> +
> +if (hdr->header == EEPROM_TABLE_HDR_BAD) {
> +dev_warn(adev->dev, "Current GPU is BAD and should be
> RMAed.\n");
> +*bad_gpu = true;
> +}
> +
> +ret = 0;
> +err:
> +mutex_unlock(&control->tbl_mutex);
> +return ret;
> +}
> +
>  int amdgpu_ras_eeprom_process_recods(struct amdgpu_ras_eeprom_control
> *control,
>      struct eeprom_table_record
> *records,
>      bool write,
> @@ -383,8 +433,10 @@ int amdgpu_ras_eeprom_process_recods(struct
> amdgpu_ras_eeprom_control *control,
>  int i, ret = 0;
>  struct i2c_msg *msgs, *msg;
>  unsigned char *buffs, *buff;
> +bool sched_ras_recovery = false;
>  struct eeprom_table_record *record;
>  struct amdgpu_device *adev = to_amdgpu_device(control);
> +struct amdgpu_ras *ras = amdgpu_ras_get_context(adev);
>
>  if (adev->asic_type != CHIP_VEGA20 && adev->asic_type !=
> CHIP_ARCTURUS)
>  return 0;
> @@ -402,6 +454,25 @@ int amdgpu_ras_eeprom_process_recods(struct
> amdgpu_ras_eeprom_control *control,
>  goto free_buff;
>  }
>
> +/*
> + * If saved bad pages number exceeds the bad page threshod for
> + * the whole VRAM, update table header to mark one BAD GPU and
> + * schedule one ras recovery after eeprom write is done, this
> + * can avoid the missing for latest records.
> + *
> + * This new header will be picked up and checked in the bootup by
> + * ras recovery, which may break bootup process to notify user this
> + * GPU is bad and to RMA(Return Merchandise Authorization) this GPU.
> + */
> +if (write && (ras->bad_page_cnt_threshold != 0xFFFFFFFF) &&
> +((control->num_recs + num) >= ras->bad_page_cnt_threshold)) {
> +dev_warn(adev->dev,
> +"Saved bad pages(%d) reaches threshold value(%d).\n",
> +control->num_recs + num, ras-
> >bad_page_cnt_threshold);
> +control->tbl_hdr.header = EEPROM_TABLE_HDR_BAD;
> +sched_ras_recovery = true;
> +}
> +
>  /* In case of overflow just start from beginning to not lose newest
> records */
>  if (write && (control->next_addr + EEPROM_TABLE_RECORD_SIZE *
> num > EEPROM_SIZE_BYTES))
>  control->next_addr = EEPROM_RECORD_START; @@ -482,6
> +553,20 @@ int amdgpu_ras_eeprom_process_recods(struct
> amdgpu_ras_eeprom_control *control,
>  __update_tbl_checksum(control, records, num,
> old_hdr_byte_sum);
>
>  __update_table_header(control, buffs);
> +
> +if (sched_ras_recovery) {
> +/*
> + * Before scheduling ras recovery, assert the related
> + * flag first, which shall bypass common bad page
> + * reservation execution in amdgpu_ras_reset_gpu.
> + */
> +amdgpu_ras_get_context(adev)->flags |=
> +AMDGPU_RAS_FLAG_SKIP_BAD_PAGE_RESV;
> +
> +dev_warn(adev->dev, "Conduct ras recovery due to bad
> "
> +"page threshold reached.\n");
> +amdgpu_ras_reset_gpu(adev);
> +}
>  } else if (!__validate_tbl_checksum(control, records, num)) {
>  DRM_WARN("EEPROM Table checksum mismatch!");
>  /* TODO Uncomment when EEPROM read/write is relliable */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h
> index b272840cb069..4ccd139248a9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras_eeprom.h
> @@ -77,9 +77,13 @@ struct eeprom_table_record {
>  unsigned char mcumc_id;
>  }__attribute__((__packed__));
>
> -int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control);
> +int amdgpu_ras_eeprom_init(struct amdgpu_ras_eeprom_control *control,
> +bool *bad_gpu);
>  int amdgpu_ras_eeprom_reset_table(struct amdgpu_ras_eeprom_control
> *control);
>
> +int amdgpu_ras_eeprom_check_bad_gpu(struct amdgpu_ras_eeprom_control
> *control,
> +bool *bad_gpu);
> +
>  int amdgpu_ras_eeprom_process_recods(struct amdgpu_ras_eeprom_control
> *control,
>      struct eeprom_table_record
> *records,
>      bool write,
> --
> 2.17.1

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux