[PATCH 2/2] drm/amdgpu: drop unnecessary dereference
Quan, Evan
Evan.Quan at amd.com
Mon Mar 11 06:20:09 UTC 2019
OK, i see. Yes, adev->gfx.ras_if needs to point to the new alloced buffer as it was NULL.
I still think drop one level dereference can make code clean and better understandable.
Anyway, it's fine with me to keep old code.
Regards,
Evan
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of Pan,
> Xinhui
> Sent: 2019年3月11日 13:45
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: RE: [PATCH 2/2] drm/amdgpu: drop unnecessary dereference
>
> adev->gfx.ras_if is a pointer and it is NULL at the first time
>
> -----Original Message-----
> From: Quan, Evan <Evan.Quan at amd.com>
> Sent: 2019年3月11日 13:41
> To: Pan, Xinhui <Xinhui.Pan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
> Subject: RE: [PATCH 2/2] drm/amdgpu: drop unnecessary dereference
>
> I cannot get your point. "ras_if" was already assigned to be same as "adev-
> >gfx.ras_if" at the start of the APIs.
> //// struct ras_common_if *ras_if = adev->gfx.ras_if; ///// Why there is
> need to set " adev->gfx.ras_if = ras_if" again?
>
> Regards,
> Evan
> > -----Original Message-----
> > From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> > Pan, Xinhui
> > Sent: 2019年3月11日 13:19
> > To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Quan, Evan
> > <Evan.Quan at amd.com>
> > Subject: RE: [PATCH 2/2] drm/amdgpu: drop unnecessary dereference
> >
> > Well, I think you forgot setting adev->gfx.ras_if = ras_if in the end.
> >
> > struct ras_common_if **ras_if is pretty simple and easy to use.
> > I prefer to keep the code as it is.
> >
> > Thanks
> > xinhui
> >
> >
> > -----Original Message-----
> > From: Evan Quan <evan.quan at amd.com>
> > Sent: 2019年3月11日 12:31
> > To: amd-gfx at lists.freedesktop.org
> > Cc: Pan, Xinhui <Xinhui.Pan at amd.com>; Deucher, Alexander
> > <Alexander.Deucher at amd.com>; Quan, Evan <Evan.Quan at amd.com>
> > Subject: [PATCH 2/2] drm/amdgpu: drop unnecessary dereference
> >
> > It's unnecessary and confusing.
> >
> > Change-Id: I77fe54a108b7ee2031851b3e11d63c4fb74c0d43
> > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 26
> > +++++++++++++------------- drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c |
> 26
> > +++++++++++++------------
> > - drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 26 +++++++++++++-------
> --
> > ----
> > 3 files changed, 39 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> > index 3fb72bf420e0..31996d448817 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> > @@ -3532,7 +3532,7 @@ static int gfx_v9_0_process_ras_data_cb(struct
> > amdgpu_device *adev, static int gfx_v9_0_ecc_late_init(void *handle) {
> > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > - struct ras_common_if **ras_if = &adev->gfx.ras_if;
> > + struct ras_common_if *ras_if = adev->gfx.ras_if;
> > struct ras_ih_if ih_info = {
> > .cb = gfx_v9_0_process_ras_data_cb,
> > };
> > @@ -3553,21 +3553,21 @@ static int gfx_v9_0_ecc_late_init(void *handle)
> > return 0;
> > }
> >
> > - if (*ras_if)
> > + if (ras_if)
> > goto resume;
> >
> > - *ras_if = kmalloc(sizeof(**ras_if), GFP_KERNEL);
> > - if (!*ras_if)
> > + ras_if = kmalloc(sizeof(*ras_if), GFP_KERNEL);
> > + if (!ras_if)
> > return -ENOMEM;
> >
> > - **ras_if = ras_block;
> > + *ras_if = ras_block;
> >
> > - r = amdgpu_ras_feature_enable(adev, *ras_if, 1);
> > + r = amdgpu_ras_feature_enable(adev, ras_if, 1);
> > if (r)
> > goto feature;
> >
> > - ih_info.head = **ras_if;
> > - fs_info.head = **ras_if;
> > + ih_info.head = *ras_if;
> > + fs_info.head = *ras_if;
> >
> > r = amdgpu_ras_interrupt_add_handler(adev, &ih_info);
> > if (r)
> > @@ -3587,16 +3587,16 @@ static int gfx_v9_0_ecc_late_init(void
> > *handle)
> >
> > return 0;
> > irq:
> > - amdgpu_ras_sysfs_remove(adev, *ras_if);
> > + amdgpu_ras_sysfs_remove(adev, ras_if);
> > sysfs:
> > - amdgpu_ras_debugfs_remove(adev, *ras_if);
> > + amdgpu_ras_debugfs_remove(adev, ras_if);
> > debugfs:
> > amdgpu_ras_interrupt_remove_handler(adev, &ih_info);
> > interrupt:
> > - amdgpu_ras_feature_enable(adev, *ras_if, 0);
> > + amdgpu_ras_feature_enable(adev, ras_if, 0);
> > feature:
> > - kfree(*ras_if);
> > - *ras_if = NULL;
> > + kfree(ras_if);
> > + ras_if = NULL;
> > return -EINVAL;
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > index 5d1ac53f7ddb..229e614d1b76 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> > @@ -714,7 +714,7 @@ static int gmc_v9_0_allocate_vm_inv_eng(struct
> > amdgpu_device *adev) static int gmc_v9_0_ecc_late_init(void *handle) {
> > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > - struct ras_common_if **ras_if = &adev->gmc.ras_if;
> > + struct ras_common_if *ras_if = adev->gmc.ras_if;
> > struct ras_ih_if ih_info = {
> > .cb = gmc_v9_0_process_ras_data_cb,
> > };
> > @@ -735,21 +735,21 @@ static int gmc_v9_0_ecc_late_init(void *handle)
> > return 0;
> > }
> > /* handle resume path. */
> > - if (*ras_if)
> > + if (ras_if)
> > goto resume;
> >
> > - *ras_if = kmalloc(sizeof(**ras_if), GFP_KERNEL);
> > - if (!*ras_if)
> > + ras_if = kmalloc(sizeof(*ras_if), GFP_KERNEL);
> > + if (!ras_if)
> > return -ENOMEM;
> >
> > - **ras_if = ras_block;
> > + *ras_if = ras_block;
> >
> > - r = amdgpu_ras_feature_enable(adev, *ras_if, 1);
> > + r = amdgpu_ras_feature_enable(adev, ras_if, 1);
> > if (r)
> > goto feature;
> >
> > - ih_info.head = **ras_if;
> > - fs_info.head = **ras_if;
> > + ih_info.head = *ras_if;
> > + fs_info.head = *ras_if;
> >
> > r = amdgpu_ras_interrupt_add_handler(adev, &ih_info);
> > if (r)
> > @@ -769,16 +769,16 @@ static int gmc_v9_0_ecc_late_init(void *handle)
> >
> > return 0;
> > irq:
> > - amdgpu_ras_sysfs_remove(adev, *ras_if);
> > + amdgpu_ras_sysfs_remove(adev, ras_if);
> > sysfs:
> > - amdgpu_ras_debugfs_remove(adev, *ras_if);
> > + amdgpu_ras_debugfs_remove(adev, ras_if);
> > debugfs:
> > amdgpu_ras_interrupt_remove_handler(adev, &ih_info);
> > interrupt:
> > - amdgpu_ras_feature_enable(adev, *ras_if, 0);
> > + amdgpu_ras_feature_enable(adev, ras_if, 0);
> > feature:
> > - kfree(*ras_if);
> > - *ras_if = NULL;
> > + kfree(ras_if);
> > + ras_if = NULL;
> > return -EINVAL;
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > index 3ac5abe937f4..521218053477 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> > @@ -1501,7 +1501,7 @@ static int sdma_v4_0_process_ras_data_cb(struct
> > amdgpu_device *adev, static int sdma_v4_0_late_init(void *handle) {
> > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > - struct ras_common_if **ras_if = &adev->sdma.ras_if;
> > + struct ras_common_if *ras_if = adev->sdma.ras_if;
> > struct ras_ih_if ih_info = {
> > .cb = sdma_v4_0_process_ras_data_cb,
> > };
> > @@ -1523,21 +1523,21 @@ static int sdma_v4_0_late_init(void *handle)
> > }
> >
> > /* handle resume path. */
> > - if (*ras_if)
> > + if (ras_if)
> > goto resume;
> >
> > - *ras_if = kmalloc(sizeof(**ras_if), GFP_KERNEL);
> > - if (!*ras_if)
> > + ras_if = kmalloc(sizeof(*ras_if), GFP_KERNEL);
> > + if (!ras_if)
> > return -ENOMEM;
> >
> > - **ras_if = ras_block;
> > + *ras_if = ras_block;
> >
> > - r = amdgpu_ras_feature_enable(adev, *ras_if, 1);
> > + r = amdgpu_ras_feature_enable(adev, ras_if, 1);
> > if (r)
> > goto feature;
> >
> > - ih_info.head = **ras_if;
> > - fs_info.head = **ras_if;
> > + ih_info.head = *ras_if;
> > + fs_info.head = *ras_if;
> >
> > r = amdgpu_ras_interrupt_add_handler(adev, &ih_info);
> > if (r)
> > @@ -1563,16 +1563,16 @@ static int sdma_v4_0_late_init(void *handle)
> >
> > return 0;
> > irq:
> > - amdgpu_ras_sysfs_remove(adev, *ras_if);
> > + amdgpu_ras_sysfs_remove(adev, ras_if);
> > sysfs:
> > - amdgpu_ras_debugfs_remove(adev, *ras_if);
> > + amdgpu_ras_debugfs_remove(adev, ras_if);
> > debugfs:
> > amdgpu_ras_interrupt_remove_handler(adev, &ih_info);
> > interrupt:
> > - amdgpu_ras_feature_enable(adev, *ras_if, 0);
> > + amdgpu_ras_feature_enable(adev, ras_if, 0);
> > feature:
> > - kfree(*ras_if);
> > - *ras_if = NULL;
> > + kfree(ras_if);
> > + ras_if = NULL;
> > return -EINVAL;
> > }
> >
> > --
> > 2.21.0
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list