RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
[AMD Official Use Only - General]
OK, Will do.
-
Best Regards,
Thomas
-Original Message-
From: Zhou1, Tao
Sent: Monday, April 3, 2023 3:21 PM
To: Chai, Thomas ; amd-gfx@lists.freedesktop.org
Cc: Zhang, Hawking ; Li, Candice ;
Yang, Stanley
Subject: RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
[AMD Official Use Only - General]
> -Original Message-
> From: Chai, Thomas
> Sent: Monday, April 3, 2023 3:00 PM
> To: Zhou1, Tao ; amd-gfx@lists.freedesktop.org
> Cc: Zhang, Hawking ; Li, Candice
> ; Yang, Stanley
> Subject: RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in
> umc_v8_10
>
> [AMD Official Use Only - General]
>
>
>
>
> -
> Best Regards,
> Thomas
>
> -Original Message-
> From: Zhou1, Tao
> Sent: Monday, April 3, 2023 11:45 AM
> To: Chai, Thomas ; amd-gfx@lists.freedesktop.org
> Cc: Zhang, Hawking ; Li, Candice
> ; Yang, Stanley
> Subject: RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in
> umc_v8_10
>
> [AMD Official Use Only - General]
>
>
>
> > -Original Message-
> > From: Chai, Thomas
> > Sent: Monday, April 3, 2023 9:59 AM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Chai, Thomas ; Zhang, Hawking
> > ; Zhou1, Tao ; Li,
> Candice
> > ; Yang, Stanley ; Chai,
> > Thomas
> > Subject: [PATCH 1/2] drm/amdgpu: optimize redundant code in
> > umc_v8_10
> >
> > Optimize redundant code in umc_v8_10
> >
> > Signed-off-by: YiPeng Chai
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c | 31
> > drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h | 7 +
> > drivers/gpu/drm/amd/amdgpu/umc_v8_10.c | 197
> > +---
> > 3 files changed, 115 insertions(+), 120 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > index 9e2e97207e53..734442315cf6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > @@ -302,3 +302,34 @@ void amdgpu_umc_fill_error_record(struct
> > ras_err_data *err_data,
> >
> > err_data->err_addr_cnt++;
> > }
> > +
> > +int amdgpu_umc_scan_all_umc_channels(struct amdgpu_device *adev,
> > + umc_func func, void *data)
> > +{
> > + uint32_t node_inst = 0;
> > + uint32_t umc_inst= 0;
> > + uint32_t ch_inst = 0;
> > + int ret = 0;
> > +
> > + if (adev->umc.node_inst_num) {
> > + LOOP_UMC_EACH_NODE_INST_AND_CH(node_inst, umc_inst,
> > ch_inst) {
> > + ret = func(adev, node_inst, umc_inst, ch_inst, data);
> > + if (ret) {
> > + dev_err(adev->dev, "Node %d umc %d ch %d
> > func returns %d\n",
> > + node_inst, umc_inst, ch_inst, ret);
> > + return ret;
> > + }
> > + }
> > + } else {
> > + LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
>
> >[Tao] for ASIC which doesn't support node, can we set its
> >node_inst_num to 1
> and retire the macro LOOP_UMC_INST_AND_CH?
>
> [Thomas] I am afraid not.
>
> " #define LOOP_UMC_NODE_INST(node_inst) \
> for_each_set_bit((node_inst), &(adev->umc.active_mask),
> adev->umc.node_inst_num) "
>
> The node instance loop of LOOP_UMC_EACH_NODE_INST_AND_CH supports
> node harvest, so node_inst_num is not the real node instance number.
[Tao] we can set both node_inst_num and active_mask to 1, but either way is
fine for me.
BTW, I think amdgpu_umc_loop_channels is simple than
amdgpu_umc_scan_all_umc_channels, with this fixed, the series is:
Reviewed-by: Tao Zhou
>
>
> > + ret = func(adev, 0, umc_inst, ch_inst, data);
> > + if (ret) {
> > + dev_err(adev->dev, "Umc %d ch %d func
> > returns %d\n",
> > + umc_inst, ch_inst, ret);
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > index d7f1229ff11f..f279c8057f96 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > @@ -47,6 +47,10 @@
> > #define LOOP_UMC_EACH
RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
[AMD Official Use Only - General]
> -Original Message-
> From: Chai, Thomas
> Sent: Monday, April 3, 2023 3:00 PM
> To: Zhou1, Tao ; amd-gfx@lists.freedesktop.org
> Cc: Zhang, Hawking ; Li, Candice
> ; Yang, Stanley
> Subject: RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
>
> [AMD Official Use Only - General]
>
>
>
>
> -
> Best Regards,
> Thomas
>
> -Original Message-
> From: Zhou1, Tao
> Sent: Monday, April 3, 2023 11:45 AM
> To: Chai, Thomas ; amd-gfx@lists.freedesktop.org
> Cc: Zhang, Hawking ; Li, Candice
> ; Yang, Stanley
> Subject: RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
>
> [AMD Official Use Only - General]
>
>
>
> > -Original Message-
> > From: Chai, Thomas
> > Sent: Monday, April 3, 2023 9:59 AM
> > To: amd-gfx@lists.freedesktop.org
> > Cc: Chai, Thomas ; Zhang, Hawking
> > ; Zhou1, Tao ; Li,
> Candice
> > ; Yang, Stanley ; Chai,
> > Thomas
> > Subject: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
> >
> > Optimize redundant code in umc_v8_10
> >
> > Signed-off-by: YiPeng Chai
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c | 31
> > drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h | 7 +
> > drivers/gpu/drm/amd/amdgpu/umc_v8_10.c | 197
> > +---
> > 3 files changed, 115 insertions(+), 120 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > index 9e2e97207e53..734442315cf6 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> > @@ -302,3 +302,34 @@ void amdgpu_umc_fill_error_record(struct
> > ras_err_data *err_data,
> >
> > err_data->err_addr_cnt++;
> > }
> > +
> > +int amdgpu_umc_scan_all_umc_channels(struct amdgpu_device *adev,
> > + umc_func func, void *data)
> > +{
> > + uint32_t node_inst = 0;
> > + uint32_t umc_inst= 0;
> > + uint32_t ch_inst = 0;
> > + int ret = 0;
> > +
> > + if (adev->umc.node_inst_num) {
> > + LOOP_UMC_EACH_NODE_INST_AND_CH(node_inst, umc_inst,
> > ch_inst) {
> > + ret = func(adev, node_inst, umc_inst, ch_inst, data);
> > + if (ret) {
> > + dev_err(adev->dev, "Node %d umc %d ch %d
> > func returns %d\n",
> > + node_inst, umc_inst, ch_inst, ret);
> > + return ret;
> > + }
> > + }
> > + } else {
> > + LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
>
> >[Tao] for ASIC which doesn't support node, can we set its node_inst_num to 1
> and retire the macro LOOP_UMC_INST_AND_CH?
>
> [Thomas] I am afraid not.
>
> " #define LOOP_UMC_NODE_INST(node_inst) \
> for_each_set_bit((node_inst), &(adev->umc.active_mask),
> adev->umc.node_inst_num) "
>
> The node instance loop of LOOP_UMC_EACH_NODE_INST_AND_CH
> supports node harvest, so node_inst_num is not the real node instance number.
[Tao] we can set both node_inst_num and active_mask to 1, but either way is
fine for me.
BTW, I think amdgpu_umc_loop_channels is simple than
amdgpu_umc_scan_all_umc_channels, with this fixed, the series is:
Reviewed-by: Tao Zhou
>
>
> > + ret = func(adev, 0, umc_inst, ch_inst, data);
> > + if (ret) {
> > + dev_err(adev->dev, "Umc %d ch %d func
> > returns %d\n",
> > + umc_inst, ch_inst, ret);
> > + return ret;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > index d7f1229ff11f..f279c8057f96 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> > @@ -47,6 +47,10 @@
> > #define LOOP_UMC_EACH_NODE_INST_AND_CH(node_inst, umc_inst,
> ch_inst)
> > \
> > LOOP_UMC_NODE_INST((node_inst))
> > LOOP_UMC_INST_AND_CH((umc_inst), (ch_inst))
> >
> > +
> > +typedef int (*umc_func)(struct amdgpu_device *adev, uint32_t node_inst,
> > + uint32_t umc_inst, uint32_t ch_inst, void *data);
> > +
> &g
RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
[AMD Official Use Only - General]
-
Best Regards,
Thomas
-Original Message-
From: Zhou1, Tao
Sent: Monday, April 3, 2023 11:45 AM
To: Chai, Thomas ; amd-gfx@lists.freedesktop.org
Cc: Zhang, Hawking ; Li, Candice ;
Yang, Stanley
Subject: RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
[AMD Official Use Only - General]
> -Original Message-
> From: Chai, Thomas
> Sent: Monday, April 3, 2023 9:59 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chai, Thomas ; Zhang, Hawking
> ; Zhou1, Tao ; Li, Candice
> ; Yang, Stanley ; Chai,
> Thomas
> Subject: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
>
> Optimize redundant code in umc_v8_10
>
> Signed-off-by: YiPeng Chai
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c | 31
> drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h | 7 +
> drivers/gpu/drm/amd/amdgpu/umc_v8_10.c | 197
> +---
> 3 files changed, 115 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> index 9e2e97207e53..734442315cf6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> @@ -302,3 +302,34 @@ void amdgpu_umc_fill_error_record(struct
> ras_err_data *err_data,
>
> err_data->err_addr_cnt++;
> }
> +
> +int amdgpu_umc_scan_all_umc_channels(struct amdgpu_device *adev,
> + umc_func func, void *data)
> +{
> + uint32_t node_inst = 0;
> + uint32_t umc_inst= 0;
> + uint32_t ch_inst = 0;
> + int ret = 0;
> +
> + if (adev->umc.node_inst_num) {
> + LOOP_UMC_EACH_NODE_INST_AND_CH(node_inst, umc_inst,
> ch_inst) {
> + ret = func(adev, node_inst, umc_inst, ch_inst, data);
> + if (ret) {
> + dev_err(adev->dev, "Node %d umc %d ch %d
> func returns %d\n",
> + node_inst, umc_inst, ch_inst, ret);
> + return ret;
> + }
> + }
> + } else {
> + LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
>[Tao] for ASIC which doesn't support node, can we set its node_inst_num to 1
>and retire the macro LOOP_UMC_INST_AND_CH?
[Thomas] I am afraid not.
" #define LOOP_UMC_NODE_INST(node_inst) \
for_each_set_bit((node_inst), &(adev->umc.active_mask),
adev->umc.node_inst_num) "
The node instance loop of LOOP_UMC_EACH_NODE_INST_AND_CH supports node
harvest, so node_inst_num is not the real node instance number.
> + ret = func(adev, 0, umc_inst, ch_inst, data);
> + if (ret) {
> + dev_err(adev->dev, "Umc %d ch %d func
> returns %d\n",
> + umc_inst, ch_inst, ret);
> + return ret;
> + }
> + }
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> index d7f1229ff11f..f279c8057f96 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> @@ -47,6 +47,10 @@
> #define LOOP_UMC_EACH_NODE_INST_AND_CH(node_inst, umc_inst, ch_inst)
> \
> LOOP_UMC_NODE_INST((node_inst))
> LOOP_UMC_INST_AND_CH((umc_inst), (ch_inst))
>
> +
> +typedef int (*umc_func)(struct amdgpu_device *adev, uint32_t node_inst,
> + uint32_t umc_inst, uint32_t ch_inst, void *data);
> +
> struct amdgpu_umc_ras {
> struct amdgpu_ras_block_object ras_block;
> void (*err_cnt_init)(struct amdgpu_device *adev); @@ -104,4 +108,7
> @@ int amdgpu_umc_process_ras_data_cb(struct amdgpu_device *adev,
> struct amdgpu_iv_entry *entry);
> int amdgpu_umc_page_retirement_mca(struct amdgpu_device *adev,
> uint64_t err_addr, uint32_t ch_inst, uint32_t umc_inst);
> +
> +int amdgpu_umc_scan_all_umc_channels(struct amdgpu_device *adev,
> + umc_func func, void *data);
> #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> b/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> index fb55e8cb9967..6dff313ac04c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> +++ b/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> @@ -76,10 +76,13 @@ static inline uint32_t
> get_umc_v8_10_reg_offset(struct amdgpu_device *adev,
> UMC_8_NODE_DIST * node_inst;
> }
>
> -static void umc_v8_10_clear_error_count_per_c
RE: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
[AMD Official Use Only - General]
> -Original Message-
> From: Chai, Thomas
> Sent: Monday, April 3, 2023 9:59 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Chai, Thomas ; Zhang, Hawking
> ; Zhou1, Tao ; Li, Candice
> ; Yang, Stanley ; Chai,
> Thomas
> Subject: [PATCH 1/2] drm/amdgpu: optimize redundant code in umc_v8_10
>
> Optimize redundant code in umc_v8_10
>
> Signed-off-by: YiPeng Chai
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c | 31
> drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h | 7 +
> drivers/gpu/drm/amd/amdgpu/umc_v8_10.c | 197 +---
> 3 files changed, 115 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> index 9e2e97207e53..734442315cf6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.c
> @@ -302,3 +302,34 @@ void amdgpu_umc_fill_error_record(struct
> ras_err_data *err_data,
>
> err_data->err_addr_cnt++;
> }
> +
> +int amdgpu_umc_scan_all_umc_channels(struct amdgpu_device *adev,
> + umc_func func, void *data)
> +{
> + uint32_t node_inst = 0;
> + uint32_t umc_inst= 0;
> + uint32_t ch_inst = 0;
> + int ret = 0;
> +
> + if (adev->umc.node_inst_num) {
> + LOOP_UMC_EACH_NODE_INST_AND_CH(node_inst, umc_inst,
> ch_inst) {
> + ret = func(adev, node_inst, umc_inst, ch_inst, data);
> + if (ret) {
> + dev_err(adev->dev, "Node %d umc %d ch %d
> func returns %d\n",
> + node_inst, umc_inst, ch_inst, ret);
> + return ret;
> + }
> + }
> + } else {
> + LOOP_UMC_INST_AND_CH(umc_inst, ch_inst) {
[Tao] for ASIC which doesn't support node, can we set its node_inst_num to 1
and retire the macro LOOP_UMC_INST_AND_CH?
> + ret = func(adev, 0, umc_inst, ch_inst, data);
> + if (ret) {
> + dev_err(adev->dev, "Umc %d ch %d func
> returns %d\n",
> + umc_inst, ch_inst, ret);
> + return ret;
> + }
> + }
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> index d7f1229ff11f..f279c8057f96 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h
> @@ -47,6 +47,10 @@
> #define LOOP_UMC_EACH_NODE_INST_AND_CH(node_inst, umc_inst, ch_inst)
> \
> LOOP_UMC_NODE_INST((node_inst))
> LOOP_UMC_INST_AND_CH((umc_inst), (ch_inst))
>
> +
> +typedef int (*umc_func)(struct amdgpu_device *adev, uint32_t node_inst,
> + uint32_t umc_inst, uint32_t ch_inst, void *data);
> +
> struct amdgpu_umc_ras {
> struct amdgpu_ras_block_object ras_block;
> void (*err_cnt_init)(struct amdgpu_device *adev); @@ -104,4 +108,7
> @@ int amdgpu_umc_process_ras_data_cb(struct amdgpu_device *adev,
> struct amdgpu_iv_entry *entry);
> int amdgpu_umc_page_retirement_mca(struct amdgpu_device *adev,
> uint64_t err_addr, uint32_t ch_inst, uint32_t umc_inst);
> +
> +int amdgpu_umc_scan_all_umc_channels(struct amdgpu_device *adev,
> + umc_func func, void *data);
> #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> b/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> index fb55e8cb9967..6dff313ac04c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> +++ b/drivers/gpu/drm/amd/amdgpu/umc_v8_10.c
> @@ -76,10 +76,13 @@ static inline uint32_t get_umc_v8_10_reg_offset(struct
> amdgpu_device *adev,
> UMC_8_NODE_DIST * node_inst;
> }
>
> -static void umc_v8_10_clear_error_count_per_channel(struct amdgpu_device
> *adev,
> - uint32_t umc_reg_offset)
> +static int umc_v8_10_clear_error_count_per_channel(struct amdgpu_device
> *adev,
> + uint32_t node_inst, uint32_t umc_inst,
> + uint32_t ch_inst, void *data)
> {
> uint32_t ecc_err_cnt_addr;
> + uint32_t umc_reg_offset =
> + get_umc_v8_10_reg_offset(adev, node_inst, umc_inst, ch_inst);
>
> ecc_err_cnt_addr =
> SOC15_REG_OFFSET(UMC, 0, regUMCCH0_0_GeccErrCnt); @@
> -87,24 +90,14 @@ static void
> umc_v8_10_clear_error_count_per_channel(struct amdgpu_device *adev,
> /* clear error count */
> WREG32_PCIE((ecc_err_cnt_addr + umc_reg_offset) * 4,
> UMC_V8_10_CE_CNT_INIT);
> +
> + return 0;
> }
>
> static void umc_v8_10_clear_error_count(struct amdgpu_device *adev) {
> - uint32_t node_inst = 0;
> - uint32_t umc_inst= 0;
> - uint32_t ch_inst = 0;
> - uint32_t
