On Fri, Aug 12, 2016 at 10:01 AM, Deucher, Alexander <Alexander.Deucher at amd.com> wrote: > In that case: For the series: > > Reviewed-by: Alex Deucher <alexander.deucher at amd.com> > > > > From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf Of > zhoucm1 > Sent: Thursday, August 11, 2016 9:52 PM > To: Deucher, Alexander; StDenis, Tom; amd-gfx at lists.freedesktop.org > > > Subject: Re: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup > > > > Agree with Tom. > > On 2016å¹´08æ??12æ?¥ 00:09, Deucher, Alexander wrote: > > 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; amd-gfx at lists.freedesktop.org > 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'; amd-gfx at lists.freedesktop.org > Cc: StDenis, Tom > Subject: RE: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup > > > >> -----Original Message----- >> From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf >> Of Tom St Denis >> Sent: Thursday, August 11, 2016 10:33 AM >> To: amd-gfx at lists.freedesktop.org >> Cc: StDenis, Tom >> Subject: [PATCH 3/4] drm/amd/amdgpu: VCE v2 register cleanup >> >> Signed-off-by: Tom St Denis <tom.stdenis at amd.com> > > 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 >> 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 > > > > > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx >
