drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
The repeated checks on grbm_soft_reset are unnecessary. Remove them.
Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
---
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
index 7bd506f06eb155de7f2edb2c1c9d5ed7232b16fc..264183ab24ec299425e6a6d0539339ee69f60c24 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
@@ -7668,19 +7668,17 @@ static int gfx_v10_0_soft_reset(struct amdgpu_ip_block *ip_block)
/* Disable MEC parsing/prefetching */
gfx_v10_0_cp_compute_enable(adev, false);
- if (grbm_soft_reset) {
- tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
- tmp |= grbm_soft_reset;
- dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
- WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
- tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
-
- udelay(50);
-
- tmp &= ~grbm_soft_reset;
- WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
- tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
- }
+ tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
+ tmp |= grbm_soft_reset;
+ dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
+ WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
+ tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
+
+ udelay(50);
+
+ tmp &= ~grbm_soft_reset;
+ WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
+ tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
/* Wait a little for things to settle down */
udelay(50);
---
base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110
change-id: 20250801-amdgfx10-f96c43cb0c59
Best regards,
--
Ethan Carter Edwards <ethan@ethancedwards.com>On Sat, Aug 2, 2025 at 4:22 AM Ethan Carter Edwards
<ethan@ethancedwards.com> wrote:
>
> The repeated checks on grbm_soft_reset are unnecessary. Remove them.
>
These are not NULL checks and they are necessary. The code is
checking if any bits are set in that register. If not, then we can
skip that code as there is nothing to do.
Alex
> Signed-off-by: Ethan Carter Edwards <ethan@ethancedwards.com>
> ---
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c | 24 +++++++++++-------------
> 1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 7bd506f06eb155de7f2edb2c1c9d5ed7232b16fc..264183ab24ec299425e6a6d0539339ee69f60c24 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -7668,19 +7668,17 @@ static int gfx_v10_0_soft_reset(struct amdgpu_ip_block *ip_block)
> /* Disable MEC parsing/prefetching */
> gfx_v10_0_cp_compute_enable(adev, false);
>
> - if (grbm_soft_reset) {
> - tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> - tmp |= grbm_soft_reset;
> - dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
> - WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> - tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> -
> - udelay(50);
> -
> - tmp &= ~grbm_soft_reset;
> - WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> - tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> - }
> + tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> + tmp |= grbm_soft_reset;
> + dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
> + WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> + tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> +
> + udelay(50);
> +
> + tmp &= ~grbm_soft_reset;
> + WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> + tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
>
> /* Wait a little for things to settle down */
> udelay(50);
>
> ---
> base-commit: b9ddaa95fd283bce7041550ddbbe7e764c477110
> change-id: 20250801-amdgfx10-f96c43cb0c59
>
> Best regards,
> --
> Ethan Carter Edwards <ethan@ethancedwards.com>
>
On Mon, Aug 04, 2025 at 10:32:43AM -0400, Alex Deucher wrote: > On Sat, Aug 2, 2025 at 4:22 AM Ethan Carter Edwards > <ethan@ethancedwards.com> wrote: > > > > The repeated checks on grbm_soft_reset are unnecessary. Remove them. > > > > These are not NULL checks and they are necessary. The code is > checking if any bits are set in that register. If not, then we can > skip that code as there is nothing to do. > It's not a null check, but it is a nested check and it's a local variable so the patch is correct enough. At this point we know that grbm_soft_reset can't be zero. regards, dan carpenter
On Mon, Aug 4, 2025 at 10:49 AM Dan Carpenter <dan.carpenter@linaro.org> wrote: > > On Mon, Aug 04, 2025 at 10:32:43AM -0400, Alex Deucher wrote: > > On Sat, Aug 2, 2025 at 4:22 AM Ethan Carter Edwards > > <ethan@ethancedwards.com> wrote: > > > > > > The repeated checks on grbm_soft_reset are unnecessary. Remove them. > > > > > > > These are not NULL checks and they are necessary. The code is > > checking if any bits are set in that register. If not, then we can > > skip that code as there is nothing to do. > > > > It's not a null check, but it is a nested check and it's a local > variable so the patch is correct enough. At this point we know that > grbm_soft_reset can't be zero. It can be 0 as far as I can see. If none of the GRBM_STATUS bits are set, then we never set any of the bits in grbm_soft_reset. Alex > > regards, > dan carpenter >
On Mon, Aug 04, 2025 at 11:08:57AM -0400, Alex Deucher wrote:
> On Mon, Aug 4, 2025 at 10:49 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Mon, Aug 04, 2025 at 10:32:43AM -0400, Alex Deucher wrote:
> > > On Sat, Aug 2, 2025 at 4:22 AM Ethan Carter Edwards
> > > <ethan@ethancedwards.com> wrote:
> > > >
> > > > The repeated checks on grbm_soft_reset are unnecessary. Remove them.
> > > >
> > >
> > > These are not NULL checks and they are necessary. The code is
> > > checking if any bits are set in that register. If not, then we can
> > > skip that code as there is nothing to do.
> > >
> >
> > It's not a null check, but it is a nested check and it's a local
> > variable so the patch is correct enough. At this point we know that
> > grbm_soft_reset can't be zero.
>
> It can be 0 as far as I can see. If none of the GRBM_STATUS bits are
> set, then we never set any of the bits in grbm_soft_reset.
>
You're missing the first check...
drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
7657 if (grbm_soft_reset) {
^^^^^^^^^^^^^^^
Checked.
7658 /* stop the rlc */
7659 gfx_v10_0_rlc_stop(adev);
7660
7661 /* Disable GFX parsing/prefetching */
7662 gfx_v10_0_cp_gfx_enable(adev, false);
7663
7664 /* Disable MEC parsing/prefetching */
7665 gfx_v10_0_cp_compute_enable(adev, false);
7666
7667 if (grbm_soft_reset) {
^^^^^^^^^^^^^^^
Unnecessary.
7668 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
7669 tmp |= grbm_soft_reset;
7670 dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
7671 WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
7672 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
7673
7674 udelay(50);
7675
7676 tmp &= ~grbm_soft_reset;
7677 WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
7678 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
7679 }
7680
7681 /* Wait a little for things to settle down */
7682 udelay(50);
7683 }
7684 return 0;
regards,
dan carpenter
On Mon, Aug 4, 2025 at 1:15 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Mon, Aug 04, 2025 at 11:08:57AM -0400, Alex Deucher wrote:
> > On Mon, Aug 4, 2025 at 10:49 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > On Mon, Aug 04, 2025 at 10:32:43AM -0400, Alex Deucher wrote:
> > > > On Sat, Aug 2, 2025 at 4:22 AM Ethan Carter Edwards
> > > > <ethan@ethancedwards.com> wrote:
> > > > >
> > > > > The repeated checks on grbm_soft_reset are unnecessary. Remove them.
> > > > >
> > > >
> > > > These are not NULL checks and they are necessary. The code is
> > > > checking if any bits are set in that register. If not, then we can
> > > > skip that code as there is nothing to do.
> > > >
> > >
> > > It's not a null check, but it is a nested check and it's a local
> > > variable so the patch is correct enough. At this point we know that
> > > grbm_soft_reset can't be zero.
> >
> > It can be 0 as far as I can see. If none of the GRBM_STATUS bits are
> > set, then we never set any of the bits in grbm_soft_reset.
> >
>
> You're missing the first check...
>
> drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> 7657 if (grbm_soft_reset) {
> ^^^^^^^^^^^^^^^
> Checked.
>
> 7658 /* stop the rlc */
> 7659 gfx_v10_0_rlc_stop(adev);
> 7660
> 7661 /* Disable GFX parsing/prefetching */
> 7662 gfx_v10_0_cp_gfx_enable(adev, false);
> 7663
> 7664 /* Disable MEC parsing/prefetching */
> 7665 gfx_v10_0_cp_compute_enable(adev, false);
> 7666
> 7667 if (grbm_soft_reset) {
> ^^^^^^^^^^^^^^^
> Unnecessary.
Yes, sorry, my brain processed this as the first check.
Alex
>
> 7668 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> 7669 tmp |= grbm_soft_reset;
> 7670 dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
> 7671 WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> 7672 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> 7673
> 7674 udelay(50);
> 7675
> 7676 tmp &= ~grbm_soft_reset;
> 7677 WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> 7678 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> 7679 }
> 7680
> 7681 /* Wait a little for things to settle down */
> 7682 udelay(50);
> 7683 }
> 7684 return 0;
>
> regards,
> dan carpenter
>
Applied all three patches with a slight tweak to the commit messages
to make it clearer what is changing.
Alex
On Mon, Aug 4, 2025 at 1:25 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Mon, Aug 4, 2025 at 1:15 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Mon, Aug 04, 2025 at 11:08:57AM -0400, Alex Deucher wrote:
> > > On Mon, Aug 4, 2025 at 10:49 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > >
> > > > On Mon, Aug 04, 2025 at 10:32:43AM -0400, Alex Deucher wrote:
> > > > > On Sat, Aug 2, 2025 at 4:22 AM Ethan Carter Edwards
> > > > > <ethan@ethancedwards.com> wrote:
> > > > > >
> > > > > > The repeated checks on grbm_soft_reset are unnecessary. Remove them.
> > > > > >
> > > > >
> > > > > These are not NULL checks and they are necessary. The code is
> > > > > checking if any bits are set in that register. If not, then we can
> > > > > skip that code as there is nothing to do.
> > > > >
> > > >
> > > > It's not a null check, but it is a nested check and it's a local
> > > > variable so the patch is correct enough. At this point we know that
> > > > grbm_soft_reset can't be zero.
> > >
> > > It can be 0 as far as I can see. If none of the GRBM_STATUS bits are
> > > set, then we never set any of the bits in grbm_soft_reset.
> > >
> >
> > You're missing the first check...
> >
> > drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> > 7657 if (grbm_soft_reset) {
> > ^^^^^^^^^^^^^^^
> > Checked.
> >
> > 7658 /* stop the rlc */
> > 7659 gfx_v10_0_rlc_stop(adev);
> > 7660
> > 7661 /* Disable GFX parsing/prefetching */
> > 7662 gfx_v10_0_cp_gfx_enable(adev, false);
> > 7663
> > 7664 /* Disable MEC parsing/prefetching */
> > 7665 gfx_v10_0_cp_compute_enable(adev, false);
> > 7666
> > 7667 if (grbm_soft_reset) {
> > ^^^^^^^^^^^^^^^
> > Unnecessary.
>
> Yes, sorry, my brain processed this as the first check.
>
> Alex
>
> >
> > 7668 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> > 7669 tmp |= grbm_soft_reset;
> > 7670 dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
> > 7671 WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> > 7672 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> > 7673
> > 7674 udelay(50);
> > 7675
> > 7676 tmp &= ~grbm_soft_reset;
> > 7677 WREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET, tmp);
> > 7678 tmp = RREG32_SOC15(GC, 0, mmGRBM_SOFT_RESET);
> > 7679 }
> > 7680
> > 7681 /* Wait a little for things to settle down */
> > 7682 udelay(50);
> > 7683 }
> > 7684 return 0;
> >
> > regards,
> > dan carpenter
> >
© 2016 - 2025 Red Hat, Inc.