Dito, using WREG32_FIELD is clearly easier to read. If the additional register writes are really an issue we could adjust the macro to skip those if the value didn't changed. Regards, Christian. Am 12.08.2016 um 03:52 schrieb zhoucm1: > 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 >> <mailto: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 >> <mailto: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 >> <mailto: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 <mailto: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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20160812/c50d34b0/attachment-0001.html>
