[PATCH 1/3] drm/amdgpu: add RAS poison creation handler (v2)
Zhang, Hawking
Hawking.Zhang at amd.com
Fri Apr 22 04:11:12 UTC 2022
Hi Tao,
I'm thinking of checking ip block first because we might want to leverage this interrupt handler as a common entry for other RAS interrupt as well. In such case, it looks more clearer if we check the ip block first.
I agree with you either way looks good.
You can have my RB for pushing the patches - We are always on the way to improve code quality.
The series is
Reviewed-by: Hawking Zhang <Hawking.Zhang at amd.com>
Regards,
Hawking
-----Original Message-----
From: Zhou1, Tao <Tao.Zhou1 at amd.com>
Sent: Friday, April 22, 2022 12:04
To: Zhang, Hawking <Hawking.Zhang at amd.com>; amd-gfx at lists.freedesktop.org; Yang, Stanley <Stanley.Yang at amd.com>; Lazar, Lijo <Lijo.Lazar at amd.com>; Ziya, Mohammad zafar <Mohammadzafar.Ziya at amd.com>; Chai, Thomas <YiPeng.Chai at amd.com>
Subject: RE: [PATCH 1/3] drm/amdgpu: add RAS poison creation handler (v2)
[AMD Official Use Only]
Hi Hawking,
The logic in my patch is:
if (poison)
if (umc)
poison_creation_handler
else
poison_consumption_handler
else
if (umc)
umc_handler
else
not supported
I think your suggestion is:
if (umc)
if (poison)
poison_creation_handler
else
umc_handler
else
if (poiosn)
poison_consumption_handler
else
not supported
Either way is OK for me, I don't think one approach is better than another.
Regards,
Tao
> -----Original Message-----
> From: Zhang, Hawking <Hawking.Zhang at amd.com>
> Sent: Thursday, April 21, 2022 10:54 PM
> To: Zhou1, Tao <Tao.Zhou1 at amd.com>; amd-gfx at lists.freedesktop.org;
> Yang, Stanley <Stanley.Yang at amd.com>; Lazar, Lijo
> <Lijo.Lazar at amd.com>; Ziya, Mohammad zafar
> <Mohammadzafar.Ziya at amd.com>; Chai, Thomas <YiPeng.Chai at amd.com>
> Cc: Zhou1, Tao <Tao.Zhou1 at amd.com>
> Subject: RE: [PATCH 1/3] drm/amdgpu: add RAS poison creation handler
> (v2)
>
> [AMD Official Use Only]
>
> Hi Tao,
>
> I was thinking more aggressive change - current
> amdgpu_ras_interrupt_handler only serves as umc poison (poison mode)
> or uncorrectable error handler (fue mode).
>
> We can still keep it as a unified entry point, but how about check ip
> block first, then if it is umc, then check poison mode to decide
> poison creation handling or legacy uncorrectable error handling.
>
> As discussed before, we'll optimize the poison creation handling
> later. so keeping poison_creation_handler and legacy umc ue handler in
> separated functions seems right direction to me.
>
> Thoughts?
>
> Regards,
> Hawking
>
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Tao
> Zhou
> Sent: Wednesday, April 20, 2022 19:30
> To: amd-gfx at lists.freedesktop.org; Zhang, Hawking
> <Hawking.Zhang at amd.com>; Yang, Stanley <Stanley.Yang at amd.com>; Lazar,
> Lijo <Lijo.Lazar at amd.com>; Ziya, Mohammad zafar
> <Mohammadzafar.Ziya at amd.com>; Chai, Thomas <YiPeng.Chai at amd.com>
> Cc: Zhou1, Tao <Tao.Zhou1 at amd.com>
> Subject: [PATCH 1/3] drm/amdgpu: add RAS poison creation handler (v2)
>
> Prepare for the implementation of poison consumption handler.
>
> v2: separate umc handler from poison creation.
>
> Signed-off-by: Tao Zhou <tao.zhou1 at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 70
> ++++++++++++++++---------
> 1 file changed, 44 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> index 4bbed76b79c8..22477f76913a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
> @@ -1515,12 +1515,45 @@ static int amdgpu_ras_fs_fini(struct
> amdgpu_device *adev)
> /* ras fs end */
>
> /* ih begin */
> +static void amdgpu_ras_interrupt_poison_creation_handler(struct
> ras_manager *obj,
> + struct amdgpu_iv_entry *entry) {
> + dev_info(obj->adev->dev,
> + "Poison is created, no user action is needed.\n"); }
> +
> +static void amdgpu_ras_interrupt_umc_handler(struct ras_manager *obj,
> + struct amdgpu_iv_entry *entry) {
> + struct ras_ih_data *data = &obj->ih_data;
> + struct ras_err_data err_data = {0, 0, 0, NULL};
> + int ret;
> +
> + if (!data->cb)
> + return;
> +
> + /* Let IP handle its data, maybe we need get the output
> + * from the callback to update the error type/count, etc
> + */
> + ret = data->cb(obj->adev, &err_data, entry);
> + /* ue will trigger an interrupt, and in that case
> + * we need do a reset to recovery the whole system.
> + * But leave IP do that recovery, here we just dispatch
> + * the error.
> + */
> + if (ret == AMDGPU_RAS_SUCCESS) {
> + /* these counts could be left as 0 if
> + * some blocks do not count error number
> + */
> + obj->err_data.ue_count += err_data.ue_count;
> + obj->err_data.ce_count += err_data.ce_count;
> + }
> +}
> +
> static void amdgpu_ras_interrupt_handler(struct ras_manager *obj) {
> struct ras_ih_data *data = &obj->ih_data;
> struct amdgpu_iv_entry entry;
> - int ret;
> - struct ras_err_data err_data = {0, 0, 0, NULL};
>
> while (data->rptr != data->wptr) {
> rmb();
> @@ -1531,30 +1564,15 @@ static void
> amdgpu_ras_interrupt_handler(struct
> ras_manager *obj)
> data->rptr = (data->aligned_element_size +
> data->rptr) % data->ring_size;
>
> - if (data->cb) {
> - if (amdgpu_ras_is_poison_mode_supported(obj->adev) &&
> - obj->head.block == AMDGPU_RAS_BLOCK__UMC)
> - dev_info(obj->adev->dev,
> - "Poison is created, no user action is needed.\n");
> - else {
> - /* Let IP handle its data, maybe we need get the output
> - * from the callback to udpate the error type/count, etc
> - */
> - memset(&err_data, 0, sizeof(err_data));
> - ret = data->cb(obj->adev, &err_data, &entry);
> - /* ue will trigger an interrupt, and in that case
> - * we need do a reset to recovery the whole system.
> - * But leave IP do that recovery, here we just dispatch
> - * the error.
> - */
> - if (ret == AMDGPU_RAS_SUCCESS) {
> - /* these counts could be left as 0 if
> - * some blocks do not count error number
> - */
> - obj->err_data.ue_count += err_data.ue_count;
> - obj->err_data.ce_count += err_data.ce_count;
> - }
> - }
> + if (amdgpu_ras_is_poison_mode_supported(obj->adev)) {
> + if (obj->head.block == AMDGPU_RAS_BLOCK__UMC)
> + amdgpu_ras_interrupt_poison_creation_handler(obj, &entry);
> + } else {
> + if (obj->head.block == AMDGPU_RAS_BLOCK__UMC)
> + amdgpu_ras_interrupt_umc_handler(obj, &entry);
> + else
> + dev_warn(obj->adev->dev,
> + "No RAS interrupt handler for
> +non-UMC block with poison disabled.\n");
> }
> }
> }
> --
> 2.35.1
>
More information about the amd-gfx
mailing list