I guess I don't really have a particularly strong opinion either
way. If others are ok with it, I'm fine with it.
Alex
*From:*StDenis, Tom
*Sent:* Thursday, August 11, 2016 11:48 AM
*To:* Deucher, Alexander; [email protected]
*Subject:* Re: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
Just trying to make it easier to read.
WREG32_FIELD(foo, FIELD, 1);
Is easier to read than
WREG32_P(foo, FOO__FIELD_MASK, ~FOO__FIELD_MASK);
(also I already pushed them after getting a RB by Christian this
morning so we might need to hold a different discussion).
I agree it's not really a functional change but making things a bit
more uniform and easier to read helps maintenance. And I did find a
bug in the process :-)
Tom
------------------------------------------------------------------------
*From:*Deucher, Alexander
*Sent:* Thursday, August 11, 2016 11:46
*To:* 'Tom St Denis'; [email protected]
<mailto:[email protected]>
*Cc:* StDenis, Tom
*Subject:* RE: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
> -----Original Message-----
> From: amd-gfx [mailto:[email protected]] On Behalf
> Of Tom St Denis
> Sent: Thursday, August 11, 2016 10:33 AM
> To: [email protected]
<mailto:[email protected]>
> Cc: StDenis, Tom
> Subject: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup
>
> Signed-off-by: Tom St Denis <[email protected]
<mailto:[email protected]>>
A couple pieces of this patch could be split out as separate cleanups
(see below). However, I'm not sure how much value there is in
switching WREG32_P for WREG_FIELD other than code churn. They
basically do the same thing.
Alex
> ---
> drivers/gpu/drm/amd/amdgpu/vce_v2_0.c | 39 +++++++++++++-------------
> ---------
> 1 file changed, 14 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> index 80a37a602181..21ba219e943b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v2_0.c
> @@ -127,15 +127,10 @@ static int vce_v2_0_start(struct amdgpu_device
> *adev)
> WREG32(mmVCE_RB_BASE_HI2, upper_32_bits(ring->gpu_addr));
> WREG32(mmVCE_RB_SIZE2, ring->ring_size / 4);
>
> - WREG32_P(mmVCE_VCPU_CNTL,
> VCE_VCPU_CNTL__CLK_EN_MASK, ~VCE_VCPU_CNTL__CLK_EN_MASK);
> -
> - WREG32_P(mmVCE_SOFT_RESET,
> - VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
> - ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
> -
> + WREG32_FIELD(VCE_VCPU_CNTL, CLK_EN, 1);
> + WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
> mdelay(100);
> -
> - WREG32_P(mmVCE_SOFT_RESET, 0,
> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
> + WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 0);
>
> for (i = 0; i < 10; ++i) {
> uint32_t status;
> @@ -150,10 +145,9 @@ static int vce_v2_0_start(struct amdgpu_device
> *adev)
> break;
>
> DRM_ERROR("VCE not responding, trying to reset the
> ECPU!!!\n");
> - WREG32_P(mmVCE_SOFT_RESET,
> VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
> -
> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
> + WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
> mdelay(10);
> - WREG32_P(mmVCE_SOFT_RESET, 0,
> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
> + WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 0);
> mdelay(10);
> r = -1;
> }
> @@ -345,13 +339,13 @@ static void vce_v2_0_set_dyn_cg(struct
> amdgpu_device *adev, bool gated)
> DRM_INFO("VCE is busy, Can't set clock
gateing");
> return;
> }
> - WREG32_P(mmVCE_VCPU_CNTL, 0,
> ~VCE_VCPU_CNTL__CLK_EN_MASK);
> - WREG32_P(mmVCE_SOFT_RESET,
> VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
> + WREG32_FIELD(VCE_VCPU_CNTL, CLK_EN, 0);
> + WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
> mdelay(100);
> WREG32(mmVCE_STATUS, 0);
> } else {
> - WREG32_P(mmVCE_VCPU_CNTL,
> VCE_VCPU_CNTL__CLK_EN_MASK, ~VCE_VCPU_CNTL__CLK_EN_MASK);
> - WREG32_P(mmVCE_SOFT_RESET,
> VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK,
> ~VCE_SOFT_RESET__ECPU_SOFT_RESET_MASK);
> + WREG32_FIELD(VCE_VCPU_CNTL, CLK_EN, 1);
> + WREG32_FIELD(VCE_SOFT_RESET, ECPU_SOFT_RESET, 1);
> mdelay(100);
> }
>
> @@ -458,9 +452,7 @@ static void vce_v2_0_mc_resume(struct
> amdgpu_device *adev)
> WREG32(mmVCE_VCPU_CACHE_SIZE2, size);
>
> WREG32_P(mmVCE_LMI_CTRL2, 0x0, ~0x100);
> -
> - WREG32_P(mmVCE_SYS_INT_EN,
> VCE_SYS_INT_EN__VCE_SYS_INT_TRAP_INTERRUPT_EN_MASK,
> -
> ~VCE_SYS_INT_EN__VCE_SYS_INT_TRAP_INTERRUPT_EN_MASK);
> + WREG32_FIELD(VCE_SYS_INT_EN,
> VCE_SYS_INT_TRAP_INTERRUPT_EN, 1);
>
> vce_v2_0_init_cg(adev);
> }
> @@ -474,11 +466,11 @@ static bool vce_v2_0_is_idle(void *handle)
>
> static int vce_v2_0_wait_for_idle(void *handle)
> {
> - unsigned i;
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + unsigned i;
>
> for (i = 0; i < adev->usec_timeout; i++) {
> - if (!(RREG32(mmSRBM_STATUS2) &
> SRBM_STATUS2__VCE_BUSY_MASK))
> + if (vce_v2_0_is_idle(handle))
> return 0;
This could be split out as a separate patch.
> }
> return -ETIMEDOUT;
> @@ -488,8 +480,7 @@ static int vce_v2_0_soft_reset(void *handle)
> {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> - WREG32_P(mmSRBM_SOFT_RESET,
> SRBM_SOFT_RESET__SOFT_RESET_VCE_MASK,
> - ~SRBM_SOFT_RESET__SOFT_RESET_VCE_MASK);
> + WREG32_FIELD(SRBM_SOFT_RESET, SOFT_RESET_VCE, 1);
> mdelay(5);
>
> return vce_v2_0_start(adev);
> @@ -516,10 +507,8 @@ static int vce_v2_0_process_interrupt(struct
> amdgpu_device *adev,
> DRM_DEBUG("IH: VCE\n");
> switch (entry->src_data) {
> case 0:
> - amdgpu_fence_process(&adev->vce.ring[0]);
> - break;
> case 1:
> - amdgpu_fence_process(&adev->vce.ring[1]);
> + amdgpu_fence_process(&adev->vce.ring[entry->src_data]);
> break;
> default:
> DRM_ERROR("Unhandled interrupt: %d %d\n",
This too.
> --
> 2.9.2
>
> _______________________________________________
> amd-gfx mailing list
> [email protected] <mailto:[email protected]>
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
