* [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks
@ 2025-08-02 1:41 Ethan Carter Edwards
2025-08-04 14:32 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Ethan Carter Edwards @ 2025-08-02 1:41 UTC (permalink / raw)
To: Alex Deucher, Christian König, David Airlie, Simona Vetter
Cc: amd-gfx, dri-devel, linux-kernel, kernel-janitors, Ethan Carter Edwards
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>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks
2025-08-02 1:41 [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks Ethan Carter Edwards
@ 2025-08-04 14:32 ` Alex Deucher
2025-08-04 14:49 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2025-08-04 14:32 UTC (permalink / raw)
To: Ethan Carter Edwards
Cc: Alex Deucher, Christian König, David Airlie, Simona Vetter,
amd-gfx, dri-devel, linux-kernel, kernel-janitors
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>
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks
2025-08-04 14:32 ` Alex Deucher
@ 2025-08-04 14:49 ` Dan Carpenter
2025-08-04 15:08 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2025-08-04 14:49 UTC (permalink / raw)
To: Alex Deucher
Cc: Ethan Carter Edwards, Alex Deucher, Christian König,
David Airlie, Simona Vetter, amd-gfx, dri-devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks
2025-08-04 14:49 ` Dan Carpenter
@ 2025-08-04 15:08 ` Alex Deucher
2025-08-04 17:15 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2025-08-04 15:08 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ethan Carter Edwards, Alex Deucher, Christian König,
David Airlie, Simona Vetter, amd-gfx, dri-devel, linux-kernel,
kernel-janitors
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks
2025-08-04 15:08 ` Alex Deucher
@ 2025-08-04 17:15 ` Dan Carpenter
2025-08-04 17:25 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2025-08-04 17:15 UTC (permalink / raw)
To: Alex Deucher
Cc: Ethan Carter Edwards, Alex Deucher, Christian König,
David Airlie, Simona Vetter, amd-gfx, dri-devel, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks
2025-08-04 17:15 ` Dan Carpenter
@ 2025-08-04 17:25 ` Alex Deucher
2025-08-04 17:30 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2025-08-04 17:25 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ethan Carter Edwards, Alex Deucher, Christian König,
David Airlie, Simona Vetter, amd-gfx, dri-devel, linux-kernel,
kernel-janitors
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks
2025-08-04 17:25 ` Alex Deucher
@ 2025-08-04 17:30 ` Alex Deucher
0 siblings, 0 replies; 7+ messages in thread
From: Alex Deucher @ 2025-08-04 17:30 UTC (permalink / raw)
To: Dan Carpenter
Cc: Ethan Carter Edwards, Alex Deucher, Christian König,
David Airlie, Simona Vetter, amd-gfx, dri-devel, linux-kernel,
kernel-janitors
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
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-08-04 17:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-08-02 1:41 [PATCH] drm/amdgpu/gfx10: remove redundant repeated null checks Ethan Carter Edwards
2025-08-04 14:32 ` Alex Deucher
2025-08-04 14:49 ` Dan Carpenter
2025-08-04 15:08 ` Alex Deucher
2025-08-04 17:15 ` Dan Carpenter
2025-08-04 17:25 ` Alex Deucher
2025-08-04 17:30 ` Alex Deucher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®